Closed Bug 1144356 Opened 5 years ago Closed 5 years ago

Crash [@ js::Debugger::fireOnGarbageCollectionHook] or Assertion failure: cx, at vm/Debugger.cpp or Assertion failure: rt->contextList.getFirst() == rt->contextList.getLast(), at jsfriendapi.cpp

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
Tracking Status
firefox39 --- affected

People

(Reporter: gkw, Assigned: fitzgen)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, regression, testcase)

Crash Data

Attachments

(2 files, 1 obsolete file)

// Randomly chosen test: js/src/jit-test/tests/debug/Memory-onGarbageCollection-03.js
dbg = Debugger();
t = dbg.addDebuggee(newGlobal())
dbg.memory.onGarbageCollection = _ => x

asserts js debug shell on m-c changeset e965a1a534ec with --fuzzing-safe --no-threads --no-ion -D at Assertion failure: cx, at vm/Debugger.cpp.

Configure options:

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-debug --enable-profiling --disable-threadsafe --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

python -u ~/fuzzing/js/compileShell.py -b "--enable-debug --enable-profiling --enable-more-deterministic" -r e965a1a534ec

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/578ba1506156
user:        Nick Fitzgerald
date:        Fri Mar 13 13:03:00 2015 +0100
summary:     Bug 1137844 - Part 3: Fire the Debugger.Memory.prototype.onGarbageCollection hook after GCs; r=sfink

Nick, is bug 1137844 a likely regressor?
Flags: needinfo?(nfitzgerald)
Attached file stack
(lldb) bt 5
* thread #1: tid = 0x143be, 0x00000001001f08f1 js-dbg-64-prof-dm-darwin-e965a1a534ec`js::Debugger::fireOnGarbageCollectionHook(this=<unavailable>, rt=<unavailable>, stats=<unavailable>) + 785 at Debugger.cpp:1335, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x00000001001f08f1 js-dbg-64-prof-dm-darwin-e965a1a534ec`js::Debugger::fireOnGarbageCollectionHook(this=<unavailable>, rt=<unavailable>, stats=<unavailable>) + 785 at Debugger.cpp:1335
    frame #1: 0x00000001003c49a9 js-dbg-64-prof-dm-darwin-e965a1a534ec`js::gcstats::Statistics::endGC() [inlined] js::Debugger::onGarbageCollection(rt=<unavailable>, stats=<unavailable>) + 68 at Debugger.h:991
    frame #2: 0x00000001003c4965 js-dbg-64-prof-dm-darwin-e965a1a534ec`js::gcstats::Statistics::endGC(this=0x0000000102025ed0) + 1621 at Statistics.cpp:977
    frame #3: 0x00000001003c4e23 js-dbg-64-prof-dm-darwin-e965a1a534ec`js::gcstats::Statistics::endSlice(this=<unavailable>) + 195 at Statistics.cpp:1029
    frame #4: 0x00000001007f6fa0 js-dbg-64-prof-dm-darwin-e965a1a534ec`js::gc::GCRuntime::collect(bool, js::SliceBudget, JS::gcreason::Reason) [inlined] js::gcstats::AutoGCSlice::~AutoGCSlice() + 12 at Statistics.h:329
(lldb)
Yeah that's the regressor.
Assignee: nobody → nfitzgerald
Status: NEW → ASSIGNED
Flags: needinfo?(nfitzgerald)
https://dxr.mozilla.org/mozilla-central/source/js/src/vm/Debugger.cpp?from=fireOnGarbageCollectionHook#1335

So it seems that we can't get a cx from the rt? Should we just return early?

:sfink, what do you think is the correct thing to do here?
Flags: needinfo?(sphink)
Derp.

This all seems pretty crazy. DefaultJSContext is only meaningful with respect to a particular embedding, and I see no reason why it is a reasonable JSContext to use in the first place. If you left an exception pending on it, I have no idea what would happen. Except it looks like it handles that by calling handleUncaughtException, so that particular problem is papered over. But what if the context *already* had an exception pending? Maybe even one for which creating an Error object is what triggered the garbage collection in the first place? This just seems weird and dangerous.

On the other hand, JSContexts don't mean very much these days.

