Closed Bug 1095290 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: JavaScript: GC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox37 --- wontfix
firefox38 --- fixed
firefox39 --- fixed
firefox-esr31 --- unaffected

People

(Reporter: RyanVM, Assigned: sfink)

References

Details

(Keywords: assertion, intermittent-failure)

Attachments

(1 file, 2 obsolete files)

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
This went away on its own.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → WORKSFORME
Status: RESOLVED → VERIFIED
I think it's an intermittent.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=6d9f01082f45
Status: VERIFIED → REOPENED
Resolution: WORKSFORME → ---
Summary: jit-test/tests/debug/Source-invisible.js will permafail on SM(ggc) runs when Gecko 36 merges to Aurora → jit-test/tests/debug/Source-invisible.js on SM(ggc): Assertion failure: hasScript()
Blocks: 1142683
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;
 }
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)
Sounds like bhackett is the expert here.
Flags: needinfo?(bhackett1024)
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.
Attachment #8576998 - Flags: review?(bhackett1024)
Assignee: nobody → sphink
Status: REOPENED → ASSIGNED
Flags: needinfo?(bhackett1024)
Oops, sorry, I messed up a last-minute change to better match up the delazification conditions. isInterpretedLazy() is a little bit different from !isInterpretedLazy().
Attachment #8577341 - Flags: review?(bhackett1024)
Attachment #8576998 - Attachment is obsolete: true
Attachment #8576998 - Flags: review?(bhackett1024)
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.
Attachment #8577341 - Flags: review?(bhackett1024) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/79328b3e58ed
Status: ASSIGNED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Should this be backported to 37/38?
Flags: needinfo?(sphink)
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
Attachment #8577341 - Attachment is obsolete: true
Flags: needinfo?(sphink)
Attachment #8578947 - Flags: approval-mozilla-beta?
Attachment #8578947 - Flags: approval-mozilla-aurora?
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-
Attachment #8578947 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Comment on attachment 8578947 [details] [diff] [review]
bug-1095290-unlazy

Forgot to carry over the r+.
Attachment #8578947 - Flags: review+
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.
Attachment #8578947 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.