250834 - livemark fails on some RSS feeds, saying: Live Bookmark feed failed to load

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Some feeds that we currently don't parse:

http://www.0xdeadbeef.com/rss/diary.rss (RSS 0.90, RDF but without <items> Seq)
http://phpbb.elixus.org/rdf.php (same)

Need to fix RDF parser to handle 0.90 feeds without items sequences.

Please comment if any additional feeds fail to parse.

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

http://frostyplace.com/rss.php - feed parses, but appears in wrong order (?)

Comment 2

[Brian Polidoro]

Here's another that fails 
http://sports.espn.go.com/espn/rss/nfl/news

I verified that this feed is valid at feedvalidator.org.  When trying to look at
the livemark entries it says live bookmark feed failed to load.

And this seems to go for all of the espn feeds.  I tried a few of them.

I'm not sure if this should be a separate bug though because these feeds are RSS
2.0.

Comment 3

[Christian Stadler]

http://www.ragnarokonline.de/news.rss

It validates at feedvalidator.org but it 'failed to load' in Firefox 1.0PR

User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; rv:1.7.3) Gecko/20040911
Firefox/0.10

Comment 4

[Danek Duvall]

I'm running

    Mozilla/5.0 (X11; U; SunOS sun4u; rv:1.7.3) Gecko/20040915 Firefox/0.10

I just wanted to add some to the list, at blogs.sun.com:

    http://blogs.sun.com/roller/rss/DaneseCooper
    http://blogs.sun.com/roller/rss/richb

I don't know if these are because of this bug, or because of bug 255275, but
most of the ones there work for me.

Comment 5

[Harshal Pradhan]

Attachment: Teach livemarks about guid

Quick hack to deal with
http://blogs.law.harvard.edu/tech/rss#ltguidgtSubelementOfLtitemgt

This make us prefer urls obtained from guid's over those from links. This is
alleged to be the intent of the spec.

This should fix http://kottke.org/index.xml, the blogs.sun.com feeds, and a few
others.

Comment 6

[Harshal Pradhan]

Comment on attachment 159113 [details] [diff] [review]
Teach livemarks about guid

Vladimir could you have a look. (This is against trunk.)

Comment 7

[Sam Ruby]

