Opened 14 years ago
Closed 10 years ago
#7523 closed defect (fixed)
DOH failures under Rhino
Reported by: | Adam Peller | Owned by: | haysmark |
---|---|---|---|
Priority: | high | Milestone: | 1.8 |
Component: | TestFramework | Version: | 1.1.1 |
Keywords: | Cc: | phiggins, James Burke | |
Blocked By: | #15497 | Blocking: |
Description (last modified by )
Also, tests._base.connect is failing at various points with the following error:
TypeError?: Cannot call method "apply" of undefined
this appears to be a regression from [14604]
Attachments (1)
Change History (19)
comment:1 Changed 14 years ago by
comment:2 Changed 14 years ago by
Description: | modified (diff) |
---|
comment:3 Changed 14 years ago by
Description: | modified (diff) |
---|---|
Owner: | changed from Adam Peller to sjmiles |
comment:4 Changed 14 years ago by
connect uses an "array-as-hash" gambit.
Function listeners are stored as an array but iterated as a hash (for in). Array is used as it provides an auto-incrementing hash key (the array index).
Elements are deleted as if from a hash (i.e. with delete). This way existing entries retain their keys (array indices). Iterating as a hash is efficient because undefined keys are not iterated.
[14604] introduced code to iterate over a copy of the listeners array so the iterator is immune to changes to the original array. The copying is done via:
copy = [].concat(original)
In Rhino, using concat on a sparse Array results in a dense Array. IOW, if an array A has elements with indices (0, 2, 4), then under Rhino, "[].concat(A)" results in indices (0, 1, 2, 3, 4), where element 1 and 3 have value "undefined". "A.slice(0)" has the same behavior.
To patch the problem, I will modify connect to make an array copy using a loop if isRhino.
comment:5 Changed 14 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
comment:6 Changed 14 years ago by
Ugh. I'm sorry I made you add more code to base. I wonder if this is something underspecified by ECMAScript or if it's a bug in Rhino? If the latter, we could just push this as a bug onto the Rhino team... I'll take a look at the ECMA spec.
comment:7 Changed 14 years ago by
We can probably use the conditional build commands to strip out the rhino code when we do browser builds. If you are interested, I can explore what sort of >> statement we can use to cut out the rhino stuff for browser builds.
comment:8 Changed 14 years ago by
comment:9 Changed 12 years ago by
Resolution: | fixed |
---|---|
Status: | closed → reopened |
So Rhino 1.7 no longer seems to have this problem. We could write it up as feature detection, but we can probably just ditch this code path altogether?
Changed 12 years ago by
Attachment: | rhino.diff added |
---|
removes the isRhino workaround, tests pass with Rhino 1.7
comment:10 Changed 12 years ago by
Cc: | phiggins James Burke added |
---|---|
Milestone: | 1.2 → 1.5 |
comment:11 Changed 12 years ago by
I'm OK with this code change, the bigger issue is deciding if we want to say Dojo only works with Rhino 1.7+. I do not have a good read on the % use of Rhino 1.6 + Dojo in the wild. peller, any insight you have? I figure your company might have some better stats on it.
comment:12 Changed 12 years ago by
Milestone: | 1.5 → tbd |
---|
yeah, I'm finding people are still using Rhino 1.6, but I'm not sure why or how bad it would be to force an upgrade.
comment:13 Changed 12 years ago by
Milestone: | tbd → 1.6 |
---|---|
Owner: | changed from sjmiles to Adam Peller |
Status: | reopened → new |
comment:14 Changed 12 years ago by
Milestone: | 1.6 → 1.7 |
---|
comment:15 Changed 10 years ago by
Owner: | changed from Adam Peller to haysmark |
---|---|
Status: | new → assigned |
need to reassess as we're now on a new loader and much of the core has changed. If there are still issues, it may make sense to start over with new ticket(s)
comment:16 Changed 10 years ago by
Component: | General → TestFramework |
---|
comment:18 Changed 10 years ago by
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
(In [14951]) Run browser-based tests conditionally. Refs #7523, #7178