541718 - The structure with which RSS Feed was developed was changed, and the HTML display of Feed became impossible.

Status: RESOLVED FIXED

(MailNews Core :: Feed Reader, defect)

Description

[Kouichi Niwa]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; ja; rv:1.9.2) Gecko/20100115 Firefox/3.6 GTBDFff GTB7.0 (.NET CLR 1.1.4322; .NET CLR 2.0.50727; .NET CLR 3.0.04506.30; .NET CLR 3.0.4506.2152; .NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; ja; rv:1.9.1.7) Gecko/20100111 Thunderbird/3.0.1

It becomes impossible The structure of RSS Feed is changed since Thunderbird 3.0, and Content-Base: for URL in base href not to be buried normally, and to display Web and. 

Reproducible: Always

Comment 1

[Kouichi Niwa]

Attachment: received feed item for TB2.x

This feed is nomality web page messeage pane.

Comment 2

[Kouichi Niwa]

Attachment: received feed item for TB3.x

received feed item for TB3.x
this feed item, can't disp webpage to message pane.

Comment 3

[Kouichi Niwa]

Attachment: feed source (By.Fx3.6)

The one that it is necessary to adopt <link>--</link> originally is handled and <guid>--</guid> is handled as a link since Thunderbird 3.0.

Comment 4

[Kouichi Niwa]