(In reply to comment #5)
> Created an attachment (id=159113)
> Teach livemarks about guid
> 
> Quick hack to deal with
> http://blogs.law.harvard.edu/tech/rss#ltguidgtSubelementOfLtitemgt

A comment on this patch: if isPermaLink is set to false, the value of linkStr
will be false.  A new nsAutoString should be used instead.

Comment 8

[Sam Ruby]

> A comment on this patch: if isPermaLink is set to false, the value of linkStr
> will be false.  A new nsAutoString should be used instead.

Upon closer inspection, a new nsAutoString by the name of isPermaLink is
declared.   The second parameter on the GetAttribute call on the next line
should make use of this variable. 

Comment 9

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 159113 [details] [diff] [review]
Teach livemarks about guid

Some minor changes, but please also make a patch against aviary -- browser/*
isn't quite in sync with trunk, and there's other platform differences between
trunk and branch.  Since firefox is shipping off aviary, that's where this
should go..

>-            if ((!isAtom && nname.Equals(NS_LITERAL_STRING("item"))) ||
>-                ( isAtom && nname.Equals(NS_LITERAL_STRING("entry"))))
>+            if ((!isAtom && nname.EqualsLiteral("item")) ||
>+                ( isAtom && nname.EqualsLiteral("entry")))

EqualsLiteral doesn't exist on aviary/1.7 branch.

>-                        if (childNname.Equals(NS_LITERAL_STRING("title"))) {
>+                        if (childNname.EqualsLiteral("title")) {

ditto.

>                             rv = FindTextNode (childNode, getter_AddRefs(textNode));
>@@ -615,8 +615,27 @@
> 
>                             nsCOMPtr<nsIDOMCharacterData> charTextNode = do_QueryInterface(textNode);
>                             charTextNode->GetData(titleStr);
>+                        } else if (!isAtom && childNname.EqualsLiteral("guid")) {

ditto for EqualsLiteral.

>+                            nsCOMPtr<nsIDOMElement> linkElem = do_QueryInterface(childNode);
>+                            if (!linkElem) break; // out of while(childNode) loop
>+                            
>+                            nsAutoString isPermaLink;
>+                            linkElem->GetAttribute(NS_LITERAL_STRING("isPermaLink"), linkStr);

  isPermaLink, not linkStr

>+                            // Ignore failures. isPermaLink defaults to true.
>+                            if (isPermaLink.EqualsLiteral("false")) continue;

EqualsLiteral, etc.

>+                            // in node's TEXT
>+                            nsCOMPtr<nsIDOMNode> textNode;
>+
>+                            rv = FindTextNode (childNode, getter_AddRefs(textNode));
>+                            if (!textNode || NS_FAILED(rv)) break;
>+
>+                            nsCOMPtr<nsIDOMCharacterData> charTextNode = do_QueryInterface(textNode);
>+                            charTextNode->GetData(linkStr);
>                         } else if (childNname.Equals(NS_LITERAL_STRING("link"))) {
>-                            if (!isAtom) {
>+                            // For RSS, don't override linkStr value
>+                            // if its already been supplied by guid.
>+                            if (!isAtom && linkStr.IsEmpty()) { 
>                                 // in node's TEXT
>                                 nsCOMPtr<nsIDOMNode> textNode;

Comment 10

[Harshal Pradhan]

Attachment: updated

Thanks. I hadn't realized that EqualsLiteral wasn't on branch. This patch drops
the new string stuff and fixes my silly isPermaLink typo and is rediffed
against branch.

I've checked that it applies fine to both trunk and branch versions of
nsBookmarksFeedHandler.cpp. And that it builds and works on trunk. However, I
don't have a branch build handy right now and can't check right away that it
builds there. Hopefully things should be fine there too, but it would be really
nice if someone could confirm.

Comment 11

[Sam Ruby]

(In reply to comment #10)
> Created an attachment (id=159121)
> updated

Two more comments (1):

  if (!isAtom && linkStr.IsEmpty()) { 
    // in node's TEXT
  } else { 
    // in HREF attribute
  }

Should really be:

  if (isAtom) {
    // in HREF attribute
  } else if (linkStr.IsEmpty()) { 
    // in node's TEXT
  }

A test case that would fail without this change would be:

  <guid>x</guid> <link>y</link> <title>z</title>

A second problem is that you only want rel="alternate" links in Atom.

  nsAutoString rel;
  linkElem->GetAttribute(NS_LITERAL_STRING("rel"), rel);
  if (!isPermaLink.EqualsLiteral("alternate")) continue;

Comment 12

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #10)
> Created an attachment (id=159121)
> updated
> 
> Thanks. I hadn't realized that EqualsLiteral wasn't on branch. This patch drops
> the new string stuff and fixes my silly isPermaLink typo and is rediffed
> against branch.
> 
> I've checked that it applies fine to both trunk and branch versions of
> nsBookmarksFeedHandler.cpp. And that it builds and works on trunk. However, I
> don't have a branch build handy right now and can't check right away that it
> builds there. Hopefully things should be fine there too, but it would be really
> nice if someone could confirm.

Cool, I'll test on the branch and will + after the build.  Thanks!

Comment 13

[Harshal Pradhan]

Attachment: updated per Sam's suggestions

Thanks Sam!

Comment 14

[Harshal Pradhan]

Attachment: Updated again

Sigh ... I attached a borked version. This one should cover all comments so far
and should be non-broken.

Comment 15

[Hixie (not reading bugmail)]

> A second problem is that you only want rel="alternate" links in Atom.
> 
>   nsAutoString rel;
>   linkElem->GetAttribute(NS_LITERAL_STRING("rel"), rel);
>   if (!isPermaLink.EqualsLiteral("alternate")) continue;

So "rel" in Atom is a single keyword, not a space-separated list of keywords 
like in HTML?

Comment 16

[Sam Ruby]

> So "rel" in Atom is a single keyword, not a space-separated list of keywords 
> like in HTML?

As of the current drafts of Atom, yes, "rel" in Atom is a single keyword.

Comment 17

[Phil Ringnalda (:philor)]

Does that get us the final atom:link, or the first, in

<atom:entry>
 <atom:link rel="alternate" type="text/plain" href="/2004/09/foo.txt"/>
 <atom:link rel="alternate" type="text/html" href="/2004/09/foo.html"/>
 <atom:link rel="alternate" type="application/xhtml+xml" href="/2004/09/foo.xhtml"/>
 <atom:link rel="alternate" type="application/pdf" href="/2004/09/foo.pdf"/>
</atom:entry>

Comment 18

[Ben]

Is this bug report also for Thunderbird? I guess the feed interpreter is the same?

Comment 19

[Ben]

Some more that don't work, all from Reuters UK. They have some 20 feeds listed
on this HTML page, none of them work, not in FF, nor in TB:
http://www.reuters.co.uk/newsrss.jhtml

Here comes a partly transcript of one of the feeds:

<?xml version="1.0" encoding="UTF-8"?>
<rss version="2.0">
<channel>
<title>Reuters: Top News</title>
<link>http://www.reuters.co.uk</link>
<description>Reuters: Top News</description>
<image>
<title>Reuters News</title>
<width>120</width>
<height>35</height>
<link>http://www.reuters.co.uk</link>

<url>http://wwwi.reuters.com/comX/images/reuters120.gif</url>
</image>
<item>
<title>Deadline nears for British hostage</title>
<guid isPermaLink="false">587077</guid>
<link>http://www.reuters.co.uk/newsPackageArticle.jhtml?type=topNews&amp;storyID=587077&amp;src=rss/uk/topNews&amp;section=news</link>
<pubDate>Mon, 20 Sep 2004 09:17:48 GMT
</pubDate>
<description>BAGHDAD (Reuters) - A deadline set by militants who have threatened
to behead a Briton and two Americans seized in Iraq is due to expire, and more
than two dozen other hostages are also facing death unless rebel demands were
met.</description>

</item>

Comment 20

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 159185 [details] [diff] [review]
Updated again

looks good; asking for a= and will check in asap.

Comment 21

[Asa Dotzler [:asa]]

Comment on attachment 159185 [details] [diff] [review]
Updated again

a=asa for aviary checkin.

Comment 22

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #19)
> Some more that don't work, all from Reuters UK. They have some 20 feeds listed
> on this HTML page, none of them work, not in FF, nor in TB:
> http://www.reuters.co.uk/newsrss.jhtml

All of their feeds work fine for me in Firefox PR1; they need to be added
manually through the bookmarks manager as they don't use <link>, but they parse
fine.

Comment 23

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Landed on aviary.  Thanks Harshal!

Resolving this bug; the majority of the issues were with the Atom rel bits that
the patch fixed.  Please file new bugs for other unparsable valid feeds.

Comment 24

[Ben]

(In reply to comment #22)
> > Some more that don't work, all from Reuters UK. They have some 20 feeds listed
> > on this HTML page, none of them work, not in FF, nor in TB:
> > http://www.reuters.co.uk/newsrss.jhtml
> 
> All of their feeds work fine for me in Firefox PR1; they need to be added
> manually through the bookmarks manager as they don't use <link>, but they
> parse fine.

Is that on a nightly that it works fine or on the original 1.0PR? None of them
can be parsed, nor in FF 1.0PR nor in TB 0.8.

Comment 25

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #24)
> Is that on a nightly that it works fine or on the original 1.0PR? None of them
> can be parsed, nor in FF 1.0PR nor in TB 0.8.

It works fine in both ffox 1.0PR and in a nightly.  I haven't tested
thunderbird.  Note that firefox and thunderbird do not share the same feed
parsing code; tbird's is probably somewhat more robust, even.   I'm curious to
see if someone else can reproduce these feeds not parsing, or if its something
local to your setup.

Comment 26

[Ger Teunis]

The RSS feeds on this page doen't seem to work (failed to load).
http://varchars.com/rss/

Comment 27

[José Jeria]

*** Bug 260810 has been marked as a duplicate of this bug. ***

Comment 28

[Ger Teunis]

(In reply to comment #26)
> The RSS feeds on this page doen't seem to work (failed to load).
> http://varchars.com/rss/

Terribly sorry for the bugspam: thunderbird seems to parse them okay.

Comment 29

[Ger Teunis]

(In reply to comment #26)
> The RSS feeds on this page doen't seem to work (failed to load).
> http://varchars.com/rss/

Terribly sorry for the bugspam: thunderbird seems to parse them okay.

Comment 30

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #26)
> The RSS feeds on this page doen't seem to work (failed to load).
> http://varchars.com/rss/

Which ones?  4-5 that I tried all worked fine.  Please file a new bug with
specific feeds that don't load.  (This is all in firefox -- tbird's RSS features
have nothing to do with firefox's.)

Comment 31

[Phil Ringnalda (:philor)]

Didn't try the suprnova feeds, then? They have a <title> and an <enclosure>
which links to a redirector to a .torrent, but no <link> or <guid>. Do you
actually want an enhancement bug to support enclosures? (And, it looks like the
feeds don't work anyway, because suprnova sniffs for a local referrer to stop
people doing things like that.)

Comment 32

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #31)
> Didn't try the suprnova feeds, then? They have a <title> and an <enclosure>
> which links to a redirector to a .torrent, but no <link> or <guid>. Do you
> actually want an enhancement bug to support enclosures? (And, it looks like the
> feeds don't work anyway, because suprnova sniffs for a local referrer to stop
> people doing things like that.)

Heh, nope, I tried the scanned advertisement ones and a few of the uspto ones. 
No plans to support <enclosure>, so the suprnova ones will stay broken then :)

Comment 33

[Ger Teunis]

(In reply to comment #30)
> (In reply to comment #26)
> > The RSS feeds on this page doen't seem to work (failed to load).
> > http://varchars.com/rss/
> 
> Which ones?  4-5 that I tried all worked fine.  Please file a new bug with
> specific feeds that don't load.  (This is all in firefox -- tbird's RSS features
> have nothing to do with firefox's.)

Sorry, indeed, only the SuprI only checked the SuprNova ones.(In reply to
comment #32)
> (In reply to comment #31)
> > Didn't try the suprnova feeds, then? They have a <title> and an <enclosure>
> > which links to a redirector to a .torrent, but no <link> or <guid>. Do you
> > actually want an enhancement bug to support enclosures? (And, it looks like the
> > feeds don't work anyway, because suprnova sniffs for a local referrer to stop
> > people doing things like that.)
> 
> Heh, nope, I tried the scanned advertisement ones and a few of the uspto ones. 
> No plans to support <enclosure>, so the suprnova ones will stay broken then :)

I forgot to test the others feeds :)
Okay, thank you for your time.

Comment 34

[Ben]

(In reply to comment #25)
> I'm curious to see if someone else can reproduce these feeds not parsing,
> or if its something local to your setup.

Well, some news. Found out that yesterday the Reuters feeds started working, but
today they don't work anymore. The catch? Proxy. The Reuters feeds do not work
through a proxy, but work fine with a direct internet connection!

Comment 35

[Sam Ruby]

(In reply to comment #23)
> Landed on aviary.  Thanks Harshal!
> 
> Resolving this bug; the majority of the issues were with the Atom rel bits that
> the patch fixed.  Please file new bugs for other unparsable valid feeds.

As near as I can tell, none of the feeds mentioned on this bug report are Atom
feeds.

Comment 36

[Brian 'netdragon' Bober]

Can you check to see if my blog's feeds were fixed by this patch?

http://www.thenetdragon.net/blog/

Comment 37

[Brian 'netdragon' Bober]

My blog's live bookmark and atom feeds now work with the latest Aviary build
from Daa. The feeds for all the ESPN.com's sites now work, too. Thanks :-)

Test run on - Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.3)
Gecko/20041019 Firefox/1.0

I guess we are waiting till it's on the trunk to mark it verified?

Comment 38

[chinshaw]

I'm getting this error (as are other users) for the BBC live bookmark that comes
"embedded" in Firefox. You click on "Latest Headlines" on the live bookmark
toolbar and the only listing is the "Live bookmark feed failed to load" error
message. Not resolved after restarting Firefox and/or rebooting machine.

I think this should be reopened.

Comment 39

[Mike Connor [:mconnor]]

The problem there is that pre-1.0 versions had fxfeeds.mozillazine.org, not
fxfeeds.mozilla.org, and the MZ redirect is offline.  We can't dynamically
update bookmarks, but in any case that's a separate issue.  Edit the live
bookmark to the correct host and you'll be fine.

Comment 40

[Alex Bishop]

(In reply to comment #39)
> The problem there is that pre-1.0 versions had fxfeeds.mozillazine.org, not
> fxfeeds.mozilla.org, and the MZ redirect is offline.  We can't dynamically
> update bookmarks, but in any case that's a separate issue.  Edit the live
> bookmark to the correct host and you'll be fine.

fxfeeds.mozillazine.org is now CNAME'd to fxfeeds.mozilla.org, so it should work
without the host needing to be changed in the bookmark.

Comment 41

[PikeUK]

Comment on attachment 159185 [details] [diff] [review]
Updated again

Some links from http://planet.mozilla.org/rss20.xml don't work. Doesn't this
patch assume that any guid elements without a permalink attribute are
permalinks, shouldn't it assume the opposite? See for example the MozillaZine
items in the previously mentioned feed.

Comment 42

[Phil Ringnalda (:philor)]

It does, and correctly ("isPermaLink is optional, its default value is true").
See bug 260918

Comment 43

[Mike Connor [:mconnor]]

sorry for bugspam, long-overdue mass reassign of ancient QA contact bugs, filter on "beltznerLovesGoats" to get rid of this mass change