336955 - Leak in Firefox software update (updates.js)

Status: RESOLVED WORKSFORME

(Toolkit :: Application Update, defect)

Description

[Pavel Cvrcek [:JasnaPaka]]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060506 Minefield/3.0a1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060506 Minefield/3.0a1

Firefox Trunk, clean profile.

Reproducible: Always

Steps to Reproduce:
1. Install Leak Monitor Extension (http://dbaron.org/mozilla/leak-monitor/).
2. Close Firefox.
3. Open Firefox.
4. Go to Help -> Check for Updates and try to update. In my situation Firefox doesn't find any updates. Click on Finish.

Actual Results:  
New Leak Alert is showed.


Nothing is showed.

Comment 1

[Pavel Cvrcek [:JasnaPaka]]

Attachment: Leak Monitor Extension - Result

Comment 2

[:Gavin Sharp [email: [e-mail]]]

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/mozapps/update/src/nsUpdateService.js.in&rev=1.106#1739

The Checker object needs to let go of it's _callback once it's done with it.

Comment 3

[Doron Rosenberg (IBM)]

I actually tried nulling it out, but the tool still reports the leak.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

In all cases? There are multiple code paths that lead to using _callback. I thought I had a patch that worked, but now I can't find it, so maybe I'm thinking of something else.

Comment 5

[Doron Rosenberg (IBM)]

(In reply to comment #4)
> In all cases? There are multiple code paths that lead to using _callback. I
> thought I had a patch that worked, but now I can't find it, so maybe I'm
> thinking of something else.
> 

I nulled out _callback when we called the cancel method when you cancel the update wizard, even dumped _callback and it was null. bizaare.

Comment 6

[Régis Caspar]

I've a patch that fixe most of the talckback leaks but there is still one when there is an update and you choose install later (a nsITimerCallback)

Comment 7

[Régis Caspar]

Attachment: partial fix (comment #6)

Comment 8

[Régis Caspar]

(In reply to comment #7)
> Created an attachment (id=221475) [edit]
> partial fix (comment #6)
Got it, just the time to prepare and verify my patch.

Comment 9

[Régis Caspar]

Attachment: Fix

This fix totally the issue by cleaning TimerManager._timers[]

Comment 10

[Dão Gottwald [:dao]]

(In reply to comment #9)
> Created an attachment (id=221481) [edit]
...
> +      for (timerID in this._timers) {
> +        this._timers[timerID].callback = null;
> +        this._timers[timerID] = null;
> +      }

Is this._timers an array? If it is, you shouldn't use "for in", it's much slower than "for (var i = 0; i < this._timers.length; i++)". In any case, don't forget "var".

By the way, is there a benefit of "this._checker = null;" over "delete this._checker;"?

Comment 11

[Régis Caspar]

(In reply to comment #10)
> Is this._timers an array? If it is, you shouldn't use "for in", it's much
> slower than "for (var i = 0; i < this._timers.length; i++)". In any case, don't
> forget "var".
OK, I used "for in" because there is another one just below of a patch addition in nsUpdateService.js
 
> By the way, is there a benefit of "this._checker = null;" over "delete
> this._checker;"?
Don't know.

(In reply to comment #9)
> This fix totally the issue by cleaning TimerManager._timers[]
Hum I don't understand everything with app.update.url.override all was OK I removed it and I'm back with a leak of nsITimerCallback (Note: there is no update on AUS)
Adding a: "notifyObservers(null, "please-dont-leak-me", null)" to gNoUpdatesPage.onPageShow seems to fix this remaining leak. Howerver, I think that perhaps the notifyObservers should be in destroyChecker() and that perhaps there are still missing "pathway" to clean (I think about there is an update -> update later). Will look into that deeply later.

Comment 12

[:Gavin Sharp [email: [e-mail]]]

*** Bug 323402 has been marked as a duplicate of this bug. ***

Comment 13

[u88484]

Regis, my bug 323402 was about a leak in chrome://mozapps/content/update/updates.xul, I know that updates.js is called upon in updates.xul but I'm just checking to make sure this is the same leak.

Comment 14

[Régis Caspar]

(In reply to comment #13)
> Regis, my bug 323402 was about a leak in
> chrome://mozapps/content/update/updates.xul, I know that updates.js is called
> upon in updates.xul but I'm just checking to make sure this is the same leak.
There are many leaks in updater.js depending where you click and where the updater goes. I still have a remaining one when later is clicked (after a download). Now I need to narrow down what is exactly needed and where because once again this things got me: I added some log to console in order to follows thing and all was nice. Then I removed my log mess in order to make a patch but the callback leaks came back. 

Comment 15

[Ria Klaassen (not reading all bugmail)]

*** Bug 338411 has been marked as a duplicate of this bug. ***

Comment 16

[Régis Caspar]

*** Bug 338412 has been marked as a duplicate of this bug. ***

Comment 17

[Pavel Cvrcek [:JasnaPaka]]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060602 Minefield/3.0a1

With Leak Monitor 0.3 New Leak Alert isn't showed.

Comment 18

[Michael Wu [:mwu]]

Leak Monitor 0.2.0 shows this, Leak Monitor 0.3.0 does not. Leak Monitor 0.3.0 fixed false positives. I'm betting this is one of them. FWIW, leak-gauge.pl doesn't show anything either.

Comment 19

[Haudy Kazemi]

I have Leak Monitor 0.3.4 on Mozilla/5.0 BonEcho/2.0a3 ID:2006070704.
I am not seeing this leak take place on this build.  I cannot reproduce it.