function g(f) { for (var j = 0; j < 9; ++j) { try { f() } catch (e) {} } } function h(code) { Function(code)(); } g([]); g([]); g([]); g([]); g([]); g([]); g([]); g([]); g([]); g([]); g([]); g([]); g([]); g([]); g([]); g([]); g([]); g([]); g([]); g([]); g([]); g([]); g([]); g([]); g([]); g([]); g([]); g([]); g([]); g([]); h("\ m = (function(stdlib, foreign) { \ \"use asm\";\ var ff=foreign.ff;\ function f(){\ ff(0);\ } \ return f \ })(this , { \ ff: arguments.callee.caller\ });\ g(m , []);\ "); h("\ m = undefined;\ gc();\ ") asserts js debug shell on m-c changeset 7d120481a6ae with --ion-eager --ion-parallel-compile=off at Assertion failure: exit.interpCodeOffset_, at jit/AsmJSModule.h but sometimes crashes instead, at js::AsmJSModule::detachIonCompilation s-s because gc seems to be involved. My configure flags are: CC="clang -Qunused-arguments" AR=ar CXX="clang++ -Qunused-arguments" sh ./configure --target=x86_64-apple-darwin12.5.0 --enable-optimize --enable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --enable-methodjit --enable-type-inference --disable-tests --enable-more-deterministic --with-ccache --enable-threadsafe <other NSPR options> autoBisect shows this is probably related to the following changeset: The first bad revision is: changeset: http://hg.mozilla.org/mozilla-central/rev/dbeea0e93b56 user: Brian Hackett date: Mon Dec 16 10:53:02 2013 -0800 summary: Bug 785905 - Build Ion MIR graph off thread, r=jandem. Brian, is bug 785905 a likely regressor?

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

I think the Brian's patch just tickled an existing bug. The sequence leading to the crash is: 1. create AsmJSModule 2. it gets linked to IonScript A of JSScript B 3. GC calls discardJitCode calls B->setIonScript(null) 4. AsmJSModule is destroyed. It sees that B->ion == null, so it doesn't detachDependentAsmJSModule 5. InvalidateBailout calls A->decref, which now tries to unlink itself from the destroyed AsmJSModule It seems like the best place to detach is whenever script->ion is repointed, since this is how ~AsmJSModule finds the IonScript. This removes one manual detach call. The call in Destroy should stay though, I think, because it is called when the JSScript is finalized and we don't call setIonScript() in that case.

Luke, which branches does this bug affect? (along with its severity)

FWIW we already run into a similar issues with the list of IonScript backedges threaded through the runtime, and call destroyBackedges (idempotently) both on destroying the IonScript and in InvalidateActivation.

To add to that, I think the only time that script->ion will be repointed without destroying the IonScript (aka the IonScript has references on it) is when there are invalidated frames on the stack. These are the references added in InvalidateActivation immediately after we destroyBackedges. Adding a detachDependentAsmJSModules call after the destroyBackedges() in InvalidateActivation fixes this testcase.

(In reply to Brian Hackett (:bhackett) from comment #5) Ah, good point. IIUC, then, a better fix would be to factor out both calls into a single IonScript::detach (or something if you have a better name) that gets called in Destroy/InvalidateActivation. Is that right?

Gary: I assume this is on all branches.

Comment on attachment 8350768 [details] [diff] [review] fix-detach-asmjsmodule Review of attachment 8350768 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/Ion.cpp @@ +1111,5 @@ > > void > +IonScript::unlinkFromRuntime(FreeOp *fop) > +{ > + if (dependentAsmJSModules) { Add a comment explaining why the modules may need to be detached prematurely? @@ +2435,5 @@ > > // The writes to the executable buffer below may clobber backedge > // jumps, so make sure that those backedges are unlinked from the > // runtime and not reclobbered with garbage if an interrupt is > // triggered. This comment could get moved into unlinkFromRuntime, and maybe add a different comment here explaining this call.

Thanks Luke!

Can we get a security rating here to determine if this needs to be tracked for landing on ESR branch?

Since I can't demonstrate otherwise, I expect the bug is exploitable. It seems like this bug would be difficult to hit in practice.

JSBugMon: This bug has been automatically verified fixed.

Luke, it is possible to have a backport of this fix for ESR24? Thanks

[Approval Request Comment] Fix Landed on Version: 29 Risk to taking this patch (and alternatives if risky): low

Dan can you help with a sec rating here?

This is the kind of bug for which sec-approval was invented, https://wiki.mozilla.org/Security/Bug_Approval_Process Given the chance we would have wanted this to ship with Firefox 28, or even 27 given we checked in a testcase that shows how to reproduce the bug last December. Or not check in the test until after the fix made it to release.

I have no idea what branch that patch was made against, but it wasn't esr24.

Oops, I had a rebased patch in my mq, but not qref'd.

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/1bb3352dc5fe https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/b3a495108228