350949 - Feed preview leaks memory

Status: VERIFIED FIXED

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

Description

[Erwan Loisant]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1b2) Gecko/20060821 Firefox/2.0b2
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1b2) Gecko/20060821 Firefox/2.0b2

Each time a feed is previewed (showing the "Subscribe to this feed using...") a few hundreds of k are leaked.

Reproducible: Always

Steps to Reproduce:
1.Open a memory monitor
2.Open a feed in Firefox
3.Reload constantly the feed (shift+click reload)
4.Watch the memory usage grow up and never be freed

Actual Results:  
Memory usage just grows up

Expected Results:  
Constant memory usage

I think the leak is in the HTMLUnescape function, in mozilla/components/feeds/src:

http://lxr.mozilla.org/mozilla1.8/source/toolkit/components/feeds/src/nsScriptableUnescapeHTML.cpp#82

We were using it to convert HTML to plain text and noticed the leak. The function itself is very short and looks good, so the leak is probably in the HTMLToTextSink.

Comment 1

[Robert Sayre]

taking.

Comment 2

[Robert Sayre]

(In reply to comment #0)
> 
> I think the leak is in the HTMLUnescape function, in
> mozilla/components/feeds/src:

I can't reproduce a leak in that function when testing it alone. Could you provide a narrower test case if you can reproduce it?

There is a leak in the FeedResultService, where nsIFeedResults are not removed from the cache if the feed rendering is interupted by a reload. You should see no leak if you wait before pressing reload.

Comment 3

[Robert Sayre]

Attachment: Check for multiple list members

When refresh is hit rapidly, the same URI instance will be added twice.

Comment 4

[Robert Sayre]

Attachment: Check for multiple list members

forgot to delete some logging

Comment 5

[Erwan Loisant]

(In reply to comment #2)
> I can't reproduce a leak in that function when testing it alone. Could you
> provide a narrower test case if you can reproduce it?

I can still reproduce it. Even if I wait for the page to be rendered, I see the memory use goes up, then down but at the level it was before. (for exemple + 300 kb then - 150 k).

In a different project, I was using the same function to strip html tags and the memory stopped leaking when a switched to simple regexp to strip the tags.

I will try to make a testcase in xpcshell to isolate the leak.

> There is a leak in the FeedResultService, where nsIFeedResults are not removed
> from the cache if the feed rendering is interupted by a reload. You should see
> no leak if you wait before pressing reload.

I think this is unrelated to the leak I am talking about.

Comment 6

[Asaf Romano (gone)]

Comment on attachment 237152 [details] [diff] [review]
Check for multiple list members

r=mano.

Comment 7

[Robert Sayre]

Checking in feeds/src/FeedConverter.js;
/cvsroot/mozilla/browser/components/feeds/src/FeedConverter.js,v  <--  FeedConverter.js
new revision: 1.20; previous revision: 1.19
done
Checking in feeds/src/FeedWriter.js;
/cvsroot/mozilla/browser/components/feeds/src/FeedWriter.js,v  <--  FeedWriter.js
new revision: 1.15; previous revision: 1.14
done

Comment 8

[Robert Sayre]

Comment on attachment 237152 [details] [diff] [review]
Check for multiple list members

drivers: this is low-risk, needed to prevent a big leak when reload is hit extremely rapidly.

Comment 9

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

Comment on attachment 237152 [details] [diff] [review]
Check for multiple list members

a=beltzner on behalf of 181drivers

Comment 10

[Robert Sayre]

Checking in FeedWriter.js;
/cvsroot/mozilla/browser/components/feeds/src/FeedWriter.js,v  <--  FeedWriter.js
new revision: 1.2.2.18; previous revision: 1.2.2.17
done
Checking in FeedConverter.js;
/cvsroot/mozilla/browser/components/feeds/src/FeedConverter.js,v  <--  FeedConverter.js
new revision: 1.1.2.20; previous revision: 1.1.2.19
done

Comment 11

[Robert Sayre]

Attachment: remove nsIFeedEntry.parent

This property is pretty useless. I never should have added it. As a bonus, it creates reference cycles.

Comment 12

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 237809 [details] [diff] [review]
remove nsIFeedEntry.parent

Sounds good to me

Comment 13

[Robert Sayre]

Comment on attachment 237809 [details] [diff] [review]
remove nsIFeedEntry.parent

drivers low risk, removing unused API.

Comment 14

[Robert Sayre]

Attachment: set reader to null for the sync methods

this was recently exposed by changing sendResult(). we don't use the sync methods in Fx, but the test harness does. saw the leak there.

Comment 15

[Mike Schroepfer]

Comment on attachment 237809 [details] [diff] [review]
remove nsIFeedEntry.parent

a=schrep for 181drivers

Comment 16

[Mike Schroepfer]

Comment on attachment 237815 [details] [diff] [review]
set reader to null for the sync methods

a=schrep for 181drivers

Comment 17

[Robert Sayre]

checkin branch attachment 237815 [details] [diff] [review] and attachment 237809 [details] [diff] [review]

Checking in public/nsIFeedEntry.idl;
/cvsroot/mozilla/toolkit/components/feeds/public/nsIFeedEntry.idl,v  <--  nsIFeedEntry.idl
new revision: 1.1.2.5; previous revision: 1.1.2.4
done
Checking in src/FeedProcessor.js;
/cvsroot/mozilla/toolkit/components/feeds/src/FeedProcessor.js,v  <--  FeedProcessor.js
new revision: 1.1.2.13; previous revision: 1.1.2.12
done

Comment 18

[Robert Sayre]

so, I've chased down everything I can find here, but the Flock guys say they have a leak, so I'm not closing it.

Comment 19

[Robert Sayre]

resolved fixed. Erwan: file a new bug and CC me if you can get a small test case.

Comment 20

[Erwan Loisant]

Thank you Robert. I'll grab a new build and see if I can reproduce the bug in a small xpcshell script.

Comment 21

[Tanner M. Young [:tmyoung]]

On Firefox 3.5, I tested the planet planet.mozilla.org feed and reloaded it repeatedly.  After a few seconds garbage collection (or something similar) occurred and memory went back to normal, and when the tab was closed it returned to the memory level from before the feed was opened.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.2)

Verifying as fixed; removing qawanted.

Is there anything else that needs to be verified or checked?