Closed Bug 1356525 Opened 7 years ago Closed 7 years ago

[meta] Broken favicons storage for many favicons which are: missing, no proper, no updating etc. after landing patches from bug #977177

Categories

(Toolkit :: Places, defect)

Product:
Toolkit
Component:
Places
Version:
55 Branch
Platform:
x86_64
Windows 7
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox-esr45 --- unaffected
firefox52 --- unaffected
firefox-esr52 --- unaffected
firefox53 --- unaffected
firefox54 --- unaffected
firefox55 + verified

People

(Reporter: Virtual, Assigned: mak)

References

Details

(4 keywords, Whiteboard: [fixed by fixing dependencies])

Attachments

(5 files)

bug1.png
(deleted), image/png
Details
bug2.png
(deleted), image/png
Details
favicon-bug.png
(deleted), image/png
Details
wrong favicon 1
(deleted), image/png
Details
wrong favicon 2
(deleted), image/png
Details 
[Tracking Requested - why for this release]: Regression



STR: Many favicons are:
- missing
- not proper
- not updating



"Speedy" Regression window (mozilla-central)
Good:
https://ftp.mozilla.org/pub/firefox/nightly/2017/04/2017-04-12-03-02-52-mozilla-central/

Bad:
https://ftp.mozilla.org/pub/firefox/nightly/2017/04/2017-04-13-03-02-27-mozilla-central/

Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f40e24f40b4c4556944c762d4764eace261297f5&tochange=819a666afddc804b6099ee1b3cff3a0fdf35ec15

Probably caused by:
d73a295b3652	Marco Bonardo — Bug 977177 - Invalidate the page-icon image cache when necessary. r=adw
42df4b3da073	Marco Bonardo — Bug 977177 - Don't expire root domain icons with history, and don't associate pages to them. r=adw
5311e5ac3b4b	Marco Bonardo — Bug 977177 - Add favicons.sqlite to profile related lists. r=adw,jmaher
864e72c60156	Marco Bonardo — Bug 977177 - Split ico files into native frames. r=adw
62f3fc3cb351	Marco Bonardo — bug 977177 - Fallback to the root domain icon. r=adw
60002894a42b	Marco Bonardo — Bug 977177 - Expire old page to icon relations to avoid serving deprecated icons. r=adw
4a0770d810dc	Marco Bonardo — Bug 977177 - Add size ref fragment to icon protocols. r=adw
90d755bcbb92	Marco Bonardo — Bug 977177 - Update favicons API consumers. r=adw
942aa1533e08	Marco Bonardo — Bug 977177 - Move favicons to a separate store. r=adw



Fixing storage by Places Maintenance 2.0.2 and Veryfy Integrity in about:support doesn't help

