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.

On the dark theme the FastMail favicon looks really bad. FastMail points at this bug.

mak, would it make sense to just have the favicon protocol be hardcoded to use 32 for now until bug 1347532 allows for custom sizes?

Or maybe actually hardcoding 16? Looks like we try to get the requested icon size or larger. [16, 32, 64, 128, 0].map(s => PlacesUtils.favicons.getFaviconURLForPage(Services.io.newURI("https://mint.intuit.com/"), v => console.log(s, v.spec), s)) 16 "https://mint.intuit.com/favicon.ico" 32 "https://mint.intuit.com/sc/ph7788.3/images/apple_touch_icon.png" 64 "https://mint.intuit.com/sc/ph7788.3/images/apple_touch_icon.png" 128 "https://mint.intuit.com/sc/ph7788.3/images/apple_touch_icon.png" 0 "https://mint.intuit.com/sc/ph7788.3/images/apple_touch_icon.png" 16 "https://www.reddit.com/favicon.ico" 32 "https://www.reddit.com/favicon.ico" 64 "https://www.redditstatic.com/icon.png" 128 "https://www.redditstatic.com/icon.png" 0 "https://www.redditstatic.com/icon.png" 16 "https://twitter.com/favicon.ico" 32 "https://abs.twimg.com/favicons/favicon.ico" 64 "https://abs.twimg.com/icons/apple-touch-icon-192x192.png" 128 "https://abs.twimg.com/icons/apple-touch-icon-192x192.png" 0 "https://abs.twimg.com/icons/apple-touch-icon-192x192.png" 16 "https://en.wikipedia.org/favicon.ico" 32 "https://en.wikipedia.org/favicon.ico" 64 "https://en.wikipedia.org/static/apple-touch/wikipedia.png" 128 "https://en.wikipedia.org/static/apple-touch/wikipedia.png" 0 "https://en.wikipedia.org/static/apple-touch/wikipedia.png" 16 "https://cdn-static-1.medium.com/_/fp/icons/favicon-rebrand-medium.3Y6xpZ-0FSdWDnPM3hSBIA.ico" 32 "https://cdn-static-1.medium.com/_/fp/icons/favicon-rebrand-medium.3Y6xpZ-0FSdWDnPM3hSBIA.ico" 64 "https://cdn-images-1.medium.com/fit/c/304/304/1*W0nmth_X8nFKjn6BZ388UQ.png" 128 "https://cdn-images-1.medium.com/fit/c/304/304/1*W0nmth_X8nFKjn6BZ388UQ.png" 0 "https://cdn-images-1.medium.com/fit/c/304/304/1*W0nmth_X8nFKjn6BZ388UQ.png" 16 "https://www.youtube.com/favicon.ico" 32 "https://www.youtube.com/yts/img/favicon_144-vfliLAfaB.png" 64 "https://www.youtube.com/yts/img/favicon_144-vfliLAfaB.png" 128 "https://www.youtube.com/yts/img/favicon_144-vfliLAfaB.png" 0 "https://www.youtube.com/yts/img/ringo/img/favicon_144-vfliLAfaB.png" 16 "https://s1.wp.com/i/favicon.ico?v=1447321881" 32 "https://s1.wp.com/i/favicon.ico?v=1447321881" 64 "https://0.gravatar.com/blavatar/653166773dc88127bd3afe0b6dfe5ea7?s=114&d=http%3A%2F%2Fs0.wp.com%2Fi%2Fwebclip.png" 128 "https://0.gravatar.com/blavatar/653166773dc88127bd3afe0b6dfe5ea7?s=114&d=http%3A%2F%2Fs0.wp.com%2Fi%2Fwebclip.png" 0 "https://0.gravatar.com/blavatar/653166773dc88127bd3afe0b6dfe5ea7?s=114&d=http%3A%2F%2Fs0.wp.com%2Fi%2Fwebclip.png"

