Closed Bug 1403508 Opened 7 years ago Closed 7 years ago

Some sites' favicon is not displayed as bookmarks/history icon

Categories

(Firefox :: Bookmarks & History, defect, P1)

Product:
Firefox
Component:
Bookmarks & History
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 58
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox56 --- unaffected
firefox57 --- verified
firefox58 --- verified

People

(Reporter: euthanasia_waltz, Assigned: mak)

References

Details

(Keywords: regression, Whiteboard: [fxsearch])

Attachments

(2 files)

Bug 1403508 - Tabbrowser is no more storing root favicons in Places for pages not defining an icon.
59 bytes, text/x-review-board-request
Mardak
: review+
Details
Bug 1403508 - Change browser_WebRequest_ancestors.js so it doesn't matter if we fetch a favicon.
59 bytes, text/x-review-board-request
mixedpuppy
: review+
Details 
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...
Assignee: nobody → mak77
Blocks: 1401777
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Keywords: regression
Priority: -- → P1
Whiteboard: [fxsearch]
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.
status-firefox57: --- → unaffected
status-firefox58: --- → affected
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
Attachment #8912779 - Flags: review?(edilee) → review+
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
Flags: needinfo?(mak77)
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.
Flags: needinfo?(mak77) → needinfo?(mixedpuppy)
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.
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
Attachment #8913231 - Flags: review?(mixedpuppy) → review+
Flags: needinfo?(mixedpuppy)
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.
Flags: needinfo?(mak77)
Ok, thank you!
Flags: needinfo?(mak77)
The fix has been uplifted as part of bug 1401777.
status-firefox57: unaffected → fixed
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
QA Whiteboard: [bugday-20171025]
As par comment 30 & comment 31 , I am marking this bug as Verified Fixed !
Status: RESOLVED → VERIFIED
status-firefox57: fixed → verified
status-firefox58: fixed → verified

I still have this bug, site example: https://preview-algo-website.netlify.com/

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

Attachment

General

Creator:
Created:
Updated:
Size: