811128 - One add-on holds a Zombie Compartment for another add-on

Status: RESOLVED INVALID

(WebExtensions :: General, defect)

Description

[Luís Miguel [:quicksaver] (not reading bugmail)]

Attachment: ZCtests.zip

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:16.0) Gecko/20100101 Firefox/16.0
Build ID: 20121024073032

Steps to reproduce:

The zip file contains two xpis, each with identical code differing only in the method through which they should add a property in a chrome window object: either directly by passing the window as an argument (ZCtestB) or indirectly through the window mediator service (ZCtestA). The code is a striped-down version of the code I use in my add-ons, however it is enough as an example of my issue.

In a firefox browser window (most of my tests were in the latest aurora):
1) Enable ZCtestA
2) Enable ZCtestB
3) Disable ZCtestA

Through the about:compartments page, I can see that the compartment for ZCtestA is still there, until I also disable ZCtestB, then both of the compartments disappear. For some reason, which I cannot make out, ZCtestB is holding the compartment for ZCtestA. This happens even though everything apparently worked as supposed to, all objects and properties were removed as well as all listeners and observers placed (as far as I can tell).

If I do the reverse (enable B, then A then disable B) ZCtestA will not hold the compartment for ZCtestB, it will be successfully removed. By themselves, they don't leave any ZC and efficiently clean up at shutdown.

(If you toy around you can make ZCtestB leave a ZC on its own. I'm aware of that and I think it's because there are many functionalities lacking, in particular a self-cleaner I have written just for that, as it's only a stripped down version. In the actual add-ons that never happened, only the ZC I mentioned is giving me trouble).

Comment 1

[Kris Maglione [:kmag]]

That's got to be the most convoluted initialization and deinitialization process I've ever seen...

I'm having a hard time figuring out what's supposed to be going on, let alone what is going on, let alone why in the indirect version you add a new object suggestion browser.xul windows should be modified and then iterate... anyway, I'm not going to try to figure this out. The two add-ons both use the same globals on the windows that they modify, and seem to step all over each other. I'm not surprised that running them alongside each other causes issues.

If you can come up with a more minimal and comprehensible testcase, I'll look again.

Comment 2

[Luís Miguel [:quicksaver] (not reading bugmail)]

If you can believe it that is the simplified version.

The idea is to be able to use XUL overlays in my bootstrapped add-ons. However I needed a way for the add-ons to communicate with each other, in particular to know what overlays have been loaded into what window and be able to overlay each other as well. The only way to do that is to use the same object in a chrome window. Apart from this ZC, the script works quite well, I can use it to load and unload several xul files at once and in phases as I would in non-bootstrapped add-ons without interfering with other add-ons using the same method, and yes, it's much better organized than this, I only wrote this because I've spent the last 3 days around the issue without finding any solution and needed help.

The object _ZCtests is a stand-in for this, in this case containing only the name of the add-on instead of the overlays details. I'll admit this script is a bit messy in these files as I tried to strip it down to the very minimum. I'll try to clarify.

- In startup(): I "start" the add-on, by placing the name of the add-on on each window's object, by calling startAddon() on every opened and to be opened window.

- The window object eventually ends up on addAll(aWindow) after it has loaded (I don't do anything on unloaded windows to prevent errors), which creates an array on aWindow._ZCtests if it doesn't have one already. To this array it pushes the name of the add-on and that is pretty much it.

- In shutdown(), it checks any opened window objects for the _ZCtests array, and if the array contains the name of the add-on it splices it from the array, and if the array's length is 0 it also deletes the the array.

- The difference between ZCtestA and ZCtestB is the way the window objects are processed:

--ZCtestA uses an indirect method, by referencing the uri of that window's document. It first adds the name of the add-on to an array inside the add-ons sandbox, then calls all opened windows and, if the window's document uri matches that which was supplied (../browser.xul) it will process it asynchronously (timeout of 0s). Because in startup() I also set a window watcher for this through the window mediator, any subsequent opened windows that match the uri will also be processed.

--ZCtestB uses a direct method, by passing the window object itself and the add-on name as arguments directly, creating the _ZCtests array in the window object without going through the window mediator service.

-- After this, in both cases, the window is processed in addAll(), it first sees if there are any entries in _ZCtests with the .loaded property as (bool) false (these would be the ones added directly), and if there are it "loads" them (in this case it just sets loaded to (bool) true). Then it proceeds to the sandbox.toAdd object (entries added indirectly), loading any into the _ZCtests that aren't already there.

- On shutdown, the name of the add-on is removed from the _ZCtests array, either by splicing the entry from the sandbox.toAdd if it was added indirectly or by setting it's .remove property to (bool) true in the _ZCtests array entry itself if it was added directly. In both cases, the name of the add-on is added to an window._ZCtestsUnload array, which will afterwards be processed in addAll() to remove the entries from _ZCtests accordingly (if they exist!).

- I wrote it in a way that, when removing an entry of lower index in the _ZCtests array, it first unloads all the others above it, from last to first. The reason for this isn't evidenced in these scripts but it's basically to ensure unloading something doesn't screw up what was loaded afterwards. It's necessary.

The methods callOnLoad, listenOnce and removeOnceListener are just self-removing "load" event listeners. The xmlHttpRequest calls are there because it's at those points I load the xul documents that will act as overlays, so I needed to keep this sample script faithful to that, ensuring there is a "break" in the loading of the add-on name to the window._ZCtests object at the point there would also be a "break" when loading the overlays.

When I follow things through with DOM Inspector, I see that the add-on names are added to the _ZCtests and removed as expected, and no errors are evident in the error console. Plus, since both compartments disappear when I disable ZCtestB after disabling ZCtestA, I can only assume everything actually worked (by that I mean everything that was supposed to be removed was removed). It seems like a very complex way to start a script, and even that only one of the ways would be needed. However while I was developing it I came across situations where it was necessary to use both ways, hence the "convoluted" initialization as you put it (I guess if you ever see the actual scripts you'll either laugh or throw a pineapple at my face).

When you said the two add-ons use the same object and step on each other, that is not true, each add-on is able to recognize which entries refer to itself and act appropriately. The script is also built in a way that each add-on can handle entries from the other add-on, as would be necessary since it needs to remove entries that were added after the one it's trying to remove. Just considering this specific test case, this process consists on splicing an item from an array or setting a bool property, hardly something that would cause two add-ons to trample on each other. (In reality of course I considered this, I have an army of checks to prevent any errors, however for this test case they are all redundant).

I hope my quick "guide" helped, however on second look, I have to agree the scripts are messy. Tomorrow I'll try to organize them better and add comments along the way, to be more easily read.

Comment 3

[Kris Maglione [:kmag]]

The most obvious problem is that the bootstrap script installs arrays from its own compartment on browser window and doesn't expect it to be removed until both add-ons are disabled. There are ways around this, but the best approach, in my opinion, is to simplify your overlay process and make both add-ons agnostic of each other.

In any case, there's nothing in this that leads me to think it's a Firefox bug. There are so many legitimate ways that two add-ons interacting the way that yours do could hold references to each other alive after one is disabled that, unless you can come up with a very minimal testcase that proves otherwise, I'm marking this Invalid.