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;
A zombie compartment is created.
The compartment should disappear.
However it is not happening in Firefox 7.0.1 and 8.0.1.
I can reproduce this with all extensions disabled. I'll leave my browser up for a while to see how long the zombie lasts.
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.
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
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.)
Regression window(cached m-c)
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:11.0a1) Gecko/20111129 Firefox/11.0a1 ID:20111129214819
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:11.0a1) Gecko/20111130 Firefox/11.0a1 ID:20111130034720
Regression window(cached m-i)
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:11.0a1) Gecko/20111129 Firefox/11.0a1 ID:20111129084520
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:11.0a1) Gecko/20111129 Firefox/11.0a1 ID:20111129110720
Looks like this must be bug 697383.
Confirmed on local build.
This problem happens after landing of Bug 697383.
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.
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.
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.
Created attachment 579239 [details] [diff] [review]
This patch uses the outer-window-destroyed observer, and fixes the zombie compartment isssue.
Not sure who wants to r? this.
> 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,
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.
Created attachment 579251 [details] [diff] [review]
Now using the innerwindowID.
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
Comment on attachment 579869 [details] [diff] [review]
new updated patch
Verifying FIXED using latest hourly build based on cset:
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.