The following testcase crashes on mozilla-central revision e49151136658 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-stdcxx-compat --disable-profiling --enable-debug --enable-optimize, run with --fuzzing-safe --ion-offthread-compile=off --ion-eager): loadFile(` function f6(x,x,x,x) { var ret = []; ret.push(arguments[i]); } actual = f6(1,2,3,4); `); loadFile(` do setJitCompilerOption('ion.forceinlineCaches', 1); while ( f2.arguments ); `); loadFile(` actual = f6(1,2,3,4); `); function loadFile(lfVarx) { try { evaluate(lfVarx); } catch (lfVare) { } } Backtrace: received signal SIGSEGV, Segmentation fault. 0x000000000077507f in js::jit::MBasicBlock::discardAllResumePoints (this=this@entry=0x7ffff69c6788, discardEntry=discardEntry@entry=true) at js/src/jit/MIRGraph.cpp:1092 #0 0x000000000077507f in js::jit::MBasicBlock::discardAllResumePoints (this=this@entry=0x7ffff69c6788, discardEntry=discardEntry@entry=true) at js/src/jit/MIRGraph.cpp:1092 #1 0x00000000007a1f4f in js::jit::MBasicBlock::clear (this=0x7ffff69c6788) at js/src/jit/MIRGraph.cpp:1106 #2 js::jit::MIRGraph::removeBlock (this=this@entry=0x7ffff69c5050, block=block@entry=0x7ffff69c6788) at js/src/jit/MIRGraph.cpp:263 #3 0x00000000007b5086 in js::jit::MIRGraph::removeSuccessorBlocks (this=0x7ffff69c5050, start=0x7ffff69c56c0) at js/src/jit/MIRGraph.cpp:218 #4 0x00000000007b525f in js::jit::MBasicBlock::BackupPoint::restore (this=this@entry=0x7fffffffac40) at js/src/jit/MIRGraph.cpp:1766 #5 0x00000000006f588b in js::jit::IonBuilder::inlineScriptedCall (this=this@entry=0x7ffff69c51c0, callInfo=..., target=<optimized out>) at js/src/jit/IonBuilder.cpp:3758 #6 0x00000000006f5e27 in js::jit::IonBuilder::inlineSingleCall (this=this@entry=0x7ffff69c51c0, callInfo=..., targetArg=targetArg@entry=0x7ffff46ad940) at js/src/jit/IonBuilder.cpp:4242 #7 0x00000000006f76bb in js::jit::IonBuilder::inlineCallsite (this=this@entry=0x7ffff69c51c0, targets=..., callInfo=...) at js/src/jit/IonBuilder.cpp:4296 #8 0x00000000006f7b5e in js::jit::IonBuilder::jsop_call (this=this@entry=0x7ffff69c51c0, argc=4, constructing=<optimized out>, ignoresReturnValue=<optimized out>) at js/src/jit/IonBuilder.cpp:5298 #9 0x00000000006fd2c6 in js::jit::IonBuilder::inspectOpcode (this=this@entry=0x7ffff69c51c0, op=op@entry=JSOP_CALL) at js/src/jit/IonBuilder.cpp:2035 #10 0x00000000006fe9c2 in js::jit::IonBuilder::visitBlock (this=this@entry=0x7ffff69c51c0, cfgblock=cfgblock@entry=0x7ffff4359760, mblock=<optimized out>) at js/src/jit/IonBuilder.cpp:1539 #11 0x00000000006f3d26 in js::jit::IonBuilder::traverseBytecode (this=this@entry=0x7ffff69c51c0) at js/src/jit/IonBuilder.cpp:1456 #12 0x00000000006f49ce in js::jit::IonBuilder::build (this=this@entry=0x7ffff69c51c0) at js/src/jit/IonBuilder.cpp:846 #13 0x000000000043340d in js::jit::IonCompile (cx=cx@entry=0x7ffff6924000, script=<optimized out>, baselineFrame=baselineFrame@entry=0x0, osrPc=osrPc@entry=0x0, recompile=<optimized out>, optimizationLevel=<optimized out>) at js/src/jit/Ion.cpp:2176 #14 0x000000000070795b in js::jit::Compile (cx=cx@entry=0x7ffff6924000, script=script@entry=..., osrFrame=osrFrame@entry=0x0, osrPc=osrPc@entry=0x0, forceRecompile=forceRecompile@entry=false) at js/src/jit/Ion.cpp:2440 #15 0x0000000000707af8 in js::jit::CanEnter (cx=cx@entry=0x7ffff6924000, state=...) at js/src/jit/Ion.cpp:2537 #16 0x0000000000536d4a in js::RunScript (cx=0x7ffff6924000, state=...) at js/src/vm/Interpreter.cpp:386 #17 0x00000000005398b9 in js::ExecuteKernel (cx=cx@entry=0x7ffff6924000, script=..., script@entry=..., envChainArg=..., newTargetValue=..., evalInFrame=..., evalInFrame@entry=..., result=result@entry=0x7fffffffc1a8) at js/src/vm/Interpreter.cpp:699 #18 0x0000000000539e10 in js::Execute (cx=cx@entry=0x7ffff6924000, script=script@entry=..., envChainArg=..., rval=rval@entry=0x7fffffffc1a8) at js/src/vm/Interpreter.cpp:732 #19 0x00000000008e01ca in ExecuteScript (cx=cx@entry=0x7ffff6924000, scope=scope@entry=..., script=script@entry=..., rval=rval@entry=0x7fffffffc1a8) at js/src/jsapi.cpp:4616 #20 0x00000000008eff88 in JS_ExecuteScript (cx=cx@entry=0x7ffff6924000, scriptArg=scriptArg@entry=..., rval=...) at js/src/jsapi.cpp:4642 #21 0x000000000045dded in Evaluate (cx=0x7ffff6924000, argc=<optimized out>, vp=<optimized out>) at js/src/shell/js.cpp:1656 #22 0x00003f9d2c232625 in ?? () #23 0x00007ffff697e140 in ?? () #24 0x00007fffffffc180 in ?? () #25 0x0000000000000000 in ?? () rax 0x0 0 rbx 0x7ffff69c6788 140737330833288 rcx 0x7ffff6c28a2d 140737333332525 rdx 0x0 0 rsi 0x7ffff6ef7770 140737336276848 rdi 0x7ffff6ef6540 140737336272192 rbp 0x7fffffffaab0 140737488333488 rsp 0x7fffffffaaa0 140737488333472 r8 0x7ffff6ef7770 140737336276848 r9 0x7ffff7fe4740 140737354024768 r10 0x58 88 r11 0x7ffff6b9f750 140737332770640 r12 0x1 1 r13 0x7ffff69c5050 140737330827344 r14 0x7ffff69c6788 140737330833288 r15 0x7fffffffab40 140737488333632 rip 0x77507f <js::jit::MBasicBlock::discardAllResumePoints(bool)+127> => 0x77507f <js::jit::MBasicBlock::discardAllResumePoints(bool)+127>: movl $0x0,0x0 0x77508a <js::jit::MBasicBlock::discardAllResumePoints(bool)+138>: ud2 Marking s-s until this has been investigated by JS developers. The assertion is in JIT code and I don't know what the implications are if something is wrong there with the inline cache.

