303567 - In live bookmarks, entries not having a <link> element should use the <link> child of root

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, enhancement, P3)

Description

[Julien "_FrnchFrgg_" RIVAUD]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; fr; rv:1.7.8) Gecko/20050517 Firefox/1.0.4 (Debian package 1.0.4-2)
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; fr; rv:1.7.8) Gecko/20050517 Firefox/1.0.4 (Debian package 1.0.4-2)

In a live bookmark folder, when an entry of the RSS has no <link> (which is
permitted by the spec), Firefox should use the (mandatory) <link> at root level.

This would allow for a "default target" in RSS, which is especially useful when
several news are related to the same page (but not all, since bookmarks wouldn't
be meaningful then). For example, when a RSS feed lists all changes to a site,
some news refer to a specific page, but several aren't bound to a specific
location, and thus are to be linked with the home page.

Currently, entries lacking a <link> aren't displayed, and if none has a <link>
the message "the feed failed to load" is displayed.

Reproducible: Always

Steps to Reproduce:
Add "http://www.komite.net/pl/hidden/avec.rss" and
"http://www.komite.net/pl/hidden/sans.rss" to the live bookmarks.
The first has five entries, each with a <link>.
The second has those same five entries, but two of them lacking their <link>.

Go and try to open the corresponding folders.
Actual Results:  
The second live bookmark has only three visible items, and the first has five.

Expected Results:  
The second live bookmark should have had five visible items, two of them
pointing towards the default <link> in the RSS.

The current behaviour has been seen on Linux Mozilla 1.0.4, Windows (1.0.6), and
Linux (Deerpark alpha 2).

Comment 1

[Julien "_FrnchFrgg_" RIVAUD]

Sorry if it is a duplicate, I did dig a lot, but I am prone to error.

Comment 2

[Peter van der Woude [:Peter6]]

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

Comment 3

[Peter van der Woude [:Peter6]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b4) Gecko/20050805
Firefox/1.0+ ID:2005080510

confirmed ->New

Comment 4

[Phil Ringnalda (:philor)]

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

Comment 5

[Julien "_FrnchFrgg_" RIVAUD]

I tried to look at the code at ttp://lxr.mozilla.org/mozilla/source/browser/components/bookmarks/src/nsBookmarksFeedHandler.cpp 

If in HandleRDFItem (line 505), instead of returning when either title or link isn't a litteral (line 527), the code just requires title to be a litteral and fetches the global link from the arg 'aLinkResource' (if I am not mistaken, it is the RSS_link elt passed to HandleRDFItem from TryParseAsRDF, which seems to be the global link elt), would it work (for the RDF case, that is) ?

For the non RDF case, it seems that the <link> elt is blindly skipped by the code that positions itself at the right place to start the processing of the <item>s, but provided some parsing is added, just using the global link when linkStr is empty at line 726 should do the trick... ?

Am I awfully off-track ?

Comment 6

[Julien "_FrnchFrgg_" RIVAUD]

First, I was totally off-track concerning the RDF part of my last post.

Second, in the Atom specification, the author is mandatory for the feed element if and only if there exists a child not having any author, but no similar system is available for the link.

