645184 - Amazon Product option picker does not work

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Mehdi Mulani [:mmm] (I don't check this)]

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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Mehdi, are you willing to hunt down a regression range?

Comment 2

[Mehdi Mulani [:mmm] (I don't check this)]

Attachment: Reduced testcase.

This fails on "i = 18" for me, passes in Chrome.

Will be using this to find a regression window. :)

Comment 3

[Mehdi Mulani [:mmm] (I don't check this)]

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..

Comment 4

[Boris Zbarsky [:bzbarsky]]

Yeah, I can confirm that this fails with -m but passes in interp and -j

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: JS shell testcase

Comment 6

[Alice0775 White]

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)

Comment 8

[David Mandelin [:dmandelin]]

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?

Comment 9

[Jeff Walden [:Waldo]]

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.

Comment 10

[David Mandelin [:dmandelin]]

Attachment: Patch

Comment 11

[Nicholas Nethercote [inactive]]

I've got deja vu for bug 596490.  cdleary, you had a partial patch for it, do you have time to revive it?

Comment 12

[David Mandelin [:dmandelin]]

http://hg.mozilla.org/tracemonkey/rev/76d04b5e5e75

Comment 13

[Robert Sayre]

This should go into 4.0.1.

Comment 14

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/76d04b5e5e75

Comment 15

[Daniel Veditz [:dveditz]]

Comment on attachment 522825 [details] [diff] [review]
Patch

Approved for the mozilla2.0 repository, a=dveditz for release-drivers

Comment 16

[David Mandelin [:dmandelin]]

http://hg.mozilla.org/releases/mozilla-2.0/rev/6f8b3c2af1b1

Comment 17

[(no longer active)]

(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.

Comment 18

[David Mandelin [:dmandelin]]

(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?)

Comment 19

[Boris Zbarsky [:bzbarsky]]

Probably the default branch of the mozilla-2.0 repo.  Not sure about documentation...

Comment 20

[(no longer active)]

(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.

Comment 21

[Nick Thomas [:nthomas] (UTC+12)]

Ehsan has that exactly right.

Comment 22

[Brendan Eich [:brendan]]

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

Comment 23

[Jeff Walden [:Waldo]]

There actually was an intelligible rule?  News to me!

Comment 24

[David Mandelin [:dmandelin]]

Bad landing backed out:

  http://hg.mozilla.org/releases/mozilla-2.0/rev/f1af289dad81

Relanded: 

  http://hg.mozilla.org/releases/mozilla-2.0/rev/d6ec21415d96