1110327 - Assertion failure: dbg->isDebuggee(node.compartment()), at vm/Debugger.cpp

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

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

// Randomly chosen test: js/src/jit-test/tests/debug/Memory-drainAllocationsLog-05.js
// -- reduced away --
// Randomly chosen test: js/src/jit-test/tests/debug/Debugger-debuggees-10.js
x = newGlobal()
x.t = this
// Randomly chosen test: js/src/jit-test/tests/debug/Debugger-findObjects-06.js
Debugger(x).findObjects()

asserts js debug shell on m-c changeset 0cf461e62ce5 with --fuzzing-safe --no-threads --ion-eager at Assertion failure: dbg->isDebuggee(node.compartment()), at vm/Debugger.cpp.

Debug 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-optimize --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

This was found by combining random js tests together with jsfunfuzz, the specific file(s) is/are:

http://hg.mozilla.org/mozilla-central/file/0cf461e62ce5/js/src/jit-test/tests/debug/Memory-drainAllocationsLog-05.js
http://hg.mozilla.org/mozilla-central/file/0cf461e62ce5/js/src/jit-test/tests/debug/Debugger-debuggees-10.js
http://hg.mozilla.org/mozilla-central/file/0cf461e62ce5/js/src/jit-test/tests/debug/Debugger-findObjects-06.js

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/3644e0bac5b6
user:        Nick Fitzgerald
date:        Wed Dec 10 09:47:21 2014 -0800
summary:     Bug 1108149 - Make ObjectQuery::findObjects use JS::ubi::RootList; r=shu

Nick, is bug 1108149 a likely regressor?

Comment 1

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

Attachment: stack

(lldb) bt 5
* thread #1: tid = 0x8535, 0x000000010069acf0 js-dbg-opt-64-dm-nsprBuild-darwin-0cf461e62ce5`js::Debugger::ObjectQuery::findObjects(JS::AutoObjectVector&) [inlined] js::detail::HashTable<js::GlobalObject* const, js::HashSet<js::GlobalObject*, js::DefaultHasher<js::GlobalObject*>, js::SystemAllocPolicy>::SetOps, js::SystemAllocPolicy>::all() const + 28 at HashTable.h:1507, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x000000010069acf0 js-dbg-opt-64-dm-nsprBuild-darwin-0cf461e62ce5`js::Debugger::ObjectQuery::findObjects(JS::AutoObjectVector&) [inlined] js::detail::HashTable<js::GlobalObject* const, js::HashSet<js::GlobalObject*, js::DefaultHasher<js::GlobalObject*>, js::SystemAllocPolicy>::SetOps, js::SystemAllocPolicy>::all() const + 28 at HashTable.h:1507
    frame #1: 0x000000010069acd4 js-dbg-opt-64-dm-nsprBuild-darwin-0cf461e62ce5`js::Debugger::ObjectQuery::findObjects(JS::AutoObjectVector&) [inlined] js::HashSet<js::GlobalObject*, js::DefaultHasher<js::GlobalObject*>, js::SystemAllocPolicy>::all(this=<unavailable>) const at HashTable.h:393
    frame #2: 0x000000010069acd4 js-dbg-opt-64-dm-nsprBuild-darwin-0cf461e62ce5`js::Debugger::ObjectQuery::findObjects(JS::AutoObjectVector&) [inlined] js::Debugger::allDebuggees(this=<unavailable>) const at Debugger.h:501
    frame #3: 0x000000010069acd4 js-dbg-opt-64-dm-nsprBuild-darwin-0cf461e62ce5`js::Debugger::ObjectQuery::findObjects(this=<unavailable>, objs=<unavailable>) + 1876 at Debugger.cpp:3566
    frame #4: 0x000000010064154b js-dbg-opt-64-dm-nsprBuild-darwin-0cf461e62ce5`js::Debugger::findObjects(cx=0x0000000101d02510, argc=<unavailable>, vp=0x00007fff5fbfe8e8) + 379 at Debugger.cpp:3687
(lldb)

Comment 2

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

Yeah, looks to be caused by my patch.

Comment 3

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

Attachment: bug-1110327-findObjects-isDebuggee-assertion-failure.patch

Comment 4

[Shu-yu Guo [:shu]]

Comment on attachment 8536895 [details] [diff] [review]
bug-1110327-findObjects-isDebuggee-assertion-failure.patch

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

::: js/src/vm/Debugger.cpp
@@ +3577,5 @@
>                  return false;
>              traversal.wantNames = false;
>  
> +            return traversal.addStart(JS::ubi::Node(&rootList)) &&
> +                traversal.traverse();

Nit: line up the 2nd line's traversal to the 1st line's traversal.

@@ +3597,3 @@
>          /* Only follow edges within our set of debuggee compartments. */
> +        JSCompartment *comp = referent.compartment();
> +        if (comp && !dbg->isDebuggee(edge.referent.compartment())) {

if (comp && !dbg->isDebuggee(comp))

@@ +3606,5 @@
> +         * add it to the vector accumulating results.
> +         */
> +
> +        if (!referent.is<JSObject>())
> +            return true;

Just to make sure I understand: this case and the className case below don't need to call traversal.abandonReferent() because they may contain edges to other JSObjects that we do care about? Whereas above, if we abandon the referent of a node that isn't in a debuggee compartment because we don't care about any objects reachable from that node, as they'll also all be in a non-debuggee compartment.

@@ +3662,5 @@
> +     * Accumulate the objects in an AutoObjectVector, instead of creating the JS
> +     * array as we go, because we mustn't allocate JS objects or GC while we
> +     * traverse the heap graph.
> +     */
> +    AutoObjectVector objects(cx);

API suggestion: perhaps move this vector into ObjectQuery entirely, instead of passing it in.

Comment 5

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

(In reply to Shu-yu Guo [:shu] from comment #4)
> @@ +3606,5 @@
> > +         * add it to the vector accumulating results.
> > +         */
> > +
> > +        if (!referent.is<JSObject>())
> > +            return true;
> 
> Just to make sure I understand: this case and the className case below don't
> need to call traversal.abandonReferent() because they may contain edges to
> other JSObjects that we do care about? Whereas above, if we abandon the
> referent of a node that isn't in a debuggee compartment because we don't
> care about any objects reachable from that node, as they'll also all be in a
> non-debuggee compartment.

Exactly, and if one of these non-debuggee objects has a cross compartment reference back into one of our debuggee compartments, then the incoming CCW edge would be an edge in our RootList. Will expand the comment to have a little more details.

> 
> @@ +3662,5 @@
> > +     * Accumulate the objects in an AutoObjectVector, instead of creating the JS
> > +     * array as we go, because we mustn't allocate JS objects or GC while we
> > +     * traverse the heap graph.
> > +     */
> > +    AutoObjectVector objects(cx);
> 
> API suggestion: perhaps move this vector into ObjectQuery entirely, instead
> of passing it in.

Will do.

Comment 6

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/f082c7cfc767

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f082c7cfc767