Fix URLChecker DCHECK/segfaults

Fixes use-after-free issues in URLChecker and
KidsManagementURLCheckerClient.

Asynchronous callbacks passed to unowned objects should be bound with
weak pointers, so if `this` is destroyed, the callback won't fire.

Test:
* As a parent, set a child's Safe Site setting to "Allow all sites"
* Add the child to a Chromebook
This CL fixes DCHECKs and (in prod) potential use-after-free segfaults
that could otherwise occur when running the steps above.

Bug: 1159898
Change-Id: I67ba8f924ded3b6316813529e9b77a3d3735b483
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2597547Reviewed-by: Aga Wronska <agawronska@chromium.org>
Reviewed-by: Toby Huang <tobyhuang@chromium.org>
Commit-Queue: Aga Wronska <agawronska@chromium.org>
Cr-Commit-Position: refs/heads/master@{#843171}

chrome/browser/supervised_user/kids_management_url_checker_client.cc

@@ -51,7 +51,7 @@ void KidsManagementURLCheckerClient::CheckURL(const GURL& url,
   kids_chrome_management_client->ClassifyURL(
       std::move(classify_url_request),
       base::BindOnce(&KidsManagementURLCheckerClient::ConvertResponseCallback,
-                     base::Unretained(this), url, std::move(callback)));
+                     weak_factory_.GetWeakPtr(), url, std::move(callback)));
 }
 
 void KidsManagementURLCheckerClient::ConvertResponseCallback(

chrome/browser/supervised_user/kids_management_url_checker_client.h

@@ -9,6 +9,7 @@
 #include <string>
 
 #include "base/macros.h"
+#include "base/memory/weak_ptr.h"
 #include "chrome/browser/supervised_user/kids_chrome_management/kids_chrome_management_client.h"
 #include "components/safe_search_api/url_checker_client.h"
 
@@ -44,6 +45,8 @@ class KidsManagementURLCheckerClient
 
   const std::string country_;
 
+  base::WeakPtrFactory<KidsManagementURLCheckerClient> weak_factory_{this};
+
   DISALLOW_COPY_AND_ASSIGN(KidsManagementURLCheckerClient);
 };
 

chrome/browser/supervised_user/kids_management_url_checker_client_unittest.cc

@@ -30,6 +30,8 @@
 #include "components/user_manager/scoped_user_manager.h"
 #endif
 
+using testing::_;
+
 namespace {
 
 using kids_chrome_management::ClassifyUrlResponse;
@@ -68,7 +70,9 @@ class KidsChromeManagementClientForTesting : public KidsChromeManagementClient {
       std::unique_ptr<kids_chrome_management::ClassifyUrlRequest> request_proto,
       KidsChromeManagementClient::KidsChromeManagementCallback callback)
       override {
-    std::move(callback).Run(std::move(response_proto_), error_code_);
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE, base::BindOnce(std::move(callback),
+                                  std::move(response_proto_), error_code_));
   }
 
   void SetupResponse(std::unique_ptr<ClassifyUrlResponse> response_proto,
@@ -134,12 +138,15 @@ class KidsManagementURLCheckerClientTest : public testing::Test {
         ->SetupResponse(std::move(response_proto), error_code);
   }
 
+  // Asynchronously checks the URL and waits until finished.
   void CheckURL(const GURL& url) {
-    url_classifier_->CheckURL(
-        url, base::BindOnce(&KidsManagementURLCheckerClientTest::OnCheckDone,
-                            base::Unretained(this)));
+    StartCheckURL(url);
+    task_environment_.RunUntilIdle();
   }
 
+  // Starts a URL check, but doesn't wait for ClassifyURL() to finish.
+  void CheckURLWithoutResponse(const GURL& url) { StartCheckURL(url); }
+
   MOCK_METHOD2(OnCheckDone,
                void(const GURL& url,
                     safe_search_api::ClientClassification classification));
@@ -154,6 +161,12 @@ class KidsManagementURLCheckerClientTest : public testing::Test {
 #endif
 
  private:
+  void StartCheckURL(const GURL& url) {
+    url_classifier_->CheckURL(
+        url, base::BindOnce(&KidsManagementURLCheckerClientTest::OnCheckDone,
+                            base::Unretained(this)));
+  }
+
   DISALLOW_COPY_AND_ASSIGN(KidsManagementURLCheckerClientTest);
 };
 
@@ -240,3 +253,16 @@ TEST_F(KidsManagementURLCheckerClientTest, ServiceError) {
   EXPECT_CALL(*this, OnCheckDone(url, classification));
   CheckURL(url);
 }
+
+TEST_F(KidsManagementURLCheckerClientTest, DestroyClientBeforeCallback) {
+  GURL url("http://randomurl7.com");
+
+  EXPECT_CALL(*this, OnCheckDone(_, _)).Times(0);
+  CheckURLWithoutResponse(url);
+
+  // Destroy the URLCheckerClient.
+  url_classifier_.reset();
+
+  // Now run the callback.
+  task_environment_.RunUntilIdle();
+}

components/safe_search_api/fake_url_checker_client.cc

@@ -7,6 +7,7 @@
 #include <utility>
 
 #include "base/callback.h"
+#include "base/threading/sequenced_task_runner_handle.h"
 
 namespace safe_search_api {
 
@@ -25,4 +26,10 @@ void FakeURLCheckerClient::RunCallback(ClientClassification classification) {
   std::move(callback_).Run(url_, classification);
 }
 
+void FakeURLCheckerClient::RunCallbackAsync(
+    ClientClassification classification) {
+  base::SequencedTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE, base::BindOnce(std::move(callback_), url_, classification));
+}
+
 }  // namespace safe_search_api

