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

Attachment: rhino.diff added

removes the isRhino workaround, tests pass with Rhino 1.7

comment:10 Changed 10 years ago by Adam Peller

Cc: phiggins James Burke added
Milestone: 1.21.5

comment:11 Changed 10 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 10 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 10 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 8 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 8 years ago by bill

Component: GeneralTestFramework

comment:17 Changed 8 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.