725194 - Better docs on how to avoid leaks in add-ons

Status: RESOLVED FIXED

(Developer Documentation Graveyard :: General, defect)

Description

[Nicholas Nethercote [inactive]]

(I'm not sure if I've put this in the right product/component.)

Add-ons leak a lot.  I've written some documentation on how to avoid leaks
in add-ons at
https://developer.mozilla.org/en/Extension/Performance_best_practices_in_extensions#Avoid_Creating_Memory_Leaks,
which links to https://developer.mozilla.org/en/Zombie_compartments.  The
latter is quite good at describing how to find certain leaks, but doesn't
have much detail about their causes.

An AMO review (Nils Maier?) wrote the following in a private email thread.

> Most common from my experience so far:
>
> - Stuffing a window/document or similar into a js module:
> let windows = [];
> function injsmod(window) {
>   // forgetting or failing to pop the window again
>   windows.push(window);
> }
> function injsmod2(window) {
>   // implicit var declaration in the js global, holding a strong ref to
> the document
>   doc = window.document;
> }
>
> - Stuffing something into the windows (worse if it is the long-lived
> browser.xul):
> // Holding a strong ref to the bootstrapped component
> var el = document.createElement("a");
> a.onlick = my_bootstrap_js_method;
>
> - addEventListener issues
>
> - Creating funky cyclic stuff:
> https://github.com/scrapmac/UIEnhancer/blob/de5ddf0ef43274eef593382fe957df81be
06e808/scripts/helper.js#L196

Nils also said "I hate and suck at writing wiki pages".  I'm happy to put stuff on the wiki, but I don't know much about writing add-ons and so can't generate the content.  But I can work with text like the above if there's enough detail (the first two bullet points are good enough for me to work with, the latter two need more details).

Does anyone want to volunteer expanding the latter two points, or adding new points?  Just attaching a text file to this bug would be fine.

Comment 1

[Nils Maier [:nmaier]]

Re: addEventListener issues
What I meant was bootstrapped (restartless) add-ons failing to clean up their event listeners as they are disabled/removed. Disabling extensions will not only happen manually by user action, but also when such an add-on is updated.
When such an add-on fails to clean up event listeners, the listeners will still reference the enclosing scope - usually the bootstrap.js Sandbox - and therefore keep that scope alive until the window is unloaded. Hence, if the window in question is browser.xul or some long-lived webapp such as gmail or simply a content tab the user forgot about, the leaked compartments/scopes might survive for quite some time.

- bootstrap.js leaks are pretty common, but limited to restartless add-ons, of course. The number of bootstrapped add-ons grows pretty fast, OTOH.

- js-modules are only affected if they get Cu.unload()ed. Unloading rarely happens unless it is a bootstrapped add-on using own modules. Forgetting to unload js-modules in bootstrapped add-ons is also quite common way to leak, but that's unrelated to event listeners. You cannot detect missing-unload leaks by looking at about:memory as such modules live in the main System compartment (maybe after CPG lands it will be possible).

- SDK based add-ons are problematic, too, although usually only content windows are affected. See bug 708192.

Here is a little demo (sans boilerplate):

function leakref() {}

function main(window) {
  // Leaks, as the window (browser.xul) will hold on to this scope via
  // the listener.
  window.addEventListener("leaky", leakref, true);

  // The following line still leaks, as useCapture differs
  // This kind of subtle bug is very common
  unload(function() window.removeEventListener("leaky", leakref, false), window);

  // Finally unleak me
  // unload(function() window.removeEventListener("leaky", leakref, true), window);
}

Comment 2

[Nils Maier [:nmaier]]

Attachment: addEventListener leaks demo add-on

The code from comment 1 as a "working" add-on.

Uses mardak's fine watchWindows boilerplate, which is also pretty commonly found in AMO listed add-ons:
https://raw.github.com/Mardak/restartless/watchWindows/bootstrap.js

STR:
- Enable and disable the add-on a couple of times, leaving it eventually disabled
- about:memory?verbose + Minimize memory usage
-> See multiple bootstrap.js compartments still present

- Restart the browser
- Hack the XPI and uncomment the "unleak" line
- Enable and disable the add-on a couple of times, leaving it eventually disabled
- about:memory?verbose + Minimize memory usage
-> No more bootstrap.js compartments

