738726 - Add more error checking to feeds to prevent restarts

Status: RESOLVED FIXED

(MailNews Core :: Feed Reader, defect)

Description

[alta88]

1. add try catch to last throwable ios.newUri code remaining.
2. workaround for bug 737966 regression.  fix up xhr parsing.
3. force an msgDatabase reparse and continue feed processing, if not available.
4. better logging.

Comment 1

[alta88]

Attachment: patch.

Comment 2

[alta88]

Attachment: patch

tweak logging.

Comment 3

[Magnus Melin [:mkmelin]]

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

>diff --git a/mailnews/extensions/newsblog/content/Feed.js b/mailnews/extensions/newsblog/content/Feed.js
>--- a/mailnews/extensions/newsblog/content/Feed.js

>                    .createInstance(Components.interfaces.nsIXMLHttpRequest
>-    if (request.status < 200 || request.status >= 300)
>+    if (request.status != 0 && (request.status < 200 || request.status >= 300))

Hmm, request.status 0 is usually connection reset or similar, why wouldn't it be an error?

Comment 4

[alta88]

Attachment: updated patch.

ah.  a remnant from testing with file:// which returns 0 if successful.

Comment 5

[alta88]

5. save untold downloading/parsing by fixing broken saving of Last-Modified timestamp.

in above patch.

Comment 6

[alta88]

david, just a ping...

Comment 7

[David :Bienvenu]

sorry, yeah, I've run with this, but got dragged away to other stuff before I could finish my review. In general, it looks reasonable.

the second line is way too long:

+    if (!(aDOM instanceof Components.interfaces.nsIDOMXMLDocument) ||
+        aDOM.documentElement.getElementsByTagNameNS("http://www.mozilla.org/newlayout/xml/parsererror.xml", "parsererror")[0])

I worry a little that reparsing all the feeds with bad db's at the same time will slow things to a crawl, but bad db's should be relatively rare going forward. Rebuilding one more time...

Comment 8

[alta88]

(In reply to David :Bienvenu from comment #7)
> sorry, yeah, I've run with this, but got dragged away to other stuff before
> I could finish my review. In general, it looks reasonable.
> 
> the second line is way too long:
> 
> +    if (!(aDOM instanceof Components.interfaces.nsIDOMXMLDocument) ||
> +       
> aDOM.documentElement.getElementsByTagNameNS("http://www.mozilla.org/
> newlayout/xml/parsererror.xml", "parsererror")[0])
> 

yeah stuff like this is awful, but i left it in the current style since the patch for bug 721517 is going to fix it, if ok.  the namespace literal will in the FeedUtils object.  it will also fix the big namespace pollution in feeds code and all this formatting/commenting/whitespace stuff for the whole subsystem, i have it mostly done.  

> I worry a little that reparsing all the feeds with bad db's at the same time
> will slow things to a crawl, but bad db's should be relatively rare going
> forward. Rebuilding one more time...

i considered that, but the reparse call is async and now we have each feed in the next good folder async as well.  in testing this (by deleting 70+ .msf files in a feed account) there was almost no noticeable difference as the rebuild was invoked for each of them.  and it is a rare situation indeed; users would have to be not selecting lots of folders.

the critical thing is that if the feed interface singleton instance throws for even the tiniest situation, it's broken for all til restart, so extra care has to be built in.

(patch is updated to fix flow, ie check msdDb before checking if a folder has feed urls, to make sure all folders are caught.)

Comment 9

[alta88]

Attachment: patch.

Comment 10

[David :Bienvenu]

Comment on attachment 612885 [details] [diff] [review]
patch.

thx for the patch

Comment 11

[alta88]

Attachment: tweak log msg to ensure no throw.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

http://hg.mozilla.org/comm-central/rev/ce3fcd559526

Comment 14

[alta88]

Comment on attachment 612917 [details] [diff] [review]
tweak log msg to ensure no throw.

[Approval Request Comment]
Regression caused by (bug #):
- none
User impact if declined:
- possible instability causing restarts, performance on biff reduced.
Testing completed (on c-c, etc.):
- local testing 
Risk to taking this patch (and alternatives if risky):
- very low

this patch addresses the last (known) cases of unhandled throws, and in particular adds .msf autorepair functionality. plus, the correct implementation of LAST-MODIFIED will be a significant performance saver on biffs.  i think it's worth considering advancing the patch by a cycle.

Comment 15

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/releases/comm-aurora/rev/17a96f4bde12