Thunderbird RSS should have a per feed limit per download

[another_sam]

Binary package hint: mozilla-thunderbird

Steps to reproduce:

Open Thunderbird 2.0.0.9 in the last version of Ubuntu 8.04 (wrote at february 11, 2007).

Thunderbird starts some task that uses 10% of CPU (centrino duo 1,8GHz) and a very high amount of physical memory.

In two minutes, 217 MB. In one hour, 400+ MB.

I currently have 3 e-mail accounts and 5 RSS subscriptions.

[another_sam (anothersam)]

Hi All. Me again.

It was my fault at 95%. One of that feeds (made by me...) had 249.000+ items (due to a design error based on un-forecasted circumstances. D'OH!).

So we are talking about a leak of feature rather than a bug. This feature I am suggesting is simply some kind of size limit on feed loading. May be implemented through a warning dialog at N MB, or a numerical setting in the preferences panel, or both, or another better idea.

Thanks.

Launchpad rocs.

[another_sam (anothersam)]

The cause of this huge memory usage is a huge (although involuntary) feed.

[Mozilla-bugs-micahscomputing (mozilla-bugs-micahscomputing)]

User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1pre) Gecko/20090523 Ubuntu/9.04 (jaunty) Shiretoko/3.5pre
Build Identifier:

User had a self-created feed with 250k entries in it. Figured it would be good if there was some type of limit on the downloads from a feed at one shot.

Ubuntu bug:
https://bugs.launchpad.net/bugs/191006

Reproducible: Always

[Micah Gersten (micahg)]

Thank you for your feature request. This request has been reported to the developers of the software. You can track it and make comments at:
https://bugzilla.mozilla.org/show_bug.cgi?id=494797

[Alta88 (alta88)]

Created attachment 754267
patch

Not testing for 0 or ∞ is never good.

This patch:
1. Prompts for confirmation on a new subscribe if the feed file item count > 100.
2. Prompts if a feed update contains > 1000 items; prompts are stacked.
3. Enables a cancel; fixes false prompt and non cancel on subscribe dialog close (Bug 349049).
4. Fixes strict js message.
5. Logs gecko xml parsing errors. This and the validation link should finish off any "it's thunderbird's fault" type support issue.

[Alta88 (alta88)]

good test feeds, with > 1000 items:
https://jazz.net/library/rss/
http://www.livejournal.com/stats/latest-rss.bml

[Alta88 (alta88)]

Created attachment 754287
patch

[Mkmelin+mozilla (mkmelin+mozilla)]

Comment on attachment 754287
patch

Review of attachment 754287:
-----------------------------------------------------------------

Suite's newsblog.properties needs updating too.

(Seems this patch needs bug 254231 applied first.)

::: mail/locales/en-US/chrome/messenger-newsblog/newsblog.properties
@@ +57,5 @@
> subscribe-confirmFeedDeletion=Are you sure you want to unsubscribe from the feed: \n %S?
>
> +subscribe-confirmFeedTitle=Confirm Feed
> +## LOCALIZATION NOTE(subscribe-confirmSubscribe): %S is the number of items in a new feed.
> +subscribe-confirmSubscribe=This feed contains %S items. Continue?

No double spacing please

@@ +62,5 @@
> +
> +## LOCALIZATION NOTE(newsblog-confirmUpdate):
> +## %1$S is the folder, %2$S is the feed url, %3$S is the number of items in an updating feed.
> +newsblog-confirmUpdate=The feed in %1$S and location %2$S contains %3$S items. Continue?
> +

no double spacing

::: mailnews/extensions/newsblog/content/Feed.js
@@ +204,2 @@
> let lastModifiedHeader = request.getResponseHeader("Last-Modified");
> + feed.mLastModified = lastModifiedHeader && feed.parseItems ?

I personally prefer () around ternary expressions, for clarity. Others may disagree...

::: mailnews/extensions/newsblog/content/utils.js
@@ +84,5 @@
> // There are no new articles for this feed
> kNewsBlogNoNewItems: 4,
> + kNewsBlogCancel: 5,
> +
> + HIGH_COUNT_LIMIT_SUBSCRIBE: 100,

100 sounds like a fairly small limit, how about 500 or something? i think i've seen valid feeds have way above 100

