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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

// 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?

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: 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)

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Yeah that's the regressor.

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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?

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Fix assertion failure in Debugger::fireOnGarbageCollectionHook

Comment 5

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

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.

Comment 6

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

This comment 0 testcase also crashes js opt shell at js::Debugger::fireOnGarbageCollectionHook

Comment 7

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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)?

Comment 8

[Terrence Cole [: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.

Comment 9

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

// 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

Comment 10

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: Stack for testcase in comment 9

(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)

Comment 11

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Fixed in bug 1150253