x = [] Object.defineProperty(x, 7, {}) function f() { "use strict" x.length = { valueOf: function() { return 7 } } } for (var m = 0; m < 9; m++) { try { f() } catch (e) {} } crashes js debug and opt shell on m-c changeset fecf1afb0830 with --fuzzing-safe --no-threads --ion-eager at JSObject::getGroup with js::jit::SetPropertyIC::update on the stack. Opt configure options: CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 sh ~/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --disable-debug --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests python -u ~/fuzzing/js/compileShell.py -b "--disable-debug --enable-more-deterministic --enable-nspr-build" -r fecf1afb0830 Debug configure options: CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 sh ~/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-debug --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests python -u ~/fuzzing/js/compileShell.py -b "--enable-debug --enable-more-deterministic --enable-nspr-build" -r fecf1afb0830 In an opt build: (lldb) dis -p -> 0x10031dc87: movq (%rcx), %rax 0x10031dc8a: testb $0x4, 0x18(%rax) 0x10031dc8e: je 0x10031dcf0 0x10031dc90: movq %rcx, 0x148(%rsp) (lldb) register read $rcx rcx = 0x00000000000bfed0 (lldb) register read $rax rax = 0x00007fff5fbfec80 (lldb) Not sure what's going on here, but weird memory addresses seem to be accessed here, so setting s-s and assuming sec-critical as a start. Is $rcx close enough to null, though? Due to skipped revisions, the first bad revision could be any of: changeset: https://hg.mozilla.org/mozilla-central/rev/da286f0f7a49 user: Jason Orendorff date: Thu Jan 29 23:02:26 2015 -0600 summary: Bug 1113369, part 2 - js::SetArrayLength ObjectOpResult support. r=Waldo. changeset: https://hg.mozilla.org/mozilla-central/rev/2b18c04de86c user: Jason Orendorff date: Fri Jan 30 11:37:07 2015 -0600 summary: Bug 1113369, part 3 - [[DefineOwnProperty]] ObjectOpResult support. r=Waldo, r=bz in dom, r=dvander in js/ipc, r=bholley in js/xpconnect. changeset: https://hg.mozilla.org/mozilla-central/rev/0712a3d4b79c user: Jason Orendorff date: Tue Feb 03 19:51:40 2015 -0600 summary: Bug 1113369, part 4 - [[Set]] ObjectOpResult support. r=Waldo, r=bz in dom, r=dvander in js/ipc, r=bholley in js/xpconnect. changeset: https://hg.mozilla.org/mozilla-central/rev/35f7c0795116 user: Jason Orendorff date: Wed Feb 04 10:20:04 2015 -0600 summary: Bug 1113369, part 5 - [[Delete]] ObjectOpResult support. r=Waldo, r=bz in dom, r=dvander in js/ipc, r=bholley in js/xpconnect. changeset: https://hg.mozilla.org/mozilla-central/rev/e85721e91692 user: Jason Orendorff date: Wed Feb 04 12:01:01 2015 -0600 summary: Bug 1113369, part 6 - [[PreventExtensions]] ObjectOpResult support. r=Waldo, r=bz in dom, r=dvander in js/ipc, r=bholley in js/xpconnect. changeset: https://hg.mozilla.org/mozilla-central/rev/41df9affe00f user: Jason Orendorff date: Thu Feb 05 16:36:50 2015 -0600 summary: Bug 1113369, part 7 - [[SetPrototypeOf]] ObjectOpResult support. r=Waldo, r=bz in dom, r=dvander in js/ipc, r=bholley in js/xpconnect. changeset: https://hg.mozilla.org/mozilla-central/rev/e9c646b6392f user: Monica Chew date: Thu Mar 05 10:54:23 2015 -0800 summary: Bug 1090754: Re-enable remote-lookups in release builds (r=sworkman,francois) changeset: https://hg.mozilla.org/mozilla-central/rev/cf124d9e2441 user: Monica Chew date: Thu Mar 05 11:55:03 2015 -0800 summary: Bug 1134954: Disable Safe Browsing in safe mode (r=francois,sworkman) changeset: https://hg.mozilla.org/mozilla-central/rev/94a0946d5a0a user: Boris Zbarsky date: Fri Mar 06 16:33:30 2015 -0500 summary: Bug 1131802 part 1. Add JS friend API to test whether a given function object has extended slots. r=terrence changeset: https://hg.mozilla.org/mozilla-central/rev/fdb146bc4052 user: Boris Zbarsky date: Fri Mar 06 16:33:31 2015 -0500 summary: Bug 1131802 part 2. Allocate functions with reserved slots for DOM Xrays so we can store the Xray wrapper reference in those slots instead of as the function parent. r=peterv changeset: https://hg.mozilla.org/mozilla-central/rev/41846743ab88 user: Boris Zbarsky date: Fri Mar 06 16:33:31 2015 -0500 summary: Bug 1131805 part 2. Remove remaining js::GetObjectParent and JS_GetParent uses in SpiderMonkey. r=waldo changeset: https://hg.mozilla.org/mozilla-central/rev/62fecc6ab96e user: Jason Orendorff date: Fri Mar 06 20:24:39 2015 -0600 summary: Fix JSErr_Limit, broken by rev 41df9affe00f. No bug, r=red. Jason, is bug 1131805 a likely regressor?

