Closed Bug 401345 Opened 12 years ago Closed 12 years ago

Calling registerProtocolHandler leaks like a sieve

Categories

(Core :: XPConnect, defect, P3)

x86
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla1.9beta2

People

(Reporter: Waldo, Assigned: peterv)

Details

(Keywords: memory-leak, Whiteboard: [proto])

Attachments

(2 files)

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.
Flags: blocking-firefox3?
Attached file Leaks
Keywords: mlk
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.
Component: File Handling → XPConnect
Flags: blocking-firefox3?
Product: Firefox → Core
QA Contact: file.handling → xpconnect
Summary: Calling registerContentHandler leaks like a sieve → Calling registerProtocolHandler leaks like a sieve
Flags: blocking1.9?
Whiteboard: [proto]
Just to be clear - this only leaks if you cancel or quit - not under normal use?
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"?
(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.
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.
Dan, this sounds like something for you to look at.
Assignee: nobody → dmose
Flags: blocking1.9? → blocking1.9+
Priority: -- → P3
Dan, have you had a chance to look into this?
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.
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
Assignee: dmose → peterv
Status: NEW → ASSIGNED
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
Attached patch v1Splinter Review
Attachment #288524 - Flags: review?(sayrer)
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.
Even if it does, peterv's patch here should still give a footprint / code duplication win.
I agree, i'm just trying to assess the priority for bug 398219
Attachment #288524 - Flags: review?(sayrer) → review+
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9 M10
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.