Closed Bug 961969 Opened 6 years ago Closed 6 years ago

crash in Interpret in HP's online catalog

Categories

(Core :: JavaScript Engine, defect, critical)

All
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla29

People

(Reporter: Logan, Assigned: luke)

References

(Blocks 1 open bug, )

Details

(Keywords: crash)

Crash Data

Attachments

(1 file, 2 obsolete files)

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.
Last good revision: 324e2cba1029
First bad revision: d2eca1d56402
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=324e2cba1029&tochange=d2eca1d56402
Better yet:
Last good revision: 357883f2205b
First bad revision: d2eca1d56402
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=357883f2205b&tochange=d2eca1d56402
Blocks: 916612
Flags: needinfo?(luke)
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 ?? ()
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 :)
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.
Flags: needinfo?(luke)
Whatever fixes this bug is likely to address the crash spike from bug 962141, as well, perhaps?
Seems quite likely.
Attached patch fix-shape (obsolete) — Splinter Review
Ideally bhackett would review this, but he's out on pto.  Could you look at this instead Bill?
Assignee: nobody → luke
Status: NEW → ASSIGNED
Attachment #8363086 - Flags: review?(wmccloskey)
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().
Duplicate of this bug: 961998
Duplicate of this bug: 962063
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 on attachment 8363086 [details] [diff] [review]
fix-shape

Talked to Luke. We'll do this now and maybe some cleanups later.
Attachment #8363086 - Flags: review?(wmccloskey) → review+
Attached patch fix-shape-2 (obsolete) — Splinter Review
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.
Attachment #8363086 - Attachment is obsolete: true
Attachment #8363230 - Flags: review?(wmccloskey)
Attached patch fix-shape-2Splinter Review
qref'd
Attachment #8363230 - Attachment is obsolete: true
Attachment #8363230 - Flags: review?(wmccloskey)
Attachment #8363233 - Flags: review?(wmccloskey)
Comment on attachment 8363233 [details] [diff] [review]
fix-shape-2

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

Fantastic. Thanks very much.
Attachment #8363233 - Flags: review?(wmccloskey) → review+
Duplicate of this bug: 961466
https://hg.mozilla.org/mozilla-central/rev/dda233b6f28e
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Since we will not be tracking the duplicate of this bug please do nominate if you feel this needs to be tracked. Thanks
Flags: needinfo?(luke)
I think we're fine.
Flags: needinfo?(luke)
Duplicate of this bug: 962141
Reproduced in nightly 2014-01-20.
Verified fixed in nightly 29.0a1(2014-01-27), mac os x 10.8.5
Status: RESOLVED → VERIFIED
Luke, is the tinyid thing sufficient to prevent this from ever happening in older versions?
Duplicate of this bug: 962730
Duplicate of this bug: 961500
Duplicate of this bug: 961788
Depends on: 1055818
You need to log in before you can comment on or make changes to this bug.