(In reply to comment #1-#3)
Attn!!
Message source in using language is Japanese.

Comment 5

[Ludovic Hirlimann [:Usul]]

Can you try to View -> Message Body As -> Html ? 

Does this happens in -safe-mode (http://kb.mozillazine.org/Safe_mode) ?

Comment 6

[Kouichi Niwa]

(In reply to comment #5)
> Can you try to View -> Message Body As -> Html ? 

RSS Feed is set as always displayed by the HTML form.


> Does this happens in -safe-mode (http://kb.mozillazine.org/Safe_mode) ?

There was no change in the phenomenon though it started in a safe mode, and RSS Feed was acquired with another ID. 
<GUID> in Feed Source is still set to Content-Base and <base href> with Thunderbird 3.x. 
The <link> syntax in Feed Source is disregarded.

Comment 7

[Kouichi Niwa]

Don's fix this probrem on TB 3.0.2 RC(build1).

Comment 8

[Wayne Mery (:wsmwk)]

Priority should only be set by developers please.  Resetting to --
See https://bugzilla.mozilla.org/page.cgi?id=fields.html#priority

Comment 9

[Fosfor]

Confirm with
   "Mozilla/5.0 (Windows; U; Windows NT 6.1; cs; rv:1.9.2.9) Gecko/20100915 Thunderbird/3.1.4" on Win7
   "Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.9) Gecko/20100915 Thunderbird/3.1.4" in WinXP-SP3.

GUID should not be used as link unless it have isPermaLink attribute. See http://www.rssboard.org/rss-specification#ltguidgtSubelementOfLtitemgt "There are no rules for the syntax of a guid. Aggregators must view them as a string. It's up to the source of the feed to establish the uniqueness of the string."

Another RSS to demonstrate this bug: http://unilever-rss.com/rss/61/348/

Thank you very much for fix this annoying bug.

Comment 10

[Fosfor]

Maybe the misunderstand is here:
http://mxr.mozilla.org/comm-central/source/mailnews/extensions/newsblog/content/feed-parser.js#133

IMHO:
<guid> - not url (*)
<guid isPermaLink> - is url
<guid isPermaLink="true"> - is url
<guid isPermaLink="meatcake"> - not url

(*) is treated as url according to the code above.

PS: hope it can help, it's my first touch with code ;)

Comment 11

[Ludovic Hirlimann [:Usul]]

(In reply to comment #10)

> PS: hope it can help, it's my first touch with code ;)

Do you feel like making a real and complete patch (we will guide you) ?

Comment 12

[Fosfor]

My pleasure to help this way:) I have some programming skills, but no knowledge of local systems. So some link to "patch-how-to" or guidance through email/jabber/ICQ will be good.
PS: now off for the lunch, in few minutes ready for patching:)

Comment 13

[Ludovic Hirlimann [:Usul]]

(In reply to comment #12)
> My pleasure to help this way:) I have some programming skills, but no knowledge
> of local systems. So some link to "patch-how-to" or guidance through
> email/jabber/ICQ will be good.
> PS: now off for the lunch, in few minutes ready for patching:)

I probably replied, but I'll add a few links. A good start point is https://developer.mozilla.org/en/comm-central explain a bunch of things and gives a few links to things you want to setup. After that when things don't work out irc and #maildev are the places to go.

Comment 14

[Fosfor]

(In reply to comment #10)
> <guid> - not url (*)
> <guid isPermaLink> - is url
> <guid isPermaLink="true"> - is url
> <guid isPermaLink="meatcake"> - not url
> 

I have some mistakes in the above comment:
<guid> IS url, becasuse isPermaLink has default value "true" and it has its default value even if it is missing
<guid isPermaLink> is error, it is not well-formated

Thanx to philor from #maildev and Mamuf to clarify it to me.

Suggested bug change: RESOLVED-INVALID ?

BTW: Feed source attached above is not specification correct - <guid> is not an URL.

Comment 15

[Phil Ringnalda (:philor)]

No, just because a feed with "<guid>not-a-url</guid>" is invalid RSS doesn't mean that the bug is invalid, or that we have to choke on it. It would be perfectly reasonable for us to always check whether a guid that we are going to use as a permalink has a value that we can actually use as a URL, and if we can't to fall back to the <link>.

Though probably the right resolution for this bug is duplicate - this is far from the first report that trusting guid doesn't always work out well.

Comment 16

[Fosfor]

(In reply to comment #15)

You mean something like check whether the guid is url and only if so, use it as a link? (And use <link> in other case.)
Is so, is there any isUrl() function around? I can try to modify my patch to realy help:)
PS: sorry for my stubbornness on irc, you were right:)

Comment 17

[Phil Ringnalda (:philor)]

Yes, but only "something like" because there's no isUrl function, because the only way to write it would be to have a little man behind a curtain using a computer exactly like the user's, let him load the possible URL, and see whether he smiles or frowns.

<guid>http://example.org/one</guid> isUrl, for sure.

Is <guid>/one</guid>? I happen to know that the intention of the spec was that it is not, because what the author meant by "a url that can be opened in a Web browser" was "if I stick this string in the href attribute of an a element in my product, Radio Userland, which doesn't use any base href and mixes items from different feeds together, it can be opened." (At least, within the limits of my memory: it's been a long time since I had a copy of Radio installed to be able to check.)

Is <guid>edk://example.org/one</guid>? Maybe on your computer, not on mine.

Is <guid>tag:example.org,2000:one</guid>? No, because RFC 4151 says that tag URIs are URIs which cannot be resolved.

So I think that the best we can do is to say that <guid isPermaLink="true"> is always a permalink, that <guid isPermaLink="anything-else"> is never a permalink, and for <guid>, it is not a permalink if nsIIOService.extractScheme == "tag", it is not a permalink if nsIIOService.newURI throws NS_ERROR_MALFORMED_URI, and otherwise it is.

Comment 18

[Fosfor]

Attachment: proposed patch

Tested on my own build. Without problem built and tested on this types of RSS feeds:

<guid isPermaLink="false"> - not permaLink
<guid isPermaLink="true"> - permaLink
<guid> - not permaLink
<guid> - permaLink
no guid

Always displayed proper URL (from link or from guid, what was configured or usable).

Comment 19

[Fosfor]

And who knows, what does the "/components/FeedProcessor.js" file do in built application? There is also some RSS-guid related code...
http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/components/feeds/src/FeedProcessor.js#478

Comment 20

[Ludovic Hirlimann [:Usul]]

Myk do you have an answer for comment 19 ?

Comment 21

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 488195 [details] [diff] [review]
proposed patch

>         // isPermaLink is true if the value is "true" or if the attribute is
>         // not present; all other values, including "false" and "False" and
>         // for that matter "TRuE" and "meatcake" are false.

Nit: this comment should be updated to reflect the new behavior.

Otherwise, this looks great.  Note: it might make sense to do this check even when isPermaLink="true", in case the source has mistakenly set that for a link that cannot be opened in a web browser.

Comment 22

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #19)
> And who knows, what does the "/components/FeedProcessor.js" file do in built
> application? There is also some RSS-guid related code...
> http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/components/feeds/src/FeedProcessor.js#478

FeedProcessor.js implements the nsIFeed* interfaces that Firefox uses to parse feeds for its feed preview page.  It was contributed by Robert Sayrer to Toolkit a few years ago and is a better parser than the older one I wrote and which Thnuderbird currently uses.

Thunderbird should probably switch to FeedProcessor.js, but doing so is a lot of work, and no one has stepped up to the plate to do the work and maintain the code afterwards.

Comment 23

[Phil Ringnalda (:philor)]

(In reply to comment #21)
> Note: it might make sense to do this check even
> when isPermaLink="true", in case the source has mistakenly set that for a link
> that cannot be opened in a web browser.

The problem with that, and the reason I didn't say we should do so in comment 17, is <guid isPermaLink="true">/one.html</guid>. I don't have any problem with saying to someone using <guid>/one.html</guid> that we have to throw out their permalink even though the spec says it's a permalink, because of the large number of producers that we know were confused by the "default value" language in the spec, but saying that we will throw out <guid isPermaLink="true">/one.html</guid> because we think someone might possibly say something was a permalink when it isn't is less defensible (though still somewhat defensible, thanks to sheep-entrail-reading saying that the author meant to, but failed to, say that relative URLs are not allowed).

Comment 24

[Myk Melez [:myk] [@mykmelez]]

For me, the question is not about following the spec or satisfying author expectations, it's about satisfying user expectations.

If we know the user will not be able to load "xyzzy" in their browser, it seems to me that we shouldn't present it to users as a link, even if it is specified by the tag <guid isPermaLink="true" reallyIMeanItIInsistThisIsALink="true">xyzzy</guid>.

In any case, this question is a separate bug.

Comment 25

[Phil Ringnalda (:philor)]

Well, I was confused about what we were doing to user expectations anyway. I thought that we resolved relative URIs, but we don't, explicitly passing a null base for anything other than Atom. Given that, it doesn't matter to me either way.

Comment 26

[Fosfor]

Tnx for review:)

(In reply to comment #21)
> Nit: this comment should be updated to reflect the new behavior.

I think this comment is correct - it's explaining the state of isPermaLink attribute (which was already parsed well). The new behavior is documented in comment:
// if attribute isPermaLink is missing, it is good to check the validity
// of <guid> value as an URL to avoid linking to non-URL strings
If you don't agree, please let me know the way how to improve it and I'll do it.

And what about checking the <guid isPermaLink="true">xyz</guid> finally? I'm not the wiser from your conversation:) It would be good to check it too (plus for the user confort), but there can be some education meaning for feed authors when we will stick to spec.

But I'm not supposed to decide it:) I know only a litle about whole project. Just tell me YES or NO and I'll update the patch.

Comment 27

[Fosfor]

(In reply to comment #22)
> FeedProcessor.js implements the nsIFeed* interfaces that Firefox uses to parse
> feeds for its feed preview page.

So should I make the patch for this file too? As new bug? Or as a new bug in FF? Who ask for the review?

Comment 28

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #26)
> I think this comment is correct - it's explaining the state of isPermaLink
> attribute (which was already parsed well). The new behavior is documented in
> comment:
> // if attribute isPermaLink is missing, it is good to check the validity
> // of <guid> value as an URL to avoid linking to non-URL strings
> If you don't agree, please let me know the way how to improve it and I'll do
> it.

The comment represents the code it comments correctly, it's just a bit misleading, since the eventual behavior of the app is different due to the new code.  But it's reasonable enough, so let's leave it as-is.


> And what about checking the <guid isPermaLink="true">xyz</guid> finally? I'm
> not the wiser from your conversation:) It would be good to check it too (plus
> for the user confort), but there can be some education meaning for feed authors
> when we will stick to spec.
> 
> But I'm not supposed to decide it:) I know only a litle about whole project.
> Just tell me YES or NO and I'll update the patch.

No, let's leave the patch as-is.

Thanks for the fix!  Marking this as checkin-needed to attract the attention of someone who will check it in.


(In reply to comment #27)
> So should I make the patch for this file too? As new bug? Or as a new bug in
> FF? Who ask for the review?

The change to FeedProcessor.js is a different change, since the codebases are different and are used by different applications.  You shouldn't feel any need to make the change to that code just because you made the change to this one.

However, if you would like to change the behavior of FeedProcessor.js, you would do so by filing a new bug in the Toolkit product <https://bugzilla.mozilla.org/enter_bug.cgi?product=Toolkit> and asking Rob Sayre for review.

Comment 29

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/9bf1ef38cc60

Comment 31

[nim_rod7]

checkind in beta 9, its present

Comment 32

[bp]

This bug still manifests itself on this feed: http://static.feed.rbc.ru/rbc/internal/rss.rbc.ru/rbc.ru/news.rss

It uses weird tags like <guid>top.rbc.ru:society:558ba7829a794712f099dec9</guid> which takes precedence over valid <link> field.