677386 - Fix jsdbg2 breakpoint GC rules

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following code crashes on jsdbg2 branch (revision 82545b1e4129, options -j -m -a -d):


var g = newGlobal('new-compartment');
g.eval("var line0 = Error().lineNumber;\n" +
       "function f() {\n" +     // line0 + 1
       "    return 2;\n" +      // line0 + 2
       "}\n");
var N = 4;
for (var i = 0; i < N; i++) {
    var dbg = Debugger(g);
    dbg.onDebuggerStatement = function (frame) {
        var handler = {hit: function () { hits++; }};
        var s = frame.eval("f").return.script;
        var offs = s.getLineOffsets(g.line0 + 2);
        for (var j = 0; j < offs.length; j++)
            s.setBreakpoint(offs[j], handler);
    };
    g.eval('debugger;');
}
gc(/c$...$/);
assertEq(g.f(), 2);

Comment 1

[Christian Holler (:decoder)]

Valgrind stack:

==11943== Invalid read of size 8
==11943==    at 0x4F53AB: js_GetMethod(JSContext*, JSObject*, jsid, unsigned int, js::Value*) (jsobj.cpp:5405)
==11943==    by 0x5CDBE2: CallMethodIfPresent(JSContext*, JSObject*, char const*, int, js::Value*, js::Value*) (Debugger.cpp:663)
==11943==    by 0x5CED36: js::Debugger::onTrap(JSContext*, js::Value*) (Debugger.cpp:893)
==11943==    by 0x74FF25: js::mjit::stubs::Trap(js::VMFrame&, unsigned int) (StubCalls.cpp:1199)
==11943==    by 0x4045339: ???
==11943==    by 0x67C9F7: js::mjit::EnterMethodJIT(JSContext*, js::StackFrame*, void*, js::Value*) (MethodJIT.cpp:686)
==11943==    by 0x67CB41: CheckStackAndEnterMethodJIT(JSContext*, js::StackFrame*, void*) (MethodJIT.cpp:716)
==11943==    by 0x67CC22: js::mjit::JaegerShot(JSContext*) (MethodJIT.cpp:733)
==11943==    by 0x4D0C94: js::RunScript(JSContext*, JSScript*, js::StackFrame*) (jsinterp.cpp:598)
==11943==    by 0x4D1083: js::Invoke(JSContext*, js::CallArgs const&, js::MaybeConstruct) (jsinterp.cpp:674)
==11943==    by 0x452DA1: js::Invoke(JSContext*, js::InvokeArgsGuard&, js::MaybeConstruct) (jsinterp.h:169)
==11943==    by 0x4D164C: js::ExternalInvoke(JSContext*, js::Value const&, js::Value const&, unsigned int, js::Value*, js::Value*) (jsinterp.cpp:793)
==11943==  Address 0xdadadadadadadba2 is not stack'd, malloc'd or (recently) free'd

Comment 2

[Jason Orendorff [:jorendorff]]

Attachment: v1

Comment 3

[Jason Orendorff [:jorendorff]]

Comment on attachment 551726 [details] [diff] [review]
v1

This isn't quite the right fix. It keeps handlers alive even if the script gets GC'd. Withdrawing.

Christian: Please continue using this patch for testing, for now.

Comment 4

[Jason Orendorff [:jorendorff]]

Attachment: v2

Comment 5

[Jason Orendorff [:jorendorff]]

Attachment: v3

This is the same as v2 but with a better comment.

Shifting review from jimb (who is on vacation) to billm.

The GC rules for breakpoints were wrong at least in the case of single-compartment GC. I just hadn't thought it through carefully. The new comment on class Breakpoint in Debugger.h lays out the rules more explicitly. It took a long time to settle on these rules, so all this looks more elegant than it is.

Anyway, here's what the comment says:

> * GC rules:
> *   - script is live, breakpoint exists, and debugger is enabled
> *      ==> debugger is live
> *   - script is live, breakpoint exists, and debugger is live
> *      ==> retain the breakpoint and the handler object is live

Rule 1 tells precisely when a breakpoint keeps a debugger alive.

Rule 2 tells precisely when a debugger keeps a breakpoint alive.

