Navigation to URLs with fragment can lead to cached favicons that never expire |
|||
Issue descriptionSteps to reproduce: 1. Type a URL in the omnibox that contains a fragment, e.g.: https://en.wikipedia.org/wiki/Fragment_identifier#Basics 2. Go to history UI and delete the entry (alternatively, wait weeks until history expires). Expected result: The corresponding favicon gets removed from the favicon database. Actual result: The corresponding favicon stays forever. This can be verified in: chrome://favicon/https://en.wikipedia.org/wiki/Fragment_identifier What's presumably going on: When a page URL includes a fragment (aka ref, e.g. #foo) HistoryBackend::SetFaviconMappingsForPageAndRedirects() associates it to the full URL as well as the stripped version which doesn't have a fragment. This is useful as documented in the CL that introduced the logic, https://codereview.chromium.org/1180403005 and the associated bug, where multiple sample sites are provided. This however assumes the URL without the fragment exists in history. If not, an orphan favicon will be stored in the database (i.e. no corresponding history entry exists), and it will never be deleted (except for full history deletion).
,
Oct 4 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/309fc52058a796fbc3936c54e903648cf56c5d50 commit 309fc52058a796fbc3936c54e903648cf56c5d50 Author: Mikel Astiz <mastiz@chromium.org> Date: Wed Oct 04 20:54:44 2017 Avoid leaking favicons when page URL contains fragments (aka refs) When updating icon->page URL mappings, the code has historically associated icons to both the original page URL as well as the fragment-stripped page URL (for page URLs that contain a fragment/ref). This can cause "orphan" entries in the favicon database, i.e. mappings that don't have a corresponding history entry (because the page URL without a fragment was never actually visited). This means the history expirer will never take care of cleaning them up, leaking until the user clears all history. The original logic was introduced to fix crbug.com/498618 which is no longer reproducible even after this patch. With recent improvements in client-side redirect handling (e.g. https://chromium-review.googlesource.com/569760 or https://chromium-review.googlesource.com/595977), it is conceivable that these historical heuristics are no longer needed. Since this is hard to prove, we put the new behavior behind a feature than can be experimented via Chrome variations. Bug: 746268 Change-Id: Ieefb0fdb175d8858f1055af2dcc54df2e6f00a9a Reviewed-on: https://chromium-review.googlesource.com/649691 Commit-Queue: Mikel Astiz <mastiz@chromium.org> Reviewed-by: Brett Wilson <brettw@chromium.org> Cr-Commit-Position: refs/heads/master@{#506510} [modify] https://crrev.com/309fc52058a796fbc3936c54e903648cf56c5d50/components/history/core/browser/history_backend.cc [modify] https://crrev.com/309fc52058a796fbc3936c54e903648cf56c5d50/components/history/core/browser/history_backend.h [modify] https://crrev.com/309fc52058a796fbc3936c54e903648cf56c5d50/components/history/core/browser/history_backend_unittest.cc
,
Dec 13 2017
Fixed?
,
Dec 13 2017
Let me clean up the feature toggle and then mark it as fixed.
,
Jan 15 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/58aeb4014e3e3ad7355b65b13af70519992f0621 commit 58aeb4014e3e3ad7355b65b13af70519992f0621 Author: Mikel Astiz <mastiz@chromium.org> Date: Mon Jan 15 17:08:22 2018 Default-enable and remove feature AvoidStrippingRefFromFaviconPageUrls We placed the toggle to rule out regressions in the quality of the icons on the NTP which looks good (neutral) in the dev&canary experiments. Let's go ahead and enable the logic by default and remove the feature toggle. Bug: 746268 Change-Id: I830c943100ebf89e7d9c788476aab26b714846c8 Reviewed-on: https://chromium-review.googlesource.com/827078 Commit-Queue: Mikel Astiz <mastiz@chromium.org> Reviewed-by: Peter Kotwicz <pkotwicz@chromium.org> Reviewed-by: Brett Wilson <brettw@chromium.org> Cr-Commit-Position: refs/heads/master@{#529303} [modify] https://crrev.com/58aeb4014e3e3ad7355b65b13af70519992f0621/chrome/browser/favicon/content_favicon_driver_browsertest.cc [delete] https://crrev.com/fbd29a85e0e8b69c62b1c29c25ed0f75f60c387b/chrome/test/data/favicon/page_with_hash_override.html [modify] https://crrev.com/58aeb4014e3e3ad7355b65b13af70519992f0621/components/history/core/browser/history_backend.cc [modify] https://crrev.com/58aeb4014e3e3ad7355b65b13af70519992f0621/components/history/core/browser/history_backend.h [modify] https://crrev.com/58aeb4014e3e3ad7355b65b13af70519992f0621/components/history/core/browser/history_backend_unittest.cc [modify] https://crrev.com/58aeb4014e3e3ad7355b65b13af70519992f0621/components/omnibox/browser/history_url_provider.cc
,
Jan 15 2018
|
|||
►
Sign in to add a comment |
|||
Comment 1 by dullweber@chromium.org
, Jul 19 2017Components: Privacy