707507 - Zombie compartment at AMO in Nightly

Status: VERIFIED FIXED

(Core :: General, defect)

Description

[Fanolian]

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)]

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]]

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)]

Yes.  And the compartment is still there after about an hour or so.

Comment 4

[Fanolian]

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)]

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)]

cc94a16 is a bad changeset for me.

Comment 7

[Justin Lebar (not reading bugmail)]

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)]

Sorry, I don't have cycles atm to bisect this.  (Literally...my CPU is tied up doing other builds.)

Comment 9

[Alice0775 White]

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)]

Thanks, Alice.

Looks like this must be bug 697383.

Comment 11

[Alice0775 White]

Confirmed on local build.
This problem happens after landing of Bug 697383.

Comment 12

[Andreas Gal :gal]

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 [:bzbarsky]]

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]]

(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 [:bzbarsky]]

We should file a followup there to not use an unload listener too....

Comment 16

[Philipp von Weitershausen [:philikon]]

(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 [:bzbarsky]]

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é]

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 [:bzbarsky]]

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)]

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é]

Attachment: 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 [:bzbarsky]]

> 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) (inactive)]

> 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é]

Attachment: updated patch

Now using the innerwindowID.

Comment 25

[[:fabrice] Fabrice Desré]

Attachment: 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) (inactive)]

Comment on attachment 579869 [details] [diff] [review]
new updated patch

Thanks.

Comment 27

[[:fabrice] Fabrice Desré]

pushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/26a3ca3d8cdc

Comment 28

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/26a3ca3d8cdc

Comment 29

[Jim Jeffery not reading bug-mail 1/2/11]

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.