Last Comment Bug 707507 - Zombie compartment at AMO in Nightly
: Zombie compartment at AMO in Nightly
Status: VERIFIED FIXED
[MemShrink:P2]
: regression
Product: Core
Classification: Components
Component: General (show other bugs)
: Trunk
: x86_64 Windows 7
: -- normal (vote)
: mozilla11
Assigned To: [:fabrice] Fabrice Desré
:
Mentors:
https://addons.mozilla.org/en-US/fire...
Depends on:
Blocks: ZombieCompartments 697383
  Show dependency treegraph
 
Reported: 2011-12-04 07:34 PST by Fanolian
Modified: 2012-01-09 11:30 PST (History)
16 users (show)
jruderman: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
patch (2.11 KB, patch)
2011-12-05 22:41 PST, [:fabrice] Fabrice Desré
no flags Details | Diff | Review
updated patch (2.12 KB, patch)
2011-12-06 00:00 PST, [:fabrice] Fabrice Desré
no flags Details | Diff | Review
new updated patch (2.16 KB, patch)
2011-12-07 15:26 PST, [:fabrice] Fabrice Desré
mrbkap: review+
Details | Diff | Review

Description Fanolian 2011-12-04 07:34:35 PST
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.
Comment 1 Justin Lebar (not reading bugmail) 2011-12-04 07:54:33 PST
I can reproduce this with all extensions disabled.  I'll leave my browser up for a while to see how long the zombie lasts.
Comment 2 Andrew McCreight [:mccr8] 2011-12-04 08:20:21 PST
There's a zombie compartment with the addon installer, but it sounds like this happens even without that.
Comment 3 Justin Lebar (not reading bugmail) 2011-12-04 08:27:52 PST
Yes.  And the compartment is still there after about an hour or so.
Comment 4 Fanolian 2011-12-04 08:29:17 PST
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
Comment 5 Justin Lebar (not reading bugmail) 2011-12-04 14:20:01 PST
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.
Comment 6 Justin Lebar (not reading bugmail) 2011-12-04 16:38:27 PST
cc94a16 is a bad changeset for me.
Comment 7 Justin Lebar (not reading bugmail) 2011-12-04 20:08:24 PST
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.
Comment 8 Justin Lebar (not reading bugmail) 2011-12-04 23:46:32 PST
Sorry, I don't have cycles atm to bisect this.  (Literally...my CPU is tied up doing other builds.)
Comment 9 Alice0775 White 2011-12-05 01:25:42 PST
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
Comment 10 Justin Lebar (not reading bugmail) 2011-12-05 01:46:37 PST
Thanks, Alice.

Looks like this must be bug 697383.
Comment 11 Alice0775 White 2011-12-05 03:36:28 PST
Confirmed on local build.
This problem happens after landing of Bug 697383.
Comment 12 Andreas Gal :gal 2011-12-05 08:19:33 PST
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.
Comment 13 Boris Zbarsky [:bz] 2011-12-05 20:23:57 PST
Hmm.  At what point does the init() code that sets this._window (and adds unload listeners, for that matter) run?
Comment 14 Philipp von Weitershausen [:philikon] 2011-12-05 20:30:41 PST
(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)
Comment 15 Boris Zbarsky [:bz] 2011-12-05 20:34:08 PST
We should file a followup there to not use an unload listener too....
Comment 16 Philipp von Weitershausen [:philikon] 2011-12-05 20:35:22 PST
(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.)
Comment 17 Boris Zbarsky [:bz] 2011-12-05 20:45:48 PST
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.
Comment 18 [:fabrice] Fabrice Desré 2011-12-05 20:51:50 PST
Setting this._window to null fixes the issue.

Boris, what's the way to listen for the inner-window-destroyed notification?
Comment 19 Boris Zbarsky [:bz] 2011-12-05 21:04:32 PST
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?
Comment 20 Justin Lebar (not reading bugmail) 2011-12-05 22:31:43 PST
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.
Comment 21 [:fabrice] Fabrice Desré 2011-12-05 22:41:47 PST
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.
Comment 22 Boris Zbarsky [:bz] 2011-12-05 22:48:03 PST
> 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....
Comment 23 Blake Kaplan (:mrbkap) (please use needinfo!) 2011-12-05 23:49:23 PST
> 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.
Comment 24 [:fabrice] Fabrice Desré 2011-12-06 00:00:07 PST
Created attachment 579251 [details] [diff] [review]
updated patch

Now using the innerwindowID.
Comment 25 [:fabrice] Fabrice Desré 2011-12-07 15:26:34 PST
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 26 Blake Kaplan (:mrbkap) (please use needinfo!) 2011-12-07 17:32:22 PST
Comment on attachment 579869 [details] [diff] [review]
new updated patch

Thanks.
Comment 27 [:fabrice] Fabrice Desré 2011-12-07 18:28:01 PST
pushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/26a3ca3d8cdc
Comment 28 Ed Morley [:emorley] 2011-12-08 08:42:26 PST
https://hg.mozilla.org/mozilla-central/rev/26a3ca3d8cdc
Comment 29 Jim Jeffery not reading bug-mail 1/2/11 2011-12-08 08:59:50 PST
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.

Note You need to log in before you can comment on or make changes to this bug.