Favicon dissappears if its HTML tag rendered before "apple-touch-icon"
Reported by
dmitryfi...@gmail.com,
Apr 16 2018
|
|||||||
Issue descriptionUserAgent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/65.0.3325.181 Safari/537.36 Example URL: Steps to reproduce the problem: 1. Open any website with both favicon and apple-touch-icon, e.g. https://medium.com/ 2. Force re-render of apple-touch-icon, e.g. by JS code in the console: document.querySelector("link[rel='apple-touch-icon']").href = document.querySelector("link[rel='apple-touch-icon']").href 3. Favicon will be disappeared. But if you force re-render of favicon tag after apple-touch-icon then favicon will appear: 4. Force re-render of favicon, e.g. document.querySelector("link[rel='icon']").href = document.querySelector("link[rel='icon']").href. 5. Favicon will be shown. What is the expected behavior? Favicon should not be affected at all. What went wrong? Favicon dissappears Does it occur on multiple sites: Yes Is it a problem with a plugin? No Did this work before? N/A Does this work in other browsers? Yes Chrome version: 65.0.3325.181 Channel: stable OS Version: 10.0 Flash Version: I reproduced it only with apple-touch-icon. Also Firefox and IE11 are OK.
,
Apr 17 2018
Thanks for filing the issue! Able to reproduce the issue on reported chrome version 65.0.3325.181 and on the latest canary 68.0.3397.0 using Windows 10, Ubuntu 14.04 and Mac 10.13.1. Bisect Information: =================== Last Good Build: 64.0.3256.0 First Bad Build: 64.0.3257.0 You are probably looking for a change made after 513427 (known good), but no later than 513428 (first known bad). CHANGELOG URL: https://chromium.googlesource.com/chromium/src/+log/8f55a147384647745bdde5d3b9f575d6a12a4ab7..dad5b484bf97a8dd7cf4fa8d7d4d68efbae6736d Suspecting: https://chromium.googlesource.com/chromium/src/+/dad5b484bf97a8dd7cf4fa8d7d4d68efbae6736d Review URL: https://chromium-review.googlesource.com/650786 @Mikel Astiz: Please help us in assigning it to the right owner if this is not related to your change.
,
Apr 17 2018
Taking a look, thanks for reporting! The suspect patch above is indeed the culprit. Before that patch, the bug existed in a different, less severe form (wrong favicon could be chosen). So I agree it's a regression.
,
Apr 17 2018
,
Apr 18 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/37e6023df7523f1339780f70fbb633382f686317 commit 37e6023df7523f1339780f70fbb633382f686317 Author: Mikel Astiz <mastiz@chromium.org> Date: Wed Apr 18 22:46:30 2018 Fix sending partial list of favicons on updates FrameHostMsg_UpdateFaviconURL() contains a list of favicons that the browser process expects to be complete (ContentFaviconDriver). On favicon changes via Javascript, handled via RenderFrameImpl::DidChangeIcon(), a partial list (filtered by icon type) was sent. This is tricky to handle and not worth optimizing for, so this patch proposes to instead always send the full list of favicons. Bug: 833476 Change-Id: Ie62f2ddcddfa740b470aecb767ae70991ca09eb7 Reviewed-on: https://chromium-review.googlesource.com/1013464 Reviewed-by: Kentaro Hara <haraken@chromium.org> Reviewed-by: Peter Kotwicz <pkotwicz@chromium.org> Commit-Queue: Mikel Astiz <mastiz@chromium.org> Cr-Commit-Position: refs/heads/master@{#551866} [modify] https://crrev.com/37e6023df7523f1339780f70fbb633382f686317/chrome/browser/favicon/content_favicon_driver_browsertest.cc [add] https://crrev.com/37e6023df7523f1339780f70fbb633382f686317/chrome/test/data/favicon/page_change_touch_icon_via_js.html [modify] https://crrev.com/37e6023df7523f1339780f70fbb633382f686317/content/renderer/render_frame_impl.cc [modify] https://crrev.com/37e6023df7523f1339780f70fbb633382f686317/content/renderer/render_frame_impl.h
,
Apr 25 2018
I haven't truly tests this on a dev channel release because there wasn't one, but a custom build with the patch applies does the job, so requesting a merge for M67.
,
Apr 26 2018
Your change meets the bar and is auto-approved for M67. Please go ahead and merge the CL to branch 3396 manually. Please contact milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Apr 26 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4a3b6476c2c54f83668d6df329e7ddfae6d7bbd1 commit 4a3b6476c2c54f83668d6df329e7ddfae6d7bbd1 Author: Mikel Astiz <mastiz@chromium.org> Date: Thu Apr 26 13:34:59 2018 Fix sending partial list of favicons on updates FrameHostMsg_UpdateFaviconURL() contains a list of favicons that the browser process expects to be complete (ContentFaviconDriver). On favicon changes via Javascript, handled via RenderFrameImpl::DidChangeIcon(), a partial list (filtered by icon type) was sent. This is tricky to handle and not worth optimizing for, so this patch proposes to instead always send the full list of favicons. Bug: 833476 Change-Id: Ie62f2ddcddfa740b470aecb767ae70991ca09eb7 Reviewed-on: https://chromium-review.googlesource.com/1013464 Reviewed-by: Kentaro Hara <haraken@chromium.org> Reviewed-by: Peter Kotwicz <pkotwicz@chromium.org> Commit-Queue: Mikel Astiz <mastiz@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#551866}(cherry picked from commit 37e6023df7523f1339780f70fbb633382f686317) Reviewed-on: https://chromium-review.googlesource.com/1030350 Reviewed-by: Mikel Astiz <mastiz@chromium.org> Cr-Commit-Position: refs/branch-heads/3396@{#321} Cr-Branched-From: 9ef2aa869bc7bc0c089e255d698cca6e47d6b038-refs/heads/master@{#550428} [modify] https://crrev.com/4a3b6476c2c54f83668d6df329e7ddfae6d7bbd1/chrome/browser/favicon/content_favicon_driver_browsertest.cc [add] https://crrev.com/4a3b6476c2c54f83668d6df329e7ddfae6d7bbd1/chrome/test/data/favicon/page_change_touch_icon_via_js.html [modify] https://crrev.com/4a3b6476c2c54f83668d6df329e7ddfae6d7bbd1/content/renderer/render_frame_impl.cc [modify] https://crrev.com/4a3b6476c2c54f83668d6df329e7ddfae6d7bbd1/content/renderer/render_frame_impl.h
,
May 14 2018
|
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by susan.boorgula@chromium.org
, Apr 16 2018