717104 - check liveness in hasAnyLiveHooks

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Steve Fink [:sfink] [:s:]]

Currently, the debugger is not checking the liveness of the hooks. As a result, it can leak scripts when destroying a context.

Comment 1

[Steve Fink [:sfink] [:s:]]

Attachment: check liveness in hasAnyLiveHooks

Comment 2

[Jason Orendorff [: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.

Comment 3

[Jason Orendorff [:jorendorff]]

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

[Jim Blandy :jimb]

(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.)

Comment 5

[Jason Orendorff [:jorendorff]]

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.)

Comment 6

[Steve Fink [:sfink] [:s:]]

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.

Comment 7

[Steve Fink [:sfink] [:s:]]

Attachment: 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?

Comment 8

[Steve Fink [:sfink] [:s:]]

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.

Comment 9

[Steve Fink [:sfink] [:s:]]

Attachment: 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.

Comment 10

[Jason Orendorff [:jorendorff]]

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);

Comment 11

[Steve Fink [:sfink] [:s:]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9c1f8a450c8b
https://hg.mozilla.org/integration/mozilla-inbound/rev/72a8d7bb4058

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9c1f8a450c8b
https://hg.mozilla.org/mozilla-central/rev/72a8d7bb4058