Last Comment Bug 727413 - Geenstijl extension 2.11.0.1 causes zombie compartments
: Geenstijl extension 2.11.0.1 causes zombie compartments
Status: RESOLVED FIXED
[MemShrink:P3]
:
Product: Core
Classification: Components
Component: General (show other bugs)
: Trunk
: x86_64 Windows 7
: -- normal (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
:
Mentors:
https://addons.mozilla.org/en-US/fire...
Depends on:
Blocks: LeakyAddons
  Show dependency treegraph
 
Reported: 2012-02-15 05:18 PST by Andrew Williamson [:eviljeff]
Modified: 2012-02-23 21:27 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments

Description Andrew Williamson [:eviljeff] 2012-02-15 05:18:47 PST
The above addon leaks a reference to the geenstijl website after the page is closed.
Just viewing a blog page such as http://www.geenstijl.nl/mt/archieven/2012/02/jonge_verzorgde_vrouw_beukt_91.html#comments is sufficient.

After the page is closed about:memory?verbose still shows a reference to the webpage.

Developer will be informed and invited to join bug.
Comment 1 Atmos 2012-02-15 06:48:16 PST
Looked at geenstijlplugin.js, the problem code must be in here.

When I started on this plug-in I used a greecemonkey template and modified it slightly.

This greecemonkey template has a function called Hitch:

// This method comes from a greecemonkey script and is used to add a method to the DOM window
atm0sNamespace("atm0s.geenstijl.mozilla.extension").hitch = function(obj, meth) {
    // Check if the given method name exists on the given object
    if (!obj[meth]) {
        throw "method '" + meth + "' does not exist on object '" + obj + "'";
    }
    
    // Reference caller
    var hitchCaller = Components.stack.caller.filename;
    // Get arguments
    var staticArgs = Array.prototype.splice.call(arguments, 2, arguments.length);
    
    // Return the method
    return function() {
        // make a copy of staticArgs (don't modify it because it gets reused for
        // every invocation).
        var args = staticArgs.concat();

        // add all the new arguments
        for (var i = 0; i < arguments.length; i++) {
            args.push(arguments[i]);
        }

        // invoke the original function with the correct this obj and the combined
        // list of static and dynamic arguments.
        return obj[meth].apply(obj, args);
    };
}

I use this method to expose a few functions to the page that act as an interface to the plug-in code, basicly so I can store and read preferences, open new tabs without the pop-up blocker interfring and also to launch webworkers.

But, these function (assigned to DOMWindow) are never removed, I think because the original greecemonkey script I followed, didn't either.

You would think that a garbagecollector should understand.. right?

If I am right, it would mean that there would be any leaks, if you first navigate to e.g. www.google.com before closing the tab, is this assumption correct?
Comment 2 Atmos 2012-02-15 06:50:35 PST
Correction:

If I am right, it would mean that there would _NOT_ be any leaks, if you first navigate to e.g. www.google.com before closing the tab, is this assumption correct?
Comment 3 Andrew Williamson [:eviljeff] 2012-02-15 07:18:52 PST
Hi, 

from further testing, navigating to another site after a page geenstijl, before closing the tab, still leads to a leak to the blog page on geenstijl.  The subsequent webpage is leaked as well(!)

I did the following:
1) navigated to http://www.geenstijl.nl/mt/archieven/2012/02/jonge_verzorgde_vrouw_beukt_91.html#comments
2) loaded google.co.uk
3) closed tab
4) checked about:memory?verbose and saw a reference to geenstijl AND google.co.uk

It doesn't seem to happen if geenstijl isn't loaded in the tab first. i.e. Just navigating to google.co.uk, then bing.com, then closing the tab, doesn't leak.
Comment 4 Atmos 2012-02-16 02:41:27 PST
Ok thanks Andrew,

Learned myself to use the build-in memory tool, and checked how to test for ghost compartments.

I noticed that the plug-in indeed caused the problem.

I uploaded a new version 2.11.0.2

This is what I changed in geenstijlplugin.js:

1) Added an unload handler that removed the referenced methods attached to the DOM window (this did not help)

2) Renamed the namespace in geenstijlplugin.js from atm0s.* to atm0sxul.* since these are also used in the injected scripts (this did help, but not completely, read below)

Now, when opening geenstijl.nl and then after page loaded completely, then closing the tab, there are no ghost compartments any more.