@@ +650,5 @@
> },
>
> +/**
> + * Prompt to continue a new subscribe if the item count is high.
> + *

trailing space

[Alta88 (alta88)]

Created attachment 755964
updated for comments

[Alta88 (alta88)]

(In reply to Magnus Melin from comment #4)
> Comment on attachment 754287
> patch
>
> Review of attachment 754287:
> -----------------------------------------------------------------
>
> Suite's newsblog.properties needs updating too.
>
> (Seems this patch needs bug 254231 applied first.)
>

it shouldn't be necessary; some minor checks/logs that happened to be there because i did it first were moved here, to make that one simple for sec.

> 100 sounds like a fairly small limit, how about 500 or something? i think
> i've seen valid feeds have way above 100
>

how about 200? even for 100 it will take 10+ seconds to complete (unlike news, content is downloaded not just headers). it's not so much legitimacy as informing the user what they're about to get into, so they can do it later etc.

[Alta88 (alta88)]

for suite/ strings.

[Mkmelin+mozilla (mkmelin+mozilla)]

Comment on attachment 755964
updated for comments

Review of attachment 755964:
-----------------------------------------------------------------

::: mailnews/extensions/newsblog/content/utils.js
@@ +673,5 @@
> + * @param obj aFeed - the feed object.
> + * @param integer aCount - number of items in the feed.
> + * @return boolean - true to continue, false to cancel.
> + */
> + confirmHighItemCount: function(aFeed, aCount) {

Appears to never get called???

[Mkmelin+mozilla (mkmelin+mozilla)]

(In reply to alta88 from comment #6)
> > (Seems this patch needs bug 254231 applied first.)
> >
>
> it shouldn't be necessary; some minor checks/logs that happened to be there
> because i did it first were moved here, to make that one simple for sec.

Doesn't apply without it...

[Neil-httl (neil-httl)]

Comment on attachment 755964
updated for comments

>+ mLastModified: null,
Not sure how this change relates.

>- win.updateStatusItem("statusText", message, aErrorCode);
>+ let code = feed.url.startsWith("http") ? aErrorCode : null;
>+ win.updateStatusItem("statusText", message, code);
Not sure how this change relates.

>- onunload="return FeedSubscriptions.onUnload();"
>+ onclose="return FeedSubscriptions.onClose();"
Not sure how this change relates.

>+ HIGH_COUNT_LIMIT_SUBSCRIBE: 200,
>+ HIGH_COUNT_LIMIT_UPDATE: 1000,
>+ CANCEL_REQUESTED: false,
What happens when there are too many items? (By comparison, for newsgroups, you can choose to either download the latest N (ignoring the rest) or the next N, although I don't know whether the RSS code would be able to let you do this. Also, N is pref-controlled, although we can leave that to a separate bug.)
Also, wouldn't it be more likely for there to be many items when you first subscribe? (I know a feed that only used to update every few days or so but the RSS feed contained the entire archive should you so want it.)

>+ confirmHighItemCount: function(aFeed, aCount) {
Where does this actually get called?

[Alta88 (alta88)]

Created attachment 756209
fixed patch

wow, the patch got mangled along the way. sorry gents. built and tested with this one.

[Alta88 (alta88)]

(In reply to <email address hidden> from comment #10)
> Comment on attachment 755964
> updated for comments
>
> >+ mLastModified: null,
> Not sure how this change relates.

it needs to be cached in case of a cancel, it would break new refreshes if set prior to full completion.
>
> >- win.updateStatusItem("statusText", message, aErrorCode);
> >+ let code = feed.url.startsWith("http") ? aErrorCode : null;
> >+ win.updateStatusItem("statusText", message, code);
> Not sure how this change relates.
>

the validation link makes no sense unless http, so hide it otherwise.

> >- onunload="return FeedSubscriptions.onUnload();"
> >+ onclose="return FeedSubscriptions.onClose();"
> Not sure how this change relates.

comment 3.
>
> >+ HIGH_COUNT_LIMIT_SUBSCRIBE: 200,
> >+ HIGH_COUNT_LIMIT_UPDATE: 1000,
> >+ CANCEL_REQUESTED: false,
> What happens when there are too many items? (By comparison, for newsgroups,
> you can choose to either download the latest N (ignoring the rest) or the
> next N, although I don't know whether the RSS code would be able to let you
> do this. Also, N is pref-controlled, although we can leave that to a
> separate bug.)
> Also, wouldn't it be more likely for there to be many items when you first
> subscribe? (I know a feed that only used to update every few days or so but
> the RSS feed contained the entire archive should you so want it.)
>

the server based newsgroup model doesn't work for feeds. imo 2 prefs are very not worth it. in all cases, the user decides, and this is to really prevent surprises.

> >+ confirmHighItemCount: function(aFeed, aCount) {
> Where does this actually get called?

bad patch before..

[Neil-httl (neil-httl)]

I still would like to know what choice this patch actually gives you. (The string somewhat unhelpfully says "Continue?" which suggests that it gives you the choice of using up lots of CPU and memory now or annoying you with another prompt again in 100 minutes.)

[Alta88 (alta88)]

The objective, per the title of the bug, is to handle an erroneous deluge, upon update. Not to nag a user about a high count feed they've already approved/subscribed. So while 1000 is a testable number, the final number could be 5000 or 10000. At that edge case, the user can cancel and unsubcribe/investigate. Or continue.

[Mkmelin+mozilla (mkmelin+mozilla)]

Comment on attachment 756209
fixed patch

Review of attachment 756209:
-----------------------------------------------------------------

Basically looks good, but i think we should skip warn on many for update prompt. I suppose such cases exist in theory, but if it's an evil feed people would unsubscribe pretty quickly once they notice there are 50000 new items in there. Unless the case is legit, and the prompt would be just annoying.

Please also get ui-r from bwinton.

::: mail/locales/en-US/chrome/messenger-newsblog/newsblog.properties
@@ +55,5 @@
> subscribe-confirmFeedDeletionTitle=Remove Feed
> ## LOCALIZATION NOTE(subscribe-confirmFeedDeletion): %S is the name of the feed the user wants to unsubscribe from.
> subscribe-confirmFeedDeletion=Are you sure you want to unsubscribe from the feed: \n %S?
>
> +subscribe-confirmFeedTitle=Confirm Feed

"Subscribe to Feed" maybe? For one case. Probably should be different string depending on if it's update or not. If we'd keep the update case.

@@ +57,5 @@
> subscribe-confirmFeedDeletion=Are you sure you want to unsubscribe from the feed: \n %S?
>
> +subscribe-confirmFeedTitle=Confirm Feed
> +## LOCALIZATION NOTE(subscribe-confirmSubscribe): %S is the number of items in a new feed.
> +subscribe-confirmSubscribe=This feed contains %S items. Continue?

I think this too needs more clarification. Maybe something like
"This feed contains a lot of items (%S) and may therefore be slow. Are you sure you want to subscribe?"

And make the button labeled &Go on instead?

::: mailnews/extensions/newsblog/content/feed-parser.js
@@ +19,5 @@
> + if (!(aDOM instanceof Ci.nsIDOMXMLDocument))
> + {
> + // No xml doc.
> + aFeed.onParseError(aFeed);
> + return new Array();

return []; is shorter

::: mailnews/extensions/newsblog/content/utils.js
@@ +699,5 @@
> + }
> +
> + if (!Services.prompt.confirmEx(null, pTitle, pMessage,
> + Ci.nsIPromptService.STD_OK_CANCEL_BUTTONS,
> + null, null, null, null, { }))

please use verbs for the action buttons instead of ok/cancel

[Alta88 (alta88)]

(In reply to Magnus Melin from comment #15)
> Comment on attachment 756209
> fixed patch
>
> Review of attachment 756209:
> -----------------------------------------------------------------
>
> Basically looks good, but i think we should skip warn on many for update
> prompt. I suppose such cases exist in theory, but if it's an evil feed
> people would unsubscribe pretty quickly once they notice there are 50000 new
> items in there. Unless the case is legit, and the prompt would be just
> annoying.

Since that's the point of this bug, per the case described by the OP, this is wontfix.

[Standard8 (mbanner)]

Comment on attachment 756209
fixed patch

Removing obsolete review request.