252006 - Bookmark with changed favicon url disappears

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Mike Goodspeed]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040626 Firefox/0.9.1
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7) Gecko/20040717 Firefox/0.9.1+

When looking at favicons of old bookmarks that hold the old-style favicons (ones
created before Vlad's big favicon rewrite), if that old bookmark changed the
location of it's favicon, then the bookmark doesn't display a favicon at all --
not even the default image (I'm using the default theme).  This has major
implications for people like my father who have been using milestones for a
while and has amassed an army of old-style bookmarks.  He can't afford to just
wipe his bookmarks clean and recreate them.  It's easier for him just to migrate
them over.

Reproducible: Always
Steps to Reproduce:
1. Edit your bookmarks.html file by changing the URL of a bookmark (I used the
plugin faq in the bookmarks toolbar).  Adding a 1 in the filename will suffice.
 This emulates an old bookmark changing the URL of it's favicon.
2. Open firefox, and hit Bookmarks -> Bookmarks Toolbar Folder.
3. Note how there is *no* icon for the plugin faq website.
4. You can visit the plugin faq website and scroll around in it to pick up the
favicon and then it'll appear in the bookmarks toolbar folder.
5. Close Firefox.
6. Look at your bookmarks.html file and notice how it didn't change the URL of
the bookmark to keep the new favicon.
7. Open Firefox.
8. As expected, the icon for the plugin faq website is still gone.  We're back
to step 3.
Actual Results:  
1. There was an empty favicon for the bookmark that had an incorrect URL for the
favicon.
2. Across sessions, there is no way to get back that icon.  Firefox forgets
about it after it shuts down.

Expected Results:  
1. Firefox should have realized there is no favicon and given it the default
"empty" icon from the current theme.
2. After visiting the bookmark in question, it should have modified the
bookmarks.html file to use the new favicon caching that new-style bookmarks use.
 Or, at the very least, it should have updated the bookmark to use the new
favicon URL.

I compiled my own Linux nightly to check out the new favicons and came across
this bug.  My WinXP 0.9.2 milestone does not exhibit this behavior.  When it
comes across a faulty URL, it throws in the default "empty" image, and I'm
pretty sure it picks up on the change and modifies the bookmarks.html to use the
new URL.

Comment 1

[Mike Goodspeed]

Original Bookmark:
<DT><A HREF="http://plugindoc.mozdev.org/faqs/" LAST_VISIT="1090171272"
ICON="http://plugindoc.mozdev.org/favicon.ico" LAST_CHARSET="UTF-8"
ID="rdf:#$i6osf2">Plug-in FAQ</A>
    <DD>Firefox Plug-in Frequently Asked Questions

Edited Bookmark:
<DT><A HREF="http://plugindoc.mozdev.org/faqs/" LAST_VISIT="1090171272"
ICON="http://plugindoc.mozdev.org/favicon1.ico" LAST_CHARSET="UTF-8"
ID="rdf:#$i6osf2">Plug-in FAQ</A>
    <DD>Firefox Plug-in Frequently Asked Questions

Comment 2

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

Ah, it's URLs to non-existent bookmarks that were causing the problem.. will fix
soon.  Thanks for the detailed report!

Comment 3

[Mike Goodspeed]

Throwing it on the radar, as this would be a big bug to miss for 1.0 (RC1).

Comment 4

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

Attachment: favicons-old-icons-fix-0.patch

Pretend like non-data: icons aren't there (leading to the "blank page" icon)
until you go visit the site.  Also mark the bookmarks as dirty once the icon is
updated, to make sure that the data urls get saved.

Comment 5

[Mike Connor [:mconnor]]

Comment on attachment 153608 [details] [diff] [review]
favicons-old-icons-fix-0.patch

>+            rv = mInner->GetTarget(aSource, aProperty, aTruthValue, aTarget);
>+            if (NS_FAILED(rv) || rv == NS_RDF_NO_VALUE) return rv;

we're really inconsistent about where to put the return statement, even within
this file.  In this case though, bumping it onto a new line is a lot more
readable, at least to me.  I'll leave it up to you whether you want to address
this, you're going to spend more time in bookmarks code than me!

Comment 6

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

In on aviary, with an intended return statement :)

Comment 7

[Mike Goodspeed]

Taking it off the radar and marking as Verified in last night's Win32 nightly
(think the linux builds are broken).  No more missing icons, and the old-style
bookmarks convert to new-style bookmarks (the better of the two solutions).

Thanks, Vlad, for your awesome turnaround time on this bug!  Report early in the
morning, patch by night, new binary next day.  Awesome!

Comment 8

[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