Zombie compartment at AMO in Nightly

VERIFIED FIXED in mozilla11

Status

()

Core
General
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: Fanolian, Assigned: fabrice)

Tracking

({regression})

Trunk
mozilla11
x86_64
Windows 7
regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox11-)

Details

(Whiteboard: [MemShrink:P2], URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:11.0a1) Gecko/20111203 Firefox/11.0a1
Build ID: 20111203031109

Steps to reproduce:

1. In Nightly, use a new profile and open https://addons.mozilla.org/en-US/firefox/ and about:memory?verbose in separate tabs;
2. Close AMO and click Minimize memory usage, GC, CC a dozen times in about:memory;


Actual results:

A zombie compartment is created.


Expected results:

The compartment should disappear.

However it is not happening in Firefox 7.0.1 and 8.0.1.
(Reporter)

Updated

6 years ago
Blocks: 668871
Whiteboard: [MemShrink]
Assignee: general → nobody
Component: JavaScript Engine → General
Keywords: regression, regressionwindow-wanted
QA Contact: general → general
I can reproduce this with all extensions disabled.  I'll leave my browser up for a while to see how long the zombie lasts.
Status: UNCONFIRMED → NEW
Ever confirmed: true
There's a zombie compartment with the addon installer, but it sounds like this happens even without that.
Yes.  And the compartment is still there after about an hour or so.
(Reporter)

Comment 4

6 years ago
Nightly build-
Last GOOD: 2011-11-30 built from: http://hg.mozilla.org/mozilla-central/rev/cc94a16983b0

First BAD: 2011-12-01 built from: http://hg.mozilla.org/mozilla-central/rev/e9686560b98d
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=cc94a16983b0&tochange=e9686560b98d

Nothing in there sticks out at me, so we may need to bisect further.
cc94a16 is a bad changeset for me.
I wonder if there's a difference between debug and release builds here, since my manual bisecting doesn't seem to match up with what I'm seeing with mozregression.
Sorry, I don't have cycles atm to bisect this.  (Literally...my CPU is tied up doing other builds.)

Comment 9

6 years ago
Regression window(cached m-c)
Works
http://hg.mozilla.org/mozilla-central/rev/cc94a16983b0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:11.0a1) Gecko/20111129 Firefox/11.0a1 ID:20111129214819
Fails
http://hg.mozilla.org/mozilla-central/rev/639fd053363e
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:11.0a1) Gecko/20111130 Firefox/11.0a1 ID:20111130034720
Pushlog
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=cc94a16983b0&tochange=639fd053363e


Regression window(cached m-i)
Works
http://hg.mozilla.org/integration/mozilla-inbound/rev/b10b930500f1
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:11.0a1) Gecko/20111129 Firefox/11.0a1 ID:20111129084520
Fails
http://hg.mozilla.org/integration/mozilla-inbound/rev/67451553bcbb
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:11.0a1) Gecko/20111129 Firefox/11.0a1 ID:20111129110720
Pushlog
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=b10b930500f1&tochange=67451553bcbb
Thanks, Alice.

Looks like this must be bug 697383.
Keywords: regressionwindow-wanted
Depends on: 697383

Comment 11

6 years ago
Confirmed on local build.
This problem happens after landing of Bug 697383.
Blocks: 697383
No longer depends on: 697383

Comment 12