It would probably make the most sense to me to create a JSContext specifically for Debugger's use (shared across the runtime, because why not.) But I don't feel like I have a firm enough grasp on things to stand behind that recommendation, so I'm going to needinfo an order of magnitude more brain cells than I possess myself.
Flags: needinfo?(sphink)
Flags: needinfo?(jwalden+bmo)
Flags: needinfo?(jorendorff)
This comment 0 testcase also crashes js opt shell at js::Debugger::fireOnGarbageCollectionHook
Crash Signature: [@ js::Debugger::fireOnGarbageCollectionHook]
Summary: Assertion failure: cx, at vm/Debugger.cpp → Crash [@ js::Debugger::fireOnGarbageCollectionHook] or Assertion failure: cx, at vm/Debugger.cpp
More related issues (from bug 1137527 comment 15):

JS::ubi::RootList (used to initialize a census traversal) calls JS_TraceRuntime,

which evicts the nursery,

which calls the onGarbageCollection hook,

which runs JS that allocates new objects in the nursery,

which causes assertions in the runtime tracing code.

------------------------------------------------------------

Is there some time/place we can call this hook that avoids the above problems and has easy access to a cx and the gc stats?

It seems to me that the existing stringified JSON hook avoids these issues by posting a runnable to run later to gecko's event loop? Should the Debugger do something similar (with a lot of SM<-->Gecko plumbing)?
Flags: needinfo?(terrence)
Yes, you pretty much have to do that way, sadly. It would be really nice if we had our own event loop, but we don't yet.
Flags: needinfo?(terrence)
Attachment #8578937 - Attachment is obsolete: true
Attachment #8578937 - Flags: review?(sphink)
Flags: needinfo?(jwalden+bmo)
Flags: needinfo?(jorendorff)
// Randomly chosen test: js/src/jit-test/tests/debug/Memory-onGarbageCollection-04.js
evaluate("\
    g = newGlobal();\
    dbg = Debugger(g);\
    dbg.memory.onGarbageCollection = function(){};\
    g.eval(\"gc()\");\
", {
    newContext: true,
})

is a similar testcase that asserts js debug shell on m-c rev 2e2244031032 with --fuzzing-safe --no-threads --no-ion at Assertion failure: rt->contextList.getFirst() == rt->contextList.getLast(), at jsfriendapi.cpp
Summary: Crash [@ js::Debugger::fireOnGarbageCollectionHook] or Assertion failure: cx, at vm/Debugger.cpp → Crash [@ js::Debugger::fireOnGarbageCollectionHook] or Assertion failure: cx, at vm/Debugger.cpp or Assertion failure: rt->contextList.getFirst() == rt->contextList.getLast(), at jsfriendapi.cpp
(lldb) bt 5
* thread #1: tid = 0x3e1fe, 0x00000001007c09df js-dbg-64-dm-nsprBuild-darwin-2e2244031032`js::DefaultJSContext(rt=<unavailable>) + 255 at jsfriendapi.cpp:1134, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x00000001007c09df js-dbg-64-dm-nsprBuild-darwin-2e2244031032`js::DefaultJSContext(rt=<unavailable>) + 255 at jsfriendapi.cpp:1134
    frame #1: 0x00000001001ead89 js-dbg-64-dm-nsprBuild-darwin-2e2244031032`js::Debugger::fireOnGarbageCollectionHook(this=0x0000000102884800, rt=<unavailable>, stats=0x00000001028676d0) + 89 at Debugger.cpp:1334
    frame #2: 0x00000001003b06f9 js-dbg-64-dm-nsprBuild-darwin-2e2244031032`js::gcstats::Statistics::endGC() [inlined] js::Debugger::onGarbageCollection(rt=<unavailable>, stats=<unavailable>) + 81 at Debugger.h:991
    frame #3: 0x00000001003b06a8 js-dbg-64-dm-nsprBuild-darwin-2e2244031032`js::gcstats::Statistics::endGC(this=0x00000001028676d0) + 1400 at Statistics.cpp:978
    frame #4: 0x00000001003b0b73 js-dbg-64-dm-nsprBuild-darwin-2e2244031032`js::gcstats::Statistics::endSlice(this=<unavailable>) + 195 at Statistics.cpp:1030
(lldb)
Depends on: 1150253
Fixed in bug 1150253
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.