494797 - Thunderbird RSS should have a per feed limit per download (don't download feeds with 250k items)

Status: RESOLVED WONTFIX

(MailNews Core :: Feed Reader, enhancement)

Description

[Micah Gersten]

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

Comment 1

[alta88]

Attachment: 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.

Comment 2

[alta88]

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

Comment 3

[alta88]

Attachment: patch

Comment 4

[Magnus Melin [:mkmelin]]

Comment on attachment 754287 [details] [diff] [review]
patch

Review of attachment 754287 [details] [diff] [review]:
-----------------------------------------------------------------

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

Comment 5

[alta88]

Attachment: updated for comments

Comment 6

[alta88]

(In reply to Magnus Melin from comment #4)
> Comment on attachment 754287 [details] [diff] [review]
> patch
> 
> Review of attachment 754287 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> 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.

Comment 7

[alta88]

for suite/ strings.

Comment 8

[Magnus Melin [:mkmelin]]

Comment on attachment 755964 [details] [diff] [review]
updated for comments

Review of attachment 755964 [details] [diff] [review]:
-----------------------------------------------------------------

::: 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???

Comment 9

[Magnus Melin [:mkmelin]]

(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...

Comment 10

[[e-mail]]

Comment on attachment 755964 [details] [diff] [review]
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?

Comment 11

[alta88]

Attachment: fixed patch

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

Comment 12

[alta88]

(In reply to [e-mail] from comment #10)
> Comment on attachment 755964 [details] [diff] [review]
> 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..

Comment 13

[[e-mail]]

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.)

Comment 14

[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.

Comment 15

[Magnus Melin [:mkmelin]]

Comment on attachment 756209 [details] [diff] [review]
fixed patch

Review of attachment 756209 [details] [diff] [review]:
-----------------------------------------------------------------

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

Comment 16

[alta88]

(In reply to Magnus Melin from comment #15)
> Comment on attachment 756209 [details] [diff] [review]
> fixed patch
> 
> Review of attachment 756209 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> 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.

Comment 17

[Mark Banner (:standard8)]

Comment on attachment 756209 [details] [diff] [review]
fixed patch

Removing obsolete review request.