Is my implementation of unloaders proper?

Question

I was re-reading this post here: https://stackoverflow.com/a/24473888/1828637

And got concerned about if I did things correctly. This is how I do unloading:

So I set up some stuff per window. And unload them on shutdown. (i dont unload on window close, i havent found a need to yet, as when it closes, everything i added to it goes with with the close [such as my mutation observers]).

All code below is theoretical, the mutation stuff is example, so there might be typos or bugs in it. I was wondering if the idea behind it is appropriate:

var unloadersOnShutdown = [];
var unloadersOnClose = [];

function startup() {
        let DOMWindows = Services.wm.getEnumerator(null);
        while (DOMWindows.hasMoreElements()) {
            let aDOMWindow = DOMWindows.getNext();
            var worker = new winWorker(aDOMWindow);
            unloadersOnShutdown.push({DOMWindow: aDOMWindow, fn: worker.destroy});
        }
}

function shutdown() {
        Array.forEach.call(unloadersOnShutdown, function(obj) {
            //should probably test if obj.DOMWindow exists/is open, but just put it in try-ctach
            try {
                obj.fn();
            } catch(ex) {
                //window was probably closed
                console.warn('on shutdown unlaoder:', ex);
            }
        });
}

function winWorker(aDOMWindow) {
    this.DOMWindow = aDOMWindow;
    this.init();
}

winWorker.prototype = {
    init: function() {
        this.gMutationObserver = new this.DOMWindow.MutationObserver(gMutationFunc.bind(this));
        this.myElement = this.DOMWindow.querySelector('#myXulEl');
        this.gMutationObserver.observe(this.myElement, gMutationConfig);
        if (this.DOMWindow.gBrowser && this.DOMWindow.gBrowser.tabContainer) {
            this.onTabSelectBinded = this.onTabSelect.bind(this);
            this.gBrowser.tabContainer.addEventListener('TabSelect', this.onTabSelectBinded, false);
        }
    },
    destroy: function() {
        this.gMutationObserver.disconnect();
        if (this.onTabSelectBinded) {
            this.gBrowser.tabContainer.removeEventListener('TabSelect', this.onTabSelectBinded, false);
        }
    },
    onTabSelect: function() {
        console.log('tab selected = ', thisDOMWindow.gBrowser.selectedTab);
    }
};

var windowListener = {
    onOpenWindow: function (aXULWindow) {},
    onCloseWindow: function (aXULWindow) {
        var DOMWindow = aXULWindow.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowInternal || Ci.nsIDOMWindow);
        for (var i=0; i<unloadersOnClose.length; i++) {
            if (unloadersOnClose.DOMWindow == DOMWindow) {
                try {
                    unloadersOnClose.fn();
                } catch(ex) {
                    console.warn('on close unloader:', ex);
                }
                unloadersOnClose.splice(i, 1);
                i--;
            }
        }
    },
    onWindowTitleChange: function (aXULWindow, aNewTitle) {},
}

I think one problem is me not using weak references with DOMWindows but I'm not sure.

score 2 · Accepted Answer · answered Jul 21 '14 at 11:26

2

The idea around unloaders in general seems to be OK, but very limited (to windows only).

The implementation is lacking. E.g. there is a big, fat bug:

unloadersOnShutdown.push({DOMWindow: aDOMWindow, fn: worker.destroy});
// and
obj.fn();
// or
unloadersOnClose.fn();

This will call winWorker.prototype.destroy with the wrong this.

The i++/i-- loop also looks, um... "interesting"?!

Also, keep in mind that there can be subtle leaks, so you should mind and test for Zombie compartments. Not only can a window leak parts of your add-on (e.g. bootstrap.js) but it is also possible to leak closed windows by keeping references in your add-on. And of course, it's not just windows you need to care about, but also e.g. observers, other types of (XPCOM) listeners etc.

answered Jul 21 '14 at 11:26

nmaier

32,336
5
63
78

Re the wrong `this`: What if he did __(1)__ `unloadersOnShutdown.push(worker)` (no object) then on shutdown he did `unloadersOnShutdown[i].destroy()`? or __(2)__ `unloadersOnShutdown.push({DOMWindow: aDOMWindow, fn: worker.destroy.bind(null)})` (bind null)? or __(3)__ `var worker = new winWorker.bind(null, DOMWindow); unloaders.push(fn:worker.destroy);` (bind on worker)? lol @ wiki link to interesting – Blagoh Jul 21 '14 at 16:51
2

@Blagoh 1) Would work. 2) Wouldn't work `this === null`. Should be `worker.destroy.bind(worker)`. 3) Doesn't make sense. Bind on worker is 2). – nmaier Jul 21 '14 at 17:05
Haha what do you mean about the i++ i-- thing? :P Thanks man. Thanks for that zombie page, I think it has answers to my question of "when to run unloaders not limited to windows". – Noitidart Jul 21 '14 at 21:17
I read most of the article and : [Common causes of memory leaks in extensions](https://developer.mozilla.org/en-US/docs/Extensions/Common_causes_of_memory_leaks_in_extensions#Forgetting_to_unload_JavaScript_modules_in_restartless_add-ons). It says that event listeners are removed on window close, so I still dont know what to unload on win close :(. The major flaw, aside from the bug you pointed out (thx!) is that I need to use weak references. Do u think theres something else I need to do? I know its best to test it with the recommended `about:memory` ill do that but just need confidence – Noitidart Jul 22 '14 at 05:19

Is my implementation of unloaders proper?

1 Answers1