487704 - [Extension] Bandwagon Extension leaks multiple nsGlobalWindows

Status: VERIFIED FIXED

Description

[Carsten Book [:Tomcat]]

Created attachment 371953 [details]
memory Leak Log with the steps to reproduce from comment 0

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.10pre) Gecko/2009040813 Firefox/3.0.10pre Debug Build refcnt enabled

Steps to reproduce:
-> New Profile
-> Install the Bandwagon Extension
-> 2 Windows, Default Homepage + the Tab that the Extension opens
-> Wait till each site is completly loaded 
-> Shutdown Firefox
--> Leak !

The Problem is, that this leak is building up. During the Tests with the steps to reproduce from above i get :

0 TOTAL                                          24   231219   572342     7291 ( 1477.93 +/-  1492.63)  1451895     3597 ( 1826.39 +/-  2914.76)

and when playing around with the extension for a while :
0 TOTAL                                          24   752955  2744888    26734 ( 3282.66 +/-  2613.12)  6449551    22260 ( 3770.16 +/-  5702.99)

I think this is a blocker for the release of the extension. CC'ing our Memory Leak Ninjas, Jonas, Ben and Peter

Comment 1

[Carsten Book [:Tomcat]]

Created attachment 371954 [details]
leak log when playing around for a while

Comment 2

[Brian King [:kinger]]

How does one go about tracking down leaks in an extension. Is there a doc somewhere listing common causes?

Comment 3

[Carsten Book [:Tomcat]]

(In reply to comment #2)
> How does one go about tracking down leaks in an extension. Is there a doc
> somewhere listing common causes?

Jonas, can you help here ?

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

So there's a cycle here, for sure. Simple fix is to add this to the serivce's uninit method:

+  this._service = null;
+  this._collectionFactory = null;
+  Bandwagon = null;
+  bandwagonService = null;

In general you've got global references in the service to an object from a browser window which guarantees that you'll hang on to a window for too long. I haven't really looked deeply enough to know exactly why the cycle collector chokes here yet... Something about the window holding the service's nsIFactory/nsIModule alive maybe?

Comment 5

[Brian King [:kinger]]

(In reply to comment #4)
> So there's a cycle here, for sure. Simple fix is to add this to the serivce's
> uninit method:
> 
> +  this._service = null;
> +  this._collectionFactory = null;
> +  Bandwagon = null;
> +  bandwagonService = null;

Thanks Ben, I've checked this in. I ran the leak guage tests and did not come up with anything, so it looks good. I'm going to run a build by Tomcat however just to make sure.

> haven't really looked deeply enough to know exactly why the cycle collector
> chokes here yet... Something about the window holding the service's
> nsIFactory/nsIModule alive maybe?

Does this look suspicious?

var BandwagonServiceFactory = {
    singleton: null,
    createInstance: function (aOuter, aIID)
    {
        if (aOuter != null)
            throw Components.results.NS_ERROR_NO_AGGREGATION;

        if (this.singleton == null)
            this.singleton = new BandwagonService();

        return this.singleton.QueryInterface(aIID);
    }
};

Comment 7

[Brian King [:kinger]]

I sent an XPI with the fix to Tomcat earlier today for testing.

Comment 8

[Brian King [:kinger]]

-> Tomcat for testing results.

Comment 9

[Carsten Book [:Tomcat]]

(In reply to comment #7)
> I sent an XPI with the fix to Tomcat earlier today for testing.

Hi Brian,

I have checked my emails, but i got no xpi ? Could you resend the xpi ? 

- Tomcat

Comment 10

[Justin Scott [:fligtar]]

Tomcat - I just emailed the latest version to you. Please let me know if you don't get it -- maybe something is blocking it because I know I've sent a few previously.

Comment 11

[Carsten Book [:Tomcat]]

yeah got it now, thanks justin.

Marking this fixed. Version 0.4 show no leak !

== BloatView: ALL (cumulative) LEAK STATISTICS

     |<----------------Class--------------->|<-----Bytes------>|<----------------Objects---------------->|<--------------References-------------->|
                                              Per-Inst   Leaked    Total      Rem      Mean       StdDev     Total      Rem      Mean       StdDev
nsTraceRefcntImpl::DumpStatistics: 782 entries
nsStringStats
 => mAllocCount:          42921
 => mReallocCount:         4777
 => mFreeCount:           42921
 => mShareCount:          41814
 => mAdoptCount:           5504
 => mAdoptFreeCount:       5504

thanks guys, marking this bug fixed

Comment 12

[Carsten Book [:Tomcat]]

verified