1359487 - The root domain icon is preferred if it has an higher resolution than the local page favicon

Status: NEW

(Toolkit :: Places, defect, P3)

Description

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

STR:
1. Open this website page:
- http://amvnews.ru/
2. Bookmark it
3. See that wrong favicon was saved in bookmark



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

Comment 1

[Marco Bonardo [:mak]]

Hm, I cannot reproduce this, I see a blue gradient with AMV text on it. both on the tab and in the bookmark.

How did you add the bookmark? Because IIRC there's an old bug where a new bookmark gets the wrong icon when created through the "New bookmark" contextual menu, but it's only visual since opening a new window shows the right icon.

Comment 2

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Attachment: 1.png

Comment 3

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Attachment: 2.png

Comment 4

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Attachment: 3.png

Comment 5

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Looks like I missed some steps.

STR:
1. Open this URL - http://amvnews.ru/ (See Attachments in Comment #2)
2. Bookmark it (See Attachments in Comment #2)
3. Open HTTPS version of that website page - https://amvnews.ru/ [unfortunately there's certificate error and site isn't configured in HTTPS] (See Attachments in Comment #3)
you can bookmark it or not, but it's not needed to reproduce it, only visit is needed
4. go back to http://amvnews.ru/ and see that wrong favicon (from HTTPS) is saved in bookmark (in HTTP) (See Attachments in Comment #4)

Comment 6

[Marco Bonardo [:mak]]

Ah, I see. The problem is that the invalid https site defines a root icon (at https://amvnews.ru/favicon.ico) and the new store considers that valid also for other pages under amvnews.ru. Moreover the root icon has better resolution than the local one.
Fixing the website should be trivial (and the new icon would appear a week later, when the previous one is expired), while knowing if an icon is valid or something that a webmaster forgot to update/remove is not, we just can't know.

It's possible bug 1347532 will help here, since given a size, we'll prefer the site defined icon to a same-size root icon. What we do currently, without a size ref, is just picking the biggest icon out of the bunch.

Another possibility would be to change FetchIconPerSpec so that it always prefers local icons. There is one disadvantage to that though, if a page has a nice hi-res root domain icon, and a lo-res local icon, we'd pick the lo-res one, just because it's local to that specific url.

I'm calling this a P3, since both approach have pro and cons and it's unclear which of the 2 would work better.
We'll collect more feedback about that and see. This is something we can change at any time, we'll still store the icons, the only thing to touch is the selection logic.

I'm also not calling this a regression, since the new logic was deliberate, we could change it, but it would still be a conscious decision, not a mistake or an overlooked case.

Comment 7

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

[Tracking Requested - why for this release]: Regression



Hmm, it's the regression for sure from user point of view (at last mine ;p ),
as proper HTTP favicon is only fetched and downloaded, but not saved to bookmarked HTTP website page, only after visiting the HTTPS version, if you won't visit HTTPS version, everything is fine.

Please see Attachment in Comment #8.

Comment 8

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Attachment: bug.png

Comment 9

[Marco Bonardo [:mak]]

Please don't override the decisions of the module owner. You're clearly free to confute and discuss them.
We took a "precision hit" to get a tangible performance and resolution advantage. The situation will improve with further work, as I said, but it was a deliberate choice and that work is not planned for 55 at the moment.
An expected behavioral change is not a regression, it's a change. For example moving the sidebar to the right is a choice, not a regression, then it could be a right or a wrong choice, no doubt.
Thank you for your help.

Comment 10

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

(In reply to Marco Bonardo [::mak] from comment #9)
> We took a "precision hit" to get a tangible performance and resolution
> advantage. The situation will improve with further work, as I said, but it
> was a deliberate choice and that work is not planned for 55 at the moment.

Awesome!
Thank you very much for your very hard work!
Especially on bugs dependent on bug #1356525, which fixed loosing favicons after migrating to Firefox 55 from Firefox 54.

Comment 11

[Marco Bonardo [:mak]]

Changing blocking bugs, this is not an indicator of a bug, but a deliberate behavioral change, we'll evaluate it in the more global hi res favicons context, than a single regression bug about broken favicons storage.

Comment 14

[Mahks]

Problem I am having is with localhost XAMPP icons getting preference over custom icons used for sites.

If the C:\xampp\htdocs\favicon.ico is higher resolution than the custom icon the custom one is ignored.
One winds up with all localhost bookmarks being the Xampp icon rather than the icon specified by the site.

The icon on the tab remains correct.

Comment 15

[Masatoshi Kimura [:emk]]

I can no longer reproduce it because now https://amvnews.ru/ returns the same favicon as http://amvnews.ru/.

Comment 17

[BugBot [:suhaib / :marco/ :calixte]]

In the process of migrating remaining bugs to the new severity system, the severity for this bug cannot be automatically determined. Please retriage this bug using the new severity system.

Comment 18

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:mak, could you have a look please?

For more information, please visit BugBot documentation.

Comment 20

[BugBot [:suhaib / :marco/ :calixte]]

The severity field for this bug is set to S4. However, the following bug duplicate has higher severity:

• Bug 1371607: S3

:mak, could you consider increasing the severity of this bug to S3?

For more information, please visit BugBot documentation.