js::NukeCrossCompartmentWrappers only does things to wrappers that have kind CrossCompartmentKey::ObjectWrapper, but (aside from the deliberately avoid strings) there is also DebuggerScript, DebuggerSource, DebuggerObject, DebuggerEnvironment. As bug 1084605 shows, this is a bad situation, so we should extend support for them there. This is probably only a danger for people who use devtools, but that's still really bad.

Thanks to billm for pointing out the relevant bits of code to me.

The obvious fix here of calling NukeCompartmentWrappers on all non-string CCWs didn't fix the leak: the Debugger object was still holding a reference to content.

Is this something you could look into, Shu? Thanks.

(In reply to Andrew McCreight [:mccr8] from comment #3) > Is this something you could look into, Shu? Thanks. Judging by comments 5 and 6 on bug 1084605, and comment 2 here that says nuking non-string CCWs didn't fix the leak: is this a Debugger issue, or a frontend issue? If it's the latter, I probably won't be of much help.

(In reply to Shu-yu Guo [:shu] from comment #4) > Judging by comments 5 and 6 on bug 1084605, and comment 2 here that says > nuking non-string CCWs didn't fix the leak: is this a Debugger issue, or a > frontend issue? If it's the latter, I probably won't be of much help. There are two issues. First, the dev tools front end code shouldn't keep alive this debugger object, which is what bug 1084605 is about. Second, Gecko shouldn't let debugger code keep alive closed pages, which is what this bug is about.

Ah ha, I figured out why calling NukeCCWs on all-non-string CCWs didn't fix the leak. UncheckedUnwrap doesn't know how to unwrap the various Debugger* wrappers, so they act as identity on the Debugger objects. This means that the target compartment will *never* match, because the Debugger objects obviously are compartmentalized to the chrome compartment, not the outer window that just closed. The fix here would be 1) Teach UncheckedUnwrap to unwrap the various Debugger wrappers. I'm fairly sure that you can end up with Debugger wrappers of CCWs. 2) Teach NukeCrossCompartmentWrapper how to nuke Debugger wrappers with a sentinel dead value. 3) Make sure that all the Debugger wrappers check their referents being the sentinel value and throw.

(In reply to Shu-yu Guo [:shu] from comment #6) > 1) Teach UncheckedUnwrap to unwrap the various Debugger wrappers. I'm > fairly sure that you can end up with Debugger wrappers of CCWs. You definitely can: wrappers can affect the debuggee's view of an object, so Debugger must preserve that behavior. We have tests for this. > 2) Teach NukeCrossCompartmentWrapper how to nuke Debugger wrappers with a > sentinel dead value. > 3) Make sure that all the Debugger wrappers check their referents being the > sentinel value and throw. Sounds perfect.

Comment on attachment 8546271 [details] [diff] [review] Hueyfix Debugger objects. Review of attachment 8546271 [details] [diff] [review]: ----------------------------------------------------------------- Switching review to sfink to ease review load on jimb.

Comment on attachment 8546271 [details] [diff] [review] Hueyfix Debugger objects. Review of attachment 8546271 [details] [diff] [review]: ----------------------------------------------------------------- This stuff is crazy and scary, but it looks ok according to my myopic understanding of the JS engine. ::: js/src/proxy/CrossCompartmentWrapper.cpp @@ +488,5 @@ > AutoWrapperRooter wobj(cx, WrapperValue(e)); > JSObject *wrapped = UncheckedUnwrap(wobj); > > if (nukeReferencesToWindow == DontNukeWindowReferences && > wrapped->getClass()->ext.innerObject) If it's easy, you could hoist up this test into a shared ShouldNotNuke, but maybe it's not worth the trouble.

So hueyfixing Debugger objects makes dt fail, and I suppose exposes leaks: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d5c0261ff15 Can we get someone from devtools to look at those?

(In reply to Shu-yu Guo [:shu] from comment #11) > So hueyfixing Debugger objects makes dt fail, and I suppose exposes leaks: > > https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d5c0261ff15 > > Can we get someone from devtools to look at those? To see that they're leaks, open the log and grep for "can't access dead object".

On second thought I'm going to unassign myself from the bug. Bug 1120784 isn't really something I'm qualified to fix.

Unrotted for testing.

(In reply to Shu-yu Guo [:shu] from comment #11) > So hueyfixing Debugger objects makes dt fail, and I suppose exposes leaks: > > https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d5c0261ff15 > > Can we get someone from devtools to look at those? Which tools' tests were failing? That link no longer points to anything.

(In reply to Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] from comment #15) > (In reply to Shu-yu Guo [:shu] from comment #11) > > So hueyfixing Debugger objects makes dt fail, and I suppose exposes leaks: > > > > https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d5c0261ff15 > > > > Can we get someone from devtools to look at those? > > Which tools' tests were failing? That link no longer points to anything. Unfortunately I don't remember, just that it was a close to every single one, probably because of how mochitests are run. Since they're run in one instance and tests are navigated from one to the next, stuff always leaks across navigations. Someone would need to submit un-bitrot the patch and submit another try run.

Dropping to P2 as this is isolated to the debugger.