1095290 - jit-test/tests/debug/Source-invisible.js on SM(ggc): Assertion failure: hasScript()

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Ryan VanderMeulen [:RyanVM]]

Note that this is a different one from the other two bugs I just filed. This is a new assert that comes when current trunk is merged to Aurora (vs. failures happening on the existing release branches).

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=2977285&repo=try

FAIL - debug/Source-invisible.js
TEST-UNEXPECTED-FAIL | js/src/jit-test/tests/debug/Source-invisible.js | Assertion failure: hasScript(), at /builds/slave/try_l64-d_sm-ggc-0000000000000/src/js/src/jsfun.h:316 (code -11, args "--ion-eager --ion-offthread-compile=off --ion-check-range-analysis --no-sse3 --no-threads")
INFO exit-status : -11
INFO timed-out : False
INFO stderr 2> Assertion failure: hasScript(), at /builds/slave/try_l64-d_sm-ggc-0000000000000/src/js/src/jsfun.h:316

Comment 1

[Ryan VanderMeulen [:RyanVM]]

This went away on its own.

Comment 2

[Steve Fink [:sfink] [:s:]]

I think it's an intermittent.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=6d9f01082f45

Comment 3

[Legacy TBPL/Treeherder Robot]

log: https://treeherder.mozilla.org/logviewer.html#?repo=mozilla-inbound&job_id=5860582
repository: mozilla-inbound
start_time: 2015-01-24T02:50:08
who: philringnalda[at]gmail[dot]com
machine: bld-linux64-spot-277
buildname: Linux x86-64 mozilla-inbound leak test spidermonkey_tier_1-generational build
revision: 42d4b2cb18b6

Assertion failure: hasScript(), at /builds/slave/m-in_l64-d_sm-ggc-000000000000/src/js/src/jsfun.h:322
TEST-UNEXPECTED-FAIL | js/src/jit-test/tests/debug/Source-invisible.js | Assertion failure: hasScript(), at /builds/slave/m-in_l64-d_sm-ggc-000000000000/src/js/src/jsfun.h:322 (code -11, args "")
INFO stderr         2> Assertion failure: hasScript(), at /builds/slave/m-in_l64-d_sm-ggc-000000000000/src/js/src/jsfun.h:322
make[1]: *** [check-jit-test] Error 2
make: *** [check-jit-test] Error 2

Comment 4

[Legacy TBPL/Treeherder Robot]

log: https://treeherder.mozilla.org/logviewer.html#?repo=mozilla-inbound&job_id=6654993
repository: mozilla-inbound
start_time: 2015-02-16T08:42:53
who: nigelbabu[at]gmail[dot]com
machine: bld-linux64-spot-249
buildname: Linux x86-64 mozilla-inbound leak test spidermonkey_tier_1-generational build
revision: 4025bc064621

Assertion failure: hasScript(), at /builds/slave/m-in_l64-d_sm-ggc-000000000000/src/js/src/jsfun.h:322
TEST-UNEXPECTED-FAIL | js/src/jit-test/tests/debug/Source-invisible.js | Assertion failure: hasScript(), at /builds/slave/m-in_l64-d_sm-ggc-000000000000/src/js/src/jsfun.h:322 (code -11, args "--no-baseline --no-ion")
INFO stderr         2> Assertion failure: hasScript(), at /builds/slave/m-in_l64-d_sm-ggc-000000000000/src/js/src/jsfun.h:322
make[1]: *** [check-jit-test] Error 2
make: *** [check-jit-test] Error 2

Comment 5

[Legacy TBPL/Treeherder Robot]

log: https://treeherder.mozilla.org/logviewer.html#?repo=mozilla-inbound&job_id=7330653
repository: mozilla-inbound
start_time: 2015-03-07T07:53:28
who: philringnalda[at]gmail[dot]com
machine: bld-linux64-spot-042
buildname: Linux x86-64 mozilla-inbound leak test spidermonkey_tier_1-generational build
revision: cb1c692e8963

Assertion failure: hasScript(), at /builds/slave/m-in_l64-d_sm-ggc-000000000000/src/js/src/jsfun.h:322
TEST-UNEXPECTED-FAIL | js/src/jit-test/tests/debug/Source-invisible.js | Assertion failure: hasScript(), at /builds/slave/m-in_l64-d_sm-ggc-000000000000/src/js/src/jsfun.h:322 (code -11, args "")
INFO stderr         2> Assertion failure: hasScript(), at /builds/slave/m-in_l64-d_sm-ggc-000000000000/src/js/src/jsfun.h:322
make[1]: *** [check-jit-test] Error 2
make: *** [check-jit-test] Error 2

