961969 - crash in Interpret in HP's online catalog

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Logan Rosen [:Logan]]

This bug was filed from the Socorro interface and is 
report bp-41eadf87-0036-46a8-b5e4-2edac2140121.
=============================================================

This is occurring for me every time I load this page:
http://www.shopping.hp.com/en_US/home-office/-/search-SimpleOfferSearch?PageSize=15&search=&SearchTerm=sevenpcs

Running the 2014-01-20 build of Firefox Nightly. I'll try to find a regression range when I get the time unless someone else gets to it first.

Comment 1

[Logan Rosen [:Logan]]

Last good revision: 324e2cba1029
First bad revision: d2eca1d56402
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=324e2cba1029&tochange=d2eca1d56402

Comment 2

[Logan Rosen [:Logan]]

Better yet:
Last good revision: 357883f2205b
First bad revision: d2eca1d56402
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=357883f2205b&tochange=d2eca1d56402

Comment 3

[Andy Wingo [:wingo]]

0x00007ffff346b0e9 in JSObject::setLastProperty (cx=cx@entry=0x7fffc75adca0, obj=obj@entry=(JSObject * const) 0x7fffbd60a440 [object Call] delegate, shape=shape@entry=0x7fffd4c1c678)
    at /hack/mozilla-central/js/src/../../js/src/jsobj.cpp:2432
2432	    JS_ASSERT(shape->numFixedSlots() == obj->numFixedSlots());
(gdb) bt
#0  0x00007ffff346b0e9 in JSObject::setLastProperty (cx=cx@entry=0x7fffc75adca0, obj=obj@entry=(JSObject * const) 0x7fffbd60a440 [object Call] delegate, shape=shape@entry=0x7fffd4c1c678)
    at /hack/mozilla-central/js/src/../../js/src/jsobj.cpp:2432
#1  0x00007ffff3591b67 in JSObject::getChildProperty (cx=cx@entry=0x7fffc75adca0, obj=obj@entry=(JSObject * const) 0x7fffbd60a440 [object Call] delegate, parent=..., parent@entry=0x7fffd4c1c650, child=...)
    at /hack/mozilla-central/js/src/../../js/src/vm/Shape.cpp:407
#2  0x00007ffff35b8591 in getOrLookupChildProperty<(js::ExecutionMode)0> (child=..., parent=..., obj=..., cx=0x7fffc75adca0) at /hack/mozilla-central/js/src/../../js/src/jsobj.h:876
#3  JSObject::addPropertyInternal<(js::ExecutionMode)0> (cx=cx@entry=0x7fffc75adca0, obj=obj@entry=(JSObject * const) 0x7fffbd60a440 [object Call] delegate, id=..., id@entry=$jsid("i"), getter=0x0, setter=0x0, 
    slot=16777215, attrs=5, flags=flags@entry=0, shortid=shortid@entry=0, spp=0x0, allowDictionary=allowDictionary@entry=true) at /hack/mozilla-central/js/src/../../js/src/vm/Shape.cpp:638
#4  0x00007ffff35b9413 in JSObject::putProperty<(js::ExecutionMode)0> (cx=cx@entry=0x7fffc75adca0, obj=obj@entry=(JSObject * const) 0x7fffbd60a440 [object Call] delegate, id=id@entry=$jsid("i"), getter=0x0, 
    setter=0x0, slot=slot@entry=16777215, attrs=attrs@entry=5, flags=flags@entry=0, shortid=shortid@entry=0) at /hack/mozilla-central/js/src/../../js/src/vm/Shape.cpp:821
#5  0x00007ffff3465fd9 in DefinePropertyOrElement<(js::ExecutionMode)0> (cx=0x7fffc75adca0, obj=(JSObject * const) 0x7fffbd60a440 [object Call] delegate, id=$jsid("i"), getter=
    0x7ffff33b8560 <JS_PropertyStub(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>)>, 
    setter=0x7ffff33b8570 <JS_StrictPropertyStub(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, bool, JS::MutableHandle<JS::Value>)>, attrs=5, flags=0, shortid=0, value=
    $jsval((JSObject *) 0x7fffbd60a4c0 [object Function "i"]), callSetterAfterwards=false, setterIsStrict=false) at /hack/mozilla-central/js/src/../../js/src/jsobj.cpp:3516
#6  0x00007ffff3466a34 in js::DefineNativeProperty (cx=cx@entry=0x7fffc75adca0, obj=obj@entry=(JSObject * const) 0x7fffbd60a440 [object Call] delegate, id=..., 
    value=value@entry=$jsval((JSObject *) 0x7fffbd60a4c0 [object Function "i"]), 
    getter=getter@entry=0x7ffff33b8560 <JS_PropertyStub(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>)>, 
    setter=setter@entry=0x7ffff33b8570 <JS_StrictPropertyStub(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, bool, JS::MutableHandle<JS::Value>)>, attrs=attrs@entry=5, flags=flags@entry=0, 
    shortid=shortid@entry=0, defineHow=defineHow@entry=0) at /hack/mozilla-central/js/src/../../js/src/jsobj.cpp:3626