> * Debugger::markAllIteratively implements these two rules. It uses
> * Debugger::hasAnyLiveHooks to check for rule 1.
> *
> * Nothing else causes a breakpoint to be retained, so if its script or
> * debugger is collected, the breakpoint is destroyed during GC sweep phase,
> * even if the debugger compartment isn't being GC'd. This is implemented in
> * JSCompartment::sweepBreakpoints.

The implementation is slightly complicated by the quirk that scripts don't have their own mark bits. So bug 674251, which fixes that, will result in further simplification here, but with no conflicts and no need for either bug to depend on the other.

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Jason Orendorff [:jorendorff] from comment #5)
> Created attachment 554097 [details] [diff] [review]
> v3
> 
> This is the same as v2 but with a better comment.
> 
> Shifting review from jimb (who is on vacation) to billm.

Can this wait until Monday? I'm also on vacation.

Comment 7

[Jason Orendorff [:jorendorff]]

Sorry about that! Of course it can wait. :)

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 554097 [details] [diff] [review]
v3

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

::: js/src/jscompartment.cpp
@@ +797,5 @@
>      }
>  }
>  
>  bool
> +JSCompartment::markTrapClosuresIteratively(JSTracer *trc)

I have an unrelated question about the code to mark the trap closures. The rules for when to mark them don't make sense to me. I think you might have reversed a branch during the !isMarked -> IsAboutToBeFinalized conversion. It might be good to audit the rest of these to make sure they're correct.

::: js/src/vm/Debugger.cpp
@@ +1018,4 @@
>                   */
> +                JSObject *dbgobj = dbg->toJSObject();
> +                JSCompartment *dbgcomp = dbgobj->compartment();
> +                if (comp != dbgcomp && comp != NULL)

Doing |if (comp && comp != dbgcomp)| looks more natural to me. Also, it's a bit confusing to have both dc and dbgcomp. Maybe just use dbgobj->compartment() in the branch and eliminate the variable?

@@ +1020,5 @@
> +                JSCompartment *dbgcomp = dbgobj->compartment();
> +                if (comp != dbgcomp && comp != NULL)
> +                    continue;
> +
> +                bool dbgLive = !IsAboutToBeFinalized(cx, dbgobj);

I think dbgMarked would be a better name. We haven't really decided if it's live yet.

::: js/src/vm/Debugger.h
@@ +455,5 @@
> + *
> + * Nothing else causes a breakpoint to be retained, so if its script or
> + * debugger is collected, the breakpoint is destroyed during GC sweep phase,
> + * even if the debugger compartment isn't being GC'd. This is implemented in
> + * JSCompartment::sweepBreakpoints.

Nice comment. Thanks.

Comment 9

[Jason Orendorff [:jorendorff]]

(In reply to Bill McCloskey (:billm) from comment #8)
> > +JSCompartment::markTrapClosuresIteratively(JSTracer *trc)
> 
> I have an unrelated question about the code to mark the trap closures. The
> rules for when to mark them don't make sense to me. I think you might have
> reversed a branch during the !isMarked -> IsAboutToBeFinalized conversion.
> It might be good to audit the rest of these to make sure they're correct.

Yep, it was upside down. I have the fix (which you reviewed on IRC--I'll check it in today, with any luck).

I reviewed all the IsAboutToBeFinalized calls in vm/Debugger.cpp (four of them) and the debugger-related ones in jscompartment.cpp (seven). All the others look OK.

> Doing |if (comp && comp != dbgcomp)| looks more natural to me. Also, it's a
> bit confusing to have both dc and dbgcomp. Maybe just use
> dbgobj->compartment() in the branch and eliminate the variable?

Done.

> Nice comment. Thanks.

Back atcha.

Comment 10

[Jason Orendorff [:jorendorff]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/db8de6cd0712
http://hg.mozilla.org/integration/mozilla-inbound/rev/6daedd1baec4

Comment 11

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/6daedd1baec4
http://hg.mozilla.org/mozilla-central/rev/db8de6cd0712

Comment 13

[Christian Holler (:decoder)]

Automatically extracted testcase for this bug was committed:

https://hg.mozilla.org/mozilla-central/rev/2e891e0db397