components/safe_search_api/fake_url_checker_client.h

@@ -23,10 +23,13 @@ class FakeURLCheckerClient : public URLCheckerClient {
   // See RunCallback() method documentation below on how to run the callback.
   void CheckURL(const GURL& url, ClientCheckCallback callback) override;
 
-  // Run the callback function input by the last call of CheckURL() with the
+  // Runs the callback function input by the last call of CheckURL() with the
   // result input with the last call of SetResult().
   void RunCallback(ClientClassification classification);
 
+  // Asynchronous version of RunCallback().
+  void RunCallbackAsync(ClientClassification classification);
+
  private:
   ClientCheckCallback callback_;
   GURL url_;

components/safe_search_api/url_checker.cc

@@ -47,11 +47,7 @@ URLChecker::Check::Check(const GURL& url, CheckCallback callback) : url(url) {
   callbacks.push_back(std::move(callback));
 }
 
-URLChecker::Check::~Check() {
-  for (const CheckCallback& callback : callbacks) {
-    DCHECK(!callback);
-  }
-}
+URLChecker::Check::~Check() = default;
 
 URLChecker::CheckResult::CheckResult(Classification classification,
                                      bool uncertain)
@@ -118,7 +114,7 @@ bool URLChecker::CheckURL(const GURL& url, CheckCallback callback) {
       std::make_unique<Check>(url, std::move(callback)));
   async_checker_->CheckURL(url,
                            base::BindOnce(&URLChecker::OnAsyncCheckComplete,
-                                          base::Unretained(this), it));
+                                          weak_factory_.GetWeakPtr(), it));
 
   return false;
 }

components/safe_search_api/url_checker.h

@@ -10,6 +10,7 @@
 
 #include "base/callback_forward.h"
 #include "base/containers/mru_cache.h"
+#include "base/memory/weak_ptr.h"
 #include "base/time/time.h"
 #include "components/safe_search_api/url_checker_client.h"
 #include "url/gurl.h"
@@ -71,6 +72,8 @@ class URLChecker {
   base::MRUCache<GURL, CheckResult> cache_;
   base::TimeDelta cache_timeout_;
 
+  base::WeakPtrFactory<URLChecker> weak_factory_{this};
+
   DISALLOW_COPY_AND_ASSIGN(URLChecker);
 };
 

components/safe_search_api/url_checker_unittest.cc

@@ -91,6 +91,7 @@ class SafeSearchURLCheckerTest : public testing::Test {
   size_t next_url_;
   FakeURLCheckerClient* fake_client_;
   std::unique_ptr<URLChecker> checker_;
+  base::test::SingleThreadTaskEnvironment task_environment_;
 };
 
 TEST_F(SafeSearchURLCheckerTest, Simple) {
@@ -196,4 +197,20 @@ TEST_F(SafeSearchURLCheckerTest, NoAllowAllGoogleURLs) {
   }
 }
 
+TEST_F(SafeSearchURLCheckerTest, DestroyURLCheckerBeforeCallback) {
+  GURL url(GetNewURL());
+  EXPECT_CALL(*this, OnCheckDone(_, _, _)).Times(0);
+
+  // Start a URL check.
+  ASSERT_FALSE(CheckURL(url));
+  fake_client_->RunCallbackAsync(
+      ToAPIClassification(Classification::SAFE, false));
+
+  // Reset the URLChecker before the callback occurs.
+  checker_.reset();
+
+  // The callback should now be invalid.
+  task_environment_.RunUntilIdle();
+}
+
 }  // namespace safe_search_api