1174950 - NukeCrossCompartmentWrappers should nuke wrappers from expanded principals

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[tyaremco]

I'm posting this at bugzilla because I'm not sure whether Firefox is at fault or not.

With the following extensions enabled, every time a tab is closed it becomes a ghost window that is never cleaned up.  Confirmed in Firefox 37 and 40a2.

* Greasemonkey 3.2 https://addons.mozilla.org/en-US/firefox/addon/greasemonkey/
   * script: AdsBypasser 5.25.1 Full https://adsbypasser.github.io/
* Nimbus Screen Capture 6.5.1 https://addons.mozilla.org/en-US/firefox/addon/nimbus-screenshot/


> Reproduction 
This is the method I found most reliable to get the ghost windows to be recorded.

1. Enable the above extensions and script.
2. Set memory.ghost_window_timeout_seconds to 1.

> Method
1. open a tab to wikipedia.org
   When testing multiple times, it's useful to open different wikipedia articles so you can easily see which get ghosted
2. close wikipedia tab
3. open about:memory
4. Click 'Minimize Memory Usage'
5. Click 'Measure'
6. Click 'Measure' _again_
5. ctrl+f for 'ghost-'

> Results
About:memory will display a ghost window for the closed wikipedia tab.  It cannot be removed.

> Other interesting results
If you disable the Adsbypasser script only, ghosts are no longer created.  However, if you disable Nimbus only, ghosts will continue to be created until you restart the browser.  This is despite the fact that Nimbus is a restartless addon.

If you disable both Adsbypasser and Nimbus and run 'Minimize Memory Usage', the ghosts will still remain.

Comment 1

[tyaremco]

After further testing, all of these addons pose the same problem:

Greasemonkey 3.2 https://addons.mozilla.org/en-US/firefox/addon/greasemonkey/
   script: AdsBypasser 5.25.1 Full https://adsbypasser.github.io/

and any one the following:
Nimbus Screen Capture https://addons.mozilla.org/en-US/firefox/addon/nimbus-screenshot/
Click to Play per-element https://addons.mozilla.org/en-US/firefox/addon/click-to-play-per-element/?src=api
Open in Wayback Machine https://addons.mozilla.org/en-US/firefox/addon/open-in-wayback-machine/

The last one being a very small test case.

Comment 2

[Loic]

Could you post a memory log (about:memory) according to this FAQ, please.
https://developer.mozilla.org/en-US/docs/Zombie_compartments

Maybe you can create a fresh profile (https://support.mozilla.org/en-US/kb/profile-manager-create-and-remove-firefox-profiles) and load add-on one by one and test each time to reproduce the memory issue.
When the memory issue is visible, minimize memory and remeasure, multiple times if necessary, then save a memory log.

Comment 3

[tyaremco]

Attachment: memory-report.json.gz

You're right, it doesn't occur in a new profile.  But I've tested over and over on my old profile and the results are consistent.

Here is my about:support:
http://pastebin.com/A8Pm8Z2p

Attached is an example of the ghost windows in memlog.

What kind of corruption would be responsible for this behaviour?  Is one of my about:config entries responsible?

Comment 4

[tyaremco]

Attachment: memory-report.json.gz

IGNORE PREVIOUS COMMENT

I had forgot to set memory.ghost_window_timeout_seconds to 1 so the ghosts weren't appearing when I expected them to.  I've confirmed the behaviour on a new profile.

Comment 5

[tyaremco]

Attachment: new profile about:support

Comment 6

[tyaremco]

Here's the fresh profile I've confirmed the issue on.

http://www6.zippyshare.com/v/jkKzJm0h/file.html

Comment 7

[tyaremco]

In case this bug is platform specific, I've only tested on Windows 7 64-bit.

Comment 8

[tyaremco]

This behaviour was confirmed by the author of AdsBypasser.
https://github.com/adsbypasser/adsbypasser/issues/707

> I've tested with GreaseMonkey 3.2 and GreaseMonkey 2.3, both will leave ghost-windows remain.
> However, if I remove the use of GM_registerMenuCommand then this will not happen.
> I believe this is related to greasemonkey/greasemonkey#2200, and I'm afraid that I can't do anything in userscript-level.

It seems that there isn't anything specific about the AdsBypasser script that is the cause.

Comment 9

[Nicholas Nethercote [inactive]]

> I believe this is related to greasemonkey/greasemonkey#2200

https://github.com/greasemonkey/greasemonkey/issues/2200 is the full link.

Comment 10

[Eric Rahm [:erahm]]

tyaremco can you test w/ the latest greasemonkey beta noted in the above link and see if that fixes things for you?

Comment 11

[Nicholas Nethercote [inactive]]

> tyaremco can you test w/ the latest greasemonkey beta noted in the above link 

That's https://addons.mozilla.org/firefox/downloads/file/327786/greasemonkey-3.3beta1-fx.xpi

Comment 12

[tyaremco]

Using the Greasemonkey beta, I can confirm that the problem still exists in Firefox 38.0.5 but NOT Firefox 39.  I repeated my test several times for both Firefox versions to make sure.

Comment 13

[Andrew McCreight [:mccr8]]

Confirmed. Thanks for the detailed steps to reproduce. Even though Greasemonkey has apparently worked around this, we should still figure out what the underlying Firefox issue is.

Comment 14

[Andrew McCreight [:mccr8]]

I investigated this a bit and talked it over with Bill. What it looks like is happening is that chrome is keeping alive a sandbox via a wrapper, and the sandbox is keeping the content document alive via another wrapper.  The chrome-to-sandbox-wrapper never gets nuked, because presumably Greasemonkey never does that (which is a bug in Greasemonkey that may have been fixed in their beta). Then, the sandbox-to-page-wrapper never gets nuked because the sandbox does not have system principal. I'm going to try making the source wrapper matching for WindowDestroyedEvent a little looser so that it also matches any expanded principals. This should only affect Firefox-y stuff so it will hopefully be okay. You'll still leak sandboxes, but not entire pages.

Comment 15

[Andrew McCreight [:mccr8]]

Attachment: Nuke wrappers from extended principals to non-system windows. WIP

Comment 16

[Andrew McCreight [:mccr8]]

Bobby, is there some better way to tell if a compartment is "browser-y" (rather than web content) than seeing if the principal is either the system principal or an expanded principal?  That seems brittle.

Comment 17

[Bobby Holley (:bholley)]

(In reply to Andrew McCreight [:mccr8] from comment #16)
> Bobby, is there some better way to tell if a compartment is "browser-y"
> (rather than web content) than seeing if the principal is either the system
> principal or an expanded principal?  That seems brittle.

That is probably the best mechanism, I think. There might be browser-y stuff that doesn't match this criterion, but there will never be web content that does, which is what we care most about, I think.

Comment 18

[Andrew McCreight [:mccr8]]

Attachment: Nuke wrappers from compartments with extended principals to non-system windows.

This extends the hueyfix to the case where a sandbox with extended principals is leaked,
for instance by an addon, and ends up entraining a content window. This fix prevents the
leak of the content window, but not the sandbox.

This callback is called on the source of each cross-compartment wrapper, once per compartment.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fda8517e13cb

Comment 19

[Bobby Holley (:bholley)]

Comment on attachment 8631870 [details] [diff] [review]
Nuke wrappers from compartments with extended principals to non-system windows.

Review of attachment 8631870 [details] [diff] [review]:
-----------------------------------------------------------------

It's "expanded", not "extended" - please fix in the commit message etc.

::: dom/base/nsGlobalWindow.cpp
@@ +8634,5 @@
> +      return true;
> +    }
> +    nsCOMPtr<nsIPrincipal> pc = nsJSPrincipals::get(JS_GetCompartmentPrincipals(c));
> +    nsCOMPtr<nsIExpandedPrincipal> epc = do_QueryInterface(pc);
> +    return !!epc;

This whole thing (including the check above) can be replaced with nsContentUtils::IsSystemOrExpandedPrincipal(pc);

Comment 20

[Andrew McCreight [:mccr8]]

(In reply to Bobby Holley (:bholley) from comment #19)
> It's "expanded", not "extended" - please fix in the commit message etc.

Oops, will do.

> This whole thing (including the check above) can be replaced with
> nsContentUtils::IsSystemOrExpandedPrincipal(pc);

Neat. Thanks for the review.

Comment 21

[Andrew McCreight [:mccr8]]

Attachment: Nuke wrappers from compartments with expanded principals to non-system windows.

Addressed review comments.

Comment 22

[Andrew McCreight [:mccr8]]

Comment on attachment 8632992 [details] [diff] [review]
Nuke wrappers from compartments with expanded principals to non-system windows.

Carrying forward bholley's r+. Tree's closed, so I'm setting checkin-needed.

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6e8b7625072b

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6e8b7625072b

Comment 25

[Andrew McCreight [:mccr8]]

This could potentially cause addon problems, though mostly the things it is fixing are at least short-term leaks.

Comment 26

[Andrew McCreight [:mccr8]]

Comment on attachment 8632992 [details] [diff] [review]
Nuke wrappers from compartments with expanded principals to non-system windows.

Approval Request Comment
[Feature/regressing bug #]: bug 1134180 for reasons that aren't really understood
[User impact if declined]: Many GreaseMonkey scripts started leaking severely in 38 for reasons that aren't entirely understood, also causing severe jank. This patch fixes the worst of the leak/jank.
[Describe test coverage new/current, TreeHerder]: This code is called very often.
[Risks and why]: I think the main danger is that we break some addons that somehow rely on being able to look at content windows from sandboxes, but I think odds are in those cases they are leaking anyways, and random failures in an addon is probably better. Bug 695480 did the same thing in a more common scenario and it did cause some minor addon breakage.
[String/UUID change made/needed]: none

Comment 27

[Nicholas Nethercote [inactive]]

> Bug 695480 did the same thing in a more
> common scenario and it did cause some minor addon breakage.

It was also resulted in the single biggest improvement in Firefox's memory usage in the past 5 years :)

Comment 28

[Lawrence Mandel [:lmandel] (use needinfo)]

(In reply to Andrew McCreight [:mccr8] from comment #26)
> Comment on attachment 8632992 [details] [diff] [review]
> Nuke wrappers from compartments with expanded principals to non-system
> windows.
> 
> Approval Request Comment
> [Feature/regressing bug #]: bug 1134180 for reasons that aren't really
> understood
> [User impact if declined]: Many GreaseMonkey scripts started leaking
> severely in 38 for reasons that aren't entirely understood, also causing
> severe jank. This patch fixes the worst of the leak/jank.

Bug 1134180 is marked as fixed in 39. Do you think there's a different regressing bug?

> [Describe test coverage new/current, TreeHerder]: This code is called very
> often.
> [Risks and why]: I think the main danger is that we break some addons that
> somehow rely on being able to look at content windows from sandboxes, but I
> think odds are in those cases they are leaking anyways, and random failures
> in an addon is probably better. Bug 695480 did the same thing in a more
> common scenario and it did cause some minor addon breakage.

I agree. Severe memory leaks and jank are worse than the occasional add-on failure. We do want to flag this for add-on authors so that they are aware of a change in behaviour. ni Jorge

Comment 29

[Lawrence Mandel [:lmandel] (use needinfo)]

Tracking due to the severe memory leaks.

Comment 30

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8632992 [details] [diff] [review]
Nuke wrappers from compartments with expanded principals to non-system windows.

The code is executed frequently and so issues should surface quickly. Let's take this in beta6. Beta+

Andrew - Do we have a way to prove that this fix is good while it's on Beta? Do we need to backport this fix to ESR38?

Comment 31

[Andrew McCreight [:mccr8]]

(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #30)
> Andrew - Do we have a way to prove that this fix is good while it's on Beta?

The STR in bug 1181122 comment 0 should do it. Note that Greasemonkey is trying to fix things from their side, too, so a newer version than the one listed may not have the issue.

> Do we need to backport this fix to ESR38?

Yeah, that's probably a good idea. I'll fill out the form...

Comment 32

[Andrew McCreight [:mccr8]]

Comment on attachment 8632992 [details] [diff] [review]
Nuke wrappers from compartments with expanded principals to non-system windows.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: bad leaks for Greasemonkey users, possibly for other addons
User impact if declined: leaks
Fix Landed on Version: 42
Risk to taking this patch (and alternatives if risky): The main risk is addon compatibility.
String or UUID changes made by this patch: none

Comment 33

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8632992 [details] [diff] [review]
Nuke wrappers from compartments with expanded principals to non-system windows.

Approved for ESR38.

Comment 34

[Jorge Villalobos [:jorgev] (he/him)]

Thanks for the heads up. The addon-compat keyword is already set, so we're good.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/365181b84985

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/dc78e255ac9b

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr38/rev/26d975da1f09

Comment 38

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Reproduced the original issue in Firefox 37, as specified in comment 0, on Windows 7 x64. The ghost window no longer displays in about:memory for the closed tab when using:
- Firefox 40 Beta 6 - BuildID=20150720220238
- latest Firefox 38 ESR tinderbox - BuildID=20150720152627 (https://hg.mozilla.org/releases/mozilla-esr38/rev/9042488f5118)

I used the following add-ons:
* Greasemonkey 3.2 https://addons.mozilla.org/en-US/firefox/addon/greasemonkey/
   * script: AdsBypasser 5.30.0 Full https://adsbypasser.github.io/
* Nimbus Screen Capture 6.5.1 https://addons.mozilla.org/en-US/firefox/addon/nimbus-screenshot/

Comment 39

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Forgot to mention that Add-on Compatibility testing was performed yesterday with Firefox 40 Beta 6 (covering the most popular add-ons), and there were no new issues encountered. This testing will also cover ESR 38.2.0 when it comes out.