Comment 3

[Nicholas Nethercote [inactive]]

Another cause:  if you forget to declare a variable with |var| it becomes a global.  This was the cause of two leaks found this week:  bug 712733 and bug 725875.  If you "use strict;" these forgotten declarations become errors.  So I think that recommending people "use strict;" is a good idea, both for avoiding leaks, and just avoiding bugs in general.

Comment 4

[Nils Maier [:nmaier]]

(In reply to Nicholas Nethercote [:njn] from comment #3)
> Another cause:  if you forget to declare a variable with |var| it becomes a
> global.  This was the cause of two leaks found this week:  bug 712733 and
> bug 725875.  If you "use strict;" these forgotten declarations become
> errors.  So I think that recommending people "use strict;" is a good idea,
> both for avoiding leaks, and just avoiding bugs in general.

Regarding leaks, that's mostly only an issue when that refs points to a "big" memory blob, like a window (see injsmod2).

Kris' Extension Test polls for new globals periodically and is good to detect any such leaks in the browser.xul scope. Of course, you have to trigger the faulty code path.
https://addons.mozilla.org/en-US/firefox/addon/extension-test/
Implicit declarations in overlays count as "polluting the global namespace" and cause a prelim. review, no matter if it leaks vast amounts of memory or not.
However, Extension Test does not cover windows created by the add-on nor js code modules nor XPCOM components.

Comment 5

[Nicholas Nethercote [inactive]]

(In reply to Nicholas Nethercote [:njn] from comment #3)
> Another cause:  if you forget to declare a variable with |var| it becomes a
> global.  This was the cause of two leaks found this week:  bug 712733 and
> bug 725875.  If you "use strict;" these forgotten declarations become
> errors.  So I think that recommending people "use strict;" is a good idea,
> both for avoiding leaks, and just avoiding bugs in general.

Another case:  bug 727552 comment 3.

Comment 6

[Nicholas Nethercote [inactive]]

I started https://developer.mozilla.org/en/Extensions/Common_causes_of_zombie_compartments_in_extensions, and put links to it from https://developer.mozilla.org/en/Zombie_compartments and https://developer.mozilla.org/en/Extensions/Performance_best_practices_in_extensions.  I've put some of the above cases up, but I fear I've done a poor job because I just don't know much about add-ons, and I need some help with the remaining cases.


> > - Stuffing something into the windows (worse if it is the long-lived
> > browser.xul):
> > // Holding a strong ref to the bootstrapped component
> > var el = document.createElement("a");
> > a.onlick = my_bootstrap_js_method;

Should |var el| be |var a| here?  That's what I put on the wiki page.  There's not enough explanation of this example on the wiki page.  I personally don't understand what it means, esp. because I don't know what a bootstrap method is.


(In reply to Nils Maier [:nmaier] from comment #1)
> Re: addEventListener issues
> What I meant was bootstrapped (restartless) add-ons failing to clean up
> their event listeners as they are disabled/removed.

I put this case on the wiki page.


> - bootstrap.js leaks are pretty common, but limited to restartless add-ons,
> of course. The number of bootstrapped add-ons grows pretty fast, OTOH.
> 
> - js-modules are only affected if they get Cu.unload()ed...
> 
> - SDK based add-ons are problematic, too, although usually only content
> windows are affected. See bug 708192.

I haven't put these on the page, I didn't know if they were related to the event listeners case or are separate cases.


> Here is a little demo (sans boilerplate):

Is this demo just about the listener case?  Seems to be.


I mentioned http://maglione-k.users.sourceforge.net/bootstrapped.xhtml.  I wasn't sure if it was talking about shutdown() or uninstall().

Sorry for my lack of knowledge about this stuff;  I need some more explanation to make further progress.

Comment 7

[Nils Maier [:nmaier]]

(In reply to Nicholas Nethercote [:njn] from comment #6)
> > > - Stuffing something into the windows (worse if it is the long-lived
> > > browser.xul):
> > > // Holding a strong ref to the bootstrapped component
> > > var el = document.createElement("a");
> > > a.onlick = my_bootstrap_js_method;
> 
> Should |var el| be |var a| here?  That's what I put on the wiki page. 
> There's not enough explanation of this example on the wiki page.  I
> personally don't understand what it means, esp. because I don't know what a
> bootstrap method is.

That is a remark about bootstrapped (restartless) addons.
https://developer.mozilla.org/en/Extensions/Bootstrapped_extensions

Those use a bootstrap.js files for setup. my_bootstrap_js_method would belong to the bootstrap.js compartment (or any other compartment created by it). The bootstrap.js compartment is internally just a js Sandbox with chrome privileges.
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/XPIProvider.jsm#3567

When you stuff such references from such a compartment into (long-lived) window objects or js modules or XPCOM components, this will prevent the bootstrap.js compartment from being freed as there are active references to it and hence that compartment will become a zombie.
This is the same issue the addEventListener stuff has, too, but I guess it would be good to mention both explicitly.

> > - bootstrap.js leaks are pretty common, but limited to restartless add-ons,
> > of course. The number of bootstrapped add-ons grows pretty fast, OTOH.
> > 
> > - js-modules are only affected if they get Cu.unload()ed...
> > 
> > - SDK based add-ons are problematic, too, although usually only content
> > windows are affected. See bug 708192.
> 
> I haven't put these on the page, I didn't know if they were related to the
> event listeners case or are separate cases.

They are all related.
You may leak js modules if something holds a reference to it, although right now they all live in the main system compartment and hence you cannot really "see" the leak.
And SDK add-on are prone to addEventListener leaks in particular via the page-mod module, as many of such contentScripts use addEventListener but do not take into account that the add-on may actually be disabled before the page is closed.

> > Here is a little demo (sans boilerplate):
> 
> Is this demo just about the listener case?  Seems to be.

The demo shows an addEventListener leak. However it would be possible to leak in a lot of other ways, like:
window.document.documentElement.leaky = leakref;
This would be a variant of my_bootstrap_js_method.

> I mentioned http://maglione-k.users.sourceforge.net/bootstrapped.xhtml.  I
> wasn't sure if it was talking about shutdown() or uninstall().

The shutdown method is the most important one, as it will be called whenever an add-on is disabled incl. when closing the browser or uninstalled, while uninstall will only run when the add-on is actually uninstalled (see the docs linked above).


Re: the MDN docs
- "Storing references to things in a JavaScript module" is not the most common cause, but something very similar:
The actual most common cause, as it turns out, is add-ons storing references to *content* window stuff in their overlay code.

let contentWindows = [];
function inbrowserxuloverlay(contentWindow) {
  // forgetting or failing to pop the content window thing again
  contentWindows.push(contentWindow);
}
Same thing with implicit variable declaration.

This will keep the content windows alive until the browser window is closed. Users often only open a single browser window per session and use tabs, so these leaks are pseudo-immortal.

The js modules issue does also apply to bootstrap.js. js-modules/bootstrap.js based leaks might keep both chrome windows and content windows alive.


- Observers
Since you mention observers already, let me try to elaborate a bit.
Strong-reffed observers are a common cause of leaking whole chrome windows; it is possible to leak content windows, too, but that's not that common.

Consider the following example:
Services.obs.addObserver({
 observe: function(s,t,d) {
  window.document.documentElement.setAttribute("pbm", (d == "enter") ? "private" : "normal");
 }
}, "private-browsing", false);
The observer service will hold a strong ref to the observer object and by this to the whole window.
This was indeed a common issue in core Firefox code in the 1.x/2.x era.
The proper way would be to implement this as a either using weak reference Services.obs.addObserver(..., ..., true); or explicitly calling removeObserver in an own unload event listener.

A lot of services besides nsIObserverService accept nsIObserver paramaters or other interfaces and will keep strong references around.

Comment 8

[Nicholas Nethercote [inactive]]

I've done some more, and change the title and location of the page to https://developer.mozilla.org/en/Extensions/Common_causes_of_memory_leaks_in_extensions because it now talks about some leaks that aren't zombie compartments.

Stuff written by Nils that I haven't incorporated:

- The bit about js-modules in comment 1.

- The stuff about SDK add-ons in comment 1 and comment 7.

The documentation isn't good enough yet to close this bug.  But I'm at the limit of what I can usefully contribute here with my woeful knowledge of how add-ons are written.  It's possible that I've made mistakes, put things in inappropriate sections, etc.  If anyone else wants to contribute, please do!  Anyone can, it's a wiki :)

Comment 9

[Nils Maier [:nmaier]]

Now that MDN lets me log in again, I tried to polish and clarify the examples a bit, added one for Cu.unload, added some context to some things, and move some stuff around a bit.
My English and the Deki editor still suck, so somebody proof-reading my mess would be appreciated ;)

Comment 10

[Nils Maier [:nmaier]]

Added a section "Be careful with setInterval/setTimeout" and added some examples on how to unregister observers again. Both need proof-reading

Comment 11

[Nicholas Nethercote [inactive]]

I just read through.  I made a few tiny changes.  It's looking *much* better, many thanks to Nils and Sheppy.

A couple of minor remaining issues:

- I found the "Problems in bootstrapped (restartless) add-ons" example unclear.  Is that code meant to be in the bootstrap.js file?  It would be clearer if that was made explicit.

- In the "Forgetting to unregister observers" section, it says "it is possible to leak content windows".  Can you detect that from about:memory by looking at the "dom+style" sub-tree?

- Again in the observers section, it says "but that might require you to properly implement weak references as well", which is a dead-end if you don't know how to implement weak references.  Is there a suitable link that could be given?

(BTW, that observers section is really good in the way that it gives examples of overlay code *and* bootstrap.js code *and* JS code modules.)

Comment 12

[Nicholas Nethercote [inactive]]

Oh, another thing:  bug 718375 comment 13 is a really nice example of a leak in Ghostery.  But I don't understand it, because it's a reference from content to chrome, as I said in bug 718375 comment 14.  If someone could elucidate either here or there that would be great...

Comment 13

[Nils Maier [:nmaier]]

(In reply to Nicholas Nethercote [:njn] from comment #11)
> I just read through.  I made a few tiny changes.  It's looking *much*
> better, many thanks to Nils and Sheppy.
> 
> A couple of minor remaining issues:
> 
> - I found the "Problems in bootstrapped (restartless) add-ons" example
> unclear.  Is that code meant to be in the bootstrap.js file?  It would be
> clearer if that was made explicit.

Yes, in a bootstrap.js file. Actually such snippets could be also found and a JS module loaded from bootstrap.js and later unloaded when the add-on gets disabled.
Anyway, I spelled out the bootstrap.js now.

> - In the "Forgetting to unregister observers" section, it says "it is
> possible to leak content windows".  Can you detect that from about:memory by
> looking at the "dom+style" sub-tree?

If you leak a content window this way, it will show up as any other content window leak, leaving an inner-window/compartment behind.

> - Again in the observers section, it says "but that might require you to
> properly implement weak references as well", which is a dead-end if you
> don't know how to implement weak references.  Is there a suitable link that
> could be given?

I'd leave it a dead-end or remove it entirely.
I did a quick search when I revised that section, but there isn't any good JS code around. There is https://developer.mozilla.org/en/Weak_reference, but that's C++. There are ways to get the same behavior in JS, but only what I consider a hack (ab)using some XPConnect implementation details. You need to implement nsISupportsWeakReference and nsIWeakReference. Or you don't, depending on the consumer. E.g.
https://bugs.downthemall.net/browser/trunk/modules/preferences.jsm#L301
This works, as XPConnect will wrap the JS object in question again, only storing a weak reference to it.

(In reply to Nicholas Nethercote [:njn] from comment #12)
> Oh, another thing:  bug 718375 comment 13 is a really nice example of a leak
> in Ghostery.  But I don't understand it, because it's a reference from
> content to chrome, as I said in bug 718375 comment 14.  If someone could
> elucidate either here or there that would be great...

I don't understand that thing either. There is an obvious error there: anchor.id is set to a function (within the chrome compartment). It should have been:
anchor.id = ghostery.bubble.id(); // note the added function call
Could be due to some XPC wrapper voodoo, or how DOMElement coerces the function to a string to fit into .id, ...

Comment 14

[Nicholas Nethercote [inactive]]

I think the docs are in good enough shape to close this bug.  It would be nice to have a section on leaks caused by content-to-chrome references.  And of course these docs should continue to evolve over time :)

Comment 15

[Eric Shepherd [:sheppy]]

FWIW, the docs team thanks you guys for rocking this.