6 years ago
Fabrice, lets check your code what global object is used when the property is resolved. We might be holding on to the global of the page somehow.
tracking-firefox11: --- → ?
Hmm.  At what point does the init() code that sets this._window (and adds unload listeners, for that matter) run?
(In reply to Boris Zbarsky (:bz) from comment #13)
> Hmm.  At what point does the init() code that sets this._window (and adds
> unload listeners, for that matter) run?

When navigator.mozApps is first accessed. See https://mxr.mozilla.org/mozilla-central/source/dom/base/nsDOMClassInfo.cpp#7088

I wonder if setting this._window = null on "unload" would help (here: https://mxr.mozilla.org/mozilla-central/source/dom/base/Webapps.js#240)
We should file a followup there to not use an unload listener too....
(In reply to Boris Zbarsky (:bz) from comment #15)
> We should file a followup there to not use an unload listener too....

Oh, interesting. What should we use instead? (This is relevant to my interests.)
I think the inner-window-destroyed notification would work better (e.g. wouldn't inhibit bfcache).  Note, by the way, that what you probably have there is a ref to the _outer_ window...  Not sure whether this causes a problem in practice or not.
(Assignee)

Comment 18

6 years ago
Setting this._window to null fixes the issue.

Boris, what's the way to listen for the inner-window-destroyed notification?
It's an observer service notification.  There's some data about it at https://developer.mozilla.org/en/Observer_Notifications#Windows and there are existing consumers similar to yours in ConsoleAPIStorage.jsm and ConsoleAPI.js that use it if you want to take a look.

I wonder why we need the explicit this._window set.  Is the |this| object there not being CCed for some reason?
In case I'm not at MemShrink tomorrow morning: This apparently happens because AMO frobs navigator.mozApps.  It should be fixed, but it's not a big MemShrink priority.
(Assignee)

Comment 21

6 years ago
Created attachment 579239 [details] [diff] [review]
patch

Boris, |this| is a js xpcom component that implements a property on navigator, using the category manager with the JavaScript-navigator-property.

This patch uses the outer-window-destroyed observer, and fixes the zombie compartment isssue.

Not sure who wants to r? this.
Assignee: nobody → fabrice
> This patch uses the outer-window-destroyed observer

That seems a bit odd.  I'd think this object's lifetime should be tied to the inner window, not the outer...  I guess this._window is an outer window, though....
> That seems a bit odd.  I'd think this object's lifetime should be tied to
> the inner window, not the outer...  I guess this._window is an outer window,
> though....

Because you need to close the window in order to observe the zombie compartment here, the leak is actually tied to the outer window's lifetime. The reason for that is that if the window navigates (and we fire inner-window-destroyed) then the outer window that we're holding on to gets transplanted to hold on to the *new* inner (which isn't a zombie). Then, when the window is closed, we fire outer-window-destroyed and null out our reference.

I think it's conceptually cleaner to listen for inner-window-destroyed (using nsDOMWindowUtils.currentInnerWindowID to wait for the correct notification) since that way we won't have a reference to a window that we don't really want to keep alive nor care about.
(Assignee)

Comment 24

6 years ago
Created attachment 579251 [details] [diff] [review]
updated patch

Now using the innerwindowID.
Attachment #579239 - Attachment is obsolete: true
Attachment #579251 - Flags: review?(mrbkap)
Whiteboard: [MemShrink] → [MemShrink:P2]
(Assignee)

Comment 25

5 years ago
Created attachment 579869 [details] [diff] [review]
new updated patch

Per discussion with blake, we now save the windowInnerId. Tested both when closing the tab and navigating away to another site.

This prevents a leak of about 5MB when going to addons.mozilla.org
Attachment #579251 - Attachment is obsolete: true
Attachment #579251 - Flags: review?(mrbkap)
Attachment #579869 - Flags: review?(mrbkap)
Comment on attachment 579869 [details] [diff] [review]
new updated patch

Thanks.
Attachment #579869 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 27

5 years ago
pushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/26a3ca3d8cdc
https://hg.mozilla.org/mozilla-central/rev/26a3ca3d8cdc
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
Verifying FIXED using latest hourly build based on cset:
https://hg.mozilla.org/mozilla-central/rev/98db2311a44c

Addons.mozilla.org now closes OK without the need to press any force clean buttons.  Just wait a minute of less, and refresh the about:memory page and its now gone.
Status: RESOLVED → VERIFIED

Updated

5 years ago
Flags: in-testsuite?

Updated

5 years ago
tracking-firefox11: ? → -
You need to log in before you can comment on or make changes to this bug.