1108159 - Assertion failure: debuggers, at vm/Debugger.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/basic/bug674776.js
var s = '';
for (var i = 0; i < 70000; i++)
 {
    s += 'function x' + i + '() { x' + i + '(); }
';
}
evaluate(s);
// Randomly chosen test: js/src/jit-test/tests/debug/Source-element-03.js
var g = newGlobal();
(new Debugger).addDebuggee(g);
g.offThreadCompileScript('debugger;',{});
g.runOffThreadScript();

asserts js debug shell on m-c changeset 29d086b32a26 without any CLI flags at Assertion failure: debuggers, at vm/Debugger.cpp.

I reproduced this using the build from:

https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-12-05-mozilla-central-debug/jsshell-linux-x86_64.zip

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

http://hg.mozilla.org/mozilla-central/file/29d086b32a26/js/src/jit-test/tests/basic/bug674776.js
http://hg.mozilla.org/mozilla-central/file/29d086b32a26/js/src/jit-test/tests/debug/Source-element-03.js

=== Tinderbox Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20141113141452" and the hash "5922f13f8439".
The "bad" changeset has the timestamp "20141113143950" and the hash "d65f01b9e6c6".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=5922f13f8439&tochange=d65f01b9e6c6

Shu-yu mentioned that bug 1062629 is a probable likely regressor.

Comment 1

[Christian Holler (:decoder)]

JSBugMon: Cannot process bug: Unknown exception (check manually)

Comment 2

[Shu-yu Guo [:shu]]

Attachment: Fix debuggers sweeping logic for off-thread "debuggee" compartments.

Comment 3

[Jim Blandy :jimb]

Comment on attachment 8532758 [details] [diff] [review]
Fix debuggers sweeping logic for off-thread "debuggee" compartments.

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

Looks good to me. Just some suggestions.

::: js/src/jscompartment.cpp
@@ +570,5 @@
>          // implies having at least one Debugger still attached. However, for
>          // off-thread compartments, which are used in off-thread parsing, they
>          // may be isDebuggee() without there being any Debuggers to prohibit
>          // asm.js.
> +        if (isDebuggee() && !options().invisibleToDebugger())

This is just a cleanup, right? Does this change have any effect?

::: js/src/vm/Debugger.cpp
@@ +2145,5 @@
>                  continue;
>  
> +            // See note in JSCompartment::sweepGlobalObject.
> +            if (c->options().invisibleToDebugger())
> +                continue;

It seems to me that this is, logically, part of the isDebuggee test ("No, *really*, is it a debuggee?"), and belongs in the 'if' condition. Is there a reason we need to check the global first??

Comment 4

[Shu-yu Guo [:shu]]

Attachment: Fix debuggers sweeping logic for off-thread "debuggee" compartments.

After discussion on IRC, we decided that just disabling asm.js compilation when
the host compartment is a debuggee is preferable to having a weird "is debuggee
but is also invisible to the debugger" state.

Comment 5

[Jim Blandy :jimb]

Comment on attachment 8533391 [details] [diff] [review]
Fix debuggers sweeping logic for off-thread "debuggee" compartments.

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

This is much, much nicer. Thanks!

Comment 6

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

Assigning to Shu-yu since he has a patch.

Comment 7

[Shu-yu Guo [:shu]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/263322550d0a

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/263322550d0a

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Fixed for Fx36 by the roll-up in bug 1114757.

Comment 10

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

The testcase in comment 0 had an accidental extra line break, so re-setting this:

// Randomly chosen test: js/src/jit-test/tests/basic/bug674776.js
var s = '';
for (var i = 0; i < 70000; i++)
 {
    s += 'function x' + i + '() { x' + i + '(); }';
}
evaluate(s);
// Randomly chosen test: js/src/jit-test/tests/debug/Source-element-03.js
var g = newGlobal();
(new Debugger).addDebuggee(g);
g.offThreadCompileScript('debugger;',{});
g.runOffThreadScript();

asserts js debug shell on m-c changeset 29d086b32a26 without any CLI flags at Assertion failure: debuggers, at vm/Debugger.cpp.