Closed Bug 1312491 Opened 3 years ago Closed 3 years ago

Assertion failure: hasScript(), at js/src/jsfun.h:430

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox50 --- unaffected
firefox51 --- fixed
firefox52 --- fixed

People

(Reporter: decoder, Assigned: shu)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [jsbugmon:update])

Attachments

(1 file)

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
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
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?
Blocks: 1297706
Flags: needinfo?(shu)
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.
Flags: needinfo?(shu) → needinfo?(jdemooij)
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.
Flags: needinfo?(jdemooij) → needinfo?(shu)
I also renamed the method to 'existingScript' (removing the
'ForInlinedFunction' suffix since the length was out of control with
'nonDelazifying' added).
Attachment #8808329 - Flags: review?(jdemooij)
(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.
Flags: needinfo?(shu)
Blocks: 1315940
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.
Attachment #8808329 - Flags: review?(jdemooij) → review+
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)
https://hg.mozilla.org/mozilla-central/rev/43c1404a52e7
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Hi :shu,
Is this worth uplifting to 51 aurora?
Flags: needinfo?(shu)
(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.
Flags: needinfo?(shu)
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
Attachment #8808329 - Flags: approval-mozilla-aurora?
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.
Attachment #8808329 - Flags: approval-mozilla-aurora? → approval-mozilla-beta+
Assignee: nobody → shu
Duplicate of this bug: 1236098
You need to log in before you can comment on or make changes to this bug.