JSBugMon: Bisection requested, result: autoBisect shows this is probably related to the following changeset: The first bad revision is: changeset: https://hg.mozilla.org/mozilla-central/rev/603e5ded7f5d user: Nicolas B. Pierron date: Mon Jun 19 16:29:51 2017 +0000 summary: Bug 1373323 - IonMonkey: Enable backtracking on inlining failures. r=jandem This iteration took 270.131 seconds to run.

Hey Nicolas, did you get a chance to look at this yet?

The problem is that the GetPropertyCache allocate a prior-resume-point, and we fail before the following call which is supposed to consume it. We should clean the maybeFallbackFunctionGetter_ field in case of failure.

This patch fixes the issue by adding MakeScopeExit clean-up code in IonBuilder::buildInline. I also added it to IonBuilder::build, such that we do not keep are resume points alive which is not attached to any basic block. (even if I would not expect that to happen, knowing that the AssertGraph* functions should have caught it.) Most of the dist from this patch comes from moving the WrapMGetPropertyCache higher in the same file, while keeping one of its function where it used to be.

Comment on attachment 8888439 [details] [diff] [review] Clear the GetPropertyCache prior resume-point at the end of IonBuilder build functions. Review of attachment 8888439 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/IonBuilder.cpp @@ +881,5 @@ > #endif > > insertRecompileCheck(); > > + auto clearLastPriorResumePoint = mozilla::MakeScopeExit([&] { self-nit: add #include "mozilla/ScopeExit.h" header.

