922597 - browser.xul windows leak if closed before load event fires

Status: NEW

(Firefox :: General, defect)

Description

[Santiago Gimeno]

I have a very simple addon that just closes / opens a window every X seconds. The module 'sdk/windows' is not really needed in this module but if you require it the memory grows with every iteration of the interval whereas it doesn't grow if you don't require it. I'm observing this behaviour using addon-sdk-1.14 w FF24 and addon-sdk-1.12 w FF17.

The main.js of my addon is:

var page_mod = require('sdk/page-mod');
var timers = require('timers');
var win_utils = require('sdk/window/utils');
var windows = require("sdk/windows");

var main_page_mod = page_mod.PageMod({
    include : ['*'],
    onAttach : function(worker) {
        var alert_window;
        timers.setInterval(function() {
            if (alert_window) {
                console.log('closing window');
                alert_window.close();
                alert_window = undefined;
            }

            alert_window = win_utils.open('chrome://browser/content/browser.xul', {
                name : 'simple addon',
                features: {
                    width : 100,
                    height : 100,
                    chrome : true,
                    left : 100,
                    top : 100
                }
            });
        }, 6000);

        worker.on('detach', function() {
            main_page_mod.destroy();
        });
    }
});

Comment 1

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

I'm not sure why I'm needinfo'ed on this, someone needs to investigate if why is this happening. If memory
usage constantly growing without being reclaimed that's a serious problem.

Comment 2

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Jordan, can you try to investigate this?

Comment 3

[Jordan Santell [:jsantell] [@jsantell]]

Confirmed, spamming window opens and closes causes memory to shoot up, mostly from window-objects -> top(none)/detached -> window([system]) -> js-compartment([SystemPrincipal], about:blank), judging from about:memory -- will look into it more

Comment 4

[Jordan Santell [:jsantell] [@jsantell]]

Not sure what's happening here quite yet. Looks like when opening/closing windows in scratchpad using the window watcher directly, it reclaims memory. When running inside jetpack code, it is not reclaimed (either using the jetpack module, or the WW directly)

https://gist.github.com/jsantell/7091737

Comment 5

[Jeff Griffiths (:canuckistani) (:⚡︎)]

How big is this leak, and does it go away after GC?

Comment 6

[Jordan Santell [:jsantell] [@jsantell]]

Around 200MB alone for creating 100 windows, and GC does not clear it

Comment 7

[Jordan Santell [:jsantell] [@jsantell]]

When rapid fire open/closing windows (using WindowWatcher xpcom, none of our Window modules are used) in an addon, the memory is never recollected (although recollected when in scratchpad with or without addons installed).

When opening and closing windows via waiting for ready event, or waiting about a second, the memory is successfully collected.

It appears that closing a window before it's completely opened, only in an addon, causes this issue. We suspect something in bootstrap or addon is holding on (doesn't look like it), or possibly the platform doesn't correctly trash objects that were 'touched' by an addon and closed correctly

Comment 8

[Jordan Santell [:jsantell] [@jsantell]]

Unassigning myself from bugs I haven't gotten around to

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

This is CustomizableUI's gListeners holding on to a TabsInTitlebar instance:

0000022E9A73B060 [BackstagePass 22e990b3a00]
    --[gListeners]--> 0000022E9A72DE80 [Set 22e995753a0]
    --[key]--> 0000022EA2959220 [Proxy <no private>]
    --[private]--> 0000022EB5FC00E0 [Object <no private>]
    --[uninit]--> 0000022EB5FEDE00 [Function TabsInTitlebar.uninit]
    --[fun_environment]--> 0000022EB5AFA070 [Block <no private>]
    --[enclosing_environment, **UNKNOWN SLOT 1**]--> 0000022EB5AF9060 [Window <no private>]

I think the easiest thing to do here is use a WeakSet in CustomizableUI.

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Seems the bigger issue is that we aren't firing unload events for these windows.  So browser.js isn't running any of its cleanup code.

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

So, I can't reproduce the part from comment 0 that says:

> The module 'sdk/windows' is not really needed in this module but if you require it the memory grows with every iteration of the interval whereas it doesn't grow if you don't require it.

Testing the gist in comment 4 still leaks, but I don't think its an addon issue.  We simply do a lot of resource initialization in browser/base/* code before the load event fires.  We then fail to clean this stuff up because unload is not fired (we try not to fire unload if load didn't fire).

I think fixing this would require a fair amount of work to fix many issues in browser/base/*.  Its not clear its worth it, though, since opening and closing a window quickly is sort of a corner case.  It doesn't really happen in practice that often.  Maybe it happens in our tests sometimes causing intermittent leaks, though.

Anyway, I'd rather focus on things our users are likely to hit in real use, so I'm not going to work this right now.

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Lets re-triage this since the current leak here is different from the original leak.