1498980 - Crash [@ js::gc::IsAboutToBeFinalizedInternal] or Assertion failure: false (IsAboutToBeFinalized(&scope_)), at js/src/vm/EnvironmentObject.cpp:1499

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd]]

The following testcase crashes on mozilla-central revision 3aca49b2df24 (build with --enable-debug, run with --fuzzing-safe --no-threads --no-baseline --no-ion):

// Adapted from randomly chosen test: js/src/jit-test/tests/debug/bug1304553.js
dbgGlobal = newGlobal();
dbg = new dbgGlobal.Debugger;
dbg.addDebuggee(this);
function f() {
    dbg.getNewestFrame().older.eval("");
}
m = parseModule("f();");
m.declarationInstantiation();
m.evaluation();
// jsfunfuzz-generated
gc();

Backtrace:

#0  0x000055fac1b2d32a in js::LiveEnvironmentVal::needsSweep (this=<optimized out>) at js/src/vm/EnvironmentObject.cpp:1499
#1  JS::StructGCPolicy<js::LiveEnvironmentVal>::needsSweep (tp=<optimized out>) at /home/ubuntu/shell-cache/js-dbg-64-linux-3aca49b2df24/objdir-js/dist/include/js/GCPolicyAPI.h:84
#2  JS::DefaultMapSweepPolicy<js::ReadBarriered<JSObject*>, js::LiveEnvironmentVal>::needsSweep (key=0x7ffa071e3c68, value=<optimized out>) at /home/ubuntu/shell-cache/js-dbg-64-linux-3aca49b2df24/objdir-js/dist/include/js/GCHashTable.h:24
#3  JS::GCHashMap<js::ReadBarriered<JSObject*>, js::LiveEnvironmentVal, js::MovableCellHasher<js::ReadBarriered<JSObject*> >, js::ZoneAllocPolicy, JS::DefaultMapSweepPolicy<js::ReadBarriered<JSObject*>, js::LiveEnvironmentVal> >::sweep (this=<optimized out>) at /home/ubuntu/shell-cache/js-dbg-64-linux-3aca49b2df24/objdir-js/dist/include/js/GCHashTable.h:80
#4  0x000055fac1b17782 in js::DebugEnvironments::sweep (this=<optimized out>) at js/src/vm/EnvironmentObject.cpp:2643
/snip

For detailed crash information, see attachment.

Setting s-s because gc is involved, as a start.

Comment 1

[Gary Kwong [:gkw] [:nth10sd]]

Attachment: Detailed Crash Information

Comment 2

[Gary Kwong [:gkw] [:nth10sd]]

autobisectjs shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/f8b19c4105d2
user:        Jon Coppeard
date:        Thu Oct 11 18:33:57 2018 +0100
summary:     Bug 1489477 - Stop modules from entraining the top-level JSScript r=sfink

Jon, is bug 1489477 a likely regressor?

(Also variants crash [@ js::gc::IsAboutToBeFinalizedInternal] )

Comment 3

[Christian Holler (:decoder)]

Debugger GC issues are unlikely to be s-s and DebugEnvironments is on the stack.

Comment 4

[Jon Coppeard (:jonco)]

s-s but requires use of debugger to trigger the problem.

Comment 5

[Jon Coppeard (:jonco)]

Attachment: bug1498980-live-env-sweep

The problem is that module environments are currently not removed from DebugEnvironments::liveEnvs map.  The patch is a minimal fix to do this explicitly after executing the body of a module.

There's another problem though that we don't take a snapshot of the environment if we have a debug environment proxy for the module environment.  I think this explains bug 1283534.

Comment 6

[Jason Orendorff [:jorendorff]]

Comment on attachment 9017217 [details] [diff] [review]
bug1498980-live-env-sweep

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

Clearing review for now.

::: js/src/builtin/ModuleObject.cpp
@@ +1171,5 @@
>          JS_ReportErrorASCII(cx, "Module declarations have not yet been instantiated");
>          return false;
>      }
>  
> +    bool result = Execute(cx, script, *scope, rval.address());

Please try fixing it here instead:
https://searchfox.org/mozilla-central/rev/c9272ef398954288525e37196eada1e5a93d93bf/js/src/vm/Interpreter.cpp#1242-1243

It took me a while to find that, but I was suspicious immediately, because by the time Execute returns, the frame has already been popped. We have already left the scope. For modules, a typical interpreter call stack where this happens should (I think) be like:

  js::Execute
    js::ExecuteKernel
      js::RunScript
        js::Interpret <https://searchfox.org/mozilla-central/rev/c9272ef398954288525e37196eada1e5a93d93bf/js/src/vm/Interpreter.cpp#4826>
          js::InterpreterFrame::epilogue
            js::UnwindAllEnvironmentsInFrame
              PopEnvironment

::: js/src/jit-test/tests/modules/bug-1498980.js
@@ +5,5 @@
> +    dbg.getNewestFrame().older.eval("");
> +}
> +m = parseModule("f();");
> +m.declarationInstantiation();
> +m.evaluation();

Great, thanks! Please also test when the module throws.

Comment 7

[Jon Coppeard (:jonco)]

Attachment: bug1498980-live-env-sweep v2

Updated patch.

Comment 8

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6b952be63f69e244ab688cdb7a84d121b009a162

Comment 9

[Sebastian Hengst [:aryx] (needinfo on intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/6b952be63f69

Comment 10

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed.

Comment 11

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed on Fx64