Just to make sure I understand correctly: does this mean we can remove the replaceMaybeFallbackFunctionGetter(nullptr); calls in build() and buildInline()? What if the ScopeExit just calls builder->replaceMaybeFallbackFunctionGetter(nullptr), can we revert most of the other changes in the patch?

(In reply to Jan de Mooij [:jandem] from comment #7) > Just to make sure I understand correctly: does this mean we can remove the > replaceMaybeFallbackFunctionGetter(nullptr); calls in build() and > buildInline()? Oh, I did not even noticed it. This seems to be a bug introduced by the addition of the MOZ_TRY :/ > What if the ScopeExit just calls > builder->replaceMaybeFallbackFunctionGetter(nullptr), can we revert most of > the other changes in the patch? Yes, I will do that a submit another patch.

Delta: - Follow better suggestion.

Comment on attachment 8889439 [details] [diff] [review] Clear the GetPropertyCache prior resume-point at the end of IonBuilder build functions. Review of attachment 8889439 [details] [diff] [review]: ----------------------------------------------------------------- Nice.

Comment on attachment 8889439 [details] [diff] [review] Clear the GetPropertyCache prior resume-point at the end of IonBuilder build functions. > [Security approval request comment] > How easily could an exploit be constructed based on the patch? This is not easy. I once tried on a similar issue and failed to do so. > Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? This highlights that we did not clean-up as expected, but it does not give any hint on what pointers one might make us of. > Which older supported branches are affected by this flaw? Since Firefox 53 (Bug 1286505). > If not all supported branches, which bug introduced the flaw? Bug 1286505 added the MOZ_TRY syntax, which skip the existing code for removing the reference to the unused resume point. > Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? The same patch should apply cleanly, or it should be trivial to adapt. > How likely is this patch to cause regressions; how much testing does it need? Unexpected. No testing should be required.

Al pinged me about this one. We still have one beta before RC1 and this is a sec-high so it meets the late beta uplift bar.

Comment on attachment 8889439 [details] [diff] [review] Clear the GetPropertyCache prior resume-point at the end of IonBuilder build functions. sec-approval+ for trunk and giving Beta approval after talking to Ritu.

Backed out for making too-long-array-splice.js perma-timeout on SM asan builds (on inbound and Beta both). Note that bug 1368978 was already tracking the same failures happening intermittently. https://hg.mozilla.org/integration/mozilla-inbound/rev/e2801fbe0885c038df8c87b7501445fdb832694a https://hg.mozilla.org/releases/mozilla-beta/rev/3d3cf32c21d539abac825ddbf8bddb162053b691 https://treeherder.mozilla.org/logviewer.html#?job_id=117769158&repo=mozilla-inbound

*sigh* The failures are still happening (and looking further back at inbound, they do in fact go back to before this landed). Re-landing on inbound now, will get Beta later. Sorry for the churn, hopefully someone can look at bug 1368978 Really Soon Now before it leads to even more confusion :( https://hg.mozilla.org/integration/mozilla-inbound/rev/d5950d83bde758cb389c14ea211babb48899ae67

https://hg.mozilla.org/mozilla-central/rev/d5950d83bde7 also set a needinfo to get someone looking into bug 1368978

This time with feeling. https://hg.mozilla.org/releases/mozilla-beta/rev/1df81caed459

JSBugMon: This bug has been automatically verified fixed.