#7  0x00007ffff346710c in DefineGeneric (attrs=5, setter=0x7ffff33b8570 <JS_StrictPropertyStub(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, bool, JS::MutableHandle<JS::Value>)>, 
    getter=0x7ffff33b8560 <JS_PropertyStub(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>)>, value=..., id=..., obj=..., cx=0x7fffc75adca0)
    at /hack/mozilla-central/js/src/../../js/src/jsobj.cpp:3247
#8  JSObject::defineGeneric (cx=0x7fffc75adca0, obj=(JSObject * const) 0x7fffbd60a440 [object Call] delegate, id=..., value=$jsval((JSObject *) 0x7fffbd60a4c0 [object Function "i"]), 
    getter=0x7ffff33b8560 <JS_PropertyStub(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>)>, 
    setter=0x7ffff33b8570 <JS_StrictPropertyStub(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, bool, JS::MutableHandle<JS::Value>)>, attrs=5) at /hack/mozilla-central/js/src/../../js/src/jsobj.cpp:3262
#9  0x00007ffff3467399 in JSObject::defineProperty (cx=cx@entry=0x7fffc75adca0, obj=obj@entry=(JSObject * const) 0x7fffbd60a440 [object Call] delegate, name=<optimized out>, 
    value=value@entry=$jsval((JSObject *) 0x7fffbd60a4c0 [object Function "i"]), getter=0x7ffff33b8560 <JS_PropertyStub(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>)>, 
    setter=0x7ffff33b8570 <JS_StrictPropertyStub(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, bool, JS::MutableHandle<JS::Value>)>, attrs=attrs@entry=5)
    at /hack/mozilla-central/js/src/../../js/src/jsobj.cpp:3271
#10 0x00007ffff35206ac in js::DefFunOperation (cx=cx@entry=0x7fffc75adca0, script=..., script@entry=0x7fffbd609768, scopeChain=..., funArg=..., 
    funArg@entry=(JSFunction * const) 0x7fffd4c1fec0 [object Function "i"]) at /hack/mozilla-central/js/src/../../js/src/vm/Interpreter.cpp:3644
#11 0x00007ffff3536b63 in Interpret (cx=cx@entry=0x7fffc75adca0, state=...) at /hack/mozilla-central/js/src/../../js/src/vm/Interpreter.cpp:2979
#12 0x00007ffff353e49a in js::RunScript (cx=cx@entry=0x7fffc75adca0, state=...) at /hack/mozilla-central/js/src/../../js/src/vm/Interpreter.cpp:421
#13 0x00007ffff353e94c in js::Invoke (cx=cx@entry=0x7fffc75adca0, args=..., construct=construct@entry=js::NO_CONSTRUCT) at /hack/mozilla-central/js/src/../../js/src/vm/Interpreter.cpp:483
#14 0x00007ffff340e391 in js_fun_call (cx=0x7fffc75adca0, argc=0, vp=0x7fffdfc176d8) at /hack/mozilla-central/js/src/../../js/src/jsfun.cpp:910
#15 0x00007ffff3550452 in js::CallJSNative (cx=0x7fffc75adca0, native=0x7ffff340e0f0 <js_fun_call(JSContext*, unsigned int, JS::Value*)>, args=...)
    at /hack/mozilla-central/js/src/../../js/src/jscntxtinlines.h:220
#16 0x00007ffff353ea27 in js::Invoke (cx=cx@entry=0x7fffc75adca0, args=..., construct=construct@entry=js::NO_CONSTRUCT) at /hack/mozilla-central/js/src/../../js/src/vm/Interpreter.cpp:464
#17 0x00007ffff3530b0b in Interpret (cx=cx@entry=0x7fffc75adca0, state=...) at /hack/mozilla-central/js/src/../../js/src/vm/Interpreter.cpp:2609
#18 0x00007ffff353e49a in js::RunScript (cx=cx@entry=0x7fffc75adca0, state=...) at /hack/mozilla-central/js/src/../../js/src/vm/Interpreter.cpp:421
#19 0x00007ffff353e94c in js::Invoke (cx=cx@entry=0x7fffc75adca0, args=..., construct=construct@entry=js::NO_CONSTRUCT) at /hack/mozilla-central/js/src/../../js/src/vm/Interpreter.cpp:483
#20 0x00007ffff341155b in js_fun_apply (cx=0x7fffc75adca0, argc=<optimized out>, vp=0x7fffffff9a08) at /hack/mozilla-central/js/src/../../js/src/jsfun.cpp:1064
#21 0x00007ffff3550452 in js::CallJSNative (cx=0x7fffc75adca0, native=0x7ffff3411040 <js_fun_apply(JSContext*, unsigned int, JS::Value*)>, args=...)
    at /hack/mozilla-central/js/src/../../js/src/jscntxtinlines.h:220
