Last Comment Bug 717104 - check liveness in hasAnyLiveHooks
: check liveness in hasAnyLiveHooks
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla13
Assigned To: Steve Fink [:sfink] [:s:] (PTO Sep23-28)
:
:
Mentors:
Depends on:
Blocks: 698580
  Show dependency treegraph
 
Reported: 2012-01-10 16:58 PST by Steve Fink [:sfink] [:s:] (PTO Sep23-28)
Modified: 2012-02-02 07:23 PST (History)
2 users (show)
sphink: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
check liveness in hasAnyLiveHooks (2.41 KB, patch)
2012-01-10 16:59 PST, Steve Fink [:sfink] [:s:] (PTO Sep23-28)
jorendorff: review-
Details | Diff | Splinter Review
imported patch bug--debugger-leak (2.28 KB, patch)
2012-01-12 16:04 PST, Steve Fink [:sfink] [:s:] (PTO Sep23-28)
no flags Details | Diff | Splinter Review
GC'ed debuggee globals should not keep their debuggers alive (1.19 KB, patch)
2012-01-16 17:05 PST, Steve Fink [:sfink] [:s:] (PTO Sep23-28)
jorendorff: review+
Details | Diff | Splinter Review

Description Steve Fink [:sfink] [:s:] (PTO Sep23-28) 2012-01-10 16:58:51 PST
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:] (PTO Sep23-28) 2012-01-10 16:59:01 PST
Created attachment 587541 [details] [diff] [review]
check liveness in hasAnyLiveHooks
Comment 2 Jason Orendorff [:jorendorff] 2012-01-12 13:58:28 PST
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] 2012-01-12 13:59:23 PST
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 2012-01-12 14:52:50 PST
(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] 2012-01-12 14:56:15 PST
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:] (PTO Sep23-28) 2012-01-12 15:35:24 PST
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:] (PTO Sep23-28) 2012-01-12 16:04:17 PST
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?
Comment 8 Steve Fink [:sfink] [:s:] (PTO Sep23-28) 2012-01-13 14:50:17 PST
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:] (PTO Sep23-28) 2012-01-16 17:05:19 PST
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.
Comment 10 Jason Orendorff [:jorendorff] 2012-01-19 13:17:16 PST
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);

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