STR: 1. Start nightly with new profile 2. Open some sites[*1] 3. Add bookmark AR: No favicon in sidebar, bookmarks menu, history menu, panels, ... (tab icon is displayed normally) ER: Favicon there. [*1] Failed for example https://www.yahoo.co.jp/ https://headlines.yahoo.co.jp/list/?m=asahi http://www.tokyo-np.co.jp/ https://translate.google.co.jp/?hl=ja http://www.itmedia.co.jp/ http://www.cpubenchmark.net/cpu_list.php https://forums.virtualbox.org/index.php https://www.amazon.co.jp/ Succeeded for example https://tv.yahoo.co.jp/listings/realtime/ https://www.cnn.co.jp/ http://www.afpbb.com/ https://www.bing.com/translator/ http://distrowatch.com/ https://www.youtube.com/ https://nodejs.org/en/ https://github.com/ mozregression: 6:43.13 INFO: Last good revision: d4a29741c62807f145203dc5689cb74bfbd83daa 6:43.13 INFO: First bad revision: c7133f6730590a3c4f8e6314f9de2023b0c5d652 6:43.13 INFO: Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=d4a29741c62807f145203dc5689cb74bfbd83daa&tochange=c7133f6730590a3c4f8e6314f9de2023b0c5d652

investigating...

Those pages don't have a <link> pointing to a favicon, so we go through tabbrowser.useDefaultIcon and for the tab we use the root /favicon.ico. I moved up the calling chain the Places code storing the icon, because off-hand it made more sense and could avoid some API calls. looks like I must revert that change. 57 is unaffected for now, until we uplift bug 1401777.

