Opened 11 years ago

Closed 7 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 Adam Peller)

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)

rhino.diff (1.3 KB) - added by Adam Peller 9 years ago.
removes the isRhino workaround, tests pass with Rhino 1.7

Download all attachments as: .zip

Change History (19)

comment:1 Changed 11 years ago by Adam Peller

(In [14951]) Run browser-based tests conditionally. Refs #7523, #7178

comment:2 Changed 11 years ago by Adam Peller

Description: modified (diff)

comment:3 Changed 11 years ago by Adam Peller

Description: modified (diff)
Owner: changed from Adam Peller to sjmiles

comment:4 Changed 11 years ago by sjmiles

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 11 years ago by sjmiles

Resolution: fixed
Status: newclosed

(In [14972]) Manual Array copy in connect.js under Rhino. Fixes #7523. !strict

comment:6 Changed 11 years ago by Adam Peller

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 11 years ago by James Burke

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 11 years ago by James Burke

(In [15043]) Refs #7523. Use conditional build options to leave out rhino-specific fix for browser builds, to limit impact on dojo base size. \!strict

comment:9 Changed 9 years ago by Adam Peller

Resolution: fixed
Status: closedreopened

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 9 years ago by Adam Peller

Attachment: rhino.diff added

removes the isRhino workaround, tests pass with Rhino 1.7

comment:10 Changed 9 years ago by Adam Peller

Cc: phiggins James Burke added
Milestone: 1.21.5

comment:11 Changed 9 years ago by James Burke

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 9 years ago by Adam Peller

Milestone: 1.5tbd

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 9 years ago by Adam Peller

Milestone: tbd1.6
Owner: changed from sjmiles to Adam Peller
Status: reopenednew

comment:14 Changed 9 years ago by Adam Peller

Milestone: 1.61.7

comment:15 Changed 7 years ago by Adam Peller

Owner: changed from Adam Peller to haysmark
Status: newassigned

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 7 years ago by bill

Component: GeneralTestFramework

comment:17 Changed 7 years ago by haysmark

Blocked By: 15497 added

There are still issues with Rhino.

comment:18 Changed 7 years ago by haysmark

Resolution: fixed
Status: assignedclosed
Note: See TracTickets for help on using tickets.