Comment 6

[Legacy TBPL/Treeherder Robot]

log: https://treeherder.mozilla.org/logviewer.html#?repo=try&job_id=5592965
repository: try
start_time: 2015-03-12T04:36:56
who: benj[at]benj[dot]me
machine: try-linux64-spot-128
buildname: Linux x86-64 try leak test spidermonkey_try-generational build
revision: 9dcc46796f42

Assertion failure: hasScript(), at /builds/slave/try_l64-d_sm-ggc-0000000000000/src/js/src/jsfun.h:322
TEST-UNEXPECTED-FAIL | js/src/jit-test/tests/debug/Source-invisible.js | Assertion failure: hasScript(), at /builds/slave/try_l64-d_sm-ggc-0000000000000/src/js/src/jsfun.h:322 (code -11, args "")
INFO stderr         2> Assertion failure: hasScript(), at /builds/slave/try_l64-d_sm-ggc-0000000000000/src/js/src/jsfun.h:322
make[1]: *** [check-jit-test] Error 2
make: *** [check-jit-test] Error 2

Comment 7

[Legacy TBPL/Treeherder Robot]

log: https://treeherder.mozilla.org/logviewer.html#?repo=mozilla-inbound&job_id=7536968
repository: mozilla-inbound
start_time: 2015-03-12T09:04:56
who: rvandermeulen[at]mozilla[dot]com
machine: bld-linux64-spot-118
buildname: Linux x86-64 mozilla-inbound leak test spidermonkey_tier_1-generational build
revision: 83fabedf6d3a

Assertion failure: hasScript(), at /builds/slave/m-in_l64-d_sm-ggc-000000000000/src/js/src/jsfun.h:322
TEST-UNEXPECTED-FAIL | js/src/jit-test/tests/debug/Source-invisible.js | Assertion failure: hasScript(), at /builds/slave/m-in_l64-d_sm-ggc-000000000000/src/js/src/jsfun.h:322 (code -11, args "--no-baseline --no-ion")
INFO stderr         2> Assertion failure: hasScript(), at /builds/slave/m-in_l64-d_sm-ggc-000000000000/src/js/src/jsfun.h:322
/builds/slave/m-in_l64-d_sm-ggc-000000000000/src/js/src/jit-test/tests/ion/bug913749.js:18:1 Error: Assertion failed: got "y.length is undefined", expected "y is undefined"
TEST-UNEXPECTED-FAIL | js/src/jit-test/tests/ion/bug913749.js | /builds/slave/m-in_l64-d_sm-ggc-000000000000/src/js/src/jit-test/tests/ion/bug913749.js:18:1 Error: Assertion failed: got "y.length is undefined", expected "y is undefined" (code 3, args "--ion-eager --ion-offthread-compile=off")
INFO stderr         2> /builds/slave/m-in_l64-d_sm-ggc-000000000000/src/js/src/jit-test/tests/ion/bug913749.js:18:1 Error: Assertion failed: got "y.length is undefined", expected "y is undefined"
make[1]: *** [check-jit-test] Error 2
make: *** [check-jit-test] Error 2

Comment 8

[Legacy TBPL/Treeherder Robot]

log: https://treeherder.mozilla.org/logviewer.html#?repo=mozilla-inbound&job_id=7543361
repository: mozilla-inbound
start_time: 2015-03-12T10:56:55
who: rvandermeulen[at]mozilla[dot]com
machine: bld-linux64-spot-042
buildname: Linux x86-64 mozilla-inbound leak test spidermonkey_tier_1-generational build
revision: 25b9c28d877e

Assertion failure: hasScript(), at /builds/slave/m-in_l64-d_sm-ggc-000000000000/src/js/src/jsfun.h:322
TEST-UNEXPECTED-FAIL | js/src/jit-test/tests/debug/Source-invisible.js | Assertion failure: hasScript(), at /builds/slave/m-in_l64-d_sm-ggc-000000000000/src/js/src/jsfun.h:322 (code -11, args "--no-baseline --no-ion")
INFO stderr         2> Assertion failure: hasScript(), at /builds/slave/m-in_l64-d_sm-ggc-000000000000/src/js/src/jsfun.h:322
make[1]: *** [check-jit-test] Error 2
make: *** [check-jit-test] Error 2

Comment 9

[Steve Fink [:sfink] [:s:]]

Ok, I'm out of my depth here. But this reproduces totally reliably for me if I change the test code to:

  var gi = newGlobal({ invisibleToDebugger: true });
  var gv = newGlobal();
  var dbg = new Debugger;
  var gvw = dbg.addDebuggee(gv);
  
  for (var i = 0; i < 1000; i++) {
    gi.eval('function f' + i + '() { return "f!" }');
    gv.f = gi['f' + i];
    gv.eval('f = clone(f);');
    gvw.getOwnPropertyDescriptor('f').value.script.source;
  }