(lldb) bt * thread #1: tid = 0x171b15, 0x000000010031dc87 js-64-dm-nsprBuild-darwin-fecf1afb0830`js::jit::SetPropertyIC::update(JSContext*, JS::Handle<JSScript*>, unsigned long, JS::Handle<JSObject*>, JS::Handle<JS::Value>) [inlined] JSObject::hasLazyGroup(this=0x00000000000bfed0) const at jsobj.h:164, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0xbfed0) * frame #0: 0x000000010031dc87 js-64-dm-nsprBuild-darwin-fecf1afb0830`js::jit::SetPropertyIC::update(JSContext*, JS::Handle<JSScript*>, unsigned long, JS::Handle<JSObject*>, JS::Handle<JS::Value>) [inlined] JSObject::hasLazyGroup(this=0x00000000000bfed0) const at jsobj.h:164 frame #1: 0x000000010031dc87 js-64-dm-nsprBuild-darwin-fecf1afb0830`js::jit::SetPropertyIC::update(JSContext*, JS::Handle<JSScript*>, unsigned long, JS::Handle<JSObject*>, JS::Handle<JS::Value>) [inlined] JSObject::getGroup(this=0x00000000000bfed0, cx=<unavailable>) at jsobjinlines.h:111 frame #2: 0x000000010031dc87 js-64-dm-nsprBuild-darwin-fecf1afb0830`js::jit::SetPropertyIC::update(cx=0x0000000101617460, cacheIndex=<unavailable>, outerScript=<unavailable>, obj=<unavailable>, value=<unavailable>) + 263 at IonCaches.cpp:3034 frame #3: 0x00000001017d5cbe (lldb)

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #0) > Jason, is bug 1131805 a likely regressor? Erm, I meant bug 1113369.

(lldb) bt * thread #1: tid = 0x1715bb, 0x000000010038411f js-dbg-64-dm-nsprBuild-darwin-fecf1afb0830`JSObject::getGroup(JSContext*) [inlined] js::ExclusiveContext::compartment(this=0x00000000000d2a60) const + 60 at jsobj.h:168, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0xd2a60) * frame #0: 0x000000010038411f js-dbg-64-dm-nsprBuild-darwin-fecf1afb0830`JSObject::getGroup(JSContext*) [inlined] js::ExclusiveContext::compartment(this=0x00000000000d2a60) const + 60 at jsobj.h:168 frame #1: 0x00000001003840e3 js-dbg-64-dm-nsprBuild-darwin-fecf1afb0830`JSObject::getGroup(this=0x00000000000d2a60, cx=0x0000000101e02590) + 19 at jsobjinlines.h:110 frame #2: 0x0000000100563607 js-dbg-64-dm-nsprBuild-darwin-fecf1afb0830`js::jit::SetPropertyIC::update(cx=0x0000000101e02590, cacheIndex=<unavailable>, outerScript=<unavailable>, obj=<unavailable>, value=<unavailable>) + 263 at IonCaches.cpp:3034 frame #3: 0x0000000101dc02f9 (lldb)

