Last Comment Bug 738726 - Add more error checking to feeds to prevent restarts
: Add more error checking to feeds to prevent restarts
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Feed Reader (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Thunderbird 14.0
Assigned To: alta88
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-23 11:27 PDT by alta88
Modified: 2012-04-23 13:35 PDT (History)
3 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed


Attachments
patch. (13.08 KB, patch)
2012-03-23 11:31 PDT, alta88
no flags Details | Diff | Review
patch (13.06 KB, patch)
2012-03-23 13:10 PDT, alta88
no flags Details | Diff | Review
updated patch. (16.09 KB, patch)
2012-03-25 19:25 PDT, alta88
no flags Details | Diff | Review
patch. (16.17 KB, patch)
2012-04-06 07:51 PDT, alta88
mozilla: review+
Details | Diff | Review
tweak log msg to ensure no throw. (16.23 KB, patch)
2012-04-06 10:03 PDT, alta88
alta88: review+
standard8: approval‑comm‑aurora+
Details | Diff | Review

Description alta88 2012-03-23 11:27:27 PDT
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 2012-03-23 11:31:59 PDT
Created attachment 608789 [details] [diff] [review]
patch.
Comment 2 alta88 2012-03-23 13:10:53 PDT
Created attachment 608842 [details] [diff] [review]
patch

tweak logging.
Comment 3 Magnus Melin 2012-03-24 10:53:55 PDT
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 2012-03-25 19:25:39 PDT
Created attachment 609190 [details] [diff] [review]
updated patch.

ah.  a remnant from testing with file:// which returns 0 if successful.
Comment 5 alta88 2012-03-25 19:28:06 PDT
5. save untold downloading/parsing by fixing broken saving of Last-Modified timestamp.

in above patch.
Comment 6 alta88 2012-04-05 12:39:37 PDT
david, just a ping...
Comment 7 David :Bienvenu 2012-04-05 15:02:13 PDT
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 2012-04-06 07:51:07 PDT
(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 2012-04-06 07:51:59 PDT
Created attachment 612885 [details] [diff] [review]
patch.
Comment 10 David :Bienvenu 2012-04-06 08:34:55 PDT
Comment on attachment 612885 [details] [diff] [review]
patch.

thx for the patch
Comment 11 alta88 2012-04-06 10:03:41 PDT
Created attachment 612917 [details] [diff] [review]
tweak log msg to ensure no throw.
Comment 12 Ryan VanderMeulen [:RyanVM] 2012-04-06 11:22:29 PDT
http://hg.mozilla.org/comm-central/rev/ce3fcd559526
Comment 13 alta88 2012-04-08 11:02:55 PDT
*** Bug 743467 has been marked as a duplicate of this bug. ***
Comment 14 alta88 2012-04-16 08:48:12 PDT
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) 2012-04-23 13:35:46 PDT
Checked in: http://hg.mozilla.org/releases/comm-aurora/rev/17a96f4bde12

Note You need to log in before you can comment on or make changes to this bug.