Some more info from duped bug #1401715 > (curtisj44 wrote in bug #1401777 comment #2) >> I believe the actual bug is that the `apple-touch-icon` is being used as the >> favicon (when it's defined) instead of the `shortcut-icon`. >> >> I've created a reduced test case here: >> >> - https://codepen.io/curtisj44/pen/GMqoeN >> - https://s.codepen.io/curtisj44/debug/GMqoeN/XxrVwDeRYmxA >> >> In this example, the `shortcut-icon` is a cat and the `apple-touch-icon` is >> a burger. The burger is used as the favicon. >> >> >> Using: Firefox Nightly v57.0a1 (2017-09-20) (64-bit) (macOS Sierra 10.12.6)

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.

Basically the problem is here http://searchfox.org/mozilla-central/rev/2ef8bd8a46a02c68ddbb1d5f25fa254dd7be1fbd/browser/base/content/tabbrowser.xml#993 After we store the icon, we should set the tab image only if it's a "meaningful" icon. We could probably pass down the icon size or some kind of meta information, to tell that it's a rich icon and don't use it for the tab (in general we should serve setIcon only once, or it may flicker).

I also suspect part of the problem is due to the fact we use image-rendering: -moz-crisp-edges; on many favicons, and that causes horrible downscaling: http://searchfox.org/mozilla-central/rev/2ef8bd8a46a02c68ddbb1d5f25fa254dd7be1fbd/browser/base/content/tabbrowser.css#74

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.

Marking this has blocking as it is an obvious visual regression.

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.

Comment on attachment 8910651 [details] Bug 1401777 - don't use rich icons for tabs. https://reviewboard.mozilla.org/r/182090/#review187540 r=Mardak for the js changes. I'm not familiar with the `image-rendering: -moz-crisp-edges;` pieces though. ::: browser/base/content/browser.js:3717 (Diff revision 2) > const {url, description, previewImageURL} = aData; > gBrowser.setPageInfo(url, description, previewImageURL); > return true; > }, > > - setIcon(aBrowser, aURL, aLoadingPrincipal) { > + setIcon(aBrowser, aURL, aLoadingPrincipal, aCanUseForTab = true) { kinda useless nit: just a few lines above `setPageInfo(aData) {` takes a whole data object that is then destructured ;) Could potentially `setIcon(aBrowser, {url, loadingPrincipal, canUseForTab = true} = {})` Ah nevermind, it doesn't have our `a` naming convention. :p

Comment on attachment 8910651 [details] Bug 1401777 - don't use rich icons for tabs. https://reviewboard.mozilla.org/r/182090/#review187550 ::: browser/base/content/browser.js:3738 (Diff revision 2) > + } catch (ex) { > + Components.utils.reportError(ex); > + } > + } > + > + if (aCanUseForTab) { Hmm, I think this will break this test http://searchfox.org/mozilla-central/source/browser/base/content/test/general/browser_discovery.js#85-113 mak, can you just take this test out, I'll fix it in https://bugzilla.mozilla.org/show_bug.cgi?id=1401894

(In reply to Nan Jiang [:nanj] from comment #23) > Hmm, I think this will break this test > http://searchfox.org/mozilla-central/source/browser/base/content/test/ > general/browser_discovery.js#85-113 Thanks, I fixed the test listening just for the ipc message, so that it also partially covers bug 1401894 since I check canUseForTab in the message. Also fixes browser_favicon_change.js that was listening to previous unrelated changes.

There is still one failure in browser/components/uitour/test/browser_UITour_availableTargets.js where the list of targets is missing "selectedTabIcon" that's indeed not exactly deterministic: http://searchfox.org/mozilla-central/rev/2ef8bd8a46a02c68ddbb1d5f25fa254dd7be1fbd/browser/components/uitour/test/browser_UITour_availableTargets.js#118

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)

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. hg error in cmd: hg rebase -s 3dfc2cc7d325 -d af867bbd41ee: rebasing 422013:3dfc2cc7d325 "Bug 1401777 - don't use rich icons for tabs and remove crisp filters making hi res icons look blocky. r=Mardak" (tip) merging browser/base/content/browser.js merging browser/base/content/tabbrowser.xml merging browser/modules/ContentLinkHandler.jsm warning: conflicts while merging browser/base/content/browser.js! (edit, then use 'hg resolve --mark') warning: conflicts while merging browser/base/content/tabbrowser.xml! (edit, then use 'hg resolve --mark') warning: conflicts while merging browser/modules/ContentLinkHandler.jsm! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)

