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}

components/history/core/browser/history_backend.cc

@@ -17,7 +17,6 @@
 #include "base/bind.h"
 #include "base/compiler_specific.h"
 #include "base/debug/dump_without_crashing.h"
-#include "base/feature_list.h"
 #include "base/files/file_enumerator.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_macros.h"
@@ -75,6 +74,11 @@ using syncer::ModelTypeChangeProcessor;
 
 namespace history {
 
+// TODO(crbug.com/746268): Clean up this toggle after impact is measured via
+// Finch, which is expected to be neutral.
+const base::Feature kAvoidStrippingRefFromFaviconPageUrls{
+    "AvoidStrippingRefFromFaviconPageUrls", base::FEATURE_DISABLED_BY_DEFAULT};
+
 namespace {
 
 void RunUnlessCanceled(
@@ -2149,7 +2153,8 @@ bool HistoryBackend::SetFaviconMappingsForPageAndRedirects(
   GetCachedRecentRedirects(page_url, &redirects);
   bool mappings_changed = SetFaviconMappingsForPages(redirects, icon_type,
                                                      icon_ids);
-  if (page_url.has_ref()) {
+  if (page_url.has_ref() &&
+      !base::FeatureList::IsEnabled(kAvoidStrippingRefFromFaviconPageUrls)) {
     // Refs often gets added by Javascript, but the redirect chain is keyed to
     // the URL without a ref.
     // TODO(crbug.com/746268): This can cause orphan favicons, i.e. without a

components/history/core/browser/history_backend.h

@@ -18,6 +18,7 @@
 #include "base/containers/flat_set.h"
 #include "base/containers/hash_tables.h"
 #include "base/containers/mru_cache.h"
+#include "base/feature_list.h"
 #include "base/files/file_path.h"
 #include "base/gtest_prod_util.h"
 #include "base/macros.h"
@@ -66,6 +67,8 @@ static const size_t kMaxFaviconsPerPage = 8;
 // the thumbnail database.
 static const size_t kMaxFaviconBitmapsPerIconURL = 8;
 
+extern const base::Feature kAvoidStrippingRefFromFaviconPageUrls;
+
 // Keeps track of a queued HistoryDBTask. This class lives solely on the
 // DB thread.
 class QueuedHistoryDBTask {

components/history/core/browser/history_backend_unittest.cc

@@ -30,6 +30,7 @@
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/histogram_tester.h"
+#include "base/test/scoped_feature_list.h"
 #include "base/test/scoped_task_environment.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "build/build_config.h"
@@ -1870,6 +1871,23 @@ TEST_F(HistoryBackendTest, SetFaviconMappingsForPageAndRedirectsWithFragment) {
   EXPECT_EQ(1u, NumIconMappingsForPageURL(url3, favicon_base::FAVICON));
 }
 
+TEST_F(HistoryBackendTest,
+       SetFaviconMappingsForPageAndRedirectsWithFragmentWithoutStripping) {
+  base::test::ScopedFeatureList override_features;
+  override_features.InitAndEnableFeature(kAvoidStrippingRefFromFaviconPageUrls);
+
+  const GURL url("http://www.google.com#abc");
+  const GURL url_without_ref("http://www.google.com");
+  const GURL icon_url("http://www.google.com/icon");
+  backend_->SetFavicons(
+      {url}, favicon_base::FAVICON, icon_url,
+      std::vector<SkBitmap>{CreateBitmap(SK_ColorBLUE, kSmallEdgeSize)});
+
+  EXPECT_EQ(1u, NumIconMappingsForPageURL(url, favicon_base::FAVICON));
+  EXPECT_EQ(0u,
+            NumIconMappingsForPageURL(url_without_ref, favicon_base::FAVICON));
+}
+
 // Test that |recent_redirects_| stores the full redirect chain in case of
 // client redirects. In this case, a server-side redirect is followed by a
 // client-side one.