413281 - Firefox leaks when Yahoo Toolbar is installed

Status: VERIFIED FIXED

(Core :: General, defect, P2)

Description

[Carsten Book [:Tomcat]]

Attachment: leak log

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b3pre) Gecko/2008012014 Firefox/3.0b3pre

Steps to reproduce:
New Firefox Profile
Install the Yahoo Toolbar https://addons.mozilla.org/en-US/firefox/addon/2032
Restart Firefox
Yahoo Toolbar loads - use the default settings, and click then on the "my yahoo" icon
Close Firefox after this site is completly loaded
Leak

0 TOTAL                                          31   559979  1147875    18506 ( 3255.57 +/-  3495.46)  2858985    25001 ( 2959.95 +/-  4484.71)





nsTraceRefcntImpl::DumpStatistics: 795 entries
nsDOMNodeAllocator leaked 238496 bytes 
nsStringStats
 => mAllocCount:          48282
 => mReallocCount:         6100
 => mFreeCount:           43979  --  LEAKED 4303 !!!
 => mShareCount:          44606
 => mAdoptCount:           5949
 => mAdoptFreeCount:       5947  --  LEAKED 2 !!!

Comment 1

[Carsten Book [:Tomcat]]

Attachment: 2nd leak log

also Firefox leaks in later session when this toolbar is installed:

nsDOMNodeAllocator leaked 238496 bytes 
nsStringStats
 => mAllocCount:          43179
 => mReallocCount:         4397
 => mFreeCount:           38903  --  LEAKED 4276 !!!
 => mShareCount:          40563
 => mAdoptCount:           5318
 => mAdoptFreeCount:       5316  --  LEAKED 2 !!!

  0 TOTAL                                          33   535707   867067    17847 ( 2753.52 +/-  3071.82)  2303435    24363 ( 2803.86 +/-  4180.62)

Comment 2

[Carsten Book [:Tomcat]]

Attachment: new leak log

leak log using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2008012113 Minefield/3.0b3pre and with peter's fixes included

Comment 3

[Peter Van der Beken [:peterv]]

Still looking at this, but the documents leak through a timer callback:

0x1899b218 [nsXPCWrappedJS (nsITimerCallback)] -> 0x17afb220 [JS Object (Object)] -> 0x17afb240 [JS Object (Function)] -> 0x17afb080 [JS Object (Call)] -> 0x16e76760 [JS Object (Object)] -> 0x16e71060 [JS Object (XPCWrappedNative_NoHelper)] -> 0x182db4a0 [XPCWrappedNative] -> 0x182db038 [nsXPCWrappedJS (nsIYahooDomBuilder)] -> 0x16e6b7c0 [JS Object (Object)] -> 0x16e6baa0 [JS Object (XMLDocument)] -> 0x182d9910 [XPCWrappedNative (XMLDocument)] -> 0x14d3e00 [nsDocument ([none]) about:blank]

0x189935c8 [nsXPCWrappedJS (nsITimerCallback)] -> 0x17afb160 [JS Object (Object)] -> 0x17afb180 [JS Object (Function)] -> 0x17afb080 [JS Object (Call)] -> 0x16e76760 [JS Object (Object)] -> 0x16e71060 [JS Object (XPCWrappedNative_NoHelper)] -> 0x182db4a0 [XPCWrappedNative] -> 0x182db038 [nsXPCWrappedJS (nsIYahooDomBuilder)] -> 0x16e6b7c0 [JS Object (Object)] -> 0x16e6baa0 [JS Object (XMLDocument)] -> 0x182d9910 [XPCWrappedNative (XMLDocument)] -> 0x14d3e00 [nsDocument ([none]) about:blank]

Comment 4

[Jesse Ruderman]

> the documents leak through a timer callback

Due to bug 407201, perhaps?

Comment 5

[Peter Van der Beken [:peterv]]

Though these look like repeating timers (TYPE_REPEATING_PRECISE).

Comment 6

[Peter Van der Beken [:peterv]]

I can make the leaks drop from about 500k to about 1k by making the extension cancel its timers on xpcom-shutdown:

--- components/nsYahooFeedProcessor_old.js	2008-01-23 00:19:10.000000000 +0100
+++ components/nsYahooFeedProcessor.js	2008-01-23 00:21:23.000000000 +0100
@@ -106,6 +106,7 @@
 	init : function() {
 		if(this.loaded == false){
 			this.notifier = Components.classes["@mozilla.org/observer-service;1"].getService(Components.interfaces.nsIObserverService);
+			this.notifier.addObserver(this, "xpcom-shutdown", false);
 			this.uniconvert = Components.classes["@mozilla.org/intl/scriptableunicodeconverter"].createInstance(Components.interfaces.nsIScriptableUnicodeConverter);
 			this.uniconvert.charset = 'utf-8';
 			this.params = new YahooHashtable();
@@ -400,7 +401,20 @@
 		context = null;
 		this.loading = false;
 	},
-	/**
+	observe: function(subject, topic, data) {
+		if (topic == "xpcom-shutdown") {
+			if (this.timer) {
+				this.timer.cancel();
+				this.timer = null;
+			}
+			if (this.bm2timer) {
+				this.bm2timer.cancel();
+				this.bm2timer = null;
+			}
+			this.notifier.removeObserver(this, "xpcom-shutdown");
+		}
+	},
+ 	/**
 	* Cache the raw feed to file.
 	* The feed will only be cached if it's a guest feed.
 	*/

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Can we stop and cancel all reoccurring timers before doing the final cycle collection? That should free the references to the timer callbacks.

Comment 8

[Peter Van der Beken [:peterv]]

Attachment: Patch v1

(In reply to comment #7)
> Can we stop and cancel all reoccurring timers before doing the final cycle
> collection?

Turns out we already do that, and just nulling out the timer on xpcom-shutdown is enough. But I can get the same effect with this patch, shutting down the TimerThread currently doesn't release the callbacks on shutdown, so if anything creates a cycle through the callback we'll leak the callback. In this case the component keeps the timer alive, and the callback keeps the component alive. Since the timer doesn't release its callback the cycle makes everything leak. Ideally we'd fix this with the cycle collector, but timers are marked threadsafe. The next best thing is probably manually releasing the callbacks on shutdown.

There's still a small number of objects leaking, I'll try to figure those out too.

Comment 9

[Johnny Stenback (:jst)]

Comment on attachment 298697 [details] [diff] [review]
Patch v1

Pav should look at this. Looks ok to me fwiw.

Comment 10

[Stuart Parmenter]

Comment on attachment 298697 [details] [diff] [review]
Patch v1

should be OK

Comment 11

[Carsten Book [:Tomcat]]

Attachment: leak log after patch v1

leak log from Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b3pre) Gecko/2008012408 Firefox/3.0b3pre after the patch v1 was checked in

   0 TOTAL                                          30     1144  1174361       26 ( 3524.13 +/-  3688.99)  2822488       40 ( 2770.33 +/-  4332.76)

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Lowering to P2 since the leak is much smaller now.

Comment 13

[Peter Van der Beken [:peterv]]

Attachment: Patch 2 v1

I can fix the remaining leaks by making nsArray participate in cycle collection.

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 301165 [details] [diff] [review]
Patch 2 v1

This seems like a good idea in general.

Comment 15

[Peter Van der Beken [:peterv]]

Thsi should be fixed now.

Comment 16

[Carsten Book [:Tomcat]]

verified fixed using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US;
rv:1.9b4pre) Gecko/2008020717 Firefox/3.0b4pre not leaking anymore, great thanks peter.