Nevertheless, Firefox could do the following :
a) try to use the entry's link
b) revert if necessary to root link, if it exists
c) return the same error as now if a & b fail (b shouldn't ever fail in RSS2)

Comment 7

[Roger Olsson]

As I see it, the only fool proof solution would be to integrate some full-scale aggregator features to the browser instead of mere bookmarks. As it stands now, people will continue to create and subscribe to feeds that don't link to individual entries (which is perfectly acceptable), and then get confused when all they get is a vague error message.

If Firefox was smart, it could offer the user to download an extension, say Sage, should this occur. Since Firefox can't show the actual content of the feed (without dereferencing links), it can't really handle the feed. Using the "root link" gives unpredictable results, since its exact meaning isn't defined in the specifications. At least the users should be presented with a useful notice instead of the "failed to load" error.

Comment 8

[Phil Ringnalda (:philor)]

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

Comment 9

[Cori Schlegel]

Attachment: rss feed that displays the behavior

since the previous links posted for reproduction seem not to exist any more, here's a file that has several RSS items without links and one that has a link element.  If you use Live Bookmarks to subscribe to this feed you get just one item, not the 30 that exist in the feed.

Comment 10

[Cori Schlegel]

The feeds in Steps to Reproduce don't exist any more - I attached a file that displays both with link and without link behavior

Comment 11

[Robert Sayre]

I agree with this idea. I think we should link to the host of the feed if there's no link element in the feed itself (even though it is required).

Comment 12

[Julien "_FrnchFrgg_" RIVAUD]

I don't think it is required to have a link in every item of the feed at least for RSS2; AFAICT such feeds validate.

Comment 15

[Dietrich Ayala (:dietrich)]

Attachment: fix

Comment 16

[Marco Bonardo [:mak]]

Comment on attachment 319920 [details] [diff] [review]
fix

? toolkit/components/places/src/.nsLivemarkService.js.swp
Index: toolkit/components/places/src/nsLivemarkService.js
===================================================================
+/**
+ *
+ * Update Behavior in Firefox:
+ *
+ * Five seconds after the browser starts up,

the browser UI starts up


+ * is started via nsILivemarkService.start(). The update timer fires immediately
+ * and then every 15 minutes thereafter (gExpiration/4).

15 minutes is true only with default options, instead i would say:

The update timer fires immediately and by default every 15 minutes (is dynamically calculated as gExpiration/4), with a maximum limit of 1 hour.

 
     for (var i = 0; i < feed.items.length; ++i) {
       let entry = feed.items.queryElementAt(i, Ci.nsIFeedEntry);
       let href = entry.link;
-      if (!href)
-        continue;
+      if (!href) {
+        if (!siteURI)
+          continue;
+        else
+          href = siteURI;
+      }

this should work and imo is more readable

let href = entry.link || siteURI;
if (!href)
  continue;

this could probably need a minitest with an example local feed.
hwv r=me

Comment 17

[Phil Ringnalda (:philor)]

Given our history with screwing up loops, a test that makes sure a feed with three entries, first and third with links and second without, continues to only use the siteURI for the second, would be a very good idea.

Comment 18

[Dietrich Ayala (:dietrich)]

Attachment: fix v2

- comments fixed
- tests added (including philor's comment)

Comment 19

[Marco Bonardo [:mak]]

Attachment: unbitrot

sorry for late (damn late!), i've unbitrotted this for you, so forgive me :)

+
+/**
+ *
+ * Update Behavior in Firefox:
+ *
+ * Five seconds after the browser UI starts up, the livemark service's update
+ * timer is started via nsILivemarkService.start(). The update timer fires
+ * immediately and by default every 15 minutes (is dynamically calculated as
+ * gExpiration/4), with a maximum limit of 1 hour.
+ *
+ * When the update timer fires, it iterates over the list of livemarks, and
will
+ * refresh a livemark *only* if it's expired.
+ *
+ * The expiration time for a livemark is determined by using information
+ * provided by the server when the feed was requested, specifically
+ * nsICacheEntryInfo.expirationTime. If no information was provided by the 
+ * server, the default expiration time is 1 hour.
+ *
+ * Users can modify the default expiration time via the 
+ * browser.bookmarks.livemark_refresh_seconds preference.
+ *
+ */

Since we are about changing this i'd remove the description for now

fixe lines over 80 chars where possible
check for exceptions when instantiating services

apart that, r=me

Comment 20

[Marco Bonardo [:mak]]

Dietrich, do you have any time slot to fix this for 3.1? should only need an unbitrot and a cleanup.

Comment 21

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv

Comment 23

[Marco Bonardo [:mak]]

Attachment: patch v1.0

Applied on top of the patch in bug 661445 (this bug adds a useful test to increase coverage for that bug)

Comment 24

[Dietrich Ayala (:dietrich)]

Comment on attachment 538412 [details] [diff] [review]
patch v1.0

r=me, thanks for fixing this!

Comment 25

[Marco Bonardo [:mak]]

http://hg.mozilla.org/projects/places/rev/d7a1485f467d

Comment 26

[Marco Bonardo [:mak]]

The test is randomly failing, I suspect it's the same issue seen in bug 561007 (so a GC running at a random time and collecting some related stuff), I can't reproduce neither on Win, nor on Linux64. Will try on Mac and on Tryserver, otherwise I'll disable test and file a follow-up to fix it :(

Comment 27

[Marco Bonardo [:mak]]

the random timeouts have been handled in bug 664986.

http://hg.mozilla.org/mozilla-central/rev/d7a1485f467d

Comment 28

[Simona Badau, Desktop QA]

Mozilla/5.0 (Windows NT 5.1; rv:9.0a1) Gecko/20110822 Firefox/9.0a1
Mozilla/5.0 (Windows NT 5.1; rv:7.0) Gecko/20100101 Firefox/7.0

Verified issue on Win XP, Win 7, Ubuntu 11.04 and Mac OS X 10.6 using the RSS feed attached in Comment 9 - entries with no link are now displayed.

Setting resolution to VERIFIED FIXED.