I think the bug is that in the IsCacheableSetPropCallPropertyOp case the code does: EmitObjectOpResultCheck<IonOOLSetterOpExitFrameLayout>(masm, failure, strict, scratchReg, argJSContextReg, argObjReg, argIdReg, argVpReg, argResultReg); that second arg should be masm.exceptionLabel() instead, I believe. Otherwise we end up branching to the "failure" label, which is the "failed IC checks, generate a new stub" code, so SetPropertyIC::update. But in this case the label to pass in is the one to branch to when an exception is thrown due to strict mode, no?

Just to confirm what bz already knows (I meant to post this yesterday): The first bad revision is: changeset: 234823:0712a3d4b79c user: Jason Orendorff <jorendorff@mozilla.com> date: Tue Feb 03 19:51:40 2015 -0600 summary: Bug 1113369, part 4 - [[Set]] ObjectOpResult support. r=Waldo, r=bz in dom, r=dvander in js/ipc, r=bholley in js/xpconnect. The patch in bug 1141154 does not fix this one, though.

Comment on attachment 8575436 [details] [diff] [review] prelude - Fix amazingly bogus ObjectOpResult-related code and assertions in IonCaches, introduced by rev 0712a3d4b79c Review of attachment 8575436 [details] [diff] [review]: ----------------------------------------------------------------- Ain't stack alignment fun? r=me ::: js/src/jit/IonCaches.cpp @@ +1471,5 @@ > + // push -1 and not worry about which 32 bits are padding and which need to > + // get the value; endianness; etc. > + static_assert(ObjectOpResult::Uninitialized == uint32_t(-1), > + "Pushing -1 must match ObjectOpResult::Uninitialized (see comment)"); > + masm.Push(ImmWord(used ? uintptr_t(-1) : 0)); I...guess we could also get away with always pushing -1 here, since if it's not used, it's not used, right? Is there a reason to add (albeit minor) complexity here? Your call, ultimately, I just have a "keep it agressively simple" approach to jitcode. ::: js/src/jit/JitFrames.h @@ +686,5 @@ > + // Union to pad the 32-bit ObjectOpResult field to pointer size on 64-bit > + // platforms. MacroAssembler stack ops always push pointer-sized values. > + union { > + JS::ObjectOpResult result_; > + void *padResultToPtrSize_; Cute.

Comment on attachment 8575443 [details] [diff] [review] Fix crash [@ JSObject::getGroup] or [@ js::jit::SetPropertyIC::update] Review of attachment 8575443 [details] [diff] [review]: ----------------------------------------------------------------- Yeah that label was for guard failure, right?

(In reply to Eric Faust [:efaust] from comment #9) > Comment on attachment 8575436 [details] [diff] [review] > > + masm.Push(ImmWord(used ? uintptr_t(-1) : 0)); > > I...guess we could also get away with always pushing -1 here I had no good reason for the 0. Changed. > Yeah that label was for guard failure, right? Yep. Spotted by bz (comment 4), would have taken me all day to track down.

Comment on attachment 8575580 [details] [diff] [review] prelude - Make ObjectOpResult pointer-sized to fix amazingly bogus code and assertions in IonCaches, introduced by rev 0712a3d4b79c Review of attachment 8575580 [details] [diff] [review]: ----------------------------------------------------------------- Yep. r=me ::: js/public/Class.h @@ +86,5 @@ > + * they're defined in js/src/js.msg. > + * > + * code_ is uintptr_t (rather than uint32_t) for the convenience of the > + * JITs, which would otherwise have to deal with either padding or stack > + * alignment on 64-bit platforms. Thanks for this comment. @@ +99,4 @@ > > ObjectOpResult() : code_(Uninitialized) {} > > + /* Return true if succeed() was called. */ Nice catch ::: js/src/jit/IonCaches.cpp @@ +1470,2 @@ > "ObjectOpResult size must match size reserved by masm.Push() here"); > + masm.Push(ImmWord(uintptr_t(ObjectOpResult::Uninitialized))); nit: is this cast necessary? I'd be surprised, I think, if it was.

https://hg.mozilla.org/integration/mozilla-inbound/rev/83199a9b218c https://hg.mozilla.org/integration/mozilla-inbound/rev/6430a8556bb0

https://hg.mozilla.org/mozilla-central/rev/83199a9b218c https://hg.mozilla.org/mozilla-central/rev/6430a8556bb0

JSBugMon: This bug has been automatically verified fixed.