Dave Townsend [:mossop] Assignee
	Description • 6 years ago

When we figure out a favicon for a page we do two things. We ask the favicon service to cache it and we assign it to a xul:image element for display on the tab. This makes us load the same url twice which, even if the cache might help here, is silly.

I have a WIP that has the favicon service always finish the load (larger than would otherwise be allowed) and then pass that out so the xul:image can display it without reloading.

	Marco Bonardo [:mak]
	Comment 1 • 6 years ago

But the favicon service doesn't load from the network in PB mode

	Dave Townsend [:mossop] Assignee
	Comment 2 • 6 years ago

(In reply to Marco Bonardo [::mak] from comment #1)
> But the favicon service doesn't load from the network in PB mode

Where does that happen? I see the network load explicitly set the private bit when in private browsing but nothing about it not loading the icon: https://searchfox.org/mozilla-central/source/toolkit/components/places/FaviconHelpers.cpp#616

	Marco Bonardo [:mak]
	Comment 3 • 6 years ago

Look for canAddToHistory in FaviconHelpers... it's set to false if the url is one that we don't store in history, or in pb mode if the page is not bookmarked. if that is false the service doesn't fetch favicons.
It's obviously possible to change that, but it's not a trivial bool change, apart from inverting the cpp logic, it will also require to fix xpcshell tests that currently don't fetch and will start fetching, because network fetches will fatal assert.

	Marco Bonardo [:mak]
	Comment 4 • 6 years ago

well, actually canAddToHistory and bookmarkedSpec are the ones used.

	Marco Bonardo [:mak]
	Comment 5 • 6 years ago

actually it's possible what you see is the old behavior, indeed just a few days ago I fixed a bug where we were fetching icons from the network even if we didn't store it... At least now some of that code makes more sense, but I pretty much did the opposite, since I didn't knew there was this plan.

	Dave Townsend [:mossop] Assignee
	Comment 7 • 6 years ago

Comment on attachment 8967558 [details]
Bug 1453751: Favicons should only be loaded once.

Ah yes I see. So in my WIP (attached) I already had to bypass one case where wee don't download if the icon is too large. It's possible we could do that for this too (and just not store in that case). Maybe that would be quite complex.

The two problems I'm trying to solve are:

1. We do two favicon load requests. That seems silly.
2. The favicon load from the xul:image tag uses a bit of a hack to get the triggering principal right (that has some problems I found in bug 1297156), a bit of a hack to get the private browsing status and isn't associated with the content's load context and so the webextension proxy API can't see which tab it is for.

I'm open to varying options to solve that. I could do another hack for the loadContext for the xul:image and it would solve my main immediate problem. Gijs suggested that maybe we should be doing the image loading and parsing in the content process for safety but that seems like a lot of work. Or I could attempt to make the favicon service work for this but that sounds like a bunch of work too.

Thoughts?

	Dave Townsend [:mossop] Assignee
	Comment 8 • 6 years ago

Actually, maybe it wouldn't actually be that difficult to just do the load once in some new service, pass it to the xul element and to the favicon service to cache and remove the loading code from the favicon service.

	Marco Bonardo [:mak]
	Comment 9 • 6 years ago

(In reply to Dave Townsend [:mossop] from comment #8)
> Actually, maybe it wouldn't actually be that difficult to just do the load
> once in some new service, pass it to the xul element and to the favicon
> service to cache and remove the loading code from the favicon service.

Yes, that was another possibility we considered in the past, the favicon service could even work by getting a blob, I'd have nothing against that. The main problem honestly has always just been resources to change things.

(In reply to Dave Townsend [:mossop] from comment #7)
> 1. We do two favicon load requests. That seems silly.

IIRC on Windows we are doing 3. Don't jumplists do their own fetch? Maybe I'm not up-to-date with that code though.

> I'm open to varying options to solve that. I could do another hack for the
> loadContext for the xul:image and it would solve my main immediate problem.
> Gijs suggested that maybe we should be doing the image loading and parsing
> in the content process for safety but that seems like a lot of work. Or I
> could attempt to make the favicon service work for this but that sounds like
> a bunch of work too.

Let's start from the principle that all of those sound feasible, with enough time.
Making the favicons service do the load regardless is not a huge amount of work, it pretty much boils down to reverting part of my patch https://hg.mozilla.org/mozilla-central/rev/ad061f4fec73#l2.75
The logic that currently decides whether we fetch will be moved to decide whether we store.
And then figuring out xpcshell-tests that will start to assert, either by starting up httpd or using dataurls.

Probably reading the favicon in content or in a separate module would be a slightly bigger amount of work (because later one should still modify the favicon service to work with blobs) but it sounds like a more future-proof approach, off-hand. I'm just wondering about the perf/mem usage to pass around large blobs via XPCOM, but that's something we are already doing for things like indexedDB, so it must be feasible.

	Marco Bonardo [:mak]
	Comment 10 • 6 years ago

Additionally, one of the reasons the current SetandFetch API code is so complex is the continuous jumps between helper and main threads, because the network code runs in the main thread. If we'd fetch the icon elsewehere, this code could probably be largely simplified and also be a little bit more efficient.

	Dave Townsend [:mossop] Assignee
	Comment 12 • 6 years ago

Comment on attachment 8968722 [details]
Bug 1453751: Load favicons in the content process.

Probably should just be a feedback pass until I run through try. Also there are a couple of questions in here.

	Dave Townsend [:mossop] Assignee
	Comment 13 • 6 years ago

Comment on attachment 8968722 [details]
Bug 1453751: Load favicons in the content process.

Did you still want to look over this? I switched to the loadgroup in the end, though the expiry checking might be worth looking at, the old Favicon code assumed ther cache expiry was from now but it looks like in the child at least it is an absolute timestamp in seconds.

	Dave Townsend [:mossop] Assignee
	Comment 14 • 6 years ago

Comment on attachment 8968722 [details]
Bug 1453751: Load favicons in the content process.

Looks like there is some more work to do here for tests...

	u408661
	Comment 16 • 6 years ago

(In reply to Dave Townsend [:mossop] from comment #13)
> Comment on attachment 8968722 [details]
> Bug 1453751: Load favicons in the content process.
> 
> Did you still want to look over this? I switched to the loadgroup in the
> end, though the expiry checking might be worth looking at, the old Favicon
> code assumed ther cache expiry was from now but it looks like in the child
> at least it is an absolute timestamp in seconds.

FWIW, the necko-related bits of this look good to me. Thanks for switching over to loadgroup!

	Dave Townsend [:mossop] Assignee
	Comment 23 • 6 years ago

Comment on attachment 8968722 [details]
Bug 1453751: Load favicons in the content process.

So I think this is the right first step here. It fixes a lot of things, and breaks a lot of tests. Those tests need fixing, some by disabling favicon loads most likely, some by properly tracking them. But, any change to the ordering of how we do the favicon loads tends to cause different tests to break so I want to get a preliminary review here before I really dig in and fix the individual tests. Obviously this won't land before the tests fixes are also reviewed and I suspect there may be minor follow-up fixes here still but I'd love to hear if there are things I'm missing here.

Note that ImageDocument support is still missing here. Do you think that is required for this to move forwards?

	Marco Bonardo [:mak]
	Comment 24 • 6 years ago

(In reply to Dave Townsend [:mossop] from comment #23)
> Note that ImageDocument support is still missing here. Do you think that is
> required for this to move forwards?

in the sense that the tabs won't show any favicon for image documents, or in the sense you didn't fix bug 403651 (that I would not expect you to fix here). I'm not sure how much we care about the former, it doesn't seem critical, but do we have a plan to fix it?
Fwiw, I just tried in Chrome and Edge, they don't seem to use images as favicons at all, instead they fallback to the root favicon if available, we could do the same, I don't see why we should be the only ones to pay the resampling cost to show something that most likely will be barely recognizable due to rescaling. We could decide bug 403651 is a wontfix and just set the tab favicon for image documents to the root favicon.

	Dave Townsend [:mossop] Assignee
	Comment 25 • 6 years ago

(In reply to Marco Bonardo [::mak] from comment #24)
> (In reply to Dave Townsend [:mossop] from comment #23)
> > Note that ImageDocument support is still missing here. Do you think that is
> > required for this to move forwards?
> 
> in the sense that the tabs won't show any favicon for image documents, or in
> the sense you didn't fix bug 403651 (that I would not expect you to fix
> here). I'm not sure how much we care about the former, it doesn't seem
> critical, but do we have a plan to fix it?
> Fwiw, I just tried in Chrome and Edge, they don't seem to use images as
> favicons at all, instead they fallback to the root favicon if available, we
> could do the same, I don't see why we should be the only ones to pay the
> resampling cost to show something that most likely will be barely
> recognizable due to rescaling. We could decide bug 403651 is a wontfix and
> just set the tab favicon for image documents to the root favicon.

Fixing bug 403651 is probably more doable with this method than previously, but yeah right now it just uses the default favicon.ico from the site for image documents, so I guess chrome-parity!

	Marco Bonardo [:mak]
	Comment 26 • 6 years ago
mozreview-review

Comment on attachment 8968722 [details]
Bug 1453751: Load favicons in the content process.

https://reviewboard.mozilla.org/r/237446/#review247286

It's going the right direction, but it's not trivial to review, thus I may need more passes. Mostly, a few code paths would benefit from a descriptive comment...

::: browser/base/content/browser.js:3723
(Diff revision 7)
>          FeedHandler.addFeed(link, aMsg.target);
>          break;
>  
>        case "Link:SetIcon":
>          this.setIcon(aMsg.target, aMsg.data.url, aMsg.data.loadingPrincipal,
> -                     aMsg.data.requestContextID, aMsg.data.canUseForTab);
> +                     aMsg.data.canUseForTab, aMsg.data.expiration, aMsg.data.dataURL);

While you're doing this, it may be interesting to see how much additional work would require to also fix https://bugzilla.mozilla.org/show_bug.cgi?id=1428751#c30 that pretty much means also passing the page url here, instead of extracting it from the browser pointed by msg.target.
loadFavicon() then would not use browser.currentURI but the passed in url.

And maybe at this point setIcon could just receive the browser and a data objects as input, instead of a ton of arguments.
And aURL should probably be clarified to aIconURL

::: browser/base/content/tabbrowser.js:777
(Diff revision 7)
> -    let requestContextID = aRequestContextID || 0;
>      let sizedIconUrl = browser.mIconURL || "";
> +
> +    // TODO Is this the right set?
> +    if (sizedIconUrl && !sizedIconUrl.startsWith("data:") && !sizedIconUrl.startsWith("chrome:")) {
> +      console.error(`Attempt to set a remote URL ${sizedIconUrl} as a tab icon.`);

And this is what your patch is preventing right? Maybe worth adding an explaining comment above this block.

::: browser/components/places/PlacesUIUtils.jsm:188
(Diff revision 7)
>      let loadType = PrivateBrowsingUtils.isWindowPrivate(win)
>        ? PlacesUtils.favicons.FAVICON_LOAD_PRIVATE
>        : PlacesUtils.favicons.FAVICON_LOAD_NON_PRIVATE;
>      let callback = this._makeCompletionCallback(win, innerWindowID);
> +
> +    if (dataURI) {

The only unfortunate downside, is that we can't remove all the network code from Places... but it's a good first step in the right direction.

::: browser/modules/ContentLinkHandler.jsm:133
(Diff revision 7)
> +}
> +
> +const FaviconLoader = {
> +  // We support sending both a normal and a rich icon to the parent.
> +  normalLoad: null,
> +  richLoad: null,

what are these for? in any case unless this object becomes instantiable these are likely not usable.

::: browser/modules/ContentLinkHandler.jsm:392
(Diff revision 7)
>  }
>  
>  var ContentLinkHandler = {
>    init(chromeGlobal) {
> -    const faviconLoads = new Map();
> +    this.chromeGlobal = chromeGlobal;
> +    this.faviconLoads = new Map();

I was about doing the same thing some time ago, but looks like each tab will invoke this init, overwriting anything stored in this, since the module is shared and we don't create one instance per tab.

::: browser/modules/ContentLinkHandler.jsm:406
(Diff revision 7)
>          load.timer.cancel();
>          load.timer = null;
> -        faviconLoads.delete(pageUrl);
> +        this.faviconLoads.delete(pageUrl);
>        }
>      });
> +    chromeGlobal.addEventListener("DOMContentLoaded", event => {

some comments would help

::: browser/modules/ContentLinkHandler.jsm:419
(Diff revision 7)
> +      }
> +    });
> +
> +    // If favicon displaying is turned off then don't infer icons.
> +    if (!Services.prefs.getBoolPref("browser.chrome.site_icons", true) ||
> +        !Services.prefs.getBoolPref("browser.chrome.guess_favicon", true)) {

what's guess_favicon?

::: browser/modules/ContentLinkHandler.jsm:423
(Diff revision 7)
> +    if (!Services.prefs.getBoolPref("browser.chrome.site_icons", true) ||
> +        !Services.prefs.getBoolPref("browser.chrome.guess_favicon", true)) {
> +      return;
> +    }
> +
> +    Services.obs.addObserver(this, "document-element-inserted");

ditto... Is this for image documents or fetching root favicons regardless?

	Dave Townsend [:mossop] Assignee
	Comment 27 • 6 years ago

(In reply to Marco Bonardo [::mak] from comment #26)
> Comment on attachment 8968722 [details]
> Bug 1453751: Load favicons in the content process.
> ::: browser/base/content/browser.js:3723
> (Diff revision 7)
> >          FeedHandler.addFeed(link, aMsg.target);
> >          break;
> >  
> >        case "Link:SetIcon":
> >          this.setIcon(aMsg.target, aMsg.data.url, aMsg.data.loadingPrincipal,
> > -                     aMsg.data.requestContextID, aMsg.data.canUseForTab);
> > +                     aMsg.data.canUseForTab, aMsg.data.expiration, aMsg.data.dataURL);
> 
> While you're doing this, it may be interesting to see how much additional
> work would require to also fix
> https://bugzilla.mozilla.org/show_bug.cgi?id=1428751#c30 that pretty much
> means also passing the page url here, instead of extracting it from the
> browser pointed by msg.target.
> loadFavicon() then would not use browser.currentURI but the passed in url.
> 
> And maybe at this point setIcon could just receive the browser and a data
> objects as input, instead of a ton of arguments.
> And aURL should probably be clarified to aIconURL

Yeah there's definitely some cleanup to be done here, I'll see if it makes sense to do it here or later.

> ::: browser/components/places/PlacesUIUtils.jsm:188
> (Diff revision 7)
> >      let loadType = PrivateBrowsingUtils.isWindowPrivate(win)
> >        ? PlacesUtils.favicons.FAVICON_LOAD_PRIVATE
> >        : PlacesUtils.favicons.FAVICON_LOAD_NON_PRIVATE;
> >      let callback = this._makeCompletionCallback(win, innerWindowID);
> > +
> > +    if (dataURI) {
> 
> The only unfortunate downside, is that we can't remove all the network code
> from Places... but it's a good first step in the right direction.

Yeah, a future step is just to pass a Blob across with the data, but for now other code passes data urls to places so I didn't want to update all of those.

> ::: browser/modules/ContentLinkHandler.jsm:133
> (Diff revision 7)
> > +}
> > +
> > +const FaviconLoader = {
> > +  // We support sending both a normal and a rich icon to the parent.
> > +  normalLoad: null,
> > +  richLoad: null,
> 
> what are these for? in any case unless this object becomes instantiable
> these are likely not usable.

So the previous code would send a tab icon and a rich icon up to the parent, these track those changes. But you're right, I have forgotten that this stuff is shared across multiple tabs so I need to rework this.

> ::: browser/modules/ContentLinkHandler.jsm:392
> (Diff revision 7)
> >  }
> >  
> >  var ContentLinkHandler = {
> >    init(chromeGlobal) {
> > -    const faviconLoads = new Map();
> > +    this.chromeGlobal = chromeGlobal;
> > +    this.faviconLoads = new Map();
> 
> I was about doing the same thing some time ago, but looks like each tab will
> invoke this init, overwriting anything stored in this, since the module is
> shared and we don't create one instance per tab.

Yep, *sigh*

> ::: browser/modules/ContentLinkHandler.jsm:419
> (Diff revision 7)
> > +      }
> > +    });
> > +
> > +    // If favicon displaying is turned off then don't infer icons.
> > +    if (!Services.prefs.getBoolPref("browser.chrome.site_icons", true) ||
> > +        !Services.prefs.getBoolPref("browser.chrome.guess_favicon", true)) {
> 
> what's guess_favicon?

I should add a comment. The web-platform tests don't anticipate us guessing a "/favicon.ico" that later gets blocked by CSP. This behaviour is not specced at all so I just added this as a way to turn off this behaviour in tests.

> ::: browser/modules/ContentLinkHandler.jsm:423
> (Diff revision 7)
> > +    if (!Services.prefs.getBoolPref("browser.chrome.site_icons", true) ||
> > +        !Services.prefs.getBoolPref("browser.chrome.guess_favicon", true)) {
> > +      return;
> > +    }
> > +
> > +    Services.obs.addObserver(this, "document-element-inserted");
> 
> ditto... Is this for image documents or fetching root favicons regardless?

Fetching the root regardless. It seemed to be the best event I could find where we could figure out the root favicon URL.

	Phabricator Automation
	Comment 28 • 6 years ago

This moves the load of favicons into the content process. We use the same logic
for finding favicons (based on waiting until none have shown up for a short
time) but then load the favicon and convert it to a data uri which we then
dispatch to the parent process. Along the way this fixes asssociating the load
with the tab for WebExtension and devtools, fixes CSP usage for the load, fixes
expiry detection of the favicon and stops us from loading the same resource
twice.

This change also merges the prefs browser.chrome.site_icons and
browser.chrome.favicons leaving just the former controlling favicon loading. It
adds the pref browser.chrome.guess_favicon to allow disabling guessing where
a favicon might be located for a site (at <hostname>/favicon.ico). This is
mainly to allow disabling this in tests where those additional yet automatic
requests are uninteresting for the test.

There are multiple clean-ups that can follow this but this is a first step along
that path.

MozReview-Commit-ID: E0Cs59UnxaF

	Phabricator Automation
	Comment 29 • 6 years ago

MozReview-Commit-ID: 9mK9dkDyjBx

	Phabricator Automation
	Comment 31 • 6 years ago

Comment on attachment 8985704 [details]
Bug 1453751: Fix webextension tests for favicons. r?aswan

Andrew Swan [:aswan] has approved the revision.

https://phabricator.services.mozilla.com/D1674

	Phabricator Automation
	Comment 32 • 6 years ago

Comment on attachment 8985703 [details]
Bug 1453751: Fix most test assumptions about favicons. r?gijs

:Gijs (he/him) has approved the revision.

https://phabricator.services.mozilla.com/D1673

	Phabricator Automation
	Comment 33 • 6 years ago

Comment on attachment 8985702 [details]
Bug 1453751: Load favicons in the content process. r?mak

Marco Bonardo [::mak] has approved the revision.

https://phabricator.services.mozilla.com/D1672

	Marco Bonardo [:mak]
	Comment 34 • 6 years ago

I strongly suggest waiting for the next merge, that is just a week away, before landing this.

	Pulsebot
	Comment 35 • 6 years ago

Pushed by dtownsend@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/638eb8a41245
Load favicons in the content process. r=mak, r=gijs, r=aswan

	Cosmin Sabou [:CosminS]
	Comment 36 • 6 years ago

Backed out for frequent linux debug e-10s failures on test_ext_webrequest_filter.html

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&filter-searchStr=4f37ae8eb9bbb3fd134a5e9bb9b9d094246e3bcd&fromchange=638eb8a41245f6d9932861afda21edd5e0b2618a&tochange=721354ad7b1622ad9e681d69591a991b1d45a17d&selectedJob=184841833

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=184841833&repo=mozilla-inbound&lineNumber=19972

Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/721354ad7b1622ad9e681d69591a991b1d45a17d

	Phabricator Automation
	Comment 37 • 6 years ago

Since we are only ever using data: URIs for icons from the content process I've
switched them to load with the system principal to hide them from webextensions.
This ended up revealing a bug where the correct arguments weren't being passed
through to loadFavicon in PlacesUIUtils.jsm causing the favicon service to still
reload the icon.

The webextension tests are no longer intermittently failing but I had to ignore
requests triggered by about:newtab for now due to bug 1471387.

	Phabricator Automation
	Comment 38 • 6 years ago

Comment on attachment 8988312 [details]
Bug 1453751: Load data: uri icons with the system principal and fix webrequest tests.

Marco Bonardo [::mak] has approved the revision.

https://phabricator.services.mozilla.com/D1850

	Phabricator Automation
	Comment 39 • 6 years ago

Comment on attachment 8988312 [details]
Bug 1453751: Load data: uri icons with the system principal and fix webrequest tests.

Andrew Swan [:aswan] has approved the revision.

https://phabricator.services.mozilla.com/D1850

	Pulsebot
	Comment 40 • 6 years ago

Pushed by dtownsend@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b11aa832c41a
Load favicons in the content process. r=mak, r=gijs, r=aswan

	Cristian Brindusan [:cbrindusan]
	Comment 41 • 6 years ago

Backed out changeset b11aa832c41a (bug 1453751) for Mochitest-15 failures in toolkit/components/extensions/test/mochitest/test_ext_webrequest_filter.html

https://hg.mozilla.org/integration/mozilla-inbound/rev/6cf60f33ac7da70f0dc7a90fd6369e42d72ae618

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=b11aa832c41ac5beef9065f804d11fb7c9887990&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=success&selectedJob=185422844 

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=185422844&repo=mozilla-inbound

	Dave Townsend [:mossop] Assignee
	Comment 42 • 6 years ago

The tests fails on non-e10s mochitests without this for reasons I don't understand.

	Phabricator Automation
	Comment 43 • 6 years ago

Comment on attachment 8988630 [details]
Bug 1453751: Mark some favicon events as optional to match other requests.

Shane Caraveo (:mixedpuppy) has approved the revision.

https://phabricator.services.mozilla.com/D1869

	Pulsebot
	Comment 44 • 6 years ago

Pushed by dtownsend@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/40ed437da7ae
Load favicons in the content process. r=mak, r=gijs, r=aswan, r=mixedpuppy

	Cristina Coroiu [:ccoroiu]
	Comment 45 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/40ed437da7ae

	Ionuț Goldan [:igoldan]
	Comment 46 • 6 years ago

Push from comment 44 won us some performance!

== Change summary for alert #14114 (as of Fri, 29 Jun 2018 12:25:12 GMT) ==

Improvements:

  3%  sessionrestore_many_windows linux64 pgo e10s stylo     1,052.46 -> 1,025.83
  2%  sessionrestore_many_windows linux64-qr opt e10s stylo  1,494.88 -> 1,458.25
  2%  sessionrestore_many_windows linux64 opt e10s stylo     1,130.12 -> 1,108.25

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=14114

	Honza Bambas (:mayhemer)
	Comment 47 • 6 years ago

We unfortunately don't have an automated test for bug 1247843, but it would be nice to check if this work has not regressed it.