I can even capture it in an rr trace. Sadly, simply doing the below patch does *not* fix the problem. Or rather, it changes it into a compartment mismatch problem. Something of a scary one, since the LazyScript.function_ pointer involved was recently moved during a compacting GC.

When the LazyScript was created, it was given a function in the same compartment as the cx at that time.

diff --git a/js/src/jsfun.cpp b/js/src/jsfun.cpp
--- a/js/src/jsfun.cpp
+++ b/js/src/jsfun.cpp
@@ -2140,18 +2140,20 @@ js::CloneFunctionObject(JSContext *cx, H
     RootedFunction cloneRoot(cx, clone);
 
     /*
      * Across compartments we have to clone the script for interpreted
      * functions. Cross-compartment cloning only happens via JSAPI
      * (JS::CloneFunctionObject) which dynamically ensures that 'script' has
      * no enclosing lexical scope (only the global scope).
      */
-    if (cloneRoot->isInterpreted() && !CloneFunctionScript(cx, fun, cloneRoot, newKindArg))
-        return nullptr;
+    if (cloneRoot->isInterpreted() && !cloneRoot->isInterpretedLazy()) {
+        if (!CloneFunctionScript(cx, fun, cloneRoot, newKindArg))
+            return nullptr;
+    }
 
     return cloneRoot;
 }

Comment 10

[Steve Fink [:sfink] [:s:]]

More detail:

It runs this code:

    } else if (fun->isInterpretedLazy()) {
        LazyScript *lazy = fun->lazyScriptOrNull();
        clone->initLazyScript(lazy);
        clone->initEnvironment(parent);
    } else {

fun is in compartment A. clone is in compartment B. This code makes them share a LazyScript, which points back to fun in compartment A. So compartment(clone.lazy.fun) != compartment(clone).

If this were a non-lazy script, it would be fixed up via

    if (cloneRoot->isInterpreted() && !cloneRoot->isInterpretedLazy()) {
        if (!CloneFunctionScript(cx, fun, cloneRoot, newKindArg))
            return nullptr;
    }

but clone inherited the laziness so this doesn't do anything. Do we need a CloneFunctionLazyScript? Or should CloneFunctionScript handle lazy scripts too? Or am I completely lost and confused? (yes)

Comment 11

[Steve Fink [:sfink] [:s:]]

Sounds like bhackett is the expert here.

Comment 12

[Legacy TBPL/Treeherder Robot]

log: https://treeherder.mozilla.org/logviewer.html#?repo=mozilla-inbound&job_id=7544001
repository: mozilla-inbound
start_time: 2015-03-12T11:03:27
who: wkocher[at]mozilla[dot]com
machine: bld-linux64-spot-271
buildname: Linux x86-64 mozilla-inbound leak test spidermonkey_tier_1-generational build
revision: 7529425ef21f

Assertion failure: hasScript(), at /builds/slave/m-in_l64-d_sm-ggc-000000000000/src/js/src/jsfun.h:322
TEST-UNEXPECTED-FAIL | js/src/jit-test/tests/debug/Source-invisible.js | Assertion failure: hasScript(), at /builds/slave/m-in_l64-d_sm-ggc-000000000000/src/js/src/jsfun.h:322 (code -11, args "--no-baseline --no-ion")
INFO stderr         2> Assertion failure: hasScript(), at /builds/slave/m-in_l64-d_sm-ggc-000000000000/src/js/src/jsfun.h:322
make[1]: *** [check-jit-test] Error 2
make: *** [check-jit-test] Error 2

Comment 13

[Steve Fink [:sfink] [:s:]]

Attachment: Make unlazified scripts same-compartment with their functions

The intermittent failure is because a zeal GC fires while creating the clone object, which lazifies the function we're cloning. Then at the end of CloneFunctionObject, there is an assumption that if the cloned function isInterpreted, that it has a nonlazy script to clone. But just fixing that assumption isn't enough, because there's an earlier problem where fun->getOrCreateScript(cx) in this case is running in a different compartment from fun, and so fun ends up getting a script in the wrong compartment.

Comment 14

[Steve Fink [:sfink] [:s:]]

Attachment: Make unlazified scripts same-compartment with their functions

Oops, sorry, I messed up a last-minute change to better match up the delazification conditions. isInterpretedLazy() is a little bit different from !isInterpretedLazy().

Comment 15

[Brian Hackett [Laid off!]]

Comment on attachment 8577341 [details] [diff] [review]
Make unlazified scripts same-compartment with their functions

Review of attachment 8577341 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsfun.cpp
@@ +1393,5 @@
>  /* static */ bool
>  JSFunction::createScriptForLazilyInterpretedFunction(JSContext *cx, HandleFunction fun)
>  {
>      MOZ_ASSERT(fun->isInterpretedLazy());
> +    JSAutoCompartment ac(cx, fun);

This JSAutoCompartment should really be wrapping the getOrCreateScript call you're adding to CloneFunctionObject.  There's a general assumption in the engine that we are working with objects in the context's current compartment, so JSAutoCompartment should really be limited to functions where that isn't the case, like CloneFunctionObject.

Comment 16

[Steve Fink [:sfink] [:s:]]

(In reply to Brian Hackett (:bhackett) from comment #15)
> Comment on attachment 8577341 [details] [diff] [review]
> Make unlazified scripts same-compartment with their functions
> 
> Review of attachment 8577341 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jsfun.cpp
> @@ +1393,5 @@
> >  /* static */ bool
> >  JSFunction::createScriptForLazilyInterpretedFunction(JSContext *cx, HandleFunction fun)
> >  {
> >      MOZ_ASSERT(fun->isInterpretedLazy());
> > +    JSAutoCompartment ac(cx, fun);
> 
> This JSAutoCompartment should really be wrapping the getOrCreateScript call
> you're adding to CloneFunctionObject.  There's a general assumption in the
> engine that we are working with objects in the context's current
> compartment, so JSAutoCompartment should really be limited to functions
> where that isn't the case, like CloneFunctionObject.

Yeah, ok. I mean, I'm well aware that that's how things are normally done. But it's not just the new getOrCreateScript that needs to be wrapped. The one that was already there was creating the script in the wrong compartment, it just somehow didn't cause any detectable problems except in this weird case (when the GC lazifies it). But sure, I can constrain the craziness to CloneFunctionObject. And maybe add an assertion somewhere deeper.

Comment 17

[Steve Fink [:sfink] [:s:]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/79328b3e58ed

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/79328b3e58ed

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Should this be backported to 37/38?

Comment 20

[Steve Fink [:sfink] [:s:]]

Attachment: bug-1095290-unlazy

Actually landed patch, for uplift.

Approval Request Comment
[Feature/regressing bug #]: bug 678037 (FF24)
[User impact if declined]: crashes
[Describe test coverage new/current, TreeHerder]: landed on inbound for a day
[Risks and why]: low risk, it's switching something that clearly isn't in the right compartment to the right compartment. It doesn't matter much in practice for this particular structure, other than crashes in obscure situations. (It's an annoying intermittent orange on SM(ggc), which is something very unlikely to trigger in actual browsing.)
[String/UUID change made/needed]: none

Comment 21

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8578947 [details] [diff] [review]
bug-1095290-unlazy

Given that this is unlikely to get triggered by actual usage of the browser and that we're going to build the final Beta build in the 37 cycle tomorrow, I don't think there is a lot of value in taking this fix in 37. Beta-

Comment 22

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8578947 [details] [diff] [review]
bug-1095290-unlazy

Forgot to carry over the r+.

Comment 23

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8578947 [details] [diff] [review]
bug-1095290-unlazy

We can, however, take this fix on Aurora. Marking Aurora+ as I should have done earlier.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/41a819532cfb

Comment 25

[Legacy TBPL/Treeherder Robot]

log: https://treeherder.mozilla.org/logviewer.html#?repo=try&job_id=9320043
repository: try
start_time: 2015-07-13T15:51:55
who: nfitzgerald[at]mozilla[dot]com
machine: b-2008-ix-0029
buildname: WINNT 5.2 try leak test spidermonkey_try-compacting build
revision: ea832514f086

ValueError: time data '' does not match format '%Y%m%d%H%M%S'
ValueError: time data '' does not match format '%Y%m%d%H%M%S'
ValueError: time data '' does not match format '%Y%m%d%H%M%S'
Assertion failure: isNative(), at c:\builds\moz2_slave\try_w32-d_sm-compacting-000000\src\js\src\jsfun.h:460
TEST-UNEXPECTED-FAIL | js\src\jit-test\tests\debug\Source-invisible.js | Assertion failure: isNative(), at c:\builds\moz2_slave\try_w32-d_sm-compacting-000000\src\js\src\jsfun.h:460 (code -2147483645, args "--baseline-eager --no-fpu")
INFO stderr         2> Assertion failure: isNative(), at c:\builds\moz2_slave\try_w32-d_sm-compacting-000000\src\js\src\jsfun.h:460