> Integrity check
+ The database is sane
> Coherence check
+ The database is coherent
> Orphans expiration
+ Database cleaned up
> Vacuum
Initial database size is 66560 KiB
+ The database has been vacuumed
Final database size is 66560 KiB
> Statistics
Database size is 66560 KiB
user_version is 37
page_size is 32768
cache_size is -2048
journal_mode is wal
synchronous is 1
History can store a maximum of 107547 unique pages
Table moz_places has 116246 records
Table moz_historyvisits has 64673 records
Table moz_inputhistory has 0 records
Table moz_hosts has 6121 records
Table moz_bookmarks has 79641 records
Table moz_bookmarks_deleted has 0 records
Table moz_keywords has 0 records
Table sqlite_sequence has 1 records
Table moz_favicons has 0 records
Table moz_anno_attributes has 9 records
Table moz_annos has 72933 records
Table moz_items_annos has 63500 records
Table sqlite_stat1 has 14 records
Index sqlite_autoindex_moz_inputhistory_1
Index sqlite_autoindex_moz_hosts_1
Index sqlite_autoindex_moz_bookmarks_deleted_1
Index sqlite_autoindex_moz_keywords_1
Index sqlite_autoindex_moz_favicons_1
Index sqlite_autoindex_moz_anno_attributes_1
Index moz_places_url_hashindex
Index moz_places_faviconindex
Index moz_places_hostindex
Index moz_places_visitcount
Index moz_places_frecencyindex
Index moz_places_lastvisitdateindex
Index moz_places_guid_uniqueindex
Index moz_historyvisits_placedateindex
Index moz_historyvisits_fromindex
Index moz_historyvisits_dateindex
Index moz_bookmarks_itemindex
Index moz_bookmarks_parentindex
Index moz_bookmarks_itemlastmodifiedindex
Index moz_bookmarks_guid_uniqueindex
Index moz_keywords_placepostdata_uniqueindex
Index moz_annos_placeattributeindex
Index moz_items_annos_itemattributeindex
(In reply to Virtual_ManPL [:Virtual] - (please needinfo? me - so I will see your comment/reply/question/etc.) from comment #0)
> [Tracking Requested - why for this release]: Regression
> 
> 
> 
> STR: Many favicons are:
> - missing
> - not proper
> - not updating

This is a little bit generic, I need examples.

For example in bug1, the icon may be correct in both cases, it's using the root icon instead of the local one, probably because the root icon has a better resolution. But it's not a completely random or wrong icon (it's indeed better than the generic invision forum favicon). It may be fixed once we add size fragments to the ui and start collecting more icons per page.
But for now I consider this a behavioral change, not a bug.

The missing icon, I need some examples where the icon is missing after the bookmark/page being visited and the browser restarted, cause otherwise we may be falling into the not updating icon case, that is bug 1356440
Depends on: 1356440
Severity: critical → normal
I found(In reply to Marco Bonardo [::mak] from comment #3)
> The missing icon, I need some examples where the icon is missing after the
> bookmark/page being visited and the browser restarted, cause otherwise we
> may be falling into the not updating icon case, that is bug 1356440

I have over 75.000 bookmarks.
In my case, I lost many favicons, at least some thousands, even not loading these pages,
so it's probably something with migrating the favicons storage from old to new version.
I'm changing severity to "critical" per "loss of data".

What's more, history is also affected, so if you want to reproduce it without having large bookmark database, you could look there for missing favicons.
I have the same problem, Nightly 14-04-17. I have a folder with 47 bugzilla bookmarks on the bookmarks toolbar and most favicons are now gone:
http://i.imgur.com/FO1OtIt.png
The favicons are not updated even after I visit the site from the bookmarks.
Yes, I think I identified a problem in the favicons migration, but I'm still investigating.
The problem doesn't happen in new profiles and that's what makes it a bit harder to follow.

PS: you can set severity to what you want, we don't use it. the only one we use is critical for crashes. That's why I reset it.
Tracking 55+ for this regression.
tracking-firefox55: ? → +
Depends on: 1356567
Pretty sure I have this issue as well.  At least for Gmail and I think TF2.com - Issue is on my Bookmark Toolbar.
Depends on: 1357555
Depends on: 1357664
Keywords: meta
All the patches for the currently known issues landed in central, please test the next nightlies and report eventual issues I didn't handle yet.

Note that missing favicons won't come back magically by themselves, in most cases you'll have to revisit the bookmark, but from that point on there should be no more dataloss.
Thanks.
Blocks: PlacesHiresFavicons
Not working in today's nightly.  Tomorrow?
(In reply to joshuaglennrussell from comment #11)
> Not working in today's nightly.  Tomorrow?
I have 53 bookmarks in my Bugzilla folder. In Nightly <22.04.17 most of the favicons were gone, after I installed Nightly 23-04-17 and opened those 53 bookmarks 50 favicons are back, 3 still missing.
As for me, Gmail and TF2.com are still missing in 4/22 build.
(In reply to joshuaglennrussell from comment #13)
> As for me, Gmail and TF2.com are still missing in 4/22 build.

they show on the tab.
Yeah, still not working for Gmail or TF2.

https://www.google.com/gmail/about/images/favicon.ico

http://media.steampowered.com/apps/tf2/blog/images/favicon.ico

Of course they show on the tab, but won't on the bookmark toolbar.
(In reply to joshuaglennrussell from comment #15)
> Yeah, still not working for Gmail or TF2.
> 
> https://www.google.com/gmail/about/images/favicon.ico
> 
> http://media.steampowered.com/apps/tf2/blog/images/favicon.ico
> 
> Of course they show on the tab, but won't on the bookmark toolbar.

Oh, that's interesting.  I got it to work by deleting and recreating the TF2 bookmark on the toolbar.  As for Gmail, I had to sign out and use its front page.  But using that url still takes you to that front page even if you're signed in.  So then I created the bookmark for my inbox again and now it's working, even on restart (no cache).
If the bookmark doesn't pick-up a favicon, please post here the exact bookmark url.
(In reply to Marco Bonardo [::mak] from comment #17)
> If the bookmark doesn't pick-up a favicon, please post here the exact
> bookmark url.
https://bugzilla.mozilla.org/show_bug.cgi?id=1210990#c23
https://bugzilla.mozilla.org/show_bug.cgi?id=1323987#c79
Screenshot:
http://i.imgur.com/vwdjYkq.png

These two bookmarks are in the folder on the bookmarks toolbar, after I open these two bookmarks in the separate tabs favicons of those tabs are rendered correctly.
My bookmarks toolbar favicon for https://feedly.com/i/latest has been replaced with the Facebook favicon. Deleting and re-adding didn't help on Nightly 55.0a1 (2017-04-24) (64 bit).

Screenshot:
http://imgur.com/a/DW9Ul
Has Regression Range: --- → yes
Has STR: --- → yes
Depends on: 1360477
Depends on: 1360891
No longer depends on: 1360158
No longer depends on: 1359487
Attached image favicon-bug.png (deleted) — 
Favicon is wrong for gmail's inbox. I never even saw that favicon. No idea where it's from.
(In reply to avada from comment #20)
> Created attachment 8864591 [details]
> favicon-bug.png
> 
> Favicon is wrong for gmail's inbox. I never even saw that favicon. No idea
> where it's from.

It looks like a Mozilla icon. Is it possible somehow you got a redirect from a mozilla website to gmail? We had this problem also in the past with the previous store, the favicons always supported redirecting bookmarks, up to 2 levels, but the algorithm cannot distinguish "valid" from "invalid" redirects. We could maybe limit it to same host redirects, but that would likely cause a different regression where some bookmark will lack a favicon.
If you wish me to do investigate that deeper, would I need to check your places.sqlite (privately) to build a list of redirects and try to understand which one may have caused that. Favicons.sqlite could also be useful if that first analysis should fail... You can reach me by mail eventually.
(In reply to Marco Bonardo [::mak] from comment #21)
> It looks like a Mozilla icon. Is it possible somehow you got a redirect from
> a mozilla website to gmail? We had this problem also in the past with the
> previous store, the favicons always supported redirecting bookmarks, up to 2
> levels, but the algorithm cannot distinguish "valid" from "invalid"
> redirects. We could maybe limit it to same host redirects, but that would
> likely cause a different regression where some bookmark will lack a favicon.
> If you wish me to do investigate that deeper, would I need to check your
> places.sqlite (privately) to build a list of redirects and try to understand
> which one may have caused that. Favicons.sqlite could also be useful if that
> first analysis should fail... You can reach me by mail eventually.

Doesn't seem likely. Outside bugzilla and AMO (which have different icons) I don't visit mozilla sites too commonly. (Also I don't remember seeing this exact favicon anywhere)
Mailto links are set to open in gmail, but that never was a problem before. Also it doesn't take me to the inbox (https://mail.google.com/mail/u/0/#inbox), which is so-far the only url affected.

What's for sure is that the favicon never gets updated to the proper one.
It looks like one of my posts wasn't sent or got swallowed.
I meant to add that the favicon is only wrong in the back/forward history of the tab, but never the tab icon itself. (I didn't realize this initially)

Something else:
I bookmarked this url: https://www.17track.net/hu
But it never gets a favicon, no matter how many times I re-bookmark it or load the bookmark.
(In reply to avada from comment #23)
> It looks like one of my posts wasn't sent or got swallowed.
> I meant to add that the favicon is only wrong in the back/forward history of
> the tab, but never the tab icon itself. (I didn't realize this initially)

Ah interesting, I couldn't figure out that screenshot was coming from the sessionhistory popup, it was lacking context.
http://searchfox.org/mozilla-central/rev/224cc663d54085994a4871ef464b7662e0721e83/browser/base/content/browser.js#4116
That menu is currently using PlacesUtils.favicons.getFaviconURLForPage, it could use page-icon protocol, but that is unlikely to fix what you see, even if it may be a good idea in general to convert it.
Either it's a problem with redirects, or it's a bug in the CopyFavicon calls in the docshell, maybe it's copying favicons across wrong uris. It's hard to tell without good STR. I will touch this code part in Bug 1337858, but if you could find STR it would be great, since otherwise I don't know what to look at. Regardless, if you find such steps, please file a separate bug.

> Something else:
> I bookmarked this url: https://www.17track.net/hu
> But it never gets a favicon, no matter how many times I re-bookmark it or
> load the bookmark.

They are serving an http icon from https, the favicons service doesn't allow that kind of mixed content for now, so it's expected. They should update their page to serve the icon through https.
(In reply to Marco Bonardo [::mak] from comment #24)
> Either it's a problem with redirects, or it's a bug in the CopyFavicon calls
> in the docshell, maybe it's copying favicons across wrong uris. It's hard to
> tell without good STR. I will touch this code part in Bug 1337858, but if
> you could find STR it would be great, since otherwise I don't know what to
> look at. Regardless, if you find such steps, please file a separate bug.

Sadly, there's not much chance of that, unless I find out where that icon actually originates from.

> 
> They are serving an http icon from https, the favicons service doesn't allow
> that kind of mixed content for now, so it's expected. They should update
> their page to serve the icon through https.

If firefox expects all websites to do this there will be a lot of iconless bookmarks.
The most critical issues have been fixed, so this can now be resolved, the work continues in dependencies of bug 492172.
If you have other issues with the new favicons store that are not tracked as a dependency of bug 492172, please file them apart.
Fixed by dependencies.
Assignee: nobody → mak77
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Awesome!
Thank you very much Marco Bonardo [::mak] for your very hard work!
Status: RESOLVED → VERIFIED
status-firefox55: affected → verified
Whiteboard: [fixed by fixing dependencies]
Summary: Mozilla Firefox Nightly 55.0a1 (2017-04-13) has broken favicons storage for many favicons which are: missing, no proper, no updating etc. → Broken favicons storage for many favicons which are: missing, no proper, no updating etc. after landing patches from bug #977177
(In reply to Marco Bonardo [::mak] from comment #27)
> Fixed by dependencies.

How do I fix favicons that are already wrong. Eg: comment #20
(In reply to avada from comment #29)
> (In reply to Marco Bonardo [::mak] from comment #27)
> > Fixed by dependencies.
> 
> How do I fix favicons that are already wrong. Eg: comment #20

Since i don't know why that icon is wrong and I don't have a db to investigate, I can't give an answer.
You could just try revisiting the page and it could be replaced by the proper icon.
(In reply to Marco Bonardo [::mak] from comment #30)
> Since i don't know why that icon is wrong and I don't have a db to
> investigate, I can't give an answer.
> You could just try revisiting the page and it could be replaced by the
> proper icon.

I meant in the generic sense. There's no obvious feature in FF, so do I need to much around in favicons.sqlite.

Also visiting the website (gmail) doesn't help. I did it several times since then.

And what about bookmarks? Those aren't supposed to update their icons on visitations, right? (That would suck, because in a bookmark you want it to remain as it always was.)


Oh yeah, speaking of comment #20 and bookmarks. It looks like it was caused by the bookmark having a wrong icon. So I guess the back froward history of a tab uses the bookmarks icon for whatever reason


The only way I could fix it is to remove the bookmark restart FF run places maintenance and recreate the bookmark again. Quite a lot of hassle.
(In reply to avada from comment #31)
> I meant in the generic sense. There's no obvious feature in FF, so do I need
> to much around in favicons.sqlite.

Yes, we don't plan to introduce a feature that corrupts the users' icons, thus we don't plan to introduce an API to fix that.
I can't exclude bugs exist, but we should try to debug and fix those and then if the fix ends up requiring cleanup code, we'll introduce that.

> And what about bookmarks? Those aren't supposed to update their icons on
> visitations, right? (That would suck, because in a bookmark you want it to
> remain as it always was.)

Yes, bookmarks update their icons on visit, every 7 days. They always did. You don't really want to be stuck with an icon from years ago.

> Oh yeah, speaking of comment #20 and bookmarks. It looks like it was caused
> by the bookmark having a wrong icon. So I guess the back froward history of
> a tab uses the bookmarks icon for whatever reason

Both of those features use the icon associated with the url, there's no concept of a bookmark favicon.

> The only way I could fix it is to remove the bookmark restart FF run places
> maintenance and recreate the bookmark again. Quite a lot of hassle.

As I said, we should identify and fix the cause before thinking how to clean it up. But there's not enough data to do the investigation at this time.
(In reply to Marco Bonardo [::mak] from comment #32)

I see.
One question: After XUL phase-out, will it be possible to have extensions like Bookmark Favicon Changer?( https://sites.google.com/site/sonthakit/ ) Which used to work for Firefox.

But there's also a Chrome version, which also doesn't work when installed in Firefox. (via Chrome Store Foxified )
(In reply to avada from comment #33)
> One question: After XUL phase-out, will it be possible to have extensions
> like Bookmark Favicon Changer?( https://sites.google.com/site/sonthakit/ )
> Which used to work for Firefox.

It's possible to use the favicons API to set a never-expiring favicon for a url, and the bookmark will just use that one.
That's not exposed to WebExtensions atm, someone could try to make an experiment.

> But there's also a Chrome version, which also doesn't work when installed in
> Firefox. (via Chrome Store Foxified )

I don't know how that extension works, I don't think Chrome has an API for favicons. I've read that some of those extensions basically replace the <link rel> tags every time you visit a given page, and that would also work in Firefox.
Not fixed in Nightly 56
All created bookmarks have the same icon
Please file your own bug, thank you.
Attached image wrong favicon 1 (deleted) — 
So nothing changed favicon-wise since FF55? Because that's when this bug was labeled fixed, and I saw no further activity. (I also follow related bugs 977177, 492172)

On the 56 branch I still get annoying wrong favicons in places. Such as the favicon being the youtube favicon which is only embedded on the webpage, or the favicon being the one of the feeds' icons in inoreader. (see attachments)

I also noticed favicons being wrong in the bookmarks.
Flags: needinfo?(Virtual)
Attached image wrong favicon 2 (deleted) — 

    
    

    
  
(In reply to avada from comment #37)
> So nothing changed favicon-wise since FF55? Because that's when this bug was
> labeled fixed, and I saw no further activity. (I also follow related bugs
> 977177, 492172)

Various fixes happened in reality.

> On the 56 branch I still get annoying wrong favicons in places.

We know some add-ons are still able to cause these, probably by replacing/setting "link" elements in a web page.
You case looks very similar to bug 1428751, causes are unknown this far.

It's also possible these icons were set by a legacy add-on, and then we can't do really anything about it, apart from suggesting to cancel favicons.sqlite or wait for bug 418144. We can't know if an icon is valid or not for a page, that's why add-ons should in general not mess with favicons.

Anyway, this bug is fixed and commenting inside it is not useful, file new bugs.
Flags: needinfo?(Virtual)

    
    

    
  
(In reply to Marco Bonardo [::mak] from comment #39)
> (In reply to avada from comment #37)
> > So nothing changed favicon-wise since FF55? Because that's when this bug was
> > labeled fixed, and I saw no further activity. (I also follow related bugs
> > 977177, 492172)
> 
> Various fixes happened in reality.
> 

I see. Where would I find these bugs? Is there a active tracking bug?

    
    

    
  
(In reply to avada from comment #40)
> I see. Where would I find these bugs? Is there a active tracking bug?

The main project is gone, most fixes are just normal bugs.
For example Bug 1449523, Bug 1403829, Bug 1422289, Bug 1401777, Bug 1402373, Bug 1401851,... and there are a more, we fixed a bunch of stuff from 55 on.

    
    

    
  
(In reply to Marco Bonardo [::mak] from comment #41)
> The main project is gone, most fixes are just normal bugs.
> For example Bug 1449523, Bug 1403829, Bug 1422289, Bug 1401777, Bug 1402373,
> Bug 1401851,... and there are a more, we fixed a bunch of stuff from 55 on.

Thanks.
Summary: Broken favicons storage for many favicons which are: missing, no proper, no updating etc. after landing patches from bug #977177 → [meta] Broken favicons storage for many favicons which are: missing, no proper, no updating etc. after landing patches from bug #977177

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: