956324 - GC: Debugger can hold pointers to dead breakpoint handler objects

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jon Coppeard (:jonco)]

The debugger mochitests are failing like this:

09:39:59  WARNING -  PROCESS-CRASH | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_pretty-print-09.js | application crashed [@ js::gc::Cell::tenuredZone() const]
09:39:59     INFO -  Crash dump filename: /tmp/tmp2fDLYU/minidumps/7dccfba4-bb16-0879-3e50a54d-58b5c9cb.dmp
09:39:59     INFO -  Operating system: Linux
09:39:59     INFO -                    0.0.0 Linux 2.6.31.5-127.fc12.i686.PAE #1 SMP Sat Nov 7 21:25:57 EST 2009 i686
09:39:59     INFO -  CPU: x86
09:39:59     INFO -       GenuineIntel family 6 model 23 stepping 10
09:39:59     INFO -       2 CPUs
09:39:59     INFO -  Crash reason:  SIGSEGV
09:39:59     INFO -  Crash address: 0xdadffffc
09:39:59     INFO -  Thread 0 (crashed)
09:39:59     INFO -   0  libxul.so!js::gc::Cell::tenuredZone() const [HeapAPI.h:8d11330faa7e : 133 + 0x0]
09:39:59     INFO -      eip = 0x02e546b3   esp = 0xbfb95d10   ebp = 0xbfb95d48   ebx = 0x0500964c
09:39:59     INFO -      esi = 0xdadadada   edi = 0xdadffffc   eax = 0xdadadada   ecx = 0x03bd8129
09:39:59     INFO -      edx = 0x13af3328   efl = 0x00210286
09:39:59     INFO -      Found by: given as instruction pointer in context
09:39:59     INFO -   1  libxul.so!js::gc::BarrieredCell<js::ObjectImpl>::zone() const [Barrier.h:8d11330faa7e : 185 + 0xc]
09:39:59     INFO -      eip = 0x02ebd004   esp = 0xbfb95d50   ebp = 0xbfb95d68   ebx = 0x0500964c
09:39:59     INFO -      esi = 0xbfb95e94   edi = 0x8ce16790
09:39:59     INFO -      Found by: call frame info
09:39:59     INFO -   2  libxul.so!MarkInternal<JSObject> [Marking.cpp:8d11330faa7e : 133 + 0x7]
09:39:59     INFO -      eip = 0x02ec4b88   esp = 0xbfb95d70   ebp = 0xbfb95da8   ebx = 0x0500964c
09:39:59     INFO -      esi = 0xbfb95e94   edi = 0x8ce16790
09:39:59     INFO -      Found by: call frame info
09:39:59     INFO -   3  libxul.so!js::Debugger::markAll(JSTracer*) [Debugger.cpp:8d11330faa7e : 1576 + 0x15]
09:39:59     INFO -      eip = 0x031f831b   esp = 0xbfb95db0   ebp = 0xbfb95e48   ebx = 0x0500964c
09:39:59     INFO -      esi = 0xbfb95e94   edi = 0x13af3320
09:39:59     INFO -      Found by: call frame info
09:39:59     INFO -   4  libxul.so!js::Nursery::collect(JSRuntime*, JS::gcreason::Reason, js::Vector<js::types::TypeObject*, 0u, js::SystemAllocPolicy>*) [Nursery.cpp:8d11330faa7e : 633 + 0x7]
09:39:59     INFO -      eip = 0x02ed3322   esp = 0xbfb95e50   ebp = 0xbfb95f78   ebx = 0x0500964c
09:39:59     INFO -      esi = 0xbfb95e94   edi = 0x08c273e0
09:39:59     INFO -      Found by: call frame info
09:39:59     INFO -   5  libxul.so!js::MinorGC(JSRuntime*, JS::gcreason::Reason) [jsgc.cpp:8d11330faa7e : 5030 + 0x1b]
09:39:59     INFO -      eip = 0x030e3587   esp = 0xbfb95f80   ebp = 0xbfb95f98   ebx = 0x0500964c
09:39:59     INFO -      esi = 0xbfb96190   edi = 0xbfb96198
09:39:59     INFO -      Found by: call frame info
09:39:59     INFO -   6  libxul.so!js::IterateScripts(JSRuntime*, JSCompartment*, void*, void (*)(JSRuntime*, void*, JSScript*)) [Iteration.cpp:8d11330faa7e : 95 + 0xe]
09:39:59     INFO -      eip = 0x02eb67db   esp = 0xbfb95fa0   ebp = 0xbfb96068   ebx = 0x0500964c
09:39:59     INFO -      esi = 0xbfb96190   edi = 0xbfb96198
09:39:59     INFO -      Found by: call frame info
09:39:59     INFO -   7  libxul.so!js::Debugger::ScriptQuery::findScripts(JS::AutoScriptVector*) [Debugger.cpp:8d11330faa7e : 2448 + 0x9]

It's not always the same test, but it's the same backtrace.  Debugger::findScripts() is causing a minor GC, and this encounters dead objects when iterating breakpoints in Debugger::markAll().

Comment 1

[Jon Coppeard (:jonco)]