Actually, since ContentLinkHandler delays handling of <link>s, this code may now run always, even if the page has its own icon defined. I'm not sure how to prevent that, since we can't know if we are handling a <link> or not (We could annotate stuff on the browser and use timeouts, but that's sort of excessive). In general, all of this code was thought for when the favicons setting was synchronous and we didn't have e10s. Though, it may actually be a positive thing, we indeed wanted to store root icons in any case and this code already does that, so I don't think I'm going to touch it. I will just fix the regression making the root domain code path store the icon in the favicons service.

Comment on attachment 8912779 [details] Bug 1403508 - Tabbrowser is no more storing root favicons in Places for pages not defining an icon. https://reviewboard.mozilla.org/r/184096/#review189268 ::: browser/base/content/tabbrowser.xml:1093 (Diff revision 1) > // do the right thing with about:-style error pages. Bug 453442 > if (!icon && this.shouldLoadFavIcon(documentURI)) { > let url = documentURI.prePath + "/favicon.ico"; > - if (!this.isFailedIcon(url)) > + if (!this.isFailedIcon(url)) { > icon = url; > + this.storeIcon(browser, icon, loadingPrincipal, requestContextID); Just to be explicit, here are the callers of `setIcon` that used to automatically get the behavior of `storeIcon`. https://searchfox.org/mozilla-central/search?q=setIcon&path=browser%2F And with this patch (and bug 1401777), these cases no longer try to `PlacesUIUtils.loadFavicon`: - failed default favicons - swapped browsers (original browser would have) - content process crash - customization mode Those seem reasonable to skip, yeah? ::: browser/modules/ContentLinkHandler.jsm:242 (Diff revision 1) > > this.ContentLinkHandler = { > init(chromeGlobal) { > const faviconLoads = new Map(); > chromeGlobal.addEventListener("DOMLinkAdded", event => { > + dump("DOMLinkAdded"); oops

obviously browser_discovery is unhappy on try, and very happy locally. What a surprise! (In reply to Ed Lee :Mardak from comment #5) > Just to be explicit, here are the callers of `setIcon` that used to > automatically get the behavior of `storeIcon`. yep, that was indeed the original scope, but I somehow missed the useDefaultIcon case.

Ah I got the reason the test is failing, previous tests are likely to add this non existing icon to the failed favicons cache, and then useDefaultIcon would skip it. Just matter of making the test remove the icon from the failed icons cache first.

Pushed by mak77@bonardo.net: https://hg.mozilla.org/integration/autoland/rev/240bd9bd8b3e Tabbrowser is no more storing root favicons in Places for pages not defining an icon. r=Mardak

Backed out for failing toolkit/modules/tests/browser/browser_WebRequest_ancestors.js because it got mid-aired by bug 1305237: https://hg.mozilla.org/integration/autoland/rev/13dcb717827363acb5ab0f83952acf15d70e7269 Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=240bd9bd8b3eb4cda22fb19c3d6d23d2e5c523ce&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Almost all failures are on Windows, but this is on mac: https://treeherder.mozilla.org/logviewer.html#?job_id=133775911&repo=autoland Failure log Windows: https://treeherder.mozilla.org/logviewer.html#?job_id=133786263&repo=autoland 10:17:02 INFO - 1099 INFO TEST-START | toolkit/modules/tests/browser/browser_WebRequest_ancestors.js 10:17:03 INFO - TEST-INFO | started process screenshot 10:17:03 INFO - TEST-INFO | screenshot: exit 0 10:17:03 INFO - Buffered messages logged at 10:17:02 10:17:03 INFO - 1100 INFO Entering test bound test_ancestors_exist 10:17:03 INFO - 1101 INFO onBeforeRequest http://mochi.test:8888/ 10:17:03 INFO - 1102 INFO TEST-PASS | toolkit/modules/tests/browser/browser_WebRequest_ancestors.js | ancestors exists [object] - 10:17:03 INFO - Buffered messages logged at 10:17:03 10:17:03 INFO - 1103 INFO onBeforeRequest http://mochi.test:8888/favicon.ico 10:17:03 INFO - Buffered messages finished 10:17:03 ERROR - 1104 INFO TEST-UNEXPECTED-FAIL | toolkit/modules/tests/browser/browser_WebRequest_ancestors.js | ancestors exists [undefined] - 10:17:03 INFO - Stack trace: 10:17:03 INFO - chrome://mochitests/content/browser/toolkit/modules/tests/browser/browser_WebRequest_ancestors.js:onBeforeRequest:14 10:17:03 INFO - resource://gre/modules/WebRequest.jsm:runChannelListener:841 10:17:03 INFO - resource://gre/modules/WebRequest.jsm:observe:612 10:17:03 INFO - 1105 INFO onBeforeRequest http://mochi.test:8888/favicon.ico

Shane, can you help me with this failure? I suspect that the test for bug 1305237 was built around this regression, and now that I am fixing the regression your test is failing. The difference compared to before is that we were no more fetching the root domain favicon of pages, now we do again. This likely confuses your test because it's an unexpected network fetch.

Fwiw, a simple fix may be to add the icon to the failed favicons cache at the beginning of the test, so we won't try to fetch it.

ehr, sorry, wrong link. this one is ok: https://treeherder.mozilla.org/#/jobs?repo=try&revision=218d2711e5fdd5148e3c12964f4389f26bc5112f

Comment on attachment 8913231 [details] Bug 1403508 - Change browser_WebRequest_ancestors.js so it doesn't matter if we fetch a favicon. https://reviewboard.mozilla.org/r/184632/#review189828

Comment on attachment 8912779 [details] Bug 1403508 - Tabbrowser is no more storing root favicons in Places for pages not defining an icon. https://reviewboard.mozilla.org/r/184096/#review189882 ::: browser/base/content/test/general/browser_discovery.js:6 (Diff revision 4) > > add_task(async function() { > - let rootDir = getRootDirectory(gTestPath); > + let url = "http://mochi.test:8888/browser/browser/base/content/test/general/discovery.html"; > - let url = rootDir + "discovery.html"; > info("Test icons discovery"); > + // Tirst we need to clear the failed favicons cache, since previous tests typo: First

Pushed by mak77@bonardo.net: https://hg.mozilla.org/integration/autoland/rev/aaf93f2c8f1a Tabbrowser is no more storing root favicons in Places for pages not defining an icon. r=Mardak https://hg.mozilla.org/integration/autoland/rev/f1cc3260d807 Change browser_WebRequest_ancestors.js so it doesn't matter if we fetch a favicon. r=mixedpuppy

The backout of bug 1305237 (backed out for causing bug 1403932) deleted browser_WebRequest_ancestors.js, so your second commit is gone now. Just a heads up.

Ok, thank you!

https://hg.mozilla.org/mozilla-central/rev/aaf93f2c8f1a https://hg.mozilla.org/mozilla-central/rev/f1cc3260d807

The fix has been uplifted as part of bug 1401777.

Hello, this doesn't seem to be fixed for me for bookmarks. It has been fixed for tabs.

(In reply to Furai from comment #27) > Hello, > > this doesn't seem to be fixed for me for bookmarks. It has been fixed for > tabs. please ensure you're not in private browsing mode, and that the icon is not appearing. The globe icon just means that page doesn't have an icon in history. That said, if it's not due to these, please file a new separate bug.

Bookmarks (especially ones on the bookmarks toolbar) appear to still use the apple-touch-icon but tabs use the correct favicon now. It's probably this issue: https://bugzilla.mozilla.org/show_bug.cgi?id=1403504

I have reproduced this bug with Nightly 58.0a1 (2017-09-27) on Windows 10 , 64 Bit ! This bug's fix is Verified with latest Beta & Nightly ! Build ID 20171023180840 User Agent Mozilla/5.0 (Windows NT 10.0; WOW64; rv:57.0) Gecko/20100101 Firefox/57.0 Build ID 20171025100449 User Agent Mozilla/5.0 (Windows NT 10.0; WOW64; rv:58.0) Gecko/20100101 Firefox/58.0 [bugday-20171025]

I have reproduced this Bug with Nightly 58.0a1 (2017-09-28) on Ubuntu 16.04 LTS! This bug's fix is Verified with latest Beta & latest Nightly ! Build ID 20171023180840 User Agent Mozilla/5.0 (X11; Linux x86_64; rv:57.0) Gecko/20100101 Firefox/57.0 Build ID 20171025100449 User Agent Mozilla/5.0 (X11; Linux x86_64; rv:58.0) Gecko/20100101 Firefox/58.0

As par comment 30 & comment 31 , I am marking this bug as Verified Fixed !