Mingyi Liu Reporter
	Description • 12 years ago

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/535.19 (KHTML, like Gecko) Chrome/18.0.1025.162 Safari/535.19

Steps to reproduce:

I created a demo addon that does nothing other than adding a context menu Menu and an Item for that Menu.
After installing the addon, in FF13.0a2, open "about:compartments?verbose" and you'd see 4 system compartments for this addon.  Bug 1 demo: Go to addon manager, disable the addon. Bug 2 demo: Without doing bug 1, and before disabling the addon, first right click on page, then click on 'demo menu leak'->'test'.


Actual results:

In bug 1, and you'll see that 'resource://testsdkleak-at-mingyi-dot-org/api-utils/lib/environment.js' is NOT released after disable.
In bug 2, you'll see that 'resource://testsdkleak-at-mingyi-dot-org/api-utils/lib/preferences-service.js' is NOT released after disable.
These two bugs are additive, so if you do both demo one by one, you'll see both js unreleased.


Expected results:

Both js files should be released. As I tested, when the menu is destroyed or not does not matter. It also does not matter if it's pagecontext or selectorcontext, or whether there's content script or not.  The leaks happened in context-menu module.
One might want to comb through context-menu.js a bit more as it already has some clear memory leaks.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 1 • 12 years ago

Did you test this with SDK 1.6.1 or higher? 1.6.1 fixed some context menu leaks.

	Mingyi Liu Reporter
	Comment 2 • 12 years ago

BTW I did not test it on 1.7b1 but I expect these bugs still remain as none of the change comments mentioned fix for sth. like them.

Also: the bug #1 above happen ONLY when you create a contextMenu.Menu. Only creating a contextMenu.Item would not cause the bug 1.  But clicking on the Item would always cause bug #2, whether the item is on top level or under a Menu

Wes: just saw your comment: Yes, I forgot to mention the demo addon is packed with SDK 1.6.1, and that memory leak fix you mentioned was reported by me too. :)  I know these two are different and are not fixed by that fix.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 3 • 12 years ago

So, I see those compartments hanging around when I test that addon. But, if after I disable the addon in the Addon Manager tab, I refresh or close the Addon Manager tab, the compartments go away the next time I refresh about:compartments.

I'm testing this on a recent Nightly, maybe something's changed since 13?

	Mingyi Liu Reporter
	Comment 4 • 12 years ago

Hmm, that might be the case. In FF13, I refreshed compartments and tried about:memory's reduce memory button, and those compartments remained.

	Alexandre Poirot [:ochameau]
	Comment 5 • 12 years ago

It took me a while to figure it out but it looks like about:addons keep a reference to the addon, so that we leak the addon until we close the about:addons tab!
(Tested current jetpack master, today's FF nightly)

Mingyi, can you give it another try by closing about:addons right after having disabling it? compartments should be gone right after.

	Mingyi Liu Reporter
	Comment 6 • 12 years ago

Yes, you're right. Closing about:addons would remove both of the lingering compartments. So this is a low priority issue then, given that it didn't happen in FF newer than v13, and it is cleaned after about:addons page is closed.  Still, without using contextmenu.Menu, even if one keeps about:addons page open, no compartment would linger after disable.  So sth. is there, but probably not worth tracking it down.

	Alexandre Poirot [:ochameau]
	Comment 7 • 12 years ago

Context-menu is a really complex piece of code. There is something weird, but it may be something wrong in Firefox code, about:addons may keep a reference to bootstrap.js compartment. (Note that it isn't only leaking preferences but all compartments! Jetpack compartments are now merged into just one in order to save memory)
There is most likely something weird in context menu too, but it would not worth digging it because of context-menu complexity and the lack of tooling to debug such memory leak.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 8 • 12 years ago

(In reply to Alexandre Poirot (:ochameau) from comment #7)
> Context-menu is a really complex piece of code. There is something weird,
> but it may be something wrong in Firefox code, about:addons may keep a
> reference to bootstrap.js compartment. (Note that it isn't only leaking
> preferences but all compartments! Jetpack compartments are now merged into
> just one in order to save memory)
> There is most likely something weird in context menu too, but it would not
> worth digging it because of context-menu complexity and the lack of tooling
> to debug such memory leak.

This might actually be a bug in the addon manager for all restartless extensions where it holds onto pieces of the extension in memory until you refresh or close the Addon Manager tab. (Something about the "undo" option, perhaps?)

	Mingyi Liu Reporter
	Comment 9 • 12 years ago

Not really. As I said, SDK-based addons that don't use contextmenu.Menu (but use contextmenu.Item) do not have this issue.  I have two other SDK-based addons that use Item but not Menu and they don't leave any compartments when disabled, whether about:addons is closed or not.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 10 • 12 years ago

Would probably be worth testing this again on a current Nightly now that chrome to content leaks should be a thing of the past.

	Alexandre Poirot [:ochameau]
	Comment 11 • 12 years ago

Various leak fixes are about to land, it would be worth testing a repacked version of your addon with bug 764831's patch (just landed). (and bug 764840 if you are testing on Nightly)

	Erik Vold [:erikvold] (please needinfo? me)
	Comment 12 • 12 years ago

Mind trying this on 1.13.2 ?

https://blog.mozilla.org/addons/2013/02/12/announcing-add-on-sdk-1-13-2/

	Mingyi Liu Reporter
	Comment 13 • 12 years ago

Sorry for the late reply! I just did the test. Using the old xpi I uploaded to this thread and FF19, I reproduced the bugs still exist for old SDK-packed addon in FF19 - many compartments remained after disabling the addon.

Using the SDK 1.13.2-packed xpi of the same demo addon code, in FF19 I confirmed that after disabling the addon, all those compartments are gone. No compartment for the addon remained after disabling.  So I'm glad to report that the bug(s) in this thread are fixed.  Thanks!