Closed Bug 1359456 Opened 8 years ago Closed 8 years ago

Favicons are not shown for urls with a ref

Categories

(Toolkit :: Places, defect, P1)

55 Branch
x86_64
Windows 7
defect

Tracking

()

VERIFIED FIXED
mozilla55
Tracking Status
firefox-esr45 --- unaffected
firefox-esr52 --- unaffected
firefox53 --- unaffected
firefox54 --- unaffected
firefox55 + verified

People

(Reporter: Virtual, Assigned: mak)

References

Details

(Keywords: nightly-community, regression, reproducible, Whiteboard: [fxsearch])

Attachments

(3 files)

STR: 1. Open these website pages: - https://www.mozilla.org/en-US/firefox/channel/desktop/#nightly - https://www.googleultron.com/ - https://www.openstreetmap.org/#map=5/51.500/-0.100 - http://wikimapia.org/#lang=pl&lat=50.000000&lon=20.000000&z=12&m=b - https://www.metadefender.com/#!/scan-file - https://www.waterfoxproject.org/ 2. Bookmark them all 3. See that no favicons were saved in bookmarks Tested with Mozilla Firefox Nightly 55.0a1 (2017-04-25) (64-bit) [Portable] with clean new fresh profile without any addons (extensions, plugins, themes, etc.) Works fine with Mozilla Firefox 53 (64-bit) [Portable] with clean new fresh profile without any addons (extensions, plugins, themes, etc.) "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
Flags: needinfo?(mak77)
Has Regression Range: --- → yes
Has STR: --- → yes
Whiteboard: [fxsearch]
[Tracking Requested - why for this release]: Regression
Tracking 55+ for this visible regression.
Investigating. For now I found that the waterfox icon is 192x192, and seems to use a lot of colors, thus it goes over the current limit (35840 bytes), it's indeed 38KiB. Same for the GoogleUltron page, it's 256x256, taking 54KiB. I think we can increase the limit, we are still in the collecting info phase. We'll make sure to add telemetry in bug 1337409 about favicon sizes. Additionally, if the icon doesn't fit, we could probably pick the next smaller size and rescale it, instead of throwing it away. The other icons have a ref, and the page-icon protocol is doing the wrong thing by ignoring it. I'm actually wondering whether we should just fix the protol, or make the service always strip ref from urls, since different refs won't mean different icons. I must check whether there would be no issue with having uris not match moz_places.
Assignee: nobody → mak77
Flags: needinfo?(mak77)
Priority: -- → P1
Status: NEW → ASSIGNED
Depends on: 1360477
I split the icon size problem to bug 1360477, here I'll handle the ref problem.
No longer depends on: 1360477
Summary: Favicons are not saved in bookmarks on some website pages after landing patches from bug #977177 → Favicons are not shown for urls with a ref
While it would be nice to remove the ref from urls, as I said at the end of comment 5, since it would allow to store a single url entry for multiple refs, we need something to map 1:1 moz_places entries to moz_pages_w_icons entries. That is currently the url. We don't use the place id because the 2 databases could be mixed up by the users and then icons would be associated to the wrong urls. Doing that requires a lot more brainstorming, I currently don't have a solid idea on how to workaround the missing relations. Thus, I think that for now I'll just fix the protocol to point to the right url.
Comment on attachment 8862927 [details] Bug 1359456 - Page-icon protocol is wrongly removing any ref from the page url. https://reviewboard.mozilla.org/r/134810/#review137858
Attachment #8862927 - Flags: review?(adw) → review+
Pushed by mak77@bonardo.net: https://hg.mozilla.org/integration/autoland/rev/5e38d248ebc4 Page-icon protocol is wrongly removing any ref from the page url. r=adw
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Thank you very much Marco Bonardo [::mak]! \o/ I'm marking this bug as VERIFIED, as it's fixed, starting from Mozilla Firefox Nightly 55.0a1 (2017-04-30).
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: