The following testcase crashes on mozilla-central revision dc89484d4b45 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug --enable-optimize, run with --fuzzing-safe --thread-count=2 --ion-offthread-compile=off --ion-eager --baseline-eager --ion-extra-checks -f test/driver-dcd.js < test/execution.log): See attachment. Backtrace: Program terminated with signal SIGSEGV, Segmentation fault. #0 0x000000000084d108 in js::DefLexicalOperation (attrs=7, name=..., varObj=..., lexicalEnv=..., cx=0x7fa827d5f000) at js/src/vm/Interpreter-inl.h:299 #1 js::jit::DefLexical (cx=0x7fa827d5f000, dn=..., attrs=7, envChain=...) at js/src/jit/VMFunctions.cpp:214 #2 0x00007fa82926e1a4 in ?? () [...] #14 0x0000000000000000 in ?? () rax 0x0 0 rbx 0x7ffd4ca95630 140725889619504 rcx 0x7fa828054a2d 140360202668589 rdx 0x0 0 rsi 0x7fa828323770 140360205612912 rdi 0x7fa828322540 140360205608256 rbp 0x7ffd4ca956e0 140725889619680 rsp 0x7ffd4ca95610 140725889619472 r8 0x7fa828323770 140360205612912 r9 0x7fa829414740 140360223377216 r10 0x0 0 r11 0x0 0 r12 0x7ffd4ca95650 140725889619536 r13 0x1da60a0 31088800 r14 0x7ffd4ca95640 140725889619520 r15 0x1da6140 31088960 rip 0x84d108 <js::jit::DefLexical(JSContext*, JS::Handle<js::PropertyName*>, unsigned int, JS::Handle<JSObject*>)+1032> => 0x84d108 <js::jit::DefLexical(JSContext*, JS::Handle<js::PropertyName*>, unsigned int, JS::Handle<JSObject*>)+1032>: movl $0x0,0x0 0x84d113 <js::jit::DefLexical(JSContext*, JS::Handle<js::PropertyName*>, unsigned int, JS::Handle<JSObject*>)+1043>: ud2 This is an assertion that I keep seeing but that is extremely difficult to isolate and provide a testcase for. The attached zip file contains a minimized log and the required driver to run it. I was not able to merge them into a single file. The testcase is intermittent but reproduces cleanly most of the time. I suggest debugging this as long as it works and reproduces so nicely. I have quite a few other crashes with the same assert that never even reproduced initially. Marking s-s due to intermittent behavior and because :shu once mentioned that this assertion is potentially dangerous.

Needinfo from :shu because this is probably related to the parser/frontend.

JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.

I ran the test a bunch of times and it never crashed with the build and shell flags. You might need to give me access to a machine where this reproduces for this.

I couldn't really reproduce this on my machine, but I think the bug is that we can bailout of Ion on global frames before the redecl check. This ends up resuming in the main body instead of the prologue because of some other constraints, and so we never do the redecl check at all. The new flag on BaselineFrame is kinda gross. Do you know of a cleaner way to check whether we bailed out during the prologue of a global frame, Jan? decoder, could you also check if this fixes the crash?

Per IRC discussion with Shu, this is probably a longer standing issue that was exposed by the rewrite. ni? him to figure that out.

Tracking 52+ since this is sec-hig - makes sense to track it.

Hm I'm not familiar with MGlobalNameConflictsCheck, but I think I understand the problem. (1) Instead of adding a flag to BaselineFrame, we can store a flag in BaselineBailoutInfo maybe? That is passed to FinishBailoutToBaseline (just make sure you read it before the js_free(bailoutInfo)..). That way it only affects the bailout code. (2) The bailoutInfo already stores the |jsbytecode* resumePC|. Is that + frame->script() enough info to figure out if we have to do this work? (3) Can we make this a separate bytecode op? I've wanted to do that for a while for all other stuff we do in the prologue (CallObject allocation, interrupt check, etc).