But, I noticed, that when a new webworker ( chrome://igeenstijlplugin/content/scripts/atm0s.geenstijl.mozilla.extension.commentFormatterWorker2.js )  is created more then once in a single TAB, ghost compartments arise.

This can be accomplished by either, reloading the page for a second time, or changing the KUDO filter (below the main post and the comments) this also invokes a new webworker.

I checked to see if the call to webworker.Terminate() is actually called, and I can confirm this.

I am running out of ideas now, and I think you were just a little to fast by stating that this is_not_a_firefox_bug.. I am not so sure.
Comment 5 Atmos 2012-02-16 02:47:00 PST
Totally of-topic:
 
I noticed that the Developers hub (since today) localized for me to Dutch.
 
O-My-God!! Even MSDN doesn't translate that-bad!! Laughing stock!

I changed back to English ;)
Comment 6 Atmos 2012-02-16 02:47:46 PST
I know, my English is evenly bad :)
Comment 7 Andrew Williamson [:eviljeff] 2012-02-16 07:12:31 PST
(In reply to Atmos from comment #4)
> But, I noticed, that when a new webworker (
> chrome://igeenstijlplugin/content/scripts/atm0s.geenstijl.mozilla.extension.
> commentFormatterWorker2.js )  is created more then once in a single TAB,
> ghost compartments arise.
> 
> This can be accomplished by either, reloading the page for a second time, or
> changing the KUDO filter (below the main post and the comments) this also
> invokes a new webworker.
> 
> I checked to see if the call to webworker.Terminate() is actually called,
> and I can confirm this.

So, the webworker holds a reference to the page and Terminate() is called but isn't terminated?  Or is terminated but there is still a reference?

Cc'ing Justin on this to see if he has any insight into the issue or if its come up with other addons.
Comment 8 Atmos 2012-02-16 07:30:27 PST
(In reply to Andrew Williamson [:eviljeff] from comment #7)
> So, the webworker holds a reference to the page and Terminate() is called
> but isn't terminated?  Or is terminated but there is still a reference?
> 
> Cc'ing Justin on this to see if he has any insight into the issue or if its
> come up with other addons.

I don't know if it is terminated or not, the webworker is eh working, everything does what it should, only it seems to be related to the zombie compartment issue, for it is the only thing different from closing the tab right away.

I can not debug firefox, only my own code, and that does what I expect it to do, maybe someone should inspect the workings of geenstijlplugin.js and see if that COULD cause a problem with memory leaks according to what is expected.

Greetz,

atm0s
Comment 9 Atmos 2012-02-16 07:31:38 PST
And please inspect that latest uploaded version, not the one mentioned above.
Comment 10 Atmos 2012-02-16 09:36:11 PST
Ah *facepalm*
 
I just realized that my webworker also uses the same namespace as the injected scripts.

I always thought that these injected scripts would be running in a different "scope", so that could not give any problems, let me first try to change this, and and see if this solves the problem.

I'll report my findings later.
Comment 11 Atmos 2012-02-18 07:35:44 PST
I tried everyhing.. I think the problem lays in the fact that my injected scripts are not running in the scope of the page, but instead in something much higher up like Chrome ..

What do I need to change to the scheme below, to inject my scripts in the security scope of the DOM content, so the garbage collector will remove them along with the rest when the page is unloaded?

-->


OnDomContentLoad = function(e) {
    // skipping some code here that checks on location and top frame checking

    // Reference window object
    var windowObj = e.target.defaultView;
    if (windowObj.wrappedJSObject) windowObj = windowObj.wrappedJSObject;

    loadScript(....)

}

loadScript = function(fileName, window) {
    // Create element
    var newScript = window.document.createElement('script');
    // Mark type as javascript
    newScript.setAttribute('type', 'text/javascript');
    // Reference local plug-in script file
    newScript.setAttribute('src', 'chrome://igeenstijlplugin/content/scripts/' + fileName);
    // Append it to the DOM
    window.document.body.appendChild(newScript);
}

Thanks in advance for your help solving this.
Comment 12 Justin Lebar (not reading bugmail) 2012-02-18 07:56:22 PST
I don't see anything obviously wrong  with the code attached in comment 11.  What makes you think this is where the problem lies?
Comment 13 Atmos 2012-02-18 08:16:48 PST
(In reply to Justin Lebar [:jlebar] from comment #12)
> I don't see anything obviously wrong  with the code attached in comment 11. 
> What makes you think this is where the problem lies?

Correct me if I am wronge,

but this problem can only happin when some reference exist between the (zombie) compartment and the plug-in, causing the garbage collector to think the zombie compartment is still in use, right?

But the only references I make, is injecting these script the way I showed comment #11, and the previously mentioned (Hitched) functions I add to the Dom Window.

These (hitched) functions, are then later set to NULL by an Unload handler (I checked by inserting alert()'s, these are always called)

Besides these two things, the only thing that is left, are the fact that I keep copies of objects in my plug-in code (passed as json strings between injected scripts and plugin-code).

Anyway, I have no idea what it could be.

Please use this latest version of the plug-in if you want to see my code, this version isn't uploaded to Mozilla yet, but includes all the namespace clean-ups I made:

http://search-geenstijl.thruhere.net/search/Geenstijl_Extension_2.11.0.3.xpi
Comment 14 Jorge Villalobos [:jorgev] 2012-02-22 13:50:00 PST
I just tested the version posted on comment #13 and I can't reproduce the leaks. Please submit it to AMO so other editors can have a look.

Thanks!
Comment 15 Andrew Williamson [:eviljeff] 2012-02-23 11:38:44 PST
fyi, I just reviewed the version uploaded to AMO and had to preliminary review it due to the memory leak still existing.  Fingers crossed that this time its just a syntax error on the removeEventListener call though.
Comment 16 Jorge Villalobos [:jorgev] 2012-02-23 11:57:54 PST
I would also like to point out that the STR in previous comments didn't work consistently for me in order to find the leak.

I can consistently reproduce the leak by going to the home page (http://www.geenstijl.nl) and then closing the tab.
Comment 17 Atmos 2012-02-23 13:38:15 PST
I uploaded a new version in which the syntax error is corrected.
 
Now, I can not reproduce the zombie compartment anymore. :)

Other issue,

if you open (with plug-in) an url like:

http://www.geenstijl.nl/mt/archieven/2012/02/rijkman_groenink_284_miljoen_i.html#c148976571

Wait for the page to completely load,

then select the combobox that says "toon: alles" and change it to "> -2", the page becomes empty, something goes horribly wronge :)

What it does, is start a new WebWorker that only regenerates the HTML, using Venkman I can see the right data being serialized and send using Postmessage() to the worker, but it does not allow me to debug the worker js files.
 
Does anyone know, how to debug this? I think this code broke with some Firefox version somewhere around version 6 or so.

Note You need to log in before you can comment on or make changes to this bug.