Closed Bug 677386 Opened 8 years ago Closed 8 years ago

Fix jsdbg2 breakpoint GC rules

Categories

(Core :: JavaScript Engine, defect, critical)

Other Branch
x86_64
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED

People

(Reporter: decoder, Assigned: jorendorff)

References

(Blocks 1 open bug)

Details

(Keywords: crash, testcase, Whiteboard: [inbound])

Crash Data

Attachments

(1 file, 2 obsolete files)

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);
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
Attached patch v1 (obsolete) — Splinter Review
Assignee: general → jorendorff
Attachment #551726 - Flags: review?(jimb)
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)
Attached patch v2 (obsolete) — Splinter Review
Attachment #551726 - Attachment is obsolete: true
Attachment #552778 - Flags: review?(jimb)
Attached patch v3Splinter Review
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.
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+
(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.
Summary: [jsdbg2] Crash [@ js_GetMethod] → Fix jsdbg2 breakpoint GC rules
Blocks: 681326
Duplicate of this bug: 681326
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.