favicons: fix manifest crash

This crash happens under the following circumstances:
1. on android.
2. request manifest from WebContents.
3. ~WebContentsImpl

As part of WebContentsImpl deletion any outstanding requests for
manifests are run. The callback scheduled at 2 may trigger running
code in ContentFaviconDriver that attempts to use web_contents().
Problem is at this time WebContentsImpl has already set the
web_contents() of ContentFaviconDriver to null, and we crash.

Fix is to route the callback to ContentFaviconDriver and have
ContentFaviconDriver do nothing if web_contents() is null.

BUG=1114237
TEST=ContentFaviconDriverTest.WebContentsDeletedWithInProgressManifestRequest

Change-Id: I0e88884c1ea7a8ffacb443b65ba09786cf4f54c2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2343718Reviewed-by: Peter Kotwicz <pkotwicz@chromium.org>
Commit-Queue: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#796889}

components/favicon/content/content_favicon_driver.cc

@@ -19,21 +19,6 @@
 #include "ui/gfx/image/image.h"
 
 namespace favicon {
-namespace {
-
-void ExtractManifestIcons(
-    ContentFaviconDriver::ManifestDownloadCallback callback,
-    const GURL& manifest_url,
-    const blink::Manifest& manifest) {
-  std::vector<FaviconURL> candidates;
-  for (const auto& icon : manifest.icons) {
-    candidates.emplace_back(icon.src, favicon_base::IconType::kWebManifestIcon,
-                            icon.sizes);
-  }
-  std::move(callback).Run(candidates);
-}
-
-}  // namespace
 
 // static
 void ContentFaviconDriver::CreateForWebContents(
@@ -80,7 +65,25 @@ ContentFaviconDriver::ContentFaviconDriver(content::WebContents* web_contents,
       FaviconDriverImpl(favicon_service),
       document_on_load_completed_(false) {}
 
-ContentFaviconDriver::~ContentFaviconDriver() {
+ContentFaviconDriver::~ContentFaviconDriver() = default;
+
+void ContentFaviconDriver::OnDidDownloadManifest(
+    ManifestDownloadCallback callback,
+    const GURL& manifest_url,
+    const blink::Manifest& manifest) {
+  // ~WebContentsImpl triggers running any pending callbacks for manifests.
+  // As we're about to be destroyed ignore the request. To do otherwise may
+  // result in calling back to this and attempting to use the WebContents, which
+  // will crash.
+  if (!web_contents())
+    return;
+
+  std::vector<FaviconURL> candidates;
+  for (const auto& icon : manifest.icons) {
+    candidates.emplace_back(icon.src, favicon_base::IconType::kWebManifestIcon,
+                            icon.sizes);
+  }
+  std::move(callback).Run(candidates);
 }
 
 int ContentFaviconDriver::DownloadImage(const GURL& url,
@@ -97,7 +100,8 @@ int ContentFaviconDriver::DownloadImage(const GURL& url,
 void ContentFaviconDriver::DownloadManifest(const GURL& url,
                                             ManifestDownloadCallback callback) {
   web_contents()->GetManifest(
-      base::BindOnce(&ExtractManifestIcons, std::move(callback)));
+      base::BindOnce(&ContentFaviconDriver::OnDidDownloadManifest,
+                     base::Unretained(this), std::move(callback)));
 }
 
 bool ContentFaviconDriver::IsOffTheRecord() {

components/favicon/content/content_favicon_driver.h

@@ -51,6 +51,11 @@ class ContentFaviconDriver
  private:
   friend class content::WebContentsUserData<ContentFaviconDriver>;
 
+  // Callback when a manifest is downloaded.
+  void OnDidDownloadManifest(ManifestDownloadCallback callback,
+                             const GURL& manifest_url,
+                             const blink::Manifest& manifest);
+
   // FaviconHandler::Delegate implementation.
   int DownloadImage(const GURL& url,
                     int max_image_size,

components/favicon/content/content_favicon_driver_unittest.cc

@@ -9,9 +9,11 @@
 
 #include "base/macros.h"
 #include "base/run_loop.h"
+#include "base/test/bind_test_util.h"
 #include "base/test/metrics/histogram_tester.h"
 #include "components/favicon/core/favicon_client.h"
 #include "components/favicon/core/favicon_handler.h"
+#include "components/favicon/core/test/favicon_driver_impl_test_helper.h"
 #include "components/favicon/core/test/mock_favicon_service.h"
 #include "content/public/browser/web_contents_observer.h"
 #include "content/public/test/navigation_simulator.h"
@@ -38,15 +40,16 @@ class ContentFaviconDriverTest : public content::RenderViewHostTestHarness {
   const GURL kIconURL = GURL("http://www.google.com/favicon.ico");
 
   ContentFaviconDriverTest() {
+    ContentFaviconDriverTest* t = this;
     ON_CALL(favicon_service_, UpdateFaviconMappingsAndFetch(_, _, _, _, _, _))
-        .WillByDefault([](auto, auto, auto, auto,
-                          favicon_base::FaviconResultsCallback callback,
-                          base::CancelableTaskTracker* tracker) {
+        .WillByDefault([t](auto, auto, auto, auto,
+                           favicon_base::FaviconResultsCallback callback,
+                           base::CancelableTaskTracker* tracker) {
           return tracker->PostTask(
               base::ThreadTaskRunnerHandle::Get().get(), FROM_HERE,
-              base::BindOnce(
-                  std::move(callback),
-                  std::vector<favicon_base::FaviconRawBitmapResult>()));
+              base::BindOnce(&ContentFaviconDriverTest::
+                                 OnCallUpdateFaviconMappingsAndFetch,
+                             base::Unretained(t), std::move(callback)));
         });
     ON_CALL(favicon_service_, GetFaviconForPageURL(_, _, _, _, _))
         .WillByDefault([](auto, auto, auto,
@@ -60,7 +63,7 @@ class ContentFaviconDriverTest : public content::RenderViewHostTestHarness {
         });
   }
 
-  ~ContentFaviconDriverTest() override {}
+  ~ContentFaviconDriverTest() override = default;
 
   // content::RenderViewHostTestHarness:
   void SetUp() override {
@@ -85,7 +88,21 @@ class ContentFaviconDriverTest : public content::RenderViewHostTestHarness {
     base::RunLoop().RunUntilIdle();
   }
 
+  // This is run *after* the callback posted by way of
+  // UpdateFaviconMappingsAndFetch() is run. It allows hooking processing
+  // immediately after it.
+  base::OnceClosure on_did_update_favicon_mappings_and_fetch_;
+
   testing::NiceMock<MockFaviconService> favicon_service_;
+
+ private:
+  // Callback from UpdateFaviconMappingsAndFetch().
+  void OnCallUpdateFaviconMappingsAndFetch(
+      favicon_base::FaviconResultsCallback callback) {
+    std::move(callback).Run({});
+    if (on_did_update_favicon_mappings_and_fetch_)
+      std::move(on_did_update_favicon_mappings_and_fetch_).Run();
+  }
 };
 
 // Test that a download is initiated when there isn't a favicon in the database
@@ -171,5 +188,39 @@ TEST_F(ContentFaviconDriverTest, FaviconUpdateNoLastCommittedEntry) {
   EXPECT_TRUE(driver->favicon_urls().empty());
 }
 
+// This test verifies a crash doesn't happen during deletion of the
+// WebContents. The crash occurred because ~WebContentsImpl would trigger
+// running callbacks for manifests. This mean FaviconHandler would be called
+// while ContentFaviconDriver::web_contents() was null, which is unexpected and
+// crashed. See https://crbug.com/1114237 for more.
+TEST_F(ContentFaviconDriverTest,
+       WebContentsDeletedWithInProgressManifestRequest) {
+  // Manifests are only downloaded with TOUCH_LARGEST. Force creating this
+  // handler so code path is exercised on all platforms.
+  favicon::ContentFaviconDriver* driver =
+      favicon::ContentFaviconDriver::FromWebContents(web_contents());
+  FaviconDriverImplTestHelper::RecreateHandlerForType(
+      driver, FaviconDriverObserver::TOUCH_LARGEST);
+
+  // Mimic a page load.
+  std::vector<blink::mojom::FaviconURLPtr> favicon_urls;
+  favicon_urls.push_back(blink::mojom::FaviconURL::New(
+      kIconURL, blink::mojom::FaviconIconType::kTouchIcon, kEmptyIconSizes));
+  TestFetchFaviconForPage(kPageURL, favicon_urls);
+
+  // Trigger downloading a manifest.
+  base::RunLoop run_loop;
+  on_did_update_favicon_mappings_and_fetch_ =
+      base::BindLambdaForTesting([&] { run_loop.Quit(); });
+  static_cast<content::WebContentsObserver*>(driver)->DidUpdateWebManifestURL(
+      web_contents()->GetMainFrame(), GURL("http://bad.manifest.com"));
+  run_loop.Run();
+
+  // The request for the manifest is still pending, delete the WebContents,
+  // which should trigger notifying the callback for the manifest and *not*
+  // crash.
+  DeleteContents();
+}
+
 }  // namespace
 }  // namespace favicon

components/favicon/core/favicon_driver_impl.cc

@@ -6,7 +6,6 @@
 
 #include <memory>
 
-#include "base/metrics/histogram_macros.h"
 #include "base/strings/string_util.h"
 #include "build/build_config.h"
 #include "components/favicon/core/core_favicon_service.h"
@@ -38,8 +37,7 @@ FaviconDriverImpl::FaviconDriverImpl(CoreFaviconService* favicon_service)
   }
 }
 
-FaviconDriverImpl::~FaviconDriverImpl() {
-}
+FaviconDriverImpl::~FaviconDriverImpl() = default;
 
 void FaviconDriverImpl::FetchFavicon(const GURL& page_url,
                                      bool is_same_document) {

components/favicon/core/favicon_driver_impl.h

@@ -51,12 +51,13 @@ class FaviconDriverImpl : public FaviconDriver,
                           const std::vector<FaviconURL>& candidates,
                           const GURL& manifest_url);
 
- protected:
   CoreFaviconService* favicon_service() { return favicon_service_; }
 
  private:
-  // KeyedService used by FaviconDriverImpl. It may be null, if non-null, it
-  // must outlive FaviconDriverImpl.
+  friend class FaviconDriverImplTestHelper;
+
+  // KeyedService used by FaviconDriverImpl. It may be null during testing,
+  // but if it is defined, it must outlive the FaviconDriverImpl.
   CoreFaviconService* favicon_service_;
 
   // FaviconHandlers used to download the different kind of favicons.

components/favicon/core/test/BUILD.gn

@@ -6,6 +6,8 @@ static_library("test_support") {
   testonly = true
 
   sources = [
+    "favicon_driver_impl_test_helper.cc",
+    "favicon_driver_impl_test_helper.h",
     "mock_favicon_service.cc",
     "mock_favicon_service.h",
   ]

components/favicon/core/test/favicon_driver_impl_test_helper.cc

+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "components/favicon/core/test/favicon_driver_impl_test_helper.h"
+
+#include <memory>
+
+#include "components/favicon/core/favicon_driver_impl.h"
+
+namespace favicon {
+
+// static
+void FaviconDriverImplTestHelper::RecreateHandlerForType(
+    FaviconDriverImpl* driver,
+    FaviconDriverObserver::NotificationIconType type) {
+  driver->handlers_.clear();
+  driver->handlers_.push_back(
+      std::make_unique<FaviconHandler>(driver->favicon_service_, driver, type));
+}
+
+}  // namespace favicon

components/favicon/core/test/favicon_driver_impl_test_helper.h

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef COMPONENTS_FAVICON_CORE_TEST_FAVICON_DRIVER_IMPL_TEST_HELPER_H_
+#define COMPONENTS_FAVICON_CORE_TEST_FAVICON_DRIVER_IMPL_TEST_HELPER_H_
+
+#include "components/favicon/core/favicon_driver_observer.h"
+
+namespace favicon {
+
+class FaviconDriverImpl;
+
+// Test helper for reaching into the internals of FaviconDriverImpl.
+class FaviconDriverImplTestHelper {
+ public:
+  FaviconDriverImplTestHelper(const FaviconDriverImplTestHelper&) = delete;
+  FaviconDriverImplTestHelper& operator=(const FaviconDriverImplTestHelper&) =
+      delete;
+  FaviconDriverImplTestHelper() = delete;
+
+  // Resets |driver->handler_| to a FaviconHandler of type |type|. This should
+  // be called at a time when there are no outstanding requests.
+  static void RecreateHandlerForType(
+      FaviconDriverImpl* driver,
+      FaviconDriverObserver::NotificationIconType type);
+};
+
+}  // namespace favicon
+
+#endif  // COMPONENTS_FAVICON_CORE_TEST_FAVICON_DRIVER_IMPL_TEST_HELPER_H_