Closed Bug 606818 Opened 14 years ago Closed 12 years ago

RSS Subscription stuck at "verifying the feed"

Categories

(MailNews Core :: Feed Reader, defect)

x86
Windows 7
defect
Not set
major

Tracking

(thunderbird10 fixed)

RESOLVED FIXED
Thunderbird 11.0
Tracking Status
thunderbird10 --- fixed

People

(Reporter: marvinhk, Assigned: alta88)

References

Details

Attachments

(2 files, 5 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-GB; rv:1.9.2.11) Gecko/20101012 Firefox/3.6.11 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-GB; rv:1.9.2.11) Gecko/20101013 Thunderbird/3.1.5

Subscription to RSS feeds from the Yahoo Blog always stop at "verifying the feed"

Reproducible: Always

Steps to Reproduce:
1. Right click an account and choose Subscribe
2. Add feed URLs from Yahoo Blog (e.g. http://hk.myblog.yahoo.com/bmanikichow/rss)
3. TB keeps verifying the feed without completion
Actual Results:  
TB keeps verifying the feed without completion

Expected Results:  
RSS subscribed to the corresponding folders of the account.
OS: All → Windows 7
Hardware: All → x86
Component: General → Feed Reader
Product: Thunderbird → MailNews Core
QA Contact: general → feed.reader
Attached patch Illustation of a workaround (obsolete) — Splinter Review
I had this problem with the Planet Postgresql feed ( http://planet.postgresql.org/rss20.xml ). Looking in Thunderbirds error console, I found this:

Error: [Exception... "Component returned failure code: 0x804b000a (NS_ERROR_MALFORMED_URI) [nsIStandardURL.init]"  nsresult: "0x804b000a (NS_ERROR_MALFORMED_URI)"  location: "JS frame :: chrome://messenger-newsblog/content/FeedItem.js :: anonymous :: line 58"  data: no]
Source File: chrome://messenger-newsblog/content/FeedItem.js
Line: 58

As a simple fix, I tried wrapping the setter found there in a try/catch. This enabled Thunerbird to get past the Verifying-state and into "Importing feed" (I think it was called), but then it stopped half way through with this in the error console:

Error: s is null
Source File: chrome://messenger-newsblog/content/utils.js
Line: 395

Treating null as the empty string allowed to feed to be imported.

The attached patch illustrates the changes I made to get the feed imported, but I don't known if this is the correct way to solve the problem.
Hum asking on irc in #maildev is your best option (try to find myk and ask him).
The happens to me in http://planet.postgresql.org/rss20.xml 

Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.14) Gecko/20110221 Thunderbird/3.1.8
Attached file Updated workaround
The workaround I now use is illustrated in this patch (unzip omni.jar, make the change and re-zip omni.jar). Instead of clearing the url, it just appends "#badurl" to the problematic urls to ease debugging.
Attachment #511932 - Attachment is obsolete: true
Attached patch handle throw. (obsolete) — Splinter Review
Assignee: nobody → alta88
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attachment #578721 - Flags: review?(neil)
Comment on attachment 578721 [details] [diff] [review]
handle throw.

I had a look to see what these invalid URIs were, and they were tag: URIs. Perhaps we should be using the IO service to parse these URIs instead of hoping that they are standard URLs?
Attachment #578721 - Attachment is patch: true
Attachment #578721 - Flags: feedback?(dbienvenu)
it seems many uris are standard, but certainly not all.  my assumption here is, if they are standard then process that way, if not then just use what the publisher gave (or how current feed processing assigns an item url based on what the publisher gave).

i don't think it matters much for the feed system - they are used as messageIds and keys into feeditems.rdf.  the critical thing is to handle this (sort of bogus) error and mush on cleanly.
(In reply to alta88 from comment #8)
> i don't think it matters much for the feed system - they are used as
> messageIds and keys into feeditems.rdf.
Then I don't see the point of trying to mash them into a standard URL...
If you are going to fall back on just using the value as is, if it can't be parsed as an url, why attempt to parse it as an url at all? why not just use the value?

At least for the feed, that I have problems with, firefox (specifically the code that shows the code when you browse to the url of the feed) seems to parse it correctly, so it might be helpful to compare the feed handling from the two applications. It turns out that the feed have an error in that the isPermaLink-attribute on the guid-element is set to "true" even though the guid-element does not contain a valid url.

If I interpret the code correctly and am looking in the right place, it seems firefox first look at the link-element of the feed entry, and then only if it is emty, look that the guid-element. Also it compares isPermaLink-attribute case-insensitively with "false". Therefor it is not tripped by the error as the link-element in my case contains a url and so it do not use the guid for the url.
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/feeds/FeedProcessor.js#462

The current thunderbird-code on the other hand, seems to use the value of the guid-element if the isPermaLink-attribute is missing or (case-sensitively) "true" and only check the format if attrbiute is missing, and else use the link-element.
http://mxr.mozilla.org/comm-central/source/mailnews/extensions/newsblog/content/feed-parser.js#123

So if the value of the guid-element, with the isPermaLink-attribute set to true, should continue to be preferred over the link-element, perhaps line 145 of feed-parser.js could just be changed from "if (!guidNode.hasAttribute("isPermaLink"))" to "if (isPermaLink)" to have the check be preformed in all cases where the guid-element is used. That just leaves the value unchecked if the value of the link-element is used.
Attached patch update. (obsolete) — Splinter Review
Attachment #579064 - Flags: review?(neil)
(In reply to andershol from comment #10)

a truly noble endeavor would be bug 450543.  but it's not clear the toolkit code is issue free, even per its author:
https://bugzilla.mozilla.org/show_bug.cgi?id=525655#c7

to your specific guid/permalink question - i believe the way Tb handles isPermaLink is correct according to the spec:
http://cyber.law.harvard.edu/rss/rss.html#ltguidgtSubelementOfLtitemgt

the solution of what to do with a guid that must be a valid url but isn't, will be the same - ensure no halt in processing, do a best effort to verify the url, but in the end if the publisher gave you non spec junk what can you do but store it and not assume anything.

so the new patch stores as is, though i lean to the fact that there is a non zero positive in making a standard url if it was meant to be a url.
Attachment #579064 - Flags: review?(neil) → review+
(In reply to alta88 from comment #12)
> so the new patch stores as is, though i lean to the fact that there is a non
> zero positive in making a standard url if it was meant to be a url.
But I still think running URLs through the nsIOService is better than directly creating a nsStandardURL object.
Attached patch new patch. (obsolete) — Splinter Review
i completely agree, and mistook your prior comment to be 'save as is'.
Attachment #578721 - Attachment is obsolete: true
Attachment #579064 - Attachment is obsolete: true
Attachment #579349 - Flags: review?(neil)
Attachment #579349 - Flags: approval-comm-beta?
Attachment #579349 - Flags: approval-comm-aurora?
Attachment #578721 - Flags: review?(neil)
Attachment #578721 - Flags: feedback?(dbienvenu)
Comment on attachment 579349 [details] [diff] [review]
new patch.

>-    var uri = uri.QueryInterface(Components.interfaces.nsIURI);
...
>+      url = ioService.newURI(aVal, null, null)
>+                     .QueryInterface(Components.interfaces.nsIURL);
I don't know why you thought you had to query to nsIURL; nsIURI was used above to get access to .spec but newURI already returns an nsIURI so you can access the spec straight away.

>+    this.mURL = url ? url.spec : aVal;
This could probably be simplified by using separate assignment statements inside the try and catch blocks respectively.
Attached patch new patch. (obsolete) — Splinter Review
Attachment #579349 - Attachment is obsolete: true
Attachment #579718 - Flags: review?(neil)
Attachment #579718 - Flags: approval-comm-beta?
Attachment #579718 - Flags: approval-comm-aurora?
Attachment #579349 - Flags: review?(neil)
Attachment #579349 - Flags: approval-comm-beta?
Attachment #579349 - Flags: approval-comm-aurora?
Comment on attachment 579718 [details] [diff] [review]
new patch.

>+    var url;
...
>+      url = ioService.newURI(aVal, null, null);
>+      this.mURL = url.spec;
(It's probably not worth having a separate variable for this.)
Attachment #579718 - Flags: review?(neil) → review+
Attached patch patch.Splinter Review
Attachment #579718 - Attachment is obsolete: true
Attachment #580424 - Flags: review+
Attachment #580424 - Flags: approval-comm-beta?
Attachment #580424 - Flags: approval-comm-aurora?
Attachment #579718 - Flags: approval-comm-beta?
Attachment #579718 - Flags: approval-comm-aurora?
Keywords: checkin-needed
Comment on attachment 580424 [details] [diff] [review]
patch.

a=Standard8 for aurora. Given where we are for beta, I don't really want to take this this late - we'll pick it up in 10.
Attachment #580424 - Flags: approval-comm-beta?
Attachment #580424 - Flags: approval-comm-beta-
Attachment #580424 - Flags: approval-comm-aurora?
Attachment #580424 - Flags: approval-comm-aurora+
Checked into trunk and aurora:

http://hg.mozilla.org/comm-central/rev/b1363ce4fd80
http://hg.mozilla.org/releases/comm-aurora/rev/0e06cd71ea6f
Status: NEW → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 11.0
You need to log in before you can comment on or make changes to this bug.