(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.

Pushed by mak77@bonardo.net: https://hg.mozilla.org/integration/autoland/rev/9c0e1bf558e6 don't use rich icons for tabs and remove crisp filters making hi res icons look blocky. r=Mardak

(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.

hm, I forgot to update the commit message, oh well. let's remember to fix it on uplift.

Backed out for failing browser_discovery.js: https://hg.mozilla.org/integration/autoland/rev/22e4997cd797715f1ff4221aeff5f5ace94bf9de Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=132681058&repo=autoland [task 2017-09-22T12:45:22.835Z] 12:45:22 INFO - TEST-PASS | browser/base/content/test/general/browser_discovery.js | data: URIs work - [task 2017-09-22T12:45:22.842Z] 12:45:22 INFO - Buffered messages logged at 12:45:17 [task 2017-09-22T12:45:22.844Z] 12:45:22 INFO - TEST-PASS | browser/base/content/test/general/browser_discovery.js | type may have optional parameters (RFC2046) - [task 2017-09-22T12:45:22.847Z] 12:45:22 INFO - TEST-PASS | browser/base/content/test/general/browser_discovery.js | Rich icons cannot be used for tabs - [task 2017-09-22T12:45:22.849Z] 12:45:22 INFO - TEST-PASS | browser/base/content/test/general/browser_discovery.js | apple-touch-icon discovered - [task 2017-09-22T12:45:22.854Z] 12:45:22 INFO - Buffered messages finished [task 2017-09-22T12:45:22.856Z] 12:45:22 INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_discovery.js | apple-touch-icon-precomposed discovered - [task 2017-09-22T12:45:22.864Z] 12:45:22 INFO - Stack trace: [task 2017-09-22T12:45:22.866Z] 12:45:22 INFO - chrome://mochitests/content/browser/browser/base/content/test/general/browser_discovery.js:runRichIconDiscoveryTest/<:111 Also update the commit message.

The changes are limited to the test, I decided to just refactor it using "modern" utils and avoiding CPOWs.

Pushed by mak77@bonardo.net: https://hg.mozilla.org/integration/autoland/rev/698252497ed8 don't use rich icons for tabs. r=Mardak

Backed out for (frequently?) failing browser-chrome's browser/base/content/test/general/browser_discovery.js: https://hg.mozilla.org/integration/autoland/rev/076a7c57b868064dc778ae77481d8245ed5b4491 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=698252497ed8387d10de995262938316faaf41de&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=132952783&repo=autoland 12:28:18 INFO - TEST-PASS | browser/base/content/test/general/browser_discovery.js | data: URIs work - 12:28:18 INFO - Console message: [JavaScript Warning: "Sending message that cannot be cloned. Are you trying to send an XPCOM object?" {file: "resource://testing-common/ContentTask.jsm" line: 81}] 12:28:18 INFO - TEST-PASS | browser/base/content/test/general/browser_discovery.js | type may have optional parameters (RFC2046) - 12:28:18 INFO - Console message: [JavaScript Warning: "Sending message that cannot be cloned. Are you trying to send an XPCOM object?" {file: "resource://testing-common/ContentTask.jsm" line: 81}] 12:28:18 INFO - Buffered messages finished 12:28:18 INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_discovery.js | apple-touch-icon discovered - 12:28:18 INFO - Stack trace: 12:28:18 INFO - chrome://mochitests/content/browser/browser/base/content/test/general/browser_discovery.js:iconDiscovery:77 12:28:18 INFO - Console message: [JavaScript Warning: "Sending message that cannot be cloned. Are you trying to send an XPCOM object?" {file: "resource://testing-common/ContentTask.jsm" line: 81}]

Hm, my try was completely green with 5 retries!

oops, there was a difference with the try build, I edited a "reject" to "reject()".

https://hg.mozilla.org/integration/autoland/rev/698252497ed8#l3.141 + try { + let msg = await Promise.race([promiseMessage, + new Promise((resolve, reject) => setTimeout(reject(), 2000))]); Probably meant `setTimeout(reject,` without `()`s

Pushed by mak77@bonardo.net: https://hg.mozilla.org/integration/autoland/rev/6b44b9c6114a don't use rich icons for tabs. r=Mardak

Backed out for frequent failures in browser/base/content/test/general/browser_discovery.js on Linux: https://hg.mozilla.org/integration/autoland/rev/7aadb083180354bf09741ea357db4428697cbe26 Push with 2 failing jobs: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=778b195af350a9228226e3c3d525a2aad68eaf72&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=133097060&repo=autoland [task 2017-09-25T12:22:37.214Z] 12:22:37 INFO - TEST-PASS | browser/base/content/test/general/browser_discovery.js | Rich icons cannot be used for tabs - [task 2017-09-25T12:22:37.217Z] 12:22:37 INFO - TEST-PASS | browser/base/content/test/general/browser_discovery.js | apple-touch-icon discovered - [task 2017-09-25T12:22:37.219Z] 12:22:37 INFO - TEST-PASS | browser/base/content/test/general/browser_discovery.js | Rich icons cannot be used for tabs - [task 2017-09-25T12:22:37.222Z] 12:22:37 INFO - TEST-PASS | browser/base/content/test/general/browser_discovery.js | apple-touch-icon-precomposed discovered - [task 2017-09-25T12:22:37.226Z] 12:22:37 INFO - Buffered messages finished [task 2017-09-25T12:22:37.231Z] 12:22:37 INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_discovery.js | fluid-icon discovered - [task 2017-09-25T12:22:37.235Z] 12:22:37 INFO - Stack trace: [task 2017-09-25T12:22:37.239Z] 12:22:37 INFO - chrome://mochitests/content/browser/browser/base/content/test/general/browser_discovery.js:iconDiscovery:79 https://treeherder.mozilla.org/logviewer.html#?job_id=133075631&repo=autoland has > browser/base/content/test/general/browser_discovery.js | apple-touch-icon-precomposed discovered

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.

Duh, that's annoying. Agreed, let's disable this rich icon test here, I'll rewrite it in bug 1401894.

(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.

Pushed by mak77@bonardo.net: https://hg.mozilla.org/integration/autoland/rev/c7133f673059 don't use rich icons for tabs. r=Mardak

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]:

Note, there's a regression, the patch in bug 1403508 will also be necessary for this uplift.

Comment on attachment 8910651 [details] Bug 1401777 - don't use rich icons for tabs. Fix the favicon, taking it. Should be in 57b4

(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.

This improved sessionrestore test results on multiple desktop platforms: == Change summary for alert #9711 (as of September 28 2017 06:54 UTC) == Improvements: 2% sessionrestore_many_windows linux64 pgo e10s 1,607.04 -> 1,573.83 1% sessionrestore_many_windows windows10-64 pgo e10s3,528.42 -> 3,505.67 1% sessionrestore_many_windows windows7-32 pgo e10s 3,527.04 -> 3,506.58 For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=9711

(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.

The issue persists in 57.0b4. Is that expected?

Yes. It will be fixed in 57.0b5.

(In reply to Marco Bonardo [::mak] from comment #61) > 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]: Setting qe-verify- based on Marco's assessment on manual testing and the fact that there's a clear intent for this to have automated coverage via Bug 1352459.

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)

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.

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.

(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.