562227 - When using the URL referenced the feed gets cut into two pieces making the feed invalid

Status: RESOLVED FIXED

(MailNews Core :: Feed Reader, defect)

Description

[M Prindle]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.3) Gecko/20100401 Firefox/3.6.3
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.1.9) Gecko/20100317 Lightning/1.0b1 Thunderbird/3.0.4

When I add this url "http://houston.craigslist.org/search/sss?query=google+nexus+(N1|one)+-(broken)+-(NOT+CHINA+KNOCK+OFF)+-(832-531-5184)+-(Dr+Cell+Phone)&format=rss" into my RSS feed TB will check the feed once, but when I close and reopen TB the feed is cut into two entries.  The first entry is "http://houston.craigslist.org/search/sss?query=google+nexus+(N1" and the 2nd entry is "one)+-(broken)+-(NOT+CHINA+KNOCK+OFF)+-(832-531-5184)+-(Dr+Cell+Phone)&format=rss".  It appears the | in the feed is breaking the url in TB.  On craigslist the | is use to OR a statement together.

The feeds are structured like this
Craigslist (Folder)
 - Feed 1
 - Feed 2
 - Feed 3

Also, as a part of this bug once the feed is broken in to two pieces I can no longer delete the feed.  I have to go to "C:\Users\username\AppData\Roaming\Thunderbird\Profiles\ve5l8jty.user\Mail\News & Blogs" and delete everything in the folder.  I'm sure there's a way to figure out what file to delete, but I'm unable to figure it out.

Reproducible: Always

Steps to Reproduce:
1.Subscribe to the feed referenced above
2.Close TB
3.Open TB and the feed is broke
Actual Results:  
1.Feed subscribes and updates once
2. After open & close the feed is broke

Expected Results:  
1.Subscribe the feed
2.Open TB and the feed updates

Comment 1

[alta88]

yes, Tb uses | as a delimiter for multiple feeds subscribed to a folder.

Comment 2

[alta88]

Attachment: patch

Comment 3

[Magnus Melin [:mkmelin] (away, back Aug 4)]

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

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

::: mailnews/extensions/newsblog/content/utils.js
@@ +270,5 @@
>      let feedurls = aFolder.getStringProperty("feedUrl");
> +    if (feedurls) {
> +      feedurls = feedurls.replace(this.kFeedUrlDelimiter + "http",
> +                                  "\x01http", "g");
> +      feedurls = feedurls.replace(this.kFeedUrlDelimiter + "file://",

we don't allow file:// feeds, at least yet. 
so you should be able to simply split on "|http", no? that should also be forward-compatible (for downgrade scenarios)

@@ +395,5 @@
> +                                        "\x01file://", "g");
> +      curFeedUrls = curFeedUrls.split("\x01");
> +    }
> +    else
> +      curFeedUrls = [];

if one if-else has braces, they all should is the style rule.

(though if yo split on "|http" you don't need an if-else at all)

Comment 4

[alta88]

(In reply to Magnus Melin from comment #3)
> Comment on attachment 751366 [details] [diff] [review]
> patch
> 
> Review of attachment 751366 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mailnews/extensions/newsblog/content/utils.js
> @@ +270,5 @@
> >      let feedurls = aFolder.getStringProperty("feedUrl");
> > +    if (feedurls) {
> > +      feedurls = feedurls.replace(this.kFeedUrlDelimiter + "http",
> > +                                  "\x01http", "g");
> > +      feedurls = feedurls.replace(this.kFeedUrlDelimiter + "file://",
> 
> we don't allow file:// feeds, at least yet. 
> so you should be able to simply split on "|http", no? that should also be
> forward-compatible (for downgrade scenarios)
> 

yes, but it is possible to override the isValidScheme function, which i do locally and it's pretty important for testing, so i'd like to have this handled up front.


> @@ +395,5 @@
> > +                                        "\x01file://", "g");
> > +      curFeedUrls = curFeedUrls.split("\x01");
> > +    }
> > +    else
> > +      curFeedUrls = [];
> 
> if one if-else has braces, they all should is the style rule.
> 
> (though if yo split on "|http" you don't need an if-else at all)

ok.  (that's how i had it initially but needed to include file://.)

Comment 5

[Magnus Melin [:mkmelin] (away, back Aug 4)]

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

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

::: mailnews/extensions/newsblog/content/utils.js
@@ +389,5 @@
>  
>      let curFeedUrls = aFolder.getStringProperty("feedUrl");
> +    if (curFeedUrls) {
> +      curFeedUrls = curFeedUrls.replace(this.kFeedUrlDelimiter + "http",
> +                                        "\x01http", "g");

Maybe do this for http:// and https:// cases separately? For edge cases.

Comment 6

[alta88]

Attachment: updated

yes, better to be stricter.  converted to a function as well.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/de4183a854a0

Comment 8

[M Prindle]

Thanks for getting this bug fixed.  I can now go back to using the RSS reader in TB.