Last Comment Bug 677386 - Fix jsdbg2 breakpoint GC rules
: Fix jsdbg2 breakpoint GC rules
Status: RESOLVED FIXED
[inbound]
: crash, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: x86_64 Linux
: -- critical (vote)
: ---
Assigned To: Jason Orendorff [:jorendorff]
:
: Jason Orendorff [:jorendorff]
Mentors:
: 681326 (view as bug list)
Depends on:
Blocks: langfuzz 681326
  Show dependency treegraph
 
Reported: 2011-08-08 15:10 PDT by Christian Holler (:decoder)
Modified: 2013-02-07 05:17 PST (History)
7 users (show)
choller: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (2.70 KB, patch)
2011-08-09 05:22 PDT, Jason Orendorff [:jorendorff]
no flags Details | Diff | Splinter Review
v2 (14.59 KB, patch)
2011-08-12 15:29 PDT, Jason Orendorff [:jorendorff]
no flags Details | Diff | Splinter Review
v3 (13.00 KB, patch)
2011-08-18 08:46 PDT, Jason Orendorff [:jorendorff]
wmccloskey: review+
Details | Diff | Splinter Review

Description Christian Holler (:decoder) 2011-08-08 15:10:52 PDT
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) 2011-08-08 15:21:39 PDT
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] 2011-08-09 05:22:42 PDT
Created attachment 551726 [details] [diff] [review]
v1
Comment 3 Jason Orendorff [:jorendorff] 2011-08-09 15:18:43 PDT
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] 2011-08-12 15:29:11 PDT
Created attachment 552778 [details] [diff] [review]
v2
Comment 5 Jason Orendorff [:jorendorff] 2011-08-18 08:46:07 PDT
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.
Comment 6 [PTO to Dec5] Bill McCloskey (:billm) 2011-08-18 13:19:45 PDT
(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] 2011-08-18 15:14:54 PDT
Sorry about that! Of course it can wait. :)
Comment 8 [PTO to Dec5] Bill McCloskey (:billm) 2011-08-22 11:30:56 PDT
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] 2011-08-24 14:01:16 PDT
(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 12 Jason Orendorff [:jorendorff] 2011-08-31 09:19:28 PDT
*** Bug 681326 has been marked as a duplicate of this bug. ***
Comment 13 Christian Holler (:decoder) 2013-02-07 05:17:03 PST
Automatically extracted testcase for this bug was committed:

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

Note You need to log in before you can comment on or make changes to this bug.