Closed Bug 1352459 Opened 7 years ago Closed 7 years ago

Collect rich icons in ContentLinkHandler

Categories

(Firefox :: New Tab Page, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 57
Tracking Status
firefox57 --- fixed

People

(Reporter: ursula, Assigned: nanj)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

With the changes that mak is working on we'll be able to store rich favicons that come from link tags like "apple-touch-icon". ContentLinkHandler.jsm should add to it's case statement (https://dxr.mozilla.org/mozilla-central/source/browser/modules/ContentLinkHandler.jsm#73) a place to parse out rich icons that are found in the page too.
Priority: -- → P2
it's currently possible to store icons of these sizes: 256, 192, 144, 96, 64, 48, 32, 16.  These were taken based on favicons sizes seen in the wild by our telemetry (most common sizes). In addition we can also store SVGs, that have no size.
When you serve hi-dpi monitors, those numbers should be halved.
Those sizes can be changed and increased in the future, but we first need to collect telemetry about the new db size (bug 1337409) before taking the hit, it's likely we can grow to 512 after a first settlement period.
In addition, we should also always try to collect the root domain icon for each page, the icon at domain/favicon.ico.
The new API allows to do a shallow association of those icons to all the pages in the same host, so we get "free" favicons for all.
Blocks: 615602
Blocks: 1249808
Blocks: 1362394
Component: Activity Streams: General → Activity Streams: Newtab
Hey mak, I just published the patch to implement this feature. Could you review it at your convenience?

Note that, unlike the default favicon load strategy (last one wins), the rich icon favors the "best" one, i.e. it uses the icon's width, and only loads the one with the largest width among all the rich icon tags.
Blocks: 1394533
(In reply to Nan Jiang [:nanj] from comment #5)
> Note that, unlike the default favicon load strategy (last one wins), the
> rich icon favors the "best" one, i.e. it uses the icon's width, and only
> loads the one with the largest width among all the rich icon tags.

It doesn't seem to do that.
First, the default favicon "strategy" is to load the first one (not the last one), indeed at the first link="icon" we set iconAdded to true, and at the next one we'll bail out.
The problem is that you copied this strategy introducing richIconAdded, and in the same way after the first rich icon link, you bail out, afaict.
Looks like you instead wanted to start all the loads and have loadFavicon decide whether to load or not through _shouldLoadFavicon. That seems to add a lot of complication and you can't be sure you won't end up still fetching more than one, indeed what seems to happen in reality is that in the worst case, if the page sort icons from the smaller to the bigger, you will fetch and store all of them.

Personally I'd probably move a bit differently. Note this is open to discussion and I'm sure Mardak may also have opinions on how to do it and about the time constraints.
I'd try to coalesce all the icons selection logic into ContentLinkHandler BEFORE even starting fetching. I'd also drop the distinction between rich and non rich icons at load time, since the favicons service doesn't really distinguish. This way we limit the number of network fetches and I/O and ensure we pick the icons we prefer.
The problem to solve is that we don't know when we get a link, if there may be another one. For that I'd probably use timers. The first time we get an icon link (any of them) I'd start a 50ms timer and collect the icon info into an array. If another link arrives before 50ms, I'd restart the timer and also add it to the array.
When the timer elapses, it means no more icon links arrived in 50ms, we assume to be done (no more collecting) and, if the tab is still open, proceed with analysis of the collected data.
At this point we can walk the array of collected icons, and extract the ones we want, that I think it's a 16x16, a 32x32 and a rich icon. For the basic (16 and 32) icons, we should prefer svg (but please for now skip any ones with mask-color) and ico if they are available, otherwise just pick on the preferred sizes, otherwise just pick the first one. For rich icons you may rely on eventual size fragment or the type (fluid, touch...) to make a pick.
We could also (possible follow-up) do a regex on the icon file name to extract things that may point out at the size, people may name icons like "favicon48.png" or such. Provided we can extract a 2 to 3 len number, we could guess it's a size.

At this point, as far as I can tell, you could just invoke loadFavicon, and forget about all the complications that are handled in a single place, ContentLinkHandler.

Please discuss this, and let me know what you think about it.
See Also: → 1247843
I added a See Also bug 1247843 because the network team is also working on fetching icons at better times, the two things should be able to happen at the same time, but better to keep an eye on each other to avoid overlapped work and bitrotting.
Comment on attachment 8901836 [details]
Bug 1352459 - Collect rich icons in ContentLinkHandler.

https://reviewboard.mozilla.org/r/173276/#review180390

Clearing for now, waiting for your evaluation of my proposal.
Attachment #8901836 - Flags: review?(mak77)
After discussing this with Mardak in detail, we've agreed on that the current roadblock is getting Talos happy with this patch.

The Talos tests (comparing to its parent) is still under going,

https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&originalRevision=c86b7150523c10e1d1dbc0be2d8ed96f205be35f&newProject=try&newRevision=9f97e48d6f3264c743a81ad6fbc6ae05f1431039&framework=1&showOnlyImportant=0

Meanwhile, ursula and I are refactoring the patches both for favicon and metadata to do this in a batching fashion so that we could simplify the code and minimize the performance impact.
(In reply to Marco Bonardo [::mak] from comment #6)
>
> At this point, as far as I can tell, you could just invoke loadFavicon, and
> forget about all the complications that are handled in a single place,
> ContentLinkHandler.

All the complexity we've added to "PlacesUIUtils.jsm" is due to this line http://searchfox.org/mozilla-central/source/browser/components/places/PlacesUIUtils.jsm#224, even if you sort out all the icons (a 16x16, a 32x32, and rich one) in the ContentLinkHandler, and call loadFavicon on each icon, it will probably cancel the previous request and wind up loading the last one.

AFAIK it does this to avoid some security issues, mak, any better plans to workaround this (to be able to load multiple favicons without cancelling the previous ones), other than introducing the richIcon code paths here?
Blocks: 1396104
(In reply to Nan Jiang [:nanj] from comment #10)
> even if you sort out all the icons (a 16x16, a 32x32,
> and rich one) in the ContentLinkHandler, and call loadFavicon on each icon,
> it will probably cancel the previous request and wind up loading the last
> one.
I believe mak's main suggestion is so that only *one* request is ever made in the common case. This means in ContentLinkHandler, it only sends one "Link:SetIcon" triggering one loadFavicon. This means for your patch, the only file that needs to change is ContentLinkHandler.jsm. No need for passing around the icon type or width, and no need to specially handle a mix of those icons.

The other suggestion is just to treat all *icon* rel types equally. This will make sure that all of them are similarly delayed and not cause a new loadFavicon unnecessarily.

It looks like instead of sending Link:SetIcon immediately, it should do it on a cancellable timer recording the timer reference as well as the width. I would think we want to store this information on a Map [something unique to this content window context (url?) -> {timer, width}]

On handling the next icon, if the width is greater than the one in the Map, cancel the timer and start a new one storing the information as was done for the first time.

If there are no more icons and the timer triggers, the one Link:SetIcon will be sent.
If we use nsITimer, the timer delay can be re-extended (probably to the same original timeout amount) so that each additional icon resets that delay even when not providing a larger width.

For example:
icon32 at time 0sec delays Link:SetIcon for 5 seconds.
icon16 at time 4sec delays icon32's SetIcon for another 5 seconds
icon64 at time 8sec cancels icon32 and starts a new timer for 5 seconds
at 13sec, timer triggers and Link:SetIcon is sent only for icon64 calling the existing loadFavicon behvaior
(In reply to Nan Jiang [:nanj] from comment #10)
> AFAIK it does this to avoid some security issues, mak, any better plans to
> workaround this (to be able to load multiple favicons without cancelling the
> previous ones), other than introducing the richIcon code paths here?

The only thing we care about for the security implications is that favicon loads don't continue indefinitely, the code is built to track a single load per page, it could be changed to handle multiple loads per page, indeed we should probably. The important thing is that all of the loads are canceled when the page is closed. I think currently it is tracking only a single load for simplicity and because there was no need to do more at that time.
In practice we don't want a server to be able to send a favicon and keep the request open indefinitely and track the browser.
Thank y'all for the inputs!

Will tweak my previous patch based on the feedback and submit a new patch soon.
Hey mak,

I've pushed a new patch to reviewboard with following changes:

* All favicon processing logic has been merged into ContentLinkHandler, and it's implemented in a timer bouncing manner.
* This line (http://searchfox.org/mozilla-central/source/browser/components/places/PlacesUIUtils.jsm#238) has been taken out from PlaceUIUtils since it already does the similar check in the ContentLinkHandler.
* Re: the icon selection logic was implemented as "svg|ico" > "other icon tags", if none of svg and ico exist, it'll use the first one. I haven't done any special handling on the 32x32 and 16x16 icons yet, because looks like loadFavicon is already doing size resampling, not sure if it's still necessary to pick the icons with those dimensions separately. We could discuss this for sure.

Let me know what do you think.
I retriggered the m-c tp runs to have at least 6 runs, and so far win7 opt has completed with 6 of 8 stylo/not-stylo tp6 tests low confidence performance improvements.

https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&originalRevision=973e8b890a62&newProject=try&newRevision=f2153f2f7a8710df82a1d592e46529b6d93cfd3a&framework=1&filter=tp6%207%20opt

It might be interesting to see how does increasing the 50ms debounce affect performance.
mak, I don't know what's the extent of tests that need fixing up due to the behavior change of delaying favicon, but what should we do for this week's deadline vs fixing within the rest of 57 vs after 57?

E.g., maybe skip some tests? Or have a pref to pushPrefEnv turning off the delay? Or have some way to pass in delayAmount=0?
Flags: needinfo?(mak77)
(In reply to Ed Lee :Mardak from comment #19)
 
> E.g., maybe skip some tests? Or have a pref to pushPrefEnv turning off the
> delay? Or have some way to pass in delayAmount=0?

That's a sound idea for the time being, will try it out :)

I've fixed some failed tests, but other ones are trickier to fix, some of them even need to be rewritten IMHO.
(In reply to Ed Lee :Mardak from comment #18)
> https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&originalRevision=973e8b890a62&newProject=try&newRevision=f2153f2f7a8710df82a1d592e46529b6d93cfd3a&framework=1&filter=tp6%207%20opt
Well, even with 20 runs before and 20 runs after, the numbers are still pretty noisy, so it doesn't show anything with high confidence, but even then across the 4 tp6 tests:

stylo single thread has about 1% regression
non-stylo has about 1% improvement except youtube 3% regression
stylo multi thread averaging 1.5% improvement

(In reply to Nan Jiang [:nanj] from comment #9)
> https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&originalRevision=c86b7150523c10e1d1dbc0be2d8ed96f205be35f&newProject=try&newRevision=9f97e48d6f3264c743a81ad6fbc6ae05f1431039&framework=1&filter=tp6%207%20opt
This old run with no batching has fewer runs and is also low confidence:

stylo single thread 0.5% regression
non-stylo 1.4% regression
stylo multi 2 tests regressing and 2 tests improving. average 1.2% improvement

So it's not really clear if batching is actually improving?

mak, should we be looking at tests other than tp6?
(In reply to Ed Lee :Mardak from comment #22)
> mak, should we be looking at tests other than tp6?

I don't have a perfectly good answer for you because often Talos demonstrated to be unpredictable, plus lately a lot of specific talos tests for amazon and other top sites have been added for QF/Perf measurements, and thus it's a moving target.
My expectation is that Tp is the kind of tests that may be affected more.
Memory/RSS tests could also show something (storing more images takes more memory).

In any case, doing a full talos run/comparison is just matter of using some server resources, and we should not be ashamed to do that, the cost looks small compared to the benefit of knowing where you are.

(In reply to Ed Lee :Mardak from comment #19)
> mak, I don't know what's the extent of tests that need fixing up due to the
> behavior change of delaying favicon, but what should we do for this week's
> deadline vs fixing within the rest of 57 vs after 57?

It depends on the tests, but surely it's all valid options.
In the try run I see a lot of leaks reported, those should be somehow figured out.
If some fail due the timer, it's likely they are just badly written, we can't predict favicon timing so every test should wait for some kind of event.
I need a final list of failures to look at, to be able to give you better feedback.
Flags: needinfo?(mak77)
Comment on attachment 8905329 [details]
Bug 1352459 - part2. Fix test failures.

https://reviewboard.mozilla.org/r/177114/#review182326

::: browser/base/content/test/general/browser_subframe_favicons_not_used.js:13
(Diff revision 1)
>    let tab = BrowserTestUtils.addTab(gBrowser, testPath + "file_bug970276_popup1.html");
>  
>    tab.linkedBrowser.addEventListener("load", function() {
>      let expectedIcon = testPath + "file_bug970276_favicon1.ico";
> +    BrowserTestUtils.waitForCondition(() => true, "wait for loadFavicon to finish", 100)
> +    .then(() => {

I'm not sure what this is doing, looks like a setTimeout hidden behind waitForCondition...
When you need to use a timer, just use setTimeout with a nice comment above it explaining why there's no other way, for example when checking that events DON'T happen.
The timeout is necessary for all of the expected-failure tests. For the pass cases you may either dispatch a CustomEvent from ContentLinkHandler to notify when it's done, or use an history observer's onPageChanged with ATTRIBUTE_FAVICON, or you could temporarily mock the browser's setIcon and check its arguments. But in general you should expect for something more specific than a timer.
Attachment #8905329 - Flags: review?(mak77) → review-
Yes, we wanted to use a timer here, choosing waitForCondition to simulate a timer was because setTimeout didn't work somehow in some chrome browser tests.

You're right, we should use something more specific like waitForFaviconLoads http://searchfox.org/mozilla-central/source/browser/components/originattributes/test/browser/browser_favicon_firstParty.js#146

Will fix them up.
Comment on attachment 8901836 [details]
Bug 1352459 - Collect rich icons in ContentLinkHandler.

https://reviewboard.mozilla.org/r/173276/#review182322

::: browser/base/content/test/general/browser_discovery.js:38
(Diff revision 2)
>    { type: "image/png; charset=utf-8", text: "type may have optional parameters (RFC2046)" }
>  ];
>  
>  function runIconDiscoveryTest() {
> +  BrowserTestUtils.waitForCondition(() => true, "wait for favicon processing to finish", 100)
> +  .then(() => {

As I said for the other part, this won't do.

::: browser/modules/ContentLinkHandler.jsm:157
(Diff revision 2)
> -                }
> +          }
> -              }
> +        }
> -            }
> +      }
> -          } else {
> +    } else {
> -            sizesType = SIZES_TELEMETRY_ENUM.NO_SIZES;
> +      sizesType = SIZES_TELEMETRY_ENUM.NO_SIZES;
> -          }
> +    }

All of this code could probably be factored out into a separate helper function

::: browser/modules/ContentLinkHandler.jsm:181
(Diff revision 2)
> +      this._faviconLoads.set(pageUrl, load);
> +    }
> +    return true;
> +  },
> +
> +  setTimeout(callback, delay) {

can be moved to an helper function in the global scope

::: browser/modules/ContentLinkHandler.jsm:237
(Diff revision 2)
> +    // Telemetry probes for measuring the sizes attribute
> +    // usage and available dimensions.
> +    let sizeHistogramTypes = Services.telemetry.
> +                             getHistogramById("LINK_ICON_SIZES_ATTR_USAGE");
> +    let sizeHistogramDimension = Services.telemetry.
> +                                 getHistogramById("LINK_ICON_SIZES_ATTR_DIMENSION");

These could happen already when you collect the sizesType and width information.
These probes are not about what we store, but about what our users encounter in the wild.
Attachment #8901836 - Flags: review?(mak77)
afaik, setTimeout should always work, but it's suggested to not use it and thus there's an eslint rule against it, see the
/* eslint-disable mozilla/no-arbitrary-setTimeout */
comments around. If you really need it you can use it.
Comment on attachment 8901836 [details]
Bug 1352459 - Collect rich icons in ContentLinkHandler.

https://reviewboard.mozilla.org/r/173276/#review182344

::: browser/base/content/test/general/browser_discovery.js:38
(Diff revision 2)
>    { type: "image/png; charset=utf-8", text: "type may have optional parameters (RFC2046)" }
>  ];
>  
>  function runIconDiscoveryTest() {
> +  BrowserTestUtils.waitForCondition(() => true, "wait for favicon processing to finish", 100)
> +  .then(() => {

In this particular test, we tried setTimeout first, and it didn't work due to some scoping issues in the timer callback. Then we just switched to waitForCondition as a workaround.

Perhaps i didn't use it correctly :). Anyway, will try waitForFaviconLoaded instead. That makes more sense.
Attachment #8905329 - Attachment is obsolete: true
Comment on attachment 8901836 [details]
Bug 1352459 - Collect rich icons in ContentLinkHandler.

https://reviewboard.mozilla.org/r/173276/#review182594

::: browser/base/content/test/general/browser_discovery.js:43
(Diff revision 3)
> -    ok(!hasSrc, testCase.text);
>  
> +  // Because there is debounce logic in ContentLinkHandler.jsm to reduce the
> +  // favicon loads, we have to wait some time before checking that icon was
> +  // stored properly.
> +  BrowserTestUtils.waitForCondition(() => {

Ended up sticking with waitForCondition for this test, it's indeed waiting for the favicon loads to finish. Added some comments here to clarify it, does it make sense? If so, I'll fix other icon related test failure in this way.

Interestingly, I've also tried to oserve the onPageChanged with Attribute_Favicon here, it couldn't capture that notification in this test.
Comment on attachment 8901836 [details]
Bug 1352459 - Collect rich icons in ContentLinkHandler.

https://reviewboard.mozilla.org/r/173276/#review182772

The patch looks good, but I cannot r+ it until the many leaks and failures shown on Try are addressed... Unless the Try I'm looking at is not the latest.

::: browser/base/content/test/general/browser_discovery.js:48
(Diff revision 3)
> +  BrowserTestUtils.waitForCondition(() => {
> +    return gBrowser.getIcon() != null;
> +  }).then(() => {
> +    ok(testCase.pass, testCase.text);
> +  }).catch(() => {
> +    ok(!testCase.pass, testCase.text);

This will take the full 5 seconds that waitForCondition expects, that sounds a bit too much.
You can pass arguments to waitForCondition to set how often it should poll and for how long. I think for this case we may poll ever 100ms for 5 times?

::: browser/base/content/test/general/browser_discovery.js:96
(Diff revision 3)
> +  let head = doc().getElementById("linkparent");
> +
> +  // Because there is debounce logic in ContentLinkHandler.jsm to reduce the
> +  // favicon loads, we have to wait some time before checking that icon was
> +  // stored properly.
> +  BrowserTestUtils.waitForCondition(() => {

ditto

::: browser/modules/ContentLinkHandler.jsm:29
(Diff revision 3)
>    DIMENSION: 2,
>    INVALID: 3,
>  };
>  
> +const FAVICON_PARSING_TIMEOUT = 50;
> +const FAVICON_RICH_ICON_MIN_WIDTH = 96;

does this take hidpi into account? Setting 96 here means you'll show a 48px icon on 2xdpi screens, is that enough for your needs?

::: browser/modules/ContentLinkHandler.jsm:34
(Diff revision 3)
> +const FAVICON_RICH_ICON_MIN_WIDTH = 96;
> +
> +function setTimeout(callback, delay) {
> +  let timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
> +  timer.initWithCallback({ notify: callback }, delay,
> +    Ci.nsITimer.TYPE_ONE_SHOT);

nit: align arguments

::: browser/modules/ContentLinkHandler.jsm:67
(Diff revision 3)
> +  }
> +
> +  // Telemetry probes for measuring the sizes attribute
> +  // usage and available dimensions.
> +  let sizeHistogramTypes = Services.telemetry.
> +                           getHistogramById("LINK_ICON_SIZES_ATTR_USAGE");

nit: align on dots
Services.telemetry
        .getHistogram...

::: browser/modules/ContentLinkHandler.jsm:70
(Diff revision 3)
> +  // usage and available dimensions.
> +  let sizeHistogramTypes = Services.telemetry.
> +                           getHistogramById("LINK_ICON_SIZES_ATTR_USAGE");
> +  sizeHistogramTypes.add(sizesType);
> +  if (width > 0) {
> +    let sizeHistogramDimension = Services.telemetry.

ditto
Attachment #8901836 - Flags: review?(mak77)
hey mak, while fixing those test failures, i got stuck by this particular one,

http://searchfox.org/mozilla-central/source/dom/security/test/general/test_bug1277803.xul

It's intended to test that "We expect 2 favicon loads, one from PlacesUIUtils.loadFavicon and one from XUL:image loads, each favicon load should have the correct loadingPrincipal" by observing "http-on-modify-request". There appears to be only one "http-on-modify-request" fired with the aSubject as httpChannel for that page, not for those two favicons.

Do you know if this patch would affect the behavior of "http-on-modify-request"? I'm also suspecting that if this has to do with onPageChanged with ATTRIBUTE_FAVICON, it couldn't capture this notification when i was trying it in the test yesterday.
Flags: needinfo?(mak77)
Well, this turns out to be an intermittent failure, http-on-modify-request does gets fired in the success cases. Perhaps it's due to the delayed favicon loading?
Just a quick update that I've fixed those window leaks, the issue was caused by ContentLinkHandler, which was holding a reference of chromeGlobal. Using WeakReference solved the problem.

Also I've updated a bunch of other existing tests to work with the new favicon workflow. The only test failure left is the "http-on-modify-request" one as mentioned above. Still investigating.
(In reply to Nan Jiang [:nanj] from comment #32)
> http://searchfox.org/mozilla-central/source/dom/security/test/general/test_bug1277803.xul
The test passes if I run with ./mach mochitest bug1277803.xul --setpref browser.newtab.preload=false

It looks like the preloaded about:newtab triggers a Link:SetIcon.
Comment on attachment 8901836 [details]
Bug 1352459 - Collect rich icons in ContentLinkHandler.

https://reviewboard.mozilla.org/r/173276/#review182962

::: browser/modules/ContentLinkHandler.jsm:83
(Diff revision 3)
>  this.ContentLinkHandler = {
> +  // Map from page URI to favicon loads
> +  _faviconLoads: new Map(),
> +
>    init(chromeGlobal) {
> +    this._chromeGlobal = chromeGlobal;

This is the source of the bug1277803.xul failure, and it looks to be an actual code error.

It looks like `init(chromeGlobal)` is called for every single content page with its own different `chromeGlobal`. However, `ContentLinkHandler` is a singleton.

This means `this._chromeGlobal` will happen to be the chromeGlobal of the latest content page, in the test failure case, the preloaded about:newtab. This then has browser.js's DOMEventHandler.setIcon short circuit because it doesn't find a tab for the preloaded about:newtab.

One simpler fix is to just pass `chromeGlobal` along with the icon, e.g., `this.handleFaviconLink(link, false/true, chromeGlobal)` (similar to how `onLinkEvent` takes a chromeGlobal).

However, it looks like various parts of this patch (as well as that of bug 1393924) assumed `init()` would be called once (per content process maybe?).

However, what's happening is that multiple content pages are sharing this one ContentLinkHandler. There could be bugs when the same page is loaded in multiple tabs.

If we want to scope things to a given page, those variables should be created inside `init()`, e.g., `iconInfos`. Or maybe there should be a `Map`ping of `chromeGlobal` -> `incoInfos`?

::: browser/modules/ContentLinkHandler.jsm:207
(Diff revision 3)
> +    }
> +    return true;
> +  },
> +
> +  faviconTimeoutCallback() {
> +    for (let [pageUrl, load] of this._faviconLoads) {

Is this the expected behavior you were trying to implement? `faviconTimeoutCallback` is called some delay after seeing a favicon on *any* page. Then here, it goes through all pending faviconLoads of every page. This means if there are multiple pages, whichever one happens to trigger the timer first will short circuit the delay/waiting period for all pages.
Attachment #8901836 - Flags: review-
ursula/mconley: At some point in implementing bug 1393924, we were wondering if `init(chromeGlobal)` is called once per content process. From my investigation in comment 37, it looks like it's actually being called for *every* single content page. I'm not sure if you two discussed that in person, but assuming I've not completely misdiagnosed the situation, it looks like we might have incorrectly implemented ContentMetaHandler.

In particular, `ContentMetaHandler.init` does `this._metaTags = new Map();`
https://hg.mozilla.org/mozilla-central/rev/1b2b3bc1d47b#l9.71

With every page calling `init()`, that means any meta tags that we're keeping around during the time of the debouncing will get blown away on the next loaded (including preloaded about:newtab) content page.
Flags: needinfo?(usarracini)
Flags: needinfo?(mconley)
(In reply to Ed Lee :Mardak from comment #37)
> Comment on attachment 8901836 [details]
> Bug 1352459 - Collect rich icons in ContentLinkHandler.
> 
> This means `this._chromeGlobal` will happen to be the chromeGlobal of the
> latest content page, in the test failure case, the preloaded about:newtab.
> This then has browser.js's DOMEventHandler.setIcon short circuit because it
> doesn't find a tab for the preloaded about:newtab.
> 
> One simpler fix is to just pass `chromeGlobal` along with the icon, e.g.,
> `this.handleFaviconLink(link, false/true, chromeGlobal)` (similar to how
> `onLinkEvent` takes a chromeGlobal).

Ah, nice catch, Mardak. Initially, I chose to hold a reference of chromeGlobal for each ContentLinkHandler to avoid passing it around among other functions. Unfortunately, this caused this issue as well as the window leaks as I mentioned above.

> However, it looks like various parts of this patch (as well as that of bug
> 1393924) assumed `init()` would be called once (per content process maybe?).
> 
> However, what's happening is that multiple content pages are sharing this
> one ContentLinkHandler. There could be bugs when the same page is loaded in
> multiple tabs.

I know what's your concern here. Let's say a page with M icon links gets loaded in N tabs at the same time, so it's timeout will get bounced by M * N times, which could generate a significantly longer favicon load delay provided M is big.  

> Is this the expected behavior you were trying to implement?
> `faviconTimeoutCallback` is called some delay after seeing a favicon on
> *any* page. Then here, it goes through all pending faviconLoads of every
> page. This means if there are multiple pages, whichever one happens to
> trigger the timer first will short circuit the delay/waiting period for all
> pages.

Nope, it's a bug. It should only handle that ONE timer, and let the others stay pending. Will fix it.
I've submit a new patch to reviewboard, looks like all the test failures are now fixed :P

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0128160d16c9d80f44ed63b3552168b619b57e36

mak, could you take another look at it plz?

Mardak, I've also fixed those issues you mentioned earlier, could you also r? it plz?

Thank y'all!
Flags: needinfo?(mak77)
Comment on attachment 8901836 [details]
Bug 1352459 - Collect rich icons in ContentLinkHandler.

https://reviewboard.mozilla.org/r/173276/#review183070

Looks like it's working/passing tests, so just some code quality suggestions. There is the open question for mak to clarify his previous "I'd also drop the distinction between rich and non rich icons at load time" from comment 6. And perhaps the value for `FAVICON_PARSING_TIMEOUT` where for description/image we ended up at 1000ms. But we can't really do that long for icons as it would delay when users see the icon appear.

::: browser/modules/ContentLinkHandler.jsm:88
(Diff revision 4)
>      });
>      chromeGlobal.addEventListener("DOMLinkChanged", (event) => {
>        this.onLinkEvent(event, chromeGlobal);
>      });
> +    chromeGlobal.addEventListener("unload", (event) => {
> +      this.onUnloadEvent(event);

Instead of having a shared onUnloadEvent handler, I think it'll be clearer/simpler if we just define the handler inline to use locally scoped objects. Similarly instead of having a shared `_faviconLoadTasks` `Map` of `Map`s, there'll just be a locally scoped single `Map`.

```js
init(chromeGlobal) {
  const faviconLoads = new Map();
  chromeGlobal.addEventListener("DOMLinkAdded", event => {
    this.onLinkEvent(event, chromeGlobal, faviconLoads);
  });
  …
  chromeGlobal.addEventListener("unload", event => {
    for (const [pageUrl, load] of faviconLoads) {
      …
    }
  });
}
```

This avoids needing to have a top level `Map` to then check if the nested `Map` still exists, because it's contained within this scope.

::: browser/modules/ContentLinkHandler.jsm:152
(Diff revision 4)
> -                  sizesType = SIZES_TELEMETRY_ENUM.DIMENSION;
> -                  sizeHistogramDimension.add(parseInt(values[1]));
> -                } else {
> -                  sizesType = SIZES_TELEMETRY_ENUM.INVALID;
> -                  break;
> +            break;
> -                }
> +          richIconAdded = this.handleFaviconLink(link, true, chromeGlobal);

nit: If we really do want to send both regular icon and rich icon (note: some sites use rich icons already for their "icon" -- I thought mak suggested that there should be no distinction between icon vs rich, and that we would just always save just the largest one.)

I think this would avoid duplication and not be too confusing (but again, code style nit):

```js
const iconAdded = [false, false]; // icon and rich icon
…
for (let relVal in rels) {
  let isRichIcon = true;
  switch (relVal) {
    case "icon":
      isRichIcon = false;
      // fall through to rich icon handling
    case "apple-touch-icon":
    case …:
      if (iconAdded[+isRichIcon] || …) { … }
      iconAdded[+isRichIcon] = this.handleFaviconLink(link, isRichIcon, chromeGlobal, faviconLoads);
```

If we don't care about rich or not, then all the `case "icon-*"` would just lead to the same code block. One thing to consider for keeping them separate vs together is that I believe the favicon in the tab will not be shown until the icon is fetched (unless falling back to the favicon for the site (?)), so there is actual user speed perception that we're touching here.

::: browser/modules/ContentLinkHandler.jsm:210
(Diff revision 4)
> +      // Ping the faviconLoads, pageUrl, and chromeGlobal to the timeout callback
> +      // so that it knows which timer gets triggered from this._faviconLoadTask.
> +      let faviconTimeoutCallback = this.faviconTimeoutCallback.bind(this, faviconLoads, pageUrl, chromeGlobal);
> +      let timer = setTimeout(faviconTimeoutCallback, FAVICON_PARSING_TIMEOUT);
> +      let load = { timer, iconInfos: [iconInfo] };
> +      faviconLoads.set(pageUrl, load);

Just like moving `onUnloadEvent` to a locally scoped callback, we can do the same with `faviconTimeoutCallback`:

```js
const load = {iconInfos: [iconInfo]};
faviconLoads.set(pageUrl, load);
load.timer = setTimeout(() => {
  // code for `faviconTimeoutCallback` goes here
  // except just directly access locally scoped load, pageUrl, chromeGlobal, faviconLoads
}, FAVICON_PARSING_TIMEOUT);
```

Although I do see that `faviconTimeoutCallback` is quite large.. so up to you. ;)

FYI, in bug 1393924, mak did have review suggestions of moving various methods to top level helper functions with javadoc instead of being methods on `ContentLinkHandler`.
(In reply to Ed Lee :Mardak from comment #42)
> There is the open question for mak to clarify his previous "I'd
> also drop the distinction between rich and non rich icons at load time" from
> comment 6.

that was limited to the code finally doing the network fetch, there it doesn't really matter what we are fetching. It's fine to have a distinction in ContentLinkHandler.

> And perhaps the value for `FAVICON_PARSING_TIMEOUT` where for
> description/image we ended up at 1000ms. But we can't really do that long
> for icons as it would delay when users see the icon appear.

Right, we can't go too high, I think 100ms for now should do. we can always adapt it in the future.

> nit: If we really do want to send both regular icon and rich icon (note:
> some sites use rich icons already for their "icon" -- I thought mak
> suggested that there should be no distinction between icon vs rich, and that
> we would just always save just the largest one.)

We need both, because a large icon downscaled will likely look worse than a natively sized icon. Indeed some pages tend to serve different icons for different sizes, so they look nice anyway.
My comment was more limited, as I said above.
In an ideal world we'd just store multiple icons (16, 32, ..., 256) and be able to pick the best later. But we're also space limited, so for now getting a small (likely 32px) and a large one should be ok.

> FYI, in bug 1393924, mak did have review suggestions of moving various
> methods to top level helper functions with javadoc instead of being methods
> on `ContentLinkHandler`.

Where possible that'd be nice, I didn't point out many comments like those here because ContentLinkHandler is old code and making it nicer is not primary scope of this bug, while in the other case we're introducing a new module, so better start it cleaner :)
(In reply to Ed Lee :Mardak from comment #38)
> With every page calling `init()`, that means any meta tags that we're
> keeping around during the time of the debouncing will get blown away on the
> next loaded (including preloaded about:newtab) content page.

From what I can see, your diagnosis is correct. To test this, I set the timeout to be 10 minutes and then I visit two pages in different tabs, one immediately after the other (before the 10m is up) and I can see that after visiting the second page in the second open tab, it did in fact blow away the first page's map entries. The same can't be said about visiting multiple different pages within the _same_ tab. So it looks like it's making a new one in each content page. 

I couldn't see another `init()` being fired for a preloaded about:newtab though, at least not in ContentMetaHandler.jsm. I know that in ContentLinkHandler.jsm we'll get an event for DOMLinkAdded and one for DOMLinkChanged for every about:newtab, because we tack on a '#' to the end of our favicon url on a visibility change, which fires a DOMLinkChanged and gets picked up in ContentLinkHandler.jsm. But I couldn't see another call to `init()` fired from a preloaded about:newtab.

I suppose this is an issue if you visit two pages in faster than 1 second. We can file a bug for this for ContentMetaHandler.jsm, we'll have to think of a way to keep state between two content pages
Flags: needinfo?(usarracini)
Review fixes have been pushed to reviewboard just now. Markak, could you take one more r?, plz?

try build was triggered as well,

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f8a6cad3a15cf40700d0d3babb9659a25907b5e5
Flags: needinfo?(mconley)
Comment on attachment 8901836 [details]
Bug 1352459 - Collect rich icons in ContentLinkHandler.

https://reviewboard.mozilla.org/r/173276/#review183444

mak, please take a look now with tests passing. Looks good to me.

::: browser/modules/ContentLinkHandler.jsm:273
(Diff revision 5)
> -    } catch (e) {
> -      // some URIs are immutable
> +
> +    // Extract the size type and width. Note that some sites use hi-res icons
> +    // without specifying them as apple-touch or fluid icons.
> +    let width = extractIconSize(link.sizes);
> +    if (width >= FAVICON_RICH_ICON_MIN_WIDTH)
> +      isRichIcon = true;

Just to be clear, this will change the downstream behavior of the icon being "rich" (preferred vs rich stuff), but in terms of its caller (`onLinkEvent`), the `iconAdded` array will update its state based on the `rel=icon` vs rich `rel=*-icon`. This means in the timer callback, we will treat a `rel=icon` as if it was rich `rel=*-icon`.

The slightly confusing `iconAdded` behavior from `onLinkEvent` looks to only exist to prevent some website doing `<link rel="icon icon icon icon"/>` causing multiple `Link:SetIcon` (??). I didn't notice this it earlier, but given that we override `isRichIcon` here, do we really want to `handleFaviconLink` twice if the website does `<link rel="icon apple-touch-icon"/>`? If not, a single plain boolean `iconAdded` with a completely shared case code block would be sufficient… ?

::: browser/modules/ContentLinkHandler.jsm:292
(Diff revision 5)
> +      load.timer.delay = FAVICON_PARSING_TIMEOUT;
> +    } else {
> +      // Pin the faviconLoads, pageUrl, and chromeGlobal to the timeout callback
> +      // so that it knows which timer gets triggered.
> +      let callback = faviconTimeoutCallback.bind(null, faviconLoads, pageUrl, chromeGlobal);
> +      let timer = setTimeout(callback, FAVICON_PARSING_TIMEOUT);

nit: With all the other related methods now top level functions, this "method" doesn't even use `this`, so it too could be top level.

nit: I believe arrow functions are preferred over `.bind`, so this could just be `setTimeout(() => faviconTimeoutCallback(faviconLoads, pageUrl, chromeGlobal), FAVICON_PARSING_TIMEOUT)` (even though the implicit arrow function bind would happen to bind global when it's top level…)

nit: mak mentioned increasing FAVICON_PARSING_TIMEOUT to 100ms. Did you run into problems increasing from 50ms to 100ms?
Attachment #8901836 - Flags: review+
Comment on attachment 8901836 [details]
Bug 1352459 - Collect rich icons in ContentLinkHandler.

https://reviewboard.mozilla.org/r/173276/#review183708

Is the bug assignee wrong?

Thank you for help in reviewing this.

Btw, one thing to note before I forget, as an heads-up. This patch will store higher res icons, and as such it's likely bookmarks/history views will start using those. It should not be a big deal, but I expect some users to file bugs for that, because the tab icon and the bookmark icon will start to diverge a little bit more and not all the icons may look nice at 32px. This will improve with bug 1347532, since once we pass size from the ui, the backend could pick a properly sized frame instead of the largest one. I'll restart work on that one once 57 work is complete.

::: browser/modules/ContentLinkHandler.jsm:40
(Diff revision 6)
> + * @param {Number} aDelay A timeout interval in millisecond.
> + * @return {nsITimer} A nsITimer object.
> + */
> +function setTimeout(aCallback, aDelay) {
> +  let timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
> +  timer.initWithCallback({ notify: aCallback }, aDelay, Ci.nsITimer.TYPE_ONE_SHOT);

nsITimerCallback has the function annotation, thus you don't need to pass an object to initWithCallback, you can directly pass aCallback to it.
Attachment #8901836 - Flags: review?(mak77) → review+
Assignee: usarracini → najiang
(In reply to Marco Bonardo [::mak] from comment #49)
> Comment on attachment 8901836 [details]
> Bug 1352459 - Collect rich icons in ContentLinkHandler.
> 
> https://reviewboard.mozilla.org/r/173276/#review183708
> 
> Is the bug assignee wrong?
> 

Heh, definitely not. Thank you both for reviewing this.


> Btw, one thing to note before I forget, as an heads-up. This patch will
> store higher res icons, and as such it's likely bookmarks/history views will
> start using those. It should not be a big deal, but I expect some users to
> file bugs for that, because the tab icon and the bookmark icon will start to
> diverge a little bit more and not all the icons may look nice at 32px. This
> will improve with bug 1347532, since once we pass size from the ui, the
> backend could pick a properly sized frame instead of the largest one. I'll
> restart work on that one once 57 work is complete.

I don't think this patch will be the end of rich icon story, quite like the metadata extraction may have storage impact on Places, some side effects on ui and followup bugs are expected.

Finished two more try builds last night and early this morning, they all look good.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=736966a39cb78d52fec40bf7644e1f3cfd324480
https://treeherder.mozilla.org/#/jobs?repo=try&revision=015dd3dc2408fcdc969652b454d25d7793220eb9

Let's ship it!
Pushed by edilee@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/5ed9b657a1ce
Collect rich icons in ContentLinkHandler. r=mak,Mardak
Blocks: 1399226
jmaher, the tp5n failure we're running into:

 PID 2168 | FATAL ERROR: Non-local network connections are disabled and a connection attempt to en.wikipedia.org (208.80.154.224) was made. [log…] 

Makes sense in that this bug is adding fetching of rich icons, and it's most likely fetching

view-source:https://en.wikipedia.org/wiki/Rorschach_test
<link rel="apple-touch-icon" href="/static/apple-touch/wikipedia.png"/>

Were the tp5n test files modified to remove any <link rel="icon"/> lines to prevent fetching those icons? Or do you know how "regular" favicons are not triggering network access?
Flags: needinfo?(jmaher)
Ah, I got the tp5n.zip via `./mach awsy-test --quick` then `cd ./obj/_tests/awsy/html/page_load_test/tp5n`

And indeed there are these link elements:

<link rel="apple-touch-icon" href="http://en.wikipedia.org/apple-touch-icon.png" />
<link rel="shortcut icon" href="../favicon.ico" />

So some hrefs are pointing to a local copy.

And other hrefs seem to be edited to be

<link rel="apple-touch-icon" href="httpdisabled://

In particular, there's 24 httpdisabled apple-touch-icon links out of 31 total apple-touch-icon links.

jmaher, how do we go about making them all httpdisabled or reference some local image?

This bug adds support for "apple-touch-icon" "apple-touch-icon-precomposed" "fluid-icon" but potentially additional rel types could be added. The tp5n files don't have any fluid-icons so a grep for apple-touch-icon seems like it'll catch all the possible rich icons from this bug.
Depends on: 1399320
Added patch to bug 1399320 to convert rich icon links to httpdisabled:
Flags: needinfo?(najiang)
Flags: needinfo?(jmaher)
Just in case other things have changed since we tried landing, I pushed rebased to latest m-c:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=482b9adb3ae355cc8fa04e7d38068034acc41721

I also pushed a -t all to check if there's any potential talos regressions with tests completing with the new tp5n.zip:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=1ed60358a8d2&newProject=try&newRevision=6a2d1717fb71f67c49a49017052d8d5b44fde049&framework=1&showOnlyImportant=0

Potentially there's a "tp5o Modified Page List Bytes opt e10s stylo_disabled" win32 7% regression?
https://wiki.mozilla.org/Buildbot/Talos/Tests#Modified_Page_List_Bytes

That could make sense in that we're using more memory keeping track of all possible icons. If that really is the regressing part, maybe we should do something like metadata, where it only keeps the best value it's seen so far instead of gathering all to pick the best at the end of the timer. Not sure if we can land with the regression and do a followup.
I pushed a quick sanity check by commenting out the `push(iconInfo)`. Talos results should show up here relative to m-c with new tp5n:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=1ed60358a8d2&newProject=try&newRevision=678e9a3205087bace9091309f1967745bbb8120b&framework=1&showOnlyImportant=0

Also, not entirely sure about the g1 failures, but those shows up on the m-c-with-new-tp5n reference, so maybe it's how I'm downloading and extracting a special tp5n that has special characters removed compared to the usual tp5n file.
Looks like there wasn't much change when commenting out, so I retriggered a 20 more runs, and the 7% regression is now maybe 2% regression or just noise.
Pushed by edilee@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/6d5fe3151e73
Collect rich icons in ContentLinkHandler. r=mak,Mardak
Blocks: 1401316
https://hg.mozilla.org/mozilla-central/rev/6d5fe3151e73
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Depends on: 1401777
Depends on: 1401851
Depends on: 1401632
Depends on: 1403504
Depends on: 1402373
This bug may have contributed to a sudden change in the Telemetry probe LINK_ICON_SIZES_ATTR_DIMENSION[1] which seems to have occurred in Nightly 20170919[2]. This seems to correspond to an equally-sudden change in LINK_ICON_SIZES_ATTR_USAGE.

I'm not 100% sure it was this bug, though, as the timing doesn't seem to line up. (This patch was only on m-c at 1714EDT on the 19, so I'm not sure that it could affect any of the Nightly builds that day). However, something drastic happened to increase the number and types of sizes of favicons Nightly users suddenly started to perceive that day.

Is this an improvement? A regression?

Is this intentional? Is this expected?

Is this probe still measuring something useful?

[1]: http://alerts.telemetry.mozilla.org/index.html#/detectors/1/metrics/628/alerts/?from=2017-09-19&to=2017-09-19
[2]: https://mzl.la/2kHQMfi
[3]: https://mzl.la/2kHKiNA
Flags: needinfo?(edilee)
This is an expected change that resets the expected baseline. Basically, more icons are being considered for the telemetry probes.

I believe the probe is still measuring something useful, but mak can probably provide more details.
Flags: needinfo?(edilee) → needinfo?(mak77)
It could just be the effect of examining Rich icons, if they provide a sizes attribute, that sounds possible.
It shouldn't have any negative impact on the product, since this measures icons we meet in the wild, not the ones that we actually store. We just now have more data to look at :)

This probe is measuring something useful, we try to understand how many icons in the wild have a sizes attribute and what's the distribution of those sizes.
Flags: needinfo?(mak77)
Spiffy!

On closer examination, LINK_ICON_SIZES_ATTR_DIMENSION (and its friend LINK_ICON_SIZES_ATTR_USAGE) don't have bug_numbers or alert_emails fields. The former is useful to provide context on when the probe was introduced, changed, or referenced. The latter is useful so that people familiar with the probe receive alert emails if/when the probe experiences a meaningful change.

Would you like to track the work to add these fields in a new bug, or in this one?
Flags: needinfo?(mak77)
(In reply to Chris H-C :chutten from comment #66)
> On closer examination, LINK_ICON_SIZES_ATTR_DIMENSION (and its friend
> LINK_ICON_SIZES_ATTR_USAGE) don't have bug_numbers or alert_emails fields.
> The former is useful to provide context on when the probe was introduced,
> changed, or referenced. The latter is useful so that people familiar with
> the probe receive alert emails if/when the probe experiences a meaningful
> change.

I filed bug 1409291.
I think an owner was not added originally because it's not much interesting to notice changes in them and it often doesn't depend on us, since it's numbers fetched from the wild. We rather check the stats periodically to make decisions about favicon changes.
Flags: needinfo?(mak77)
No longer blocks: 1412832
Depends on: 1412832
Depends on: 1419039
See Also: → 1374382
Component: Activity Streams: Newtab → New Tab Page
You need to log in before you can comment on or make changes to this bug.