(In reply to Jan de Mooij [:jandem] from comment #8) > (3) Can we make this a separate bytecode op? I've wanted to do that for a > while for all other stuff we do in the prologue (CallObject allocation, > interrupt check, etc). This may be annoying for the debugger though, but that might be easier to handle than the code we have in the JITs now, not sure. The bailout is probably caused by the overrecursion check, at least that would explain why it's hard to repro. Maybe we should add a testing function to trigger random failures there (something like, after emitting x bailouts, emit one that bails unconditionally). For this bug, your patch + (1) or (2) above is probably safest.

Comment on attachment 8804951 [details] [diff] [review] Fix global redeclaration check for prologue bailouts from Ion. Clearing review per comments 8/9.

(In reply to Jan de Mooij [:jandem] from comment #8) > Hm I'm not familiar with MGlobalNameConflictsCheck, but I think I understand > the problem. > > (1) Instead of adding a flag to BaselineFrame, we can store a flag in > BaselineBailoutInfo maybe? That is passed to FinishBailoutToBaseline (just > make sure you read it before the js_free(bailoutInfo)..). That way it only > affects the bailout code. I'll do this. > > (2) The bailoutInfo already stores the |jsbytecode* resumePC|. Is that + > frame->script() enough info to figure out if we have to do this work? > No, this is the same problem as the one that has come up many times before: a pc with offset 0 pre- or mid-prologue is indistinguishable from post-prologue. Which leads us to (3)... > (3) Can we make this a separate bytecode op? I've wanted to do that for a > while for all other stuff we do in the prologue (CallObject allocation, > interrupt check, etc). Yes, this would be nice. But not doing this rearchitecting for this patch. It would make not just this case but other bailout cases (and possibly weird debug mode OSR code as well) simpler.

Reworked the patch per comment 8. decoder, since I still can't repro this reliably, could you check again if the new patch still makes the crashes go away?

Comment on attachment 8808311 [details] [diff] [review] Fix global redeclaration check for prologue bailouts from Ion. Review of attachment 8808311 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!

Tracking for 51, once this has sec approval please request uplift.

https://hg.mozilla.org/mozilla-central/rev/5174d6ae8e9c seems this landed in https://hg.mozilla.org/integration/mozilla-inbound/rev/5174d6ae8e9c without sec-approval

Why did you land this, Shu? You need to mark the sec-approval? flag on the patch and answer the sec-approval template questions so we know if you just 0day'd our users. Comment 6 says that this issue may have been "exposed by the rewrite." What version of Firefox did that land in? I need to know if Firefox 50 is exposed or not since it is marked "?" in the status flags above.

(In reply to Al Billings [:abillings] from comment #16) > Why did you land this, Shu? You need to mark the sec-approval? flag on the > patch and answer the sec-approval template questions so we know if you just > 0day'd our users. > My apologies. It did not occur to me this bug was still marked s-s. It shouldn't be. The bug is that certain JavaScript programs under rare circumstances would exhibit non-spec-compliant behavior. Specifically, global scope 'let' or 'const' declarations would be allowed to be redeclared. There's no security issue here. Christian marked this s-s based on the signature of the crash itself, before an analysis was made. > Comment 6 says that this issue may have been "exposed by the rewrite." What > version of Firefox did that land in? I need to know if Firefox 50 is exposed > or not since it is marked "?" in the status flags above. It is, but per above, this bug is not s-s and was conservatively marked s-s in the beginning.

Ah, Ok. Should we open this up? Dan?

Comment on attachment 8808311 [details] [diff] [review] Fix global redeclaration check for prologue bailouts from Ion. Fix confirmed.

Hi :shu, Is this worth uplifting to Beta51?

(In reply to Gerry Chang [:gchang] from comment #20) > Hi :shu, > Is this worth uplifting to Beta51? Given the extreme rarity of this triggering, I think no, just let the fix ride the trains as normal.