344983 - Firefox leaks every nsIFeedResult created as a result of visiting feed pages

Status: RESOLVED FIXED

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

Description

[Jeff Walden [:Waldo]]

Feed result objects are created and added to the feed result service when feeds are visited, but they're never removed.  Consequently, whatever's in those objects will stay around until shutdown.

Comment 1

[Jeff Walden [:Waldo]]

To be fixed after bug 344651 and bug 340677...

Comment 2

[Robert Sayre]

Attachment: load a bunch iframes with the same feed, all at once

Comment 3

[Robert Sayre]

Attachment: store the feed by nsIURI instance

waldo, could you verify that the patch fixes the leak?

Comment 4

[Robert Sayre]

Attachment: store the feed by nsIURI instance

OK, this doesn't leak in my tests

Comment 5

[Robert Sayre]

Attachment: store the feed by nsIURI instance

had an extra file in the previous patch

Comment 6

[Jeff Walden [:Waldo]]

(In reply to comment #3)
> waldo, could you verify that the patch fixes the leak?

I'll try to do this, but I don't think I'm likely to have the time in the immediate future; I have too many other bugs to address now.  :-\

Comment 7

[Adam Guthrie]

I'm pretty sure that you can test whether stuff is leaking using dbaron's Leak Gauge tool [0].

That said, the latest patch doesn't make the leaks I'm seeing go away.

[0] http://www.squarefree.com/2006/01/13/memory-leak-detection-tool/

Comment 8

[Adam Guthrie]

Attachment: output from leak-guage.pl

This is what I did:

1. Start Firefox with ./firefox -P trunk http://slashdot.org/. 
2. Click on the RSS icon in the location bar.
2. Go to mozilla.com (I just had a bookmark in my bookmarks toolbar and it was convenient to click).

I could be seeing a completely different leak. Who knows! But I'm assuming this is the leak described in this bug. 

Also, using the trace refcnt leak tool (defining XPCOM_MEM_LEAK_LOG, etc.) it looks like we're leaking the world with these steps above.

Comment 9

[Robert Sayre]

(In reply to comment #8)
> 
> I could be seeing a completely different leak.

I think you might be. I can reproduce the leak you describe though, so I will check it out and file a new bug if needed.

Comment 10

[Robert Sayre]

>
> I think you might be. I can reproduce the leak you describe though, so I will
> check it out and file a new bug if needed.

OK, I think this is something different.

<http://lxr.mozilla.org/seamonkey/source/browser/components/feeds/src/FeedWriter.js#486 >

This seems to fix the leak you're reporting:

487   /**
488    * See nsIFeedWriter
489    */
490   close: function FW_close() {

+     this._document = null;
+     this._window = null;

491     var prefs =   
492         Cc["@mozilla.org/preferences-service;1"].
493         getService(Ci.nsIPrefBranch2);
494     prefs.removeObserver(PREF_SELECTED_ACTION, this);
495     prefs.removeObserver(PREF_SELECTED_READER, this);
496     prefs.removeObserver(PREF_SELECTED_APP, this);
497   },
498     

Comment 11

[Robert Sayre]

(In reply to comment #9)
> (In reply to comment #8)
> > 
> > I could be seeing a completely different leak.
> 
> I think you might be. I can reproduce the leak you describe though, so I will
> check it out and file a new bug if needed.
> 

Bug 347824

Comment 12

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 231758 [details] [diff] [review]
store the feed by nsIURI instance

r=[e-mail]

Comment 13

[Robert Sayre]

Checking in public/nsIFeedResultService.idl;
/cvsroot/mozilla/browser/components/feeds/public/nsIFeedResultService.idl,v  <--  nsIFeedResultService.idl
new revision: 1.6; previous revision: 1.5
done
Checking in src/FeedConverter.js;
/cvsroot/mozilla/browser/components/feeds/src/FeedConverter.js,v  <--  FeedConverter.js
new revision: 1.14; previous revision: 1.13
done
Checking in src/FeedWriter.js;
/cvsroot/mozilla/browser/components/feeds/src/FeedWriter.js,v  <--  FeedWriter.js
new revision: 1.10; previous revision: 1.9
done

Comment 14

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 231758 [details] [diff] [review]
store the feed by nsIURI instance

a=drivers for MOZILLA_1_8_BRANCH

Comment 15

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

Comment on attachment 231758 [details] [diff] [review]
store the feed by nsIURI instance

This patch doesn't apply cleanly to the branch.

Comment 16

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 231758 [details] [diff] [review]
store the feed by nsIURI instance

Robert, can we get an unbitrotted patch?

Comment 17

[Robert Sayre]

Attachment: branch patch

The rot happened when Mano upgraded the interface - no risk here.

Comment 18

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 234279 [details] [diff] [review]
branch patch

a=beltzner on behalf of drivers for the MOZILLA_1_8_BRANCH, please mark fixed1.8.1 when you check in.

Comment 19

[Robert Sayre]

Checking in browser/components/feeds/src/FeedConverter.js;
/cvsroot/mozilla/browser/components/feeds/src/FeedConverter.js,v  <--  FeedConverter.js
new revision: 1.1.2.18; previous revision: 1.1.2.17
done
Checking in browser/components/feeds/src/FeedWriter.js;
/cvsroot/mozilla/browser/components/feeds/src/FeedWriter.js,v  <--  FeedWriter.js
new revision: 1.2.2.14; previous revision: 1.2.2.13
done