Closed Bug 250834 Opened 16 years ago Closed 16 years ago

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

Categories

(Firefox :: Bookmarks & History, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: vlad, Assigned: vlad)

References

Details

(Keywords: fixed-aviary1.0)

Attachments

(1 file, 3 obsolete files)

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.
http://frostyplace.com/rss.php - feed parses, but appears in wrong order (?)
Status: NEW → ASSIGNED
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.
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
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.
Attached patch Teach livemarks about guid (obsolete) — Splinter Review
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 on attachment 159113 [details] [diff] [review]
Teach livemarks about guid

Vladimir could you have a look. (This is against trunk.)
Attachment #159113 - Flags: review?(vladimir)
(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.
> 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 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;
Attachment #159113 - Flags: review?(vladimir) → review-
Attached patch updated (obsolete) — Splinter Review
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.
Attachment #159113 - Attachment is obsolete: true
(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;
(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!

Attached patch updated per Sam's suggestions (obsolete) — Splinter Review
Thanks Sam!
Attachment #159121 - Attachment is obsolete: true
Attached patch Updated againSplinter Review
Sigh ... I attached a borked version. This one should cover all comments so far
and should be non-broken.
Attachment #159126 - Attachment is obsolete: true
Attachment #159185 - Flags: review?(vladimir)
> 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?
> 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.
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>
Is this bug report also for Thunderbird? I guess the feed interpreter is the same?
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 on attachment 159185 [details] [diff] [review]
Updated again

looks good; asking for a= and will check in asap.
Attachment #159185 - Flags: review?(vladimir)
Attachment #159185 - Flags: review+
Attachment #159185 - Flags: approval-aviary?
Comment on attachment 159185 [details] [diff] [review]
Updated again

a=asa for aviary checkin.
Attachment #159185 - Flags: approval-aviary? → approval-aviary+
(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.
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.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Keywords: fixed-aviary1.0
(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.
(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.
The RSS feeds on this page doen't seem to work (failed to load).
http://varchars.com/rss/
*** Bug 260810 has been marked as a duplicate of this bug. ***
(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.
(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.
(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.)
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.)
(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 :)
(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.

(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!
(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.
Can you check to see if my blog's feeds were fixed by this patch?

http://www.thenetdragon.net/blog/
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?
Summary: livemark fails to parse RSS feed → livemark fails on some RSS feeds, saying: Live Bookmark feed failed to load
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.
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.
(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 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.
It does, and correctly ("isPermaLink is optional, its default value is true").
See bug 260918
sorry for bugspam, long-overdue mass reassign of ancient QA contact bugs, filter on "beltznerLovesGoats" to get rid of this mass change
QA Contact: mconnor → bookmarks
You need to log in before you can comment on or make changes to this bug.