[Extension] Bandwagon Extension leaks multiple nsGlobalWindows

VERIFIED FIXED in BW-M6

Status

defect
--
critical
VERIFIED FIXED
10 years ago
3 years ago

People

(Reporter: cbook, Assigned: cbook)

Tracking

unspecified
BW-M6
x86
macOS

Details

Attachments

(2 attachments)

Assignee

Description

10 years ago
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
How does one go about tracking down leaks in an extension. Is there a doc somewhere listing common causes?
Target Milestone: --- → BW-M5
Assignee

Comment 3

10 years ago
(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 ?
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?
(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);
    }
};

Updated

10 years ago
Duplicate of this bug: 470248
I sent an XPI with the fix to Tomcat earlier today for testing.
Target Milestone: BW-M5 → BW-M6
-> Tomcat for testing results.
Assignee: brian → cbook
Assignee

Comment 9

10 years ago
(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
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.
Assignee

Comment 11

10 years ago
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
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Assignee

Comment 12

10 years ago
verified
Status: RESOLVED → VERIFIED
Component: Collections → Collector Extension
QA Contact: collections → collector-extension
Product: addons.mozilla.org → addons.mozilla.org Graveyard
You need to log in before you can comment on or make changes to this bug.