Make all favicon requests on-demand (fixes brave/internal#716)

This patch can be dropped once https://crbug.com/1096660 is fixed
as long as the test is retained and still passes.

chromium_src/components/history/core/browser/thumbnail_database_browsertest.cc

@@ -0,0 +1,189 @@
+/* Copyright (c) 2020 The Brave Authors. All rights reserved.
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
+ * You can obtain one at https://mozilla.org/MPL/2.0/. */
+
+#include "base/path_service.h"
+#include "brave/browser/brave_content_browser_client.h"
+#include "brave/common/brave_paths.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/common/chrome_content_client.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "chrome/test/base/ui_test_utils.h"
+#include "components/network_session_configurator/common/network_switches.h"
+#include "content/public/test/browser_test.h"
+#include "content/public/test/browser_test_utils.h"
+#include "content/public/test/test_utils.h"
+#include "net/dns/mock_host_resolver.h"
+#include "net/test/embedded_test_server/default_handlers.h"
+#include "net/test/embedded_test_server/http_request.h"
+#include "net/test/embedded_test_server/http_response.h"
+
+class BraveThumbnailDatabaseBrowserTest : public InProcessBrowserTest {
+ public:
+  BraveThumbnailDatabaseBrowserTest()
+      : https_server_(net::EmbeddedTestServer::TYPE_HTTPS) {}
+
+  void SetUpOnMainThread() override {
+    InProcessBrowserTest::SetUpOnMainThread();
+
+    content_client_.reset(new ChromeContentClient);
+    content::SetContentClient(content_client_.get());
+    browser_content_client_.reset(new BraveContentBrowserClient());
+    content::SetBrowserClientForTesting(browser_content_client_.get());
+
+    host_resolver()->AddRule("*", "127.0.0.1");
+
+    brave::RegisterPathProvider();
+    base::PathService::Get(brave::DIR_TEST_DATA, &test_data_dir_);
+    https_server_.ServeFilesFromDirectory(test_data_dir_);
+    https_server_.AddDefaultHandlers(GetChromeTestDataDir());
+    https_server_.RegisterRequestHandler(
+        base::BindRepeating(&BraveThumbnailDatabaseBrowserTest::HandleRequest,
+                            base::Unretained(this)));
+    https_server_.RegisterRequestMonitor(
+        base::BindRepeating(&BraveThumbnailDatabaseBrowserTest::SaveRequest,
+                            base::Unretained(this)));
+
+    ASSERT_TRUE(https_server_.Start());
+
+    fav0_url_ = https_server_.GetURL("fav0.a.com", "/favicon.ico");
+    fav1_url_ = https_server_.GetURL("fav1.a.com", "/favicon.ico");
+    fav2_url_ = https_server_.GetURL("fav2.a.com", "/favicon.ico");
+    fav3_url_ = https_server_.GetURL("fav3.a.com", "/favicon.ico");
+    landing_url_ = https_server_.GetURL("a.com", "/simple.html");
+    read_url_ = https_server_.GetURL("a.com", "/favicon_read.html");
+    set_url_ = https_server_.GetURL("a.com", "/favicon_set.html");
+  }
+
+  void SetUpCommandLine(base::CommandLine* command_line) override {
+    InProcessBrowserTest::SetUpCommandLine(command_line);
+    // This is needed to load pages from "domain.com" without an interstitial.
+    command_line->AppendSwitch(switches::kIgnoreCertificateErrors);
+  }
+
+  std::unique_ptr<net::test_server::HttpResponse> HandleRequest(
+      const net::test_server::HttpRequest& request) {
+    std::string relative_path(request.GetURL().path());
+    if (!base::EndsWith(relative_path, "/favicon.ico",
+                        base::CompareCase::SENSITIVE)) {
+      return nullptr;
+    }
+
+    std::unique_ptr<net::test_server::BasicHttpResponse> http_response(
+        new net::test_server::BasicHttpResponse());
+    http_response->set_code(net::HTTP_OK);
+
+    http_response->set_content_type("image/vnd.microsoft.icon");
+    std::string file_contents;
+    if (!base::ReadFileToString(test_data_dir_.AppendASCII("favicon.ico"),
+                                &file_contents)) {
+      return nullptr;
+    }
+    http_response->set_content(file_contents);
+
+    return std::move(http_response);
+  }
+
+  void SaveRequest(const net::test_server::HttpRequest& request) {
+    base::AutoLock auto_lock(save_request_lock_);
+
+    GURL requested_host("https://" + request.headers.at("Host"));
+    GURL::Replacements replace_host;
+    replace_host.SetHostStr(requested_host.host());
+
+    requests_.push_back(request.GetURL().ReplaceComponents(replace_host));
+  }
+
+  void ClearRequests() {
+    base::AutoLock auto_lock(save_request_lock_);
+    requests_.clear();
+  }
+
+  GURL GetLastRequest() {
+    base::AutoLock auto_lock(save_request_lock_);
+    return requests_.back();
+  }
+
+  bool WasRequested(const GURL& url) {
+    base::AutoLock auto_lock(save_request_lock_);
+    return std::find(requests_.begin(), requests_.end(), url.spec()) !=
+           requests_.end();
+  }
+
+  void TearDown() override {
+    browser_content_client_.reset();
+    content_client_.reset();
+  }
+
+  const net::EmbeddedTestServer& https_server() { return https_server_; }
+
+  const GURL& fav0_url() { return fav0_url_; }
+  const GURL& fav1_url() { return fav1_url_; }
+  const GURL& fav2_url() { return fav2_url_; }
+  const GURL& fav3_url() { return fav3_url_; }
+  const GURL& landing_url() { return landing_url_; }
+
+  GURL read_url(std::string uid) {
+    GURL::Replacements replace_query;
+    replace_query.SetQueryStr("uid=" + uid);
+    return read_url_.ReplaceComponents(replace_query);
+  }
+
+  GURL set_url(std::string values) {
+    GURL::Replacements replace_query;
+    replace_query.SetQueryStr("values=" + values);
+    return set_url_.ReplaceComponents(replace_query);
+  }
+
+  content::WebContents* contents() {
+    return browser()->tab_strip_model()->GetActiveWebContents();
+  }
+
+  void NavigateToURLAndWaitForRedirects(const GURL& url) {
+    ui_test_utils::UrlLoadObserver load_complete(
+        landing_url(), content::NotificationService::AllSources());
+    ui_test_utils::NavigateToURL(browser(), url);
+    EXPECT_EQ(contents()->GetMainFrame()->GetLastCommittedURL(), url);
+    load_complete.Wait();
+
+    EXPECT_EQ(contents()->GetLastCommittedURL(), landing_url());
+    EXPECT_EQ(GetLastRequest().path(), landing_url().path());
+
+    // Navigate again to make sure all of the favicons finished loading.
+    ui_test_utils::NavigateToURL(browser(), GURL("about:blank"));
+  }
+
+ private:
+  GURL fav0_url_;
+  GURL fav1_url_;
+  GURL fav2_url_;
+  GURL fav3_url_;
+  GURL landing_url_;
+  GURL read_url_;
+  GURL set_url_;
+  base::FilePath test_data_dir_;
+  std::unique_ptr<ChromeContentClient> content_client_;
+  std::unique_ptr<BraveContentBrowserClient> browser_content_client_;
+  std::vector<GURL> requests_;
+  mutable base::Lock save_request_lock_;
+
+  base::ScopedTempDir temp_user_data_dir_;
+  net::test_server::EmbeddedTestServer https_server_;
+};
+
+IN_PROC_BROWSER_TEST_F(BraveThumbnailDatabaseBrowserTest, SetRead1001) {
+  ClearRequests();
+  NavigateToURLAndWaitForRedirects(set_url("1001"));
+  EXPECT_EQ(WasRequested(fav0_url()), true);
+  EXPECT_EQ(WasRequested(fav1_url()), false);
+  EXPECT_EQ(WasRequested(fav2_url()), false);
+  EXPECT_EQ(WasRequested(fav3_url()), true);
+
+  ClearRequests();
+  NavigateToURLAndWaitForRedirects(read_url("read1001"));
+  EXPECT_EQ(WasRequested(fav0_url()), true);
+  EXPECT_EQ(WasRequested(fav1_url()), true);
+  EXPECT_EQ(WasRequested(fav2_url()), true);
+  EXPECT_EQ(WasRequested(fav3_url()), true);
+}

patches/components-history-core-browser-thumbnail_database.cc.patch

@@ -0,0 +1,31 @@
+diff --git a/components/history/core/browser/thumbnail_database.cc b/components/history/core/browser/thumbnail_database.cc
+index 19c7722a778b24fcc50e7ab9a4e5e368399bf52d..625f5be347adbbfeb207ba45880372435b1a4e80 100644
+--- a/components/history/core/browser/thumbnail_database.cc
++++ b/components/history/core/browser/thumbnail_database.cc
+@@ -519,6 +519,7 @@ FaviconBitmapID ThumbnailDatabase::AddFaviconBitmap(
+     base::Time time,
+     const gfx::Size& pixel_size) {
+   DCHECK(icon_id);
++  type = FaviconBitmapType::ON_DEMAND; // Make all icons ON_DEMAND.
+
+   sql::Statement statement(db_.GetCachedStatement(
+       SQL_FROM_HERE,
+@@ -573,8 +574,8 @@ bool ThumbnailDatabase::SetFaviconBitmap(
+   } else {
+     statement.BindNull(0);
+   }
+-  statement.BindInt64(1, time.ToDeltaSinceWindowsEpoch().InMicroseconds());
+-  statement.BindInt64(2, 0);
++  statement.BindInt64(1, 0);
++  statement.BindInt64(2, time.ToDeltaSinceWindowsEpoch().InMicroseconds());
+   statement.BindInt64(3, bitmap_id);
+
+   return statement.Run();
+@@ -583,6 +584,7 @@ bool ThumbnailDatabase::SetFaviconBitmap(
+ bool ThumbnailDatabase::SetFaviconBitmapLastUpdateTime(
+     FaviconBitmapID bitmap_id,
+     base::Time time) {
++  return true; // Avoid changing the icon type to ON_VISIT.
+   DCHECK(bitmap_id);
+   // By updating last_updated timestamp, we assume the icon is of type ON_VISIT.
+   // If it is ON_DEMAND, reset last_requested to 0 and thus silently change the

test/BUILD.gn

@@ -547,6 +547,7 @@ test("brave_browser_tests") {
     "//brave/chromium_src/chrome/browser/ui/views/location_bar/location_bar_view_browsertest.cc",
     "//brave/chromium_src/chrome/browser/ui/views/tabs/tab_hover_card_bubble_view_browsertest.cc",
     "//brave/chromium_src/components/content_settings/core/browser/brave_content_settings_registry_browsertest.cc",
+    "//brave/chromium_src/components/history/core/browser/thumbnail_database_browsertest.cc",
     "//brave/chromium_src/third_party/blink/public/platform/disable_client_hints_browsertest.cc",
     "//brave/chromium_src/third_party/blink/renderer/core/frame/reporting_observer_browsertest.cc",
     "//brave/chromium_src/third_party/blink/renderer/modules/battery/navigator_batterytest.cc",

test/data/favicon_read.html

@@ -0,0 +1,33 @@
+<html>
+<head>
+<title>Favicon Read</title>
+<script>
+function redirectToNextHop() {
+  const urlParams = new URLSearchParams(window.location.search);
+  const uid = urlParams.get('uid');
+
+  bit = parseInt(urlParams.get('bit') ? urlParams.get('bit') : "0");
+  nextBit = bit + 1;
+  if (bit > 3) {
+    url = new URL(window.location);
+    url.hostname = 'a.com';
+    url.pathname = '/simple.html';
+    url.search = '';
+    window.location = url.toString();
+  } else {
+    rand = parseInt(Math.random() * 1000000);
+    nextHop = new URL(window.location);
+    nextHop.hostname = 'fav' + bit + '.a.com';
+    nextHop.pathname = '/favicon_read.html';
+    nextHop.search = '?bit=' + nextBit + '&uid=' + uid + '&rand=' + rand;
+    window.location = nextHop.toString();
+  }
+}
+</script>
+</head>
+<body>
+<script>
+setTimeout(redirectToNextHop, 100);
+</script>
+</body>
+</html>

test/data/favicon_set.html

@@ -0,0 +1,38 @@
+<html>
+<head>
+<title>Favicon Set</title>
+<script>
+function redirectToNextHop() {
+  const urlParams = new URLSearchParams(window.location.search);
+  const values = urlParams.get('values');
+
+  bit = parseInt(urlParams.get('bit') ? urlParams.get('bit') : "0");
+  do {
+     if (values[bit] == '1') {
+       break;
+     }
+     bit += 1;
+  } while (bit < 3);
+
+  nextBit = bit + 1;
+  if (bit > 3) {
+    url = new URL(window.location);
+    url.hostname = 'a.com';
+    url.pathname = '/simple.html';
+    url.search = '';
+    window.location = url.toString();
+  } else {
+    nextHop = new URL(window.location);
+    nextHop.hostname = 'fav' + bit + '.a.com';
+    nextHop.search = '?bit=' + nextBit + '&values=' + values;
+    window.location = nextHop.toString();
+  }
+}
+</script>
+</head>
+<body>
+<script>
+setTimeout(redirectToNextHop, 100);
+</script>
+</body>
+</html>