1084626 - Remove support for Chrome -> Content leaks through debugger objects

Status: NEW

(Core :: JavaScript Engine, defect)

Description

[Andrew McCreight [:mccr8]]

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.

Comment 1

[Andrew McCreight [:mccr8]]

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

Comment 2

[Andrew McCreight [:mccr8]]

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.

Comment 3

[Andrew McCreight [:mccr8]]

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

Comment 4

[Shu-yu Guo [:shu]]

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

Comment 5

[Andrew McCreight [:mccr8]]

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

Comment 6

[Shu-yu Guo [:shu]]

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.

Comment 7

[Jim Blandy :jimb]

(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 8

[Shu-yu Guo [:shu]]

Attachment: Hueyfix Debugger objects.

Comment 9

[Shu-yu Guo [:shu]]

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 10

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

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.

Comment 11

[Shu-yu Guo [:shu]]

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?

Comment 12

[Shu-yu Guo [:shu]]

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

Comment 13

[Shu-yu Guo [:shu]]

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

Comment 14

[Shu-yu Guo [:shu]]

Attachment: Hueyfix Debugger objects.

Unrotted for testing.

Comment 15

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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

Comment 16

[Shu-yu Guo [:shu]]

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

Comment 17

[Eric Rahm [:erahm]]

Dropping to P2 as this is isolated to the debugger.

Comment 18

[BugBot [:suhaib / :marco/ :calixte]]

Clear a needinfo that is pending on an inactive user.

Inactive users most likely will not respond; if the missing information is essential and cannot be collected another way, the bug maybe should be closed as INCOMPLETE.

For more information, please visit auto_nag documentation.