The following testcase crashes on mozilla-central revision 215f96861176 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug --enable-optimize, run with --fuzzing-safe --ion-offthread-compile=off --ion-eager): var g = newGlobal(); evaluate(` function g(a) { a(); } function f(y) { for (var i = 0; i < 7; ++i) { var q; q = function() { (function * get() { class get {} }) (y); var m = 1; var z = function() { appendToActual(m); } }; g(q); } } for (var i = 0; i < 5; ++i) f(i); `); Backtrace: received signal SIGSEGV, Segmentation fault. 0x0000000000464f80 in JSFunction::hasUncompiledScript (this=<optimized out>) at js/src/jsfun.h:430 #0 0x0000000000464f80 in JSFunction::hasUncompiledScript (this=<optimized out>) at js/src/jsfun.h:430 #1 JSFunction::nonLazyScript (this=<optimized out>) at js/src/jsfun.h:435 #2 0x0000000000bb8c90 in AssertScopeMatchesEnvironment (scope=<optimized out>, originalEnv=<optimized out>) at js/src/vm/Stack.cpp:114 #3 0x0000000000bb95c3 in js::InterpreterFrame::prologue (this=0x7ffff022e020, cx=0x7ffff695f000) at js/src/vm/Stack.cpp:230 #4 0x0000000000b0437f in Interpret (cx=0x7ffff695f000, state=...) at js/src/vm/Interpreter.cpp:1776 #5 0x0000000000b11356 in js::RunScript (cx=cx@entry=0x7ffff695f000, state=...) at js/src/vm/Interpreter.cpp:404 #6 0x0000000000b11605 in js::InternalCallOrConstruct (cx=cx@entry=0x7ffff695f000, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:476 #7 0x0000000000b11836 in InternalCall (cx=cx@entry=0x7ffff695f000, args=...) at js/src/vm/Interpreter.cpp:503 #8 0x0000000000b1198e in js::Call (cx=cx@entry=0x7ffff695f000, fval=..., fval@entry=..., thisv=..., thisv@entry=..., args=..., rval=..., rval@entry=...) at js/src/vm/Interpreter.cpp:522 #9 0x000000000085db00 in js::jit::InvokeFunction (cx=<optimized out>, obj=..., constructing=<optimized out>, argc=1, argv=<optimized out>, rval=...) at js/src/jit/VMFunctions.cpp:114 #10 0x00007ffff7e4464a in ?? () #11 0x0000000000000000 in ?? () rax 0x0 0 rbx 0x1103340 17838912 rcx 0x7ffff6c28a2d 140737333332525 rdx 0x0 0 rsi 0x7ffff6ef7770 140737336276848 rdi 0x7ffff6ef6540 140737336272192 rbp 0x7fffffffa7f0 140737488332784 rsp 0x7fffffffa7f0 140737488332784 r8 0x7ffff6ef7770 140737336276848 r9 0x7ffff7fe4740 140737354024768 r10 0x58 88 r11 0x7ffff6b9f750 140737332770640 r12 0x7ffff695f000 140737330409472 r13 0x7fffffffa8b0 140737488332976 r14 0x1da5d60 31087968 r15 0x7ffff0700a20 140737227262496 rip 0x464f80 <JSFunction::nonLazyScript() const+48> => 0x464f80 <JSFunction::nonLazyScript() const+48>: movl $0x0,0x0 0x464f8b <JSFunction::nonLazyScript() const+59>: ud2

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/78ff3244294b user: Shu-yu Guo date: Mon Sep 05 09:11:46 2016 +0200 summary: Bug 1297706 - Syntax parse class declarations. r=jandem This iteration took 246.649 seconds to run.

Shu, can you please take a look?

So the interpreter has the assumption that the callee of a CallObject that's on the environment chain is never lazy. The bug here seems to be that Ion can inline a lambda script without delazifying the cloned JSFunction when running the inlined lambda. Concretely, in the fuzz test above, |q| is a cloned lambda per iteration of the loop in |f|. |q| is passed to |g|, which calls it. Ion inlines |q| into |g|. When |q| gets called, it drops back into Interpreter via Invoke to call the generator function |get|. This calls AssertScopeMatchesEnvironment and hits the assertion. I'd like to keep the invariant that live CallObjects have delazified callees. Jan, is this a bug in Ion? That is, should Ion take care to delazify the callee when inlining lambdas? If this was an explicit decision to not delazify, I can rework the assertion.

