New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 895175 link

Starred by 33 users

Issue metadata

Status: Fixed
Owner:
Closed: Nov 26
Cc:
Components:
EstimatedDays: ----
NextAction: 2018-10-29
OS: Linux , Windows , Chrome , Mac
Pri: 3
Type: Bug



Sign in to add a comment

history.pushState triggers icon downloading

Project Member Reported by mgiuca@chromium.org, Oct 14

Issue description

Reported by user on Twitter (https://twitter.com/nekrtemplar/status/1051618550052507650). Unknown Chrome version, platform, etc. (@nekrtemplar, can you provide this info?)

I couldn't reproduce this when quickly trying it from the console on Linux Chrome 71.

Apparently, using history.pushState() redownloads the page favicon unnecessarily.

This compounds with  Issue 324820  where all icons are downloaded, not just the appropriately sized one. So every pushState causes like 8 icons to be downloaded.
 
Note: According to the report, this isn't anything to do with the manifest icon downloading. It's the <link> tag icon downloading.
I'll try to make a repro. It's related to PWA Conpat library (Chrome tem makes it) which generates link tags on the fly in JS. Maybe this causes they to be redowbloaded on each pushState, not sure. But it definitely happens.
Labels: Needs-Feedback
NextAction: 2018-10-29
Status: Unconfirmed (was: Untriaged)
I couldn't reproduce this with a simple test page.  Reporter, would you provide a complete HTML please?

This does it for me. A request is fired per icon, per pushState.
issue_895175.html
344 bytes View Download

Comment 5 Deleted

The NextAction date has arrived: 2018-10-29
Cc: mastiz@chromium.org pkotw...@chromium.org sky@chromium.org
Labels: -Needs-Feedback
Status: Untriaged (was: Unconfirmed)
Confirmed.

Add //components/favicon/ owners.

Labels: OS-Chrome OS-Linux OS-Mac OS-Windows
Status: Available (was: Untriaged)
Thanks all for reporting. I suspect this issue is unrelated to history.pushState() and repros on desktop only.

Short answer: the issue should go away if the site provided 16x16 and 32x32 icons (dependent on screen density).

Wrt comment 4: the provided example differs from the original report because the favicons don't provide a 'sizes' hint. It's still a legit feature request, but a different one, and better tracked in https://bugs.chromium.org/p/chromium/issues/detail?id=672076.

If I read the twitter thread correctly, all icon paths are absolute and they don't change from one page load to another (well, history.pushState()). I'll assume that the page's URL itself is dynamically changing.

In such circumstances, Chrome will sort the listed favicons according to their |sizes| (most desirable first) and process them one by one. At this point, the reported scenario can be reproduced on desktop, *if* no ideal favicon exists (based on [1]), which probably means 16x16 or 32x32. On desktop, we don't optimize these loads.

Fixing this should be relatively easy (essentially do something similar to what we do for mobile), but we should be careful with candidates that do *not* provide a 'sizes' hint (perhaps they should score second-highest on desktop).

I'd be happy to review code contributions but cannot myself commit to any improvement.

[1] https://cs.chromium.org/chromium/src/components/favicon/core/favicon_handler.cc?l=241&rcl=02b4622b5182f80acd62e85aa17a0d86ad77ab8b 
In 72.0.3595.2 I no longer see excess icon requests, sizes hint or not.
Project Member

Comment 10 by bugdroid1@chromium.org, Nov 26

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e9abde5a831c773c8e0ab85605254a591246925f

commit e9abde5a831c773c8e0ab85605254a591246925f
Author: Mikel Astiz <mastiz@chromium.org>
Date: Mon Nov 26 10:14:50 2018

Avoid unnecessary favicon downloads on desktop

Prior to this patch, desktop favicon downloading would only stop when
a truly ideal icon is found, which means a favicon with perfect bitmaps
sizes for all supported screen densities (often a .ico file with a
16x16 AND a 32x32 bitmap). This often leaded to all icons being
downloaded, even for cases where it's obviously unnecessary.

The patch adopts the logic we currently use on mobile within
FaviconHandler::UpdateFaviconCandidate() in order to stop processing
favicon candidates if there is little hope that the next ones will be
any better.

This requires some smartness for favicon candidates without an explicit
'sizes' attribute, which would score very low prior to this patch and
hence would rarely be processed (if other candidates exist). Hence,
we special-case them for desktop and score them highest during initial
sorting, which mimics the behavior prior to this patch and guarantees
they will be processed unless an ideal favicon is found before.

It's rather trivial to rule out regressions for the following common
scenarios:
A. Mobile platforms: no behavioral changes as score stays zero for
   candidates without explicit 'sizes' attribute.
B. A single favicon is listed by the page.
C. Multiple favicons are listed and none have a 'sizes' attribute
   (sorting does nothing and all are processed unless an ideal one is
   found).
D. Multiple favicons are listed and they all have an accurate 'sizes'
   attribute (scenario improved in this patch).

The more complex scenario is when a web page lists multiple favicons
and only some have a 'sizes' attribute. In this case, there is some
minor risk for regressions given that the patch scores highest the
favicons without a 'sizes' attribute, so they will be downloaded and
processed earlier.

Bug:  895175 , 705900 , 672076 
Change-Id: I8216652ede25f55332cacc54d3e0a806fd8b7bc3
Reviewed-on: https://chromium-review.googlesource.com/c/1323551
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Peter Kotwicz <pkotwicz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#610800}
[modify] https://crrev.com/e9abde5a831c773c8e0ab85605254a591246925f/components/favicon/core/favicon_handler.cc
[modify] https://crrev.com/e9abde5a831c773c8e0ab85605254a591246925f/components/favicon/core/favicon_handler.h
[modify] https://crrev.com/e9abde5a831c773c8e0ab85605254a591246925f/components/favicon/core/favicon_handler_unittest.cc

Owner: mastiz@chromium.org
Status: Fixed (was: Available)
Should be fixed now.
Cc: y...@yoav.ws
 Issue 324820  has been merged into this issue.
Project Member

Comment 13 by bugdroid1@chromium.org, Nov 30

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4e76a579eae62db2bbb6ed808b1665076aa25815

commit 4e76a579eae62db2bbb6ed808b1665076aa25815
Author: Mikel Astiz <mastiz@chromium.org>
Date: Fri Nov 30 13:06:58 2018

Stop downloading favicons regardless of last outcome

The decision of whether the next candidate needs to be downloaded or not
should be independent of possible errors in the last download. Let's
move that logic to a dedicated predicate (ShouldDownloadNextCandidate())
and make sure it's honored in all codepaths (e.g. 404s,
or if image_skia.isNull()).

Bug:  895175 
Change-Id: Id4d41ea873b62559c3d191952045f9c714fe729f
Reviewed-on: https://chromium-review.googlesource.com/c/1350631
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Peter Kotwicz <pkotwicz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612625}
[modify] https://crrev.com/4e76a579eae62db2bbb6ed808b1665076aa25815/components/favicon/core/favicon_handler.cc
[modify] https://crrev.com/4e76a579eae62db2bbb6ed808b1665076aa25815/components/favicon/core/favicon_handler.h
[modify] https://crrev.com/4e76a579eae62db2bbb6ed808b1665076aa25815/components/favicon/core/favicon_handler_unittest.cc

Sign in to add a comment