349379 - nsIUpdateTimerManager registerTimer is leaking

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Robert Strong (they/them - no direct email)]

Found while debugging bug 349173. For each registerTimer I see 48 leaks.

Comment 1

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

Perhaps related to bug 321667 (though that bug is pretty low on details, and indicates that it may be fixed).

Comment 2

[Robert Strong (they/them - no direct email)]

Yep... I suspect this is a duplicate of that bug... I searched, I swear I did. :)

Comment 3

[Adam Guthrie]

This isn't a duplicate of bug 321667, but the root cause could be related.

I think this may have something to do with how these timers persist across shutdown; e.g. they're never canceled. The update service seems to be removing references to its timers on shutdown properly, though: http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/toolkit/mozapps/update/src/nsUpdateService.js.in&rev=1.116&mark=2563-2564#2554.

Comment 4

[Robert Strong (they/them - no direct email)]

Attachment: patch

Leak stats
Before patch:
                          Per-Inst   Leaked
 74 XPCWrappedNative         56       1176
584 nsXPCWrappedJS           48        192
585 nsXPCWrappedJSClass      44         88
610 xptiInterfaceInfo        20         40

After patch:
                          Per-Inst   Leaked
74 XPCWrappedNative          56       1176

Comment 5

[Robert Strong (they/them - no direct email)]

(In reply to comment #4)
> Leak stats
> Before patch:
>                           Per-Inst   Leaked
>  74 XPCWrappedNative         56       1176
Should be
74 XPCWrappedNative            56       1400

Comment 6

[Robert Strong (they/them - no direct email)]

Attachment: simple startup / shutdown leak log for before and after the patch

Comment 7

[Benjamin Smedberg]

Comment on attachment 237702 [details] [diff] [review]
patch

We're sure that the timers have already been canceled?

Comment 8

[Robert Strong (they/them - no direct email)]

These are actually timer callbacks and not timers which is used from the single nsITimer in nsIUpdateTimerManager

Checked in to trunk

Comment 9

[Robert Strong (they/them - no direct email)]

This looks good!

From Linux balsa-trunk Dep GTK1 (gcc 3.4)

--FIXED-LEAKS---------------------------------leaks------leaks%
XPCWrappedNative                               1120     -16.67%
nsXPCWrappedJSClass                               0    -100.00%
nsXPCWrappedJS                                    0    -100.00%
xptiInterfaceInfo                                 0    -100.00%
TOTAL                                          1120

Before:
RLk:2.30KB
Lk:347KB
MH:13.1MB
A:585K

After:
RLk:1.77KB
Lk:269KB
MH:13.1MB
A:475K

Comment 10

[Robert Strong (they/them - no direct email)]

Comment on attachment 237702 [details] [diff] [review]
patch

Drivers, a simple fix for a memory leak where gc requires an extra cycle to collect which won't happen since we are shutting down.

Comment 11

[Mike Schroepfer]

Comment on attachment 237702 [details] [diff] [review]
patch

a=schrep

Comment 12

[Robert Strong (they/them - no direct email)]

Checked in to MOZILLA_1_8_BRANCH

--FIXED-LEAKS---------------------------------leaks------leaks%
XPCWrappedNative                                224     -50.00%
nsXPCWrappedJSClass                               0    -100.00%
xptiInterfaceInfo                                 0    -100.00%
nsXPCWrappedJS                                    0    -100.00%
TOTAL                                           224

Before:
RLk:1.71KB
Lk:274KB
MH:13.0MB
A:898K

After:
RLk:1.18KB
Lk:254KB
MH:12.3MB
A:885K

Comment 13

[Robert Strong (they/them - no direct email)]

Very nice...

-23.05% leaks on trunk

-31.12% leaks on MOZILLA_1_8_BRANCH