#22 0x00007ffff353ea27 in js::Invoke (cx=cx@entry=0x7fffc75adca0, args=..., construct=construct@entry=js::NO_CONSTRUCT) at /hack/mozilla-central/js/src/../../js/src/vm/Interpreter.cpp:464
#23 0x00007ffff354151b in js::Invoke (cx=cx@entry=0x7fffc75adca0, thisv=..., fval=..., argc=argc@entry=2, argv=argv@entry=0x7fffffff9dd0, rval=JSVAL_VOID)
    at /hack/mozilla-central/js/src/../../js/src/vm/Interpreter.cpp:520
#24 0x00007ffff323e866 in js::jit::DoCallFallback (cx=0x7fffc75adca0, frame=0x7fffffff9e40, stub=0x7fffdde26258, argc=2, vp=0x7fffffff9dc0, res=JSVAL_VOID)
    at /hack/mozilla-central/js/src/../../js/src/jit/BaselineIC.cpp:8097
#25 0x00007fffe6e22383 in ?? ()
#26 0x00007fffbfad71f0 in ?? ()
#27 0x00007fffffff9d78 in ?? ()
#28 0x0000000000000000 in ?? ()

Comment 4

[Andy Wingo [:wingo]]

Reverting this change seems to fix the issue: https://hg.mozilla.org/mozilla-central/rev/e046db9b732a

Specifically the second part about not setting a shortid for the stackshape binding.  See https://bugzilla.mozilla.org/show_bug.cgi?id=916612#c25 for some comments.  Leaving to Luke to figure out what the deal is :)

Comment 5

[Luke Wagner [:luke]]

Ah, this is bizarre: the shortid is included in the hash/match of the shape but the numFixedSlots is not.  Taking out the unique shortid causes an incorrect match between two shapes with the same lineage but different numFixedSlots.  The simplest testcase to reproduce this is:

(()=>{ var x,y,z; ()=>{x++;y++;z++} })();
(()=>{ var x,y; if (true) { function z() { x++;y++ } } })();

The first function will put 'z' in the fixed slots (since the Shape is computed at compile time).  The second function will put 'z' in the dynamic slots (because of the whole dynamic block-scoped-function quirk).  The call objects of the two functions are the same in every respect except for numFixedSlots, so it asserts.

Of course, this is total clownshoes and I'm not convinced the same thing can't happen with objects.  I think the fix is to just include numFixed in the hash/match.

Comment 6

[Nathan Froyd [:froydnj]]

Whatever fixes this bug is likely to address the crash spike from bug 962141, as well, perhaps?

Comment 7

[Luke Wagner [:luke]]

Seems quite likely.

Comment 8

[Luke Wagner [:luke]]

Attachment: fix-shape

Ideally bhackett would review this, but he's out on pto.  Could you look at this instead Bill?

Comment 9

[Luke Wagner [:luke]]

I should add that, as an intermediate step, I kept the 'nfixed' argument to getChild, asserted (child.numFixedSlots() == nfixed) in getChild, and ran all the jit-tests.  I also audited each caller of getChild to ensure that nfixed originated from the same value as child.numFixedSlots().

Comment 12

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8363086 [details] [diff] [review]
fix-shape

Review of attachment 8363086 [details] [diff] [review]:
-----------------------------------------------------------------

I'm worried that there's still a problem here, although I'm still unsure of all the right invariants. It seems like it would be nice if all shapes in a lineage had the same value for numFixedSlots. As far as I can tell, we mostly respect that invariant. If this invariant were true, then we wouldn't really need to put numFixedSlots in the hash: getInitialShape already keys on the number of fixed slots, and everything after that wouldn't need to worry.

The real trouble is that Bindings::initWithTemporaryStorage violates this invariant. It calls getInitialShape, asking for two slots. But then it calls getChildBinding, which assumes more fixed slots. That doesn't seem right to me. However, maybe this is just a case where call objects are an exception to the general object invariant. If that's the case, can you please explain the details?

Comment 13

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8363086 [details] [diff] [review]
fix-shape

Talked to Luke. We'll do this now and maybe some cleanups later.

Comment 14

[Luke Wagner [:luke]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dda233b6f28e

Comment 15

[Luke Wagner [:luke]]

Attachment: fix-shape-2

Bill's suggestion (enforcing the entire shape lineage has the same nfixed) turns out to be quite simple (once the CallObject case was fixed) and better fix to the underlying bug overall.

Comment 16

[Luke Wagner [:luke]]

Attachment: fix-shape-2

qref'd

Comment 17

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8363233 [details] [diff] [review]
fix-shape-2

Review of attachment 8363233 [details] [diff] [review]:
-----------------------------------------------------------------

Fantastic. Thanks very much.

Comment 19

[Luke Wagner [:luke]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/363b31e32272

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/dda233b6f28e

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/363b31e32272

Comment 22

[Benjamin Kerensa [:bkerensa]]

Since we will not be tracking the duplicate of this bug please do nominate if you feel this needs to be tracked. Thanks

Comment 23

[Luke Wagner [:luke]]

I think we're fine.

Comment 25

[Paul Silaghi, QA [:pauly]]

Reproduced in nightly 2014-01-20.
Verified fixed in nightly 29.0a1(2014-01-27), mac os x 10.8.5

Comment 26

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Luke, is the tinyid thing sufficient to prevent this from ever happening in older versions?