Fix jsdbg2 breakpoint GC rules

RESOLVED FIXED

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: decoder, Assigned: jorendorff)

Tracking

(Blocks: 1 bug, {crash, testcase})

Other Branch
x86_64
Linux
crash, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [inbound], crash signature)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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);
(Reporter)

Comment 1

6 years ago
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
(Assignee)

Comment 2

6 years ago
Created attachment 551726 [details] [diff] [review]
v1
Assignee: general → jorendorff
Attachment #551726 - Flags: review?(jimb)
(Assignee)

Comment 3

6 years ago
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.
Attachment #551726 - Flags: review?(jimb)
(Assignee)

Comment 4

6 years ago
Created attachment 552778 [details] [diff] [review]
v2
Attachment #551726 - Attachment is obsolete: true
Attachment #552778 - Flags: review?(jimb)
(Assignee)

Comment 5

6 years ago
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.

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.
Attachment #552778 - Attachment is obsolete: true
Attachment #552778 - Flags: review?(jimb)
Attachment #554097 - Flags: review?(wmccloskey)
(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.
(Assignee)

Comment 7

6 years ago
Sorry about that! Of course it can wait. :)
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.
Attachment #554097 - Flags: review?(wmccloskey) → review+
(Assignee)

Comment 9

6 years ago
(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.
(Assignee)

Updated

6 years ago
Summary: [jsdbg2] Crash [@ js_GetMethod] → Fix jsdbg2 breakpoint GC rules
(Assignee)

Comment 10

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/db8de6cd0712
http://hg.mozilla.org/integration/mozilla-inbound/rev/6daedd1baec4
Whiteboard: [inbound]
Blocks: 681326
http://hg.mozilla.org/mozilla-central/rev/6daedd1baec4
http://hg.mozilla.org/mozilla-central/rev/db8de6cd0712
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Updated

6 years ago
Duplicate of this bug: 681326
(Reporter)

Comment 13

4 years ago
Automatically extracted testcase for this bug was committed:

https://hg.mozilla.org/mozilla-central/rev/2e891e0db397
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.