385178 - Memory leak when viewing (bbc news) RSS feed

Status: RESOLVED FIXED

(Firefox Graveyard :: RSS Discovery and Preview, defect)

Description

[Steve England [:stevee]]

1. New profile, start firefox with leak logging enabled.
2. Navigate to http://news.bbc.co.uk/
3. Notice the RSS feed icon in the URL bar. Left click it to view the feed details.
4. Close firefox
5. Analyse leak log

Leaked inner window 2be9d18 (outer 2580148) at address 2be9d18.
 ... with URI "http://newsrss.bbc.co.uk/rss/newsonline_uk_edition/front_page/rss
.xml".
Leaked outer window 2580148 at address 2580148.
Leaked document at address 2c67ed8.
 ... with URI "chrome://global/content/bindings/menulist.xml".
 ... with URI "jar:file:///C:/Documents%20and%20Settings/Ste/Desktop/Firefox%20T
runk%20(PLACES)/firefox/chrome/toolkit.jar!/content/global/bindings/menulist.xml
".
Leaked document at address 2b7dde0.
 ... with URI "http://newsrss.bbc.co.uk/rss/newsonline_uk_edition/front_page/rss
.xml".
 ... with URI "jar:file:///C:/Documents%20and%20Settings/Ste/Desktop/Firefox%20T
runk%20(PLACES)/firefox/chrome/browser.jar!/content/browser/feeds/subscribe.xhtm
l".
Leaked document at address 4297020.
 ... with URI "chrome://global/content/bindings/checkbox.xml".
 ... with URI "jar:file:///C:/Documents%20and%20Settings/Ste/Desktop/Firefox%20T
runk%20(PLACES)/firefox/chrome/toolkit.jar!/content/global/bindings/checkbox.xml
".
Summary:
Leaked 2 out of 11 DOM Windows
Leaked 3 out of 44 documents
Leaked 0 out of 4 docshells

I see this in:
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a6pre) Gecko/20070619
Minefield/3.0a6pre ID:2007061904

But not in:
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.1.5pre) Gecko/2007053104
BonEcho/2.0.0.5pre

Bug 347824 covered a similar kind of leak, which landed on both trunk and branch.

Comment 1

[Robert Sayre]

I was able to mitigate most of the leaks by adding the following to FeedWriter::close() :

> this.__faviconService = null;
> this.__bundle = null;
> this._selectedApplicationItemWrapped = null;
> this._defaultSystemReaderItemWrapped = null;
> this._FeedURI = null;

I'm guessing the remaining leak is due to calling XPCNativeWrapper() in handleEvent.

Comment 2

[Robert Sayre]

(In reply to comment #1)
> I'm guessing the remaining leak is due to calling XPCNativeWrapper() in
> handleEvent.

I guessed wrong. More data in a bit.

Comment 3

[Robert Sayre]

I was able to further winnow the leaking objects by setting the feed processor to null in FeedConverter._releaseHandles, but I am still leaking a BackstagePass and an nsXBLDocGlobalObject.

>  /** 
>   * Release our references to various things once we're done using them.
>   */
>  _releaseHandles: function FC__releaseHandles() {
>    this._listener = null;
>    this._request = null;
>    this._processor = null;
>  },

Comment 4

[Robert Sayre]

Attachment: leak stats

Comment 5

[Robert Sayre]

Attachment: shutdown JS heap

Comment 6

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

It might help to try defining DEBUG_CC at the top of nsCycleCollectionParticipant.h, rebuilding (the world), and seeing what the printfs from ExplainLiveExpectedGarbage tell you?  (Maybe also apply the patch in bug 386665 comment 3?)

That would probably only do anything without the first patch, unless you add some nsCycleCollector_DEBUG_{shouldBeFreed,wasFreed} calls to tell the cycle collector to explain why it didn't collect the objects that still leak with that patch.

Comment 7

[Robert Sayre]

Attachment: DEBUG_CC messages

The leak stats in attachment 272330 [details] were captured with both patches above applied. This attachment shows the DEBUG_CC messages on shutdown with both patches applied.

Comment 8

[Robert Sayre]

Attachment: DEBUG_CC messages with removeEventListener calls

I changed FeedWriter.close() to call removeEventListener. This drastically changed the DEBUG_CC output, but did not change the refcount leaks.

Comment 9

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Did you try the patch from bug 386665 comment 3?  That may significantly change the output that's showing most of the roots as XPCWrappedNatives (and hopefully give you something more useful).

It's also interesting that windows are being destroyed after cycle collector shutdown.  That means you're really leaking quite a bit more that's not showing up in the leak stats.

Comment 10

[Robert Sayre]

Attachment: shutdown DEBUG_CC and leak stats output

(In reply to comment #9)
> Did you try the patch from bug 386665 comment 3?  That may significantly change
> the output that's showing most of the roots as XPCWrappedNatives (and hopefully
> give you something more useful).

Hmm, I don't get any windows in the output with the patch from bug 386665 comment 3 applied.

Comment 11

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Do you know why the DOM windows are being released after cycle collector shutdown?  That's *really* late in shutdown, and could cause leaks because things they free aren't really disconnected.  (Just break in the debugger on cycle collector shutdown, and then break on ~nsGlobalWindow, and get the stack.)

Comment 12

[Robert Sayre]

Attachment: gdb session, with backtrace as recommended by dbaron

gdb wouldn't let me break on ~nsGlobalWindow, but it did allow me to do so on nsGlobalWindow::CleanUp, which seems to do the trick.

The attached log shows the windows getting released in mozJSComponentLoader::UnloadModules. I get 6 DOM windows released after cycle collector shutdown on my system. The log on shows the backtrace for one of them, but I verified that the backtrace is identical for all 6. Should be simple enough to identify the JS file as a next step.

Comment 13

[Robert Sayre]

Attachment: patch to reduce leak stats

This is the patch I have applied to reduce the amount of leakage, and make the remaining leaks easier to see.

Comment 14

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

You could probably identify the JS path (and file) by applying the patch in bug 387224 (I really need to fix that up and get it in...), **un**applying the patch in bug 386665 comment 3, and regenerating comment 7.

Comment 15

[Robert Sayre]

Attachment: DEBUG_CC output with event listeners still attached

If I remove the event listeners in FeedWriter::Close, I get the same uniformative messages from DEBUG_CC with this patch applied.

Comment 17

[Robert Sayre]

Attachment: wallpaper leak fix

This technique now fixes all of the leaks, probably because of GC fixes elsewhere. I think we should take it as wallpaper.

Comment 18

[Steve England [:stevee]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a8pre) Gecko/2007082103 Minefield/3.0a8pre ID:2007082103
This now WFM. STR in comment 0 no longer result in a reported leak from leak-gauge.pl

Comment 19

[Robert Sayre]

underlying cycle now collected, due to bug 397804.