405032 - Changing feed handler to web handler leaks nsGlobalWindow

Status: RESOLVED FIXED

Description

[Carsten Book [:Tomcat]]

Created attachment 289852 [details]
leak log

Changing RSS Handlers via Tools -> Options -> Applications leak

steps to reproduce
-Start Firefox
-Go to tools - Options - Application
-Change the RSS handler from ask me to Google (or bloglines)
- LEAK !

Comment 1

[Myk Melez [:myk] [@mykmelez]]

When you set the feed handler to a web app, feedHandlerInfo calls nsIWebContentConverterService::setAutoHandler with the nsIWebContentHandlerInfo representing the app.  That nsIWebContentHandlerInfo comes from nsIWebContentConverterService and is referenced by feedHandlerInfo in its _possibleApplicationHandlers property.

It's not clear what is happening, but I suspect that somewhere we're creating a circular reference between the converter service and the preferences dialog via the handler info object.

Comment 2

[Myk Melez [:myk] [@mykmelez]]

Note that the problem doesn't occur if you set the feed to "preview in Firefox".  It only happens if you set it to a handler app.

Comment 3

[Myk Melez [:myk] [@mykmelez]]

I just checked what happens if I change the feed handler to a web handler via the feed preview page, and it leaks the same way there too, so this is not specific to the Applications prefpane, it's a problem with the web content converter service and its setAutoHandler method.

To test with the feed preview page:

1. set your feed pref in Preferences > Applications to "preview in Firefox";
2. load a web page with a feed (f.e. http://planet.mozilla.org/);
3. click the feed icon in the location bar to load the feed preview page;
4. select a web handler from the "Subscribe to this feed using" dropdown menu;
5. check the "Always use [handler] to subscribe to feeds" checkbox;
6. press the "Subscribe Now" button.

cc: a couple other folks who have touched the service recently for their input.

Comment 4

[Myk Melez [:myk] [@mykmelez]]

Mano might also have some insight here, since he put together the previous incarnation of the feed handling prefs.

Comment 5

[Carsten Book [:Tomcat]]

Created attachment 298388 [details]
new leak log

new Leak Log with a debug build (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2008012113 Minefield/3.0b3pre) after Bug 412491 was checked in

Comment 6

[Myk Melez [:myk] [@mykmelez]]

I've taken several looks at this, and I still can't figure out what's leaking.  I'm using XPCOM_MEM_LEAK_LOG and bloatdiff.pl, but it's returning different output than in the leak log Carsten posted.  How is that one generated?

Comment 7

[Carsten Book [:Tomcat]]

Created attachment 305756 [details]
new recent build leak log

Hi Myk, here is a new leak log from Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b4pre)
Gecko/2008022515 Firefox/3.0b4pre.

I was only able to reproduce this leak when switching the web-handler in the Applications Options tab (like from Google to Bloglines etc).

From the Leak Log it seems that the nsGlobalWindows Leak was fixed by our other fixes that were checked in during the last weeks.

In general i use TraceRefcnt Debug Build http://wiki.mozilla.org/MozillaQualityAssurance:Home_Page:Firefox_3.0_TestPlan:Leaks:LeakTesting-How-To:Debug_Builds

Comment 8

[Mike Connor [:mconnor]]

Not blocking on this bug for final ship. Would take a safe enough patch if one comes through.

Comment 9

[Peter Van der Beken [:peterv]]

Created attachment 338343 [details] [diff] [review]
v1

Putting XPCOM objects on the prototype keeps them alive until the nsIModule/nsIFactory are released, which is after the last cycle collection. This component is used as a service, so not putting things on the prototype is ok, we don't need to share between multiple instances.