check liveness in hasAnyLiveHooks

RESOLVED FIXED in mozilla13

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: sfink, Assigned: sfink)

Tracking

unspecified
mozilla13
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

6 years ago
Currently, the debugger is not checking the liveness of the hooks. As a result, it can leak scripts when destroying a context.
(Assignee)

Comment 1

6 years ago
Created attachment 587541 [details] [diff] [review]
check liveness in hasAnyLiveHooks
Attachment #587541 - Flags: review?(jorendorff)
Comment on attachment 587541 [details] [diff] [review]
check liveness in hasAnyLiveHooks

I think Debugger::hasAnyLiveHooks is correct without the patch.

As it stands, GC can't cause an enabled Debugger to stop firing events. If you create a Debugger, add a debuggee, then leave the Debugger enabled but drop all references to it, it stays around. Debugging events still fire (which is observable; and they still receive the Debugger object as the this-value, so it can become gc-reachable again--in particular, it can be disabled and thus become garbage).

With this patch, that Debugger would vanish (unless one of its hooks happens to still be gc-reachable by some other path, perhaps via a global object or something). That seems wrong to me; GC shouldn't have that kind of side effect.
Attachment #587541 - Flags: review?(jorendorff) → review-
However there might be a leak here, in that if the Debugger has no debuggees, it really ought to be GC'd. Even though it is enabled, no events are going to fire.

Comment 4

6 years ago
(In reply to Jason Orendorff [:jorendorff] from comment #2)
> Debugging events still fire
> (which is observable; and they still receive the Debugger object as the
> this-value, so it can become gc-reachable again--in particular, it can be
> disabled and thus become garbage).

I agree with Jason here. Debuggers have visible side effects, so as long as there is debuggee code that might provoke it, a Debugger should be retained. (Saying that "it can become gc-reachable again" isn't how I would put it: the Debugger *is* gc-reachable all along.)
jimb pointed out that if a Debugger has no debuggees, and is otherwise unreachable, it will be GC'd. However we still think there is a bug in Debugger::markAllIteratively. After this code:

>    for (GlobalObjectSet::Range r = debuggees.all(); !r.empty(); r.popFront()) {
>        GlobalObject *global = r.front();

it should skip the rest of the loop if global itself is not marked. It doesn't; instead a garbage global can keep a Debugger alive, which is a leak.

(Ordinarily all the garbage globals will be collected, though, and then the Debugger will have no debuggees left, and so the *next* GC cycle will collect it. So the leak fixes itself--but if the Debugger happens to hold a strong reference to the debuggee global somehow, each will keep the other alive, and the cyclic garbage will never be GC'd.)
(Assignee)

Comment 6

6 years ago
Ok. I foolishly discarded my test case where I found the leak. It was a stripped-down version of jsapi-test/testDebugger.cpp (though the original worked too), but where I tracked the set of valid scripts in a hashtable using callbacks that were invoked under the same conditions as the debug script creation/destruction hooks. One of the scripts was not getting finalized (and therefore never triggered its destruction hooks.)

I'll try to reproduce that test case.
(Assignee)

Comment 7

6 years ago
Created attachment 588225 [details] [diff] [review]
imported patch bug--debugger-leak

This is my test case, but it won't show the problem for you. If you print out the scripts being created in js_CallNewScriptHook and the ones being destroyed in js_CallDestroyScriptHook (after the !script->callDestroyHook test), you'll find that the dummy script created for Function.prototype is never destroyed. I was trying to figure out how to make a jsapi-test test case for it, but I'd need to find the script and then override its debug hooks, which interferes with what's being tested. Is this enough to go on?
Attachment #588225 - Flags: review?
Attachment #588225 - Flags: review? → review?(jorendorff)
(Assignee)

Comment 8

6 years ago
Comment on attachment 588225 [details] [diff] [review]
imported patch bug--debugger-leak

Sorry, I posted the attachment via a script I was testing, and didn't mean to mark it for review. (Found a bug, though!) There's no reason to commit this.
Attachment #588225 - Flags: review?(jorendorff)
(Assignee)

Comment 9

6 years ago
Created attachment 589067 [details] [diff] [review]
GC'ed debuggee globals should not keep their debuggers alive

I don't pretend to understand how Debugger marking works, but your patch in comment 5 fixes my test case. And it even makes sense if I pretend to understand what's going on.
Attachment #589067 - Flags: review?(jorendorff)
(Assignee)

Updated

6 years ago
Attachment #587541 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Blocks: 698580
Comment on attachment 589067 [details] [diff] [review]
GC'ed debuggee globals should not keep their debuggers alive

Hmm, testing. I think findReferences can test that the debugger is reachable from the debuggee, but it can't test that the debugger is *not* reachable from a *dead* debuggee. But makeFinalizeObserver can do it--imprecisely.

This jit-test, at least, fails for me without the patch and passes with it. Please add it.

Actually please add two versions of it, one with the "dbg.loop = g" line and one without it.


// Bug 717104 - Unreachable debuggee globals should not keep their debuggers
// alive. The loop is to defeat conservative stack scanning; if the same stack
// locations are used each time through the loop, at least three of the
// debuggers should be collected.

for (var i = 0; i < 4; i++) {
    var g = newGlobal('new-compartment');
    var dbg = new Debugger(g);
    dbg.onDebuggerStatement = function () { throw "FAIL"; };
    dbg.o = makeFinalizeObserver();
    dbg.loop = g; // make a cycle of strong references with dbg and g
}

gc();
assertEq(finalizeCount() > 0, true);
Attachment #589067 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 11

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c1f8a450c8b
https://hg.mozilla.org/integration/mozilla-inbound/rev/72a8d7bb4058
Assignee: general → sphink
Flags: in-testsuite+
Target Milestone: --- → mozilla12
https://hg.mozilla.org/mozilla-central/rev/9c1f8a450c8b
https://hg.mozilla.org/mozilla-central/rev/72a8d7bb4058
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: mozilla12 → mozilla13
You need to log in before you can comment on or make changes to this bug.