Your analysis makes sense to me. The *canonical* function must be non-lazy though, as it has a baseline script (Ion only inlines functions with baseline scripts, and functions with JIT code are not relazified). We have JSFunction::existingScriptForInlinedFunction to handle this case: http://searchfox.org/mozilla-central/rev/f5c9e9a249637c9abd88754c8963ecb3838475cb/js/src/jsfun.h#414 Unfortunately that function also delazifies the JSFunction, so it's probably not great to use it in a MOZ_ASSERT. Maybe we should change that, or add a similar function that doesn't modify the callee. We could also add a calleeScript() accessor to CallObject and audit callers of CallObject::callee() to call that instead.

I also renamed the method to 'existingScript' (removing the 'ForInlinedFunction' suffix since the length was out of control with 'nonDelazifying' added).

(In reply to Jan de Mooij [:jandem] from comment #4) > Your analysis makes sense to me. The *canonical* function must be non-lazy > though, as it has a baseline script (Ion only inlines functions with > baseline scripts, and functions with JIT code are not relazified). We have > JSFunction::existingScriptForInlinedFunction to handle this case: > http://searchfox.org/mozilla-central/rev/ > f5c9e9a249637c9abd88754c8963ecb3838475cb/js/src/jsfun.h#414 > > Unfortunately that function also delazifies the JSFunction, so it's probably > not great to use it in a MOZ_ASSERT. Maybe we should change that, or add a > similar function that doesn't modify the callee. We could also add a > calleeScript() accessor to CallObject and audit callers of > CallObject::callee() to call that instead. I don't particularly like getting scripts by not explicitly going through the JSFunction *just* for CallObjects. I'm not happy with the large number of script getters, but better to force folks to choose one than hide it imo.

Comment on attachment 8808329 [details] [diff] [review] Use correct JSScript getter when getting CallObject scripts during scope/env chain checks. Review of attachment 8808329 [details] [diff] [review]: ----------------------------------------------------------------- Great! r=me with comment below addressed. ::: js/src/jsfun.h @@ +418,5 @@ > // Get the script from the canonical function. Ion used the > // canonical function to inline the script and because it has > // Baseline code it has not been relazified. Note that we can't > // use lazyScript->script_ here as it may be null in some cases, > // see bug 976536. Hm I think this is no longer possible, I filed bug 1315940 to simplify this after this patch lands. @@ +425,5 @@ > MOZ_ASSERT(fun); > JSScript* script = fun->nonLazyScript(); > > if (shadowZone()->needsIncrementalBarrier()) > js::LazyScript::writeBarrierPre(lazy); This should be in existingScript(): there we're eliminating the fun->lazy edge so we need to trace the lazy script for incremental GC.

Pushed by shu@rfrn.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/43c1404a52e7 Use correct JSScript getter when getting CallObject scripts during scope/env chain checks. (r=jandem)

Hi :shu, Is this worth uplifting to 51 aurora?

(In reply to Gerry Chang [:gchang] from comment #10) > Hi :shu, > Is this worth uplifting to 51 aurora? Hmm, yeah why not. My intuition is that this crash will almost never trigger in the wild, but it's a simple bug fix and should be easy to uplift.

Comment on attachment 8808329 [details] [diff] [review] Use correct JSScript getter when getting CallObject scripts during scope/env chain checks. Approval Request Comment [Feature/regressing bug #]: Don't know, but surfaced by bug 1263355 [User impact if declined]: Very rare crashes [Describe test coverage new/current, TreeHerder]: On m-c [Risks and why]: Low, bug fix only [String/UUID change made/needed]: None

Comment on attachment 8808329 [details] [diff] [review] Use correct JSScript getter when getting CallObject scripts during scope/env chain checks. Since merge day is passed, 51 became beta. So I remove aurora flag and change it to 51 beta. This patch fixes a regression. Beta51+. This should be in 51 beta 2.