More often test browser/devtools/debugger/test/browser_dbg_source-maps-02.js crashes like this.

It's reproducable locally but only by running the test in a loop for ~20mins.

The problem is that a breakpoint's handler object has been collected, but the breakpoint is still alive.  This should not be possible as Debugger::markAllIncrementally() marks the handler object if the breakpoint is alive.

Comment 2

[Jon Coppeard (:jonco)]

Debugger::markAllIteratively() marks breakpoint handler objects, but it finds them via their debuggees.  This has the consequence that if a debugger has all of its debuggees removed while it still has breakpoints, those breakpoints are not marked and the possibility arises that the handler object can be collected while the debugger still has a pointer to it.

This is actually not a GGC specific problem, it's just that the list of breakpoints is rarely traversed.  The markAll() function called at the start of minor GC does this though.

I don't know what the semantics of the debugger are meant to be in this case, but a possible fix is to remove all of a debugger's breakpoints when the last of its debuggees is removed.

Jim does this sound like a good way to proceed?

Comment 3

[Jon Coppeard (:jonco)]

Attachment: debugger-crash

Potential fix.

Comment 4

[Jon Coppeard (:jonco)]

Comment on attachment 8359133 [details] [diff] [review]
debugger-crash

Jim doesn't seem to be around, so assigning this to you for review.  Feel free to r- if we actually don't want this behaviour.

Comment 5

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

Nice find. I'd love to give you an r+ on this. I suspect it's been causing intermittent oranges for a long time. I don't know enough to say whether the fix is correct, though. It seems possible that we want this behavior: add breakpoint, remove global from debugger, add it back, expect the breakpoint is still there. I asked Jason whether this is desirable and he said to ask Jim :-).

However, if it *is* safe to remove breakpoints when removing a debuggee global, I think we should remove them unconditionally (not just if the debugger no longer has any more globals). Otherwise the semantics would just be weird.

An alternate approach would be to simplify markAllIteratively. It's really complex, and I'm not sure why the complexity is needed. It looks like we have a list of all debuggers in the runtime (JSRuntime::debuggerList). Why don't we just iterate over that to find all the debuggers? Then we wouldn't have to worry about missing debuggers without any debugees.

I no longer remember why we did it the current way. I think it had something to do with compartment GCs, but that really doesn't make any sense right now. I think a full traversal of all debuggers would be a lot faster than what we're doing now (going through all compartments).

Comment 6

[Jim Blandy :jimb]

In our tools, we never actually remove debuggees (it seems). So forgetting a debuggee's breakpoints when it is removed as a debuggee won't be a compatibility issue. I'll take a look at the patch.

Comment 7

[Jim Blandy :jimb]

Comment on attachment 8359133 [details] [diff] [review]
debugger-crash

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

::: js/src/gc/Marking.cpp
@@ +154,5 @@
>      JS_ASSERT_IF(IS_GC_MARKING_TRACER(trc) && AsGCMarker(trc)->getMarkColor() == GRAY,
>                   !thing->zone()->isGCMarkingBlack() || rt->isAtomsZone(thing->zone()));
>  
> +    JS_ASSERT_IF(IS_GC_MARKING_TRACER(trc),
> +                 !(thing->zone()->isGCSweeping() || thing->zone()->isGCFinished()));

This is just saying, "If we're marking with this tracer, then we certainly shouldn't be sweeping, and we shouldn't be marked as done with the GC", right?

::: js/src/vm/Debugger.cpp
@@ +2316,5 @@
> +        for (Breakpoint *bp = firstBreakpoint(); bp; bp = nextbp) {
> +            nextbp = bp->nextInDebugger();
> +            bp->destroy(fop);
> +        }
> +    }

Could we change this to instead always drop breakpoints in the removed debuggee? That seems easier to reason about than having special behavior when the last debuggee is removed. ("My breakpoints will persist, unless at some intervening time the debuggee count dropped to zero" is not very friendly.)

Comment 8

[Jon Coppeard (:jonco)]

(In reply to Jim Blandy :jimb from comment #7)
> > +    JS_ASSERT_IF(IS_GC_MARKING_TRACER(trc),
> > +                 !(thing->zone()->isGCSweeping() || thing->zone()->isGCFinished()));

This is for incremental sweeping, to say that we're not trying to mark something in a zone that we already finished marking in.  That would mean we might have already swept something that turned out to be reachable after all.  We sweep 
zones in groups and calculate the order such that this should be impossible.

> Could we change this to instead always drop breakpoints in the removed
> debuggee? That seems easier to reason about than having special behavior
> when the last debuggee is removed. ("My breakpoints will persist, unless at
> some intervening time the debuggee count dropped to zero" is not very
> friendly.)

That sounds good to me.

Comment 9

[Jon Coppeard (:jonco)]

Attachment: debugger-crash-v2

Updated patch.

Comment 10

[Jon Coppeard (:jonco)]

Comment on attachment 8361138 [details] [diff] [review]
debugger-crash-v2

Jim doesn't seem to be responding.  Bill could you review?

As above, Jim says we do want to drop all breakpoints for a debuggee when that debuggee is removed.

Comment 11

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b8a6bf4b4fa3

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b8a6bf4b4fa3