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.
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
Ah, it's URLs to non-existent bookmarks that were causing the problem.. will fix soon. Thanks for the detailed report!
Status: NEW → ASSIGNED
Throwing it on the radar, as this would be a big bug to miss for 1.0 (RC1).
Created attachment 153608 [details] [diff] [review] 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.
14 years ago
Attachment #153608 - Flags: review?(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!
Attachment #153608 - Flags: review?(mconnor) → review+
In on aviary, with an intended return statement :)
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
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!
Status: RESOLVED → VERIFIED
Flags: blocking-aviary1.0RC1? → blocking-aviary1.0RC1-
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.