Status

()

The Mozilla Toolkit is a set of APIs, built on top of Gecko, which provide advanced services to XUL applications. These services include Profile Management, Chrome Registration, Browsing History, Extension and Theme Management, Application Update Service, and Safe Mode. (More info)

Something seems to have made many favicon start looking bad in Nightly within the last few days -- I suspect bug 1352459, but didn't actually check for a specific regression.
Attached is a screenshot comparing a current Nightly (9/20) with one from 9/18, showing a number of common sites. Notice how the icons at the top are consistent with the icons in Chrome (at the bottom) -- all look pretty good, and seem to be appropriately sized for my Retina display.
But in the middle screenshot, they all appear pixelated and awkwardly (re?)sized. A number are obviously completely different images, so I assume whatever's happening we've switched from using images tuned to the display size to other (older?) images that are being rescaled.

(I don't know why Chrome has a nicer YouTube favicon than both Nightly versions, so I assume that's some other bug and can be ignored here. I think the site's been changing recently, as I've previously noticed different YouTube icons showing up in the awesomebar.)

I'm seeing this in Linux Nightly as well. (I particularly noticed that the Twitter icon is now awkwardly smaller than most of my other favicons.)
Confirmed via mozregression that this behavior changed as of bug 1401777. (That bug's cset is where the Twitter bird & Wikipedia "W"-character got smaller in their favicon, and where the Medium changed to the green "M" instead of the black "M".)
[Tracking Requested - why for this release]: regression in 57

(In reply to Daniel Holbert [:dholbert] from comment #2)
> Confirmed via mozregression that this behavior changed as of bug 1401777.
er, meant to say "...as of bug 1352459" (i.e. this manifests as a regression from that bug)

mak expected this bug in bug 1352459 comment 49. He said there bug 1347532 will help. Looks like various places are just using the highest resolution icon available by default.
I've attached a screenshot of mint.com where the high res icon is something I've never seen on mint before. I first noticed it on activity stream, so it's easy to spot when it's used elsewhere too -- in this case the autocomplete.

I believe those icons are downscaled rich icons.
Bug 1352459 collects both rich icons and regular ones, looks like the frontend code picked the rich one and downscaled it to 16x16 in this case. Bug 1347532, which lets the frontend choose the better favicon, will improve this.

Here's a screenshot showing how this looks for my pinned twitter tab now. The bird is awkwardly small -- in pre-regression builds, it's a good bit larger.
From looking at twitter's source, I'm guessing the old icon was:
https://abs.twimg.com/favicons/favicon.ico
...whereas the new icon is:
https://abs.twimg.com/icons/apple-touch-icon-192x192.png
Notably: in the old icon (favicon.ico), the blue bird is nearly the full height + width of the image. Whereas in the "touch" icon, there's a bunch more padding at the edges of the image. That might make it better for an "app button" or something like that, but it makes it a much less suitable tiny-favicon...
> mak expected this bug in bug 1352459 comment 49. He said there bug 1347532
> will help. Looks like various places are just using the highest resolution
> icon available by default.
It sounds like that'll make us prefer twitter's 32x32 favicon.ico over its 192x192 apple-touch-icon-192x192.png for the tabstrip -- if so, that seems good.

There are 2 different paths for favicons.
1. The favicon service handles all the favicon requests in the UI, BUT the tabs. Those are likely to pick the rich icon until bug 1347532 (and eventual foolow-ups) will be fixed.
2. the tab directly get the favicon from setIcon in tabbrowser.
The regression I was expecting was in bookmarks and similar views, the tab icon regression is unexpected. Looks like tabbrowser should not use rich icons, even if those should be stored in Places.
Whether we can default to 32px until bug 1347532 is fixed is a good question, we could indeed (and then AS should pass its own frame size ref or request to the API), but that wouldn't fix the tab's icon, because the code path is not the same.

Finally, I'd suggest to invert the order we tore favicons from ContentLinkHandler, so that we store rich icons first, other icons later, so that the latter can eventually overwrite the former for certain sizes.

In the patch I'm removing the crips-edges from most places using favicons (I retained them in places we don't control like search engine icons). I think it may be the time, we are storing better icons from 55, and we started getting bugs showing that it does more harm than benefit. Sure, it's possible some icons will look blurry, but at the same time some other icons will stop looking blocky. The rich icons patch has the advantage to store even more hi dpi icons, so it sounds like in the end crips-edges will do more damage than benefit. We should continue working on tweaking the ContentLinkHandler algo to store better icons in the 58 cycle.
I am filing a separate bug to add a test that checks we don't pick mask favicons and that we don't use a rich favicon for the tab, so there's a bit more time to work on it.

Comment on attachment 8910651[details]Bug 1401777 - don't use rich icons for tabs.
https://reviewboard.mozilla.org/r/182090/#review187516
Not sure if this is desired or not, but if we want to ensure all tabs get a favicon even if they only provide rich icons, `setIconForLink` probably needs to pass in the flag as opposed to getting `isRichIcon` from the icon object. E.g.,
```js
if ((preferredIcon || defaultIcon) && largestRichIcon) {
setIconForLink(largestRichIcon, aChromeGlobal, true /* isRichIcon */);
}
// Set the icon for the tab
setIconForLink(preferredIcon || defaultIcon || largestRichIcon, aChromeGlobal, false /* pretend not rich even if rich */);
```
::: browser/modules/ContentLinkHandler.jsm:117
(Diff revision 1)
> function setIconForLink(aIconInfo, aChromeGlobal) {
> aChromeGlobal.sendAsyncMessage(
> "Link:SetIcon",
> - { url: aIconInfo.iconUri.spec, loadingPrincipal: aIconInfo.loadingPrincipal });
> + { url: aIconInfo.iconUri.spec,
> + loadingPrincipal: aIconInfo.loadingPrincipal,
> + isRichIcon: aIconInfo.isRichIcon,
There is a behavior change here compared to before the rich icon patch. Sites that were using high res icons as their "icon" would no longer show that icon in the tab. Might be okay unless the site doesn't have a non-rich icon.
The "icon" gets "upgraded" here:
https://searchfox.org/mozilla-central/source/browser/modules/ContentLinkHandler.jsm#190-194

(In reply to Ed Lee :Mardak from comment #19)
> Not sure if this is desired or not, but if we want to ensure all tabs get a
> favicon even if they only provide rich icons
This is an interesting outcome, but note we never supported rich icons in tabs, thus not having an icon in that rare case wouldn't be a regression.
> ::: browser/modules/ContentLinkHandler.jsm:117
> There is a behavior change here compared to before the rich icon patch.
> Sites that were using high res icons as their "icon" would no longer show
> that icon in the tab. Might be okay unless the site doesn't have a non-rich
> icon.
Ok, I guess that code needs to move down so that isRichIcon is actually trustable.
so basically if (icon.isRichIcon) { becomes if (icon.isRichIcon || icon.width > FAVICON_RICH_ICON_MIN_WIDTH) {
I can do both changes.

Dolske, I set the review on you mostly for the crisp removal decision. We have those from some time, so it's not a trivial "peer" decision imo, but I think they made their time, with support for hi dpi and AS patches to store svg and ico more often, we could end up in a situation where sub-dpi icons will look blocky-blurry while hi-dpi icon will look blocky. there's no win.
Without the crisp rules we are back to sub-dpi icons being blurry, but the others would look fine (maybe downscaled, but not blocky).
Apart from that decision, feel free to redirect the request.

(In reply to Marco Bonardo [::mak] from comment #18)
> In the patch I'm removing the crips-edges from most places using favicons (I
> retained them in places we don't control like search engine icons). I think
> it may be the time, we are storing better icons from 55, and we started
> getting bugs showing that it does more harm than benefit. Sure, it's
> possible some icons will look blurry, but at the same time some other icons
> will stop looking blocky.
This was added in bug 1041845 (but I suspect even earlier, for the original Retina support, but my hg archaeology is failing me).
The basic issue is that upscaling 16x16 favicons usually results in a blurry mess, which is quite noticeable when the rest of the UI is sharp and crisp on a hidpi display. (This is the same basic issue why hidpi support was so important to do in the first place, and why legacy apps that just get the whole window upscaled look surprisingly bad.)
That this -moz-crisp-edges gets applied to favicons that are *downscaled* is unintentional. Originally this wasn't a big deal (as most sites were really only targeting 16x16 and 32x32, with the exception of some sites that slapped in a bigger image and just hoped for the best). But that's probably changing now.
Would you perhaps have a simple code snippet to dump out some info on how often Places now has a > 16x16 favicon available? I'd suspect only having a 16x16 is very very common, but could be wrong.
In bug 1081224 dholbert suggests that "image-rendering: pixelated" will eventually do what we want (same as -moz-crisp-edges when upscaling, but a normal/smooth algorithm when downscaling). But it sounds like that was depending on bug 1072703, which hasn't been fixed yet. And I'll just assume it's too late to fix for 57.
Hmm, do we have enough info in Places now that we could work around this ourselves? e.g., set a "pixelate" attribute on favicons that are only available as 16x16, and add a CSS attribute selector to keep using -moz-crisp-edges in that case? [I forget what we're doing these days with ICO files, for a while we just let imagelib figure out which bitmap to draw from the ICO that was most appropriate for the screen DPI, is that still the case? That wouldn't work well for this.] Or the inverse -- when we know we have a high-res favicon, set a "nopixelate" attribute to suppress the -moz-crisp-edges?

Per IRC discussion with Dolske, I split the crisp change to bug 1402250. We don't have enough data and time to make a call for 57. Still replying inline, even if it's just for future thoughts at this point:
(In reply to Justin Dolske [:Dolske] from comment #31)
> That this -moz-crisp-edges gets applied to favicons that are *downscaled* is
> unintentional. Originally this wasn't a big deal (as most sites were really
> only targeting 16x16 and 32x32, with the exception of some sites that
> slapped in a bigger image and just hoped for the best). But that's probably
> changing now.
Right, it's changing for various reasons:
1. pages care more about touch devices that in general use larger icons
2. we started storing more hi-res favicons
> Would you perhaps have a simple code snippet to dump out some info on how
> often Places now has a > 16x16 favicon available? I'd suspect only having a
> 16x16 is very very common, but could be wrong.
My local database is biased by my special workflow (bugzilla's favicon) so I'm not sure it would be of any value.
We can add telemetry for it in dependencies of bug 1402250.
> Hmm, do we have enough info in Places now that we could work around this
> ourselves?
Surely not for 57. Also, the UI in general doesn't know the size of the image it's going to render until it's rendered. The Places page-icon protocol knows though, but it can't pass out that info in the binary stream... Though it could internally rescale with the the algo we prefer, if we have such util available. This would require adding a PB storage to the favicons service so that it can be used also on tabs, though.
> I forget what we're doing these days with
> ICO files, for a while we just let imagelib figure out which bitmap to draw
> from the ICO that was most appropriate for the screen DPI, is that still the
> case?
Yes it's still the case for the tab, the rest of the ui uses the favicons service, that just pushes out pngs at the required size (currently not set yet, bug 1347532, so it just pushes out the largest one)

(In reply to Mozilla Autoland from comment #34)
> 422013:3dfc2cc7d325 "Bug 1401777 - don't use rich icons for tabs and remove
> crisp filters making hi res icons look blocky. r=Mardak" (tip)
I see this conflicted with bug 1247843. Seems likely we'll want to uplift this bug to Beta 57. The conflicting bug maybe too wants to be uplifted but that has changes to dom/layout/image loading..
If only uplifting this bug, any good way to keep the original attempted autoland that failed in comment 34 for uplift? I suppose mak can just save that commit and repush after pushing a different commit resolving conflicts.

(In reply to Ed Lee :Mardak from comment #35)
> I see this conflicted with bug 1247843. Seems likely we'll want to uplift
> this bug to Beta 57. The conflicting bug maybe too wants to be uplifted but
> that has changes to dom/layout/image loading..
I will directly take care of the uplift when it's the time.

This intermittent is really hard to reproduce and debug, I surely can't reproduce locally, and tens of retriggers on Try didn't trigger it. I suspect GC is involved, either something is GCed at the wrong time, or a long GC causes us to timeout the fetch.
I will just temporarily disable the rich icons part of the tests for now, bug 1401894 will still modify this test, so we can follow-up there, hopefully we can capture some debugging log on Try to understand the intermittent.

(In reply to Nan Jiang [:nanj] from comment #56)
> Agreed, let's disable this rich icon test here, I'll rewrite it in bug
> 1401894.
I don't think it needs a complete rewrite, we need to add a bunch of dumps and try to understand what happens on Try. Or we could change the approach, instead of listening to the message, do something else.

Comment on attachment 8910651[details]Bug 1401777 - don't use rich icons for tabs.
Approval Request Comment
[Feature/Bug causing the regression]: bug 1352459
[User impact if declined]: On some pages the tab may show a rich favicon downscaled by a large amount, that doesn't look really nice
[Is this code covered by automated tests?]: Bug 1401894 has been filed to create one
[Has the fix been verified in Nightly?]: yes (I just did)
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: The worst case is some edge case involving favicons
[String changes made/needed]:

(In reply to Sylvestre Ledru [:sylvestre] from comment #64)
> Fix the favicon, taking it.
> Should be in 57b4
Please, note comment 62. We should uplift the 2 patches together, but the other one just bounced back due to a mid-air with another bug.
This one will also need an unbitrot for beta due to bug 1247843 touching the same code.

My plan is to make a single beta patch including this and bug 1403508. But I first must know if Bug 1247843 will be uplifted or not, since if not, I can just unbitrot this. Otherwise it doesn't make sense to unbitrot both, they should just land in the same order as in Nightly.

(In reply to Ionuț Goldan [:igoldan], Performance Sheriffing from comment #68)
> This improved sessionrestore test results on multiple desktop platforms:
I suspect the improvement is related to the regression and could disappear once we fix it in bug 1403508.

(In reply to Ralf Jung from comment #80)
> The GitLab icon is working fine. However, for some other sites, the icon in
> the bookmarks is not updating to what Firefox now shows in the tab.
The bookmarks currently are not expected to show the same icon as the tab.
(In reply to marian.domanik@gmail.com from comment #81)
> I've already commented here
> https://bugzilla.mozilla.org/show_bug.cgi?id=1403963
>
> I have the exact same issues for some websites, the favicons are not visible
> in tabs even in 57b5 on both Windows and Linux.
If the icon appears SOMETIMES it's not this bug. This bug about the icon NEVER appearing.
I will reopen your bug and ask for more info there.
(In reply to Maxim Nosovets from comment #79)
> Problems with favicons not fixed in 57.0b5
> https://de-v.net/v/2017-10-04_10-45-18.png
> No favicon on Facebook Messenger, Google Search and Google Translate (bug
> #1405253)
I will reopen your bug and ask for more info there, it may be something else.

> The bookmarks currently are not expected to show the same icon as the tab.
For GitLab, the bookmarks (and awesomebar) still use the "bad" icon with the black background.
When you say "not expect", do you mean "this is a known bug (tracked elsewhere) that still needs to be fixed", or "this is behavior is intended"?

(In reply to Ralf Jung from comment #83)
> When you say "not expect", do you mean "this is a known bug (tracked
> elsewhere) that still needs to be fixed", or "this is behavior is intended"?
It's a known bug that should be mostly fixed by bug 1347532 and follow-up work to pick better icons (bug 1403829). But it won't be fixed in 57 because we're out of time for a safe fix.

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