Closed Bug 645184 Opened 13 years ago Closed 13 years ago

Amazon Product option picker does not work

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla5
Tracking Status
blocking2.0 --- Macaw+
status2.0 --- .1-fixed

People

(Reporter: mmm, Assigned: dmandelin)

References

()

Details

(Keywords: regression, Whiteboard: fixed-in-tracemonkey)

Attachments

(3 files)

At the URL (iPad 2), the option picker (Capacity, Connectivity and Color) does not work. Clicking on any of the options does not evoke an action which is does in Firefox 3.6 along with other browsers (Chrome, etc.).

The Error Console shows:
Error: parentNode['-1'] is undefined
Source File: http://z-ecx.images-amazon.com/images/G/01/twister/beta/twister-dpf.f04748f12b0f1d52db496934b7d3542d._V1_.js
Line: 12

and apparently changing line 11 of the twister file from:

if(depth==(oVarKeys.length-1)){if(parentNode['-1']==null){parentNode['-1']=new Array();}

to

if(depth==(oVarKeys.length-1)){if(parentNode['-1']==null){parentNode['-1']=new Array();} if(parentNode['-1']==null){parentNode['-1']=new Array();}

solves the problem. Though I have not been able to actually test this.
Mehdi, are you willing to hunt down a regression range?
Attached file Reduced testcase.
This fails on "i = 18" for me, passes in Chrome.

Will be using this to find a regression window. :)
In beta 6, the test fails with "i = 2". Not in beta 7, tracked it down to:
20100911030943 73ab2c3c5ad9  PASS
20100912030939 cd3c926a7413  i = 2

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=73ab2c3c5ad9&tochange=cd3c926a7413

Jaegermonkey landed then, *sigh* should've checked those nightlies first..
Yeah, I can confirm that this fails with -m but passes in interp and -j
FYI

Regression 1 window(TM nightly),
Works:
http://hg.mozilla.org/tracemonkey/rev/9ce0b72525ba
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b6pre) Gecko/20100908 Firefox/4.0b6pre ID:20100908044322
Fails(FAIL: i = 2):
http://hg.mozilla.org/tracemonkey/rev/f5f47e8fbc15
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b6pre) Gecko/20100909 Firefox/4.0b6pre ID:20100909043955
Pushlog:
http://hg.mozilla.org/tracemonkey/pushloghtml?fromchange=9ce0b72525ba&tochange=f5f47e8fbc15

In local build,
build from 9a8b156c7396 : Fails(FAIL: i = 2):
build from a59676d2e7b5 : works
Triggered by:
9a8b156c7396	Brian Hackett — PIC for addprop, bug 561506. r=dmandelin * * * Bug 561506, add context owner checks for addprop. no_r=7:00am

Regression 2 window(cached TM hourly),
Fails(FAIL: i = 2):
http://hg.mozilla.org/tracemonkey/rev/bf89669b34cb
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b12pre) Gecko/20110211 Firefox/4.0b12pre ID:20110211142808
Fails(FAIL: i = 18):
http://hg.mozilla.org/tracemonkey/rev/f569d49576bb
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b12pre) Gecko/20110211 Firefox/4.0b12pre ID:20110211163527
Pushlog:
http://hg.mozilla.org/tracemonkey/pushloghtml?fromchange=bf89669b34cb&tochange=f569d49576bb
Triggered by:
f569d49576bb	Bill McCloskey — Bug 631951 - Shrink methodjit memory usage by interpreting a few times before compiling (r=dvander)
Thanks for the reduced test cases! Diagnosis was pretty easy given those.

The cause: In the method jit, we create an addprop PIC stub for |obj["-1"] = new Array|. In the process of creating the stub, we make this call:

            const Shape *shape =
                obj->putProperty(cx, id, getter, clasp->setProperty,
                                 SHAPE_INVALID_SLOT, JSPROP_ENUMERATE, flags, 0);

The intent is to create the property, so that we see what the new shape should be after that property is added. The problem is that this creates a property with an internal id (the jsid) of |"-1"|, a string. But everywhere else, the engine converts such a property id to |-1|, an integer. So, when we do the lookup later, we look for |-1|, and don't find it. This bug is fixed by adding this line of code above the putProperty call:

            id = js_CheckForStringIndex(id);

But I wonder if that's really the right thing to do. putProperty seems like an internalish API that maybe we shouldn't be using here. Maybe we should be using setProperty? I think that's what we did before. It's a bit tricky to do the setProperty in the middle of PIC generation, but if that's the right way to do it, we can certainly do that. Any advice from the property/addprop experts?
I don't believe there's any logic to js_CheckForStringIndex sprinkling and non-sprinkling right now, so, um...yeah.  jsid should really and always be canonical, there should be some other type for maybe-not-canonical, and the C++ type system should enforce the difference.  So I'm inclined to say just do whatever happens to fix it, and we should fix the general problem without delay in a new bug.
Attached patch PatchSplinter Review
Assignee: general → dmandelin
Status: NEW → ASSIGNED
Attachment #522825 - Flags: review?(dvander)
Attachment #522825 - Flags: review?(dvander) → review+
I've got deja vu for bug 596490.  cdleary, you had a partial patch for it, do you have time to revive it?
blocking-fx: --- → ?
http://hg.mozilla.org/tracemonkey/rev/76d04b5e5e75
Whiteboard: fixed-in-tracemonkey
blocking2.0: --- → .x+
This should go into 4.0.1.
Attachment #522825 - Flags: approval2.0?
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
blocking2.0: .x+ → Macaw
status2.0: --- → wanted
Comment on attachment 522825 [details] [diff] [review]
Patch

Approved for the mozilla2.0 repository, a=dveditz for release-drivers
Attachment #522825 - Flags: approval2.0? → approval2.0+
(In reply to comment #16)
> http://hg.mozilla.org/releases/mozilla-2.0/rev/6f8b3c2af1b1

This was landed on the relbranch by mistake, I think.
(In reply to comment #17)
> (In reply to comment #16)
> > http://hg.mozilla.org/releases/mozilla-2.0/rev/6f8b3c2af1b1
> 
> This was landed on the relbranch by mistake, I think.

What is the correct branch to land it on? (And where is that documented?)
Probably the default branch of the mozilla-2.0 repo.  Not sure about documentation...
Target Milestone: --- → mozilla2.2
Version: unspecified → Trunk
(In reply to comment #19)
> Probably the default branch of the mozilla-2.0 repo.  Not sure about
> documentation...

Yes, the default branch.  Generally, all check-ins should go on the default branch except if explicitly asked by release drivers.

And fwiw, the right thing to do would be to back out your patch on the relbranch, just in case we need to do a quick chemspill release off of it.
Ehsan has that exactly right.
Re: comment 9, there certainly is a rule for calling js_CheckForStringIndex, from the pre-JIT days: JSObjectOps (now js::ObjectOps) entry points such as getProperty, setProperty, etc. must check for a string-encoded index id. The lower-level (formerly JSScope, not JSObject, method or C poor-man's-method) APIs do not need to check. They should have asserted that the id args they got were all canonical, though -- d'oh.

This rule simply wasn't followed as the JITs started using the lower-level internal APIs. The patch is the right spot fix.

Bug 596490 is well worth fixing as njn's comment suggests.

/be
There actually was an intelligible rule?  News to me!
blocking-fx: ? → ---
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: