Closed Bug 1174950 Opened 5 years ago Closed 4 years ago

NukeCrossCompartmentWrappers should nuke wrappers from expanded principals

Categories

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

x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox39 --- wontfix
firefox40 + verified
firefox41 + fixed
firefox42 --- fixed
firefox-esr38 --- verified

People

(Reporter: tyaremco, Assigned: mccr8)

References

(Blocks 1 open bug)

Details

(Keywords: addon-compat, Whiteboard: [MemShrink:P1])

Attachments

(3 files, 3 obsolete files)

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.
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.
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.
Flags: needinfo?(tyaremco)
Attached file memory-report.json.gz (obsolete) —
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?
Flags: needinfo?(tyaremco)
Attached file 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.
Attachment #8623771 - Attachment is obsolete: true
Here's the fresh profile I've confirmed the issue on.

http://www6.zippyshare.com/v/jkKzJm0h/file.html
In case this bug is platform specific, I've only tested on Windows 7 64-bit.
Whiteboard: [MemShrink]
Whiteboard: [MemShrink]
Whiteboard: [MemShrink]
Component: General → DOM
Product: Firefox → Core
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.
tyaremco can you test w/ the latest greasemonkey beta noted in the above link and see if that fixes things for you?
Flags: needinfo?(tyaremco)
> 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
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.
Blocks: GhostWindows
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(tyaremco)
Assignee: nobody → continuation
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.
Summary: Extension interaction creates ghost windows out of all closed tabs → NukeCrossCompartmentWrappers should nuke wrappers from expanded principals to principals they subsume
See Also: → 1181122
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.
Flags: needinfo?(bobbyholley)
(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.
Flags: needinfo?(bobbyholley)
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
Attachment #8631870 - Flags: review?(bobbyholley)
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);
Attachment #8631870 - Flags: review?(bobbyholley) → review+
(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.
Attachment #8631329 - Attachment is obsolete: true
Addressed review comments.
Attachment #8631870 - Attachment is obsolete: true
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.
Attachment #8632992 - Flags: review+
Keywords: checkin-needed
Summary: NukeCrossCompartmentWrappers should nuke wrappers from expanded principals to principals they subsume → NukeCrossCompartmentWrappers should nuke wrappers from expanded principals
https://hg.mozilla.org/mozilla-central/rev/6e8b7625072b
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
This could potentially cause addon problems, though mostly the things it is fixing are at least short-term leaks.
Keywords: addon-compat
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
Attachment #8632992 - Flags: approval-mozilla-beta?
Attachment #8632992 - Flags: approval-mozilla-aurora?
> 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 :)
(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
Flags: needinfo?(jorge)
Tracking due to the severe memory leaks.
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?
Flags: needinfo?(continuation)
Attachment #8632992 - Flags: approval-mozilla-beta?
Attachment #8632992 - Flags: approval-mozilla-beta+
Attachment #8632992 - Flags: approval-mozilla-aurora?
Attachment #8632992 - Flags: approval-mozilla-aurora+
(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...
Flags: needinfo?(continuation)
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
Attachment #8632992 - Flags: approval-mozilla-esr38?
Whiteboard: [MemShrink] → [MemShrink:P1]
Comment on attachment 8632992 [details] [diff] [review]
Nuke wrappers from compartments with expanded principals to non-system windows.

Approved for ESR38.
Attachment #8632992 - Flags: approval-mozilla-esr38? → approval-mozilla-esr38+
Thanks for the heads up. The addon-compat keyword is already set, so we're good.
Flags: needinfo?(jorge)
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/
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.
See Also: 1181122
Duplicate of this bug: 1181122
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.