401345 - Calling registerProtocolHandler leaks like a sieve

Status: RESOLVED FIXED

(Core :: XPConnect, defect, P3)

Description

[Jeff Walden [:Waldo]]

1. Open Minefield.
2. Open attachment 286379 [details].
3. Exit.

Instead of zero leaks, I see a ton of them (see attachment for full list).

This could be a whole host of different things, but for now we'll call it a bug in Firefox's front-end for this.  I haven't really looked into this to figure out what might be wrong.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Leaks

Comment 2

[Jeff Walden [:Waldo]]

How's this for fun: the leak occurs in _getFormattedString and occurs when |this.stringBundle.formatStringFromName(key, params, params.length)| is called.  Throwing before the return of that variable doesn't leak, throwing after assigning it to a variable does.  Better yet, if I add a |return NS_ERROR_NOT_AVAILABLE;| to nsStringBundle::FormatStringFromName, I hit that, registerProtocolHandler throws, and we still leak.  It looks like somewhere between the JS engine and nsStringBundle XPConnect is leaking something, presumably the array being passed to _getFormattedString (which would entrain lots of stuff via __parent__).

I'm out of time now to test whether registerContentHandler similarly leaks, as it too calls the affected function, but I'll bet it does.

Comment 3

[Shawn Wilsher :sdwilsh]

Just to be clear - this only leaks if you cancel or quit - not under normal use?

Comment 4

[Jeff Walden [:Waldo]]

I don't know why you don't think this is normal use; it seems to me like closing the browser after visiting some arbitrary site that uses this (oh, say, Gmail or Yahoo) is completely normal.

That said, it leaks when I visit the page, open a new tab, close the original, and browse to a couple sites; would you call that "normal use"?

Comment 5

[Shawn Wilsher :sdwilsh]

(In reply to comment #4)
> I don't know why you don't think this is normal use; it seems to me like
> closing the browser after visiting some arbitrary site that uses this (oh, say,
> Gmail or Yahoo) is completely normal.
I didn't say it didn't happen under normal use.  Your steps to reproduce seem to indicate that it only leaked when you quit or canceled.  I'm just looking for clarification.

Comment 6

[Jeff Walden [:Waldo]]

It's not really possible to tell what's leaked while the browser's running unless you trace the entire heap, and without enabling rtti for everything and doing a type-aware trace that's not possible.  From a more practical and less theoretical standpoint, the refcount logging code only prints leak info at shutdown.

Comment 7

[Johnny Stenback (:jst)]

Dan, this sounds like something for you to look at.

Comment 8

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

Dan, have you had a chance to look into this?

Comment 9

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

Oh, so it sounds leak is elsewhere. Jeff, exactly what code are you talking about in comment 2? I can only find _getFormattedString in feeds code.

Comment 10

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

Peter, do you have the cycles to look at this? Let me know if not. I think this one might be showing up in a lot of the UI testing if we leak any time we localize strings using stringBundle.formatStringFromName

Comment 11

[Peter Van der Beken [:peterv]]

This looks like a combination of declaring a component (WebContentConverterRegistrar) as a variable and bug 398219. WebContentConverterRegistrar holds a string bundle in a property. The XPConnect wrapper for that bundle holds a window alive through one of its functions (bug 398219). WebContentConverterRegistrar is probably kept alive through Module, haven't figured out how exactly. Converting the component to use XPCOMUtils.jsm fixes this.

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/browser/components/feeds/src/WebContentConverter.js&rev=1.22&mark=164-175#160

Comment 12

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 13

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

So would fixing bug 398219 also have fixed this? If so, I think we need to up the priority over there as I'd prefer if we can find a general fix for this rather than discourage the pattern.

Comment 14

[Dan Mosedale (:dmosedale, :dmose)]

Even if it does, peterv's patch here should still give a footprint / code duplication win.

Comment 15

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

I agree, i'm just trying to assess the priority for bug 398219