Resolve crash caused by enterprise whitelist checking

In the previous implementation, a raw ProfileIOData* is passed around to
facilitate enterprise whitelist checking. However, it is possible (e.g.
during browser shutdown) the UrlCheckerDelegate outlives ProfileIOData,
thus causes crash when calling this raw pointer.

To fix this lifespan issue, we remove the ProfileIOData raw pointer that
is indirectly passed to URLCheckerDelegateImpl, and move all the
enterprise whitelist checking logic to earlier stage where we're sure
the ProfileIOData pointer is valid.

This CL doesn't change the existing behavior of enterprise whitelist
checking, therefore SafeBrowsingBlockingPageBrowserTest::
VerifyEnterpriseWhitelist still passes after this change with or without
enabling kNetworkService feature.

Bug: 818559
Change-Id: Idba3c03f90dad3a78e033c43016179b8cc6e5515
Reviewed-on: https://chromium-review.googlesource.com/969642
Commit-Queue: Jialiu Lin <jialiul@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: Varun Khaneja <vakh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#544922}

chrome/browser/chrome_content_browser_client.cc

@@ -3826,8 +3826,14 @@ ChromeContentBrowserClient::CreateURLLoaderThrottles(
   std::vector<std::unique_ptr<content::URLLoaderThrottle>> result;
 
 #if defined(SAFE_BROWSING_DB_LOCAL) || defined(SAFE_BROWSING_DB_REMOTE)
-  if (network_service_enabled ||
-      base::FeatureList::IsEnabled(safe_browsing::kCheckByURLLoaderThrottle)) {
+  ProfileIOData* io_data = ProfileIOData::FromResourceContext(resource_context);
+  bool matches_enterprise_whitelist =
+      io_data && safe_browsing::IsURLWhitelistedByPolicy(
+                     request.url, io_data->safe_browsing_whitelist_domains());
+  if (!matches_enterprise_whitelist &&
+      (network_service_enabled ||
+       base::FeatureList::IsEnabled(
+           safe_browsing::kCheckByURLLoaderThrottle))) {
     auto* delegate = GetSafeBrowsingUrlCheckerDelegate(resource_context);
     if (delegate && !delegate->ShouldSkipRequestCheck(
                         resource_context, request.url, frame_tree_node_id,
@@ -4249,9 +4255,9 @@ ChromeContentBrowserClient::GetSafeBrowsingUrlCheckerDelegate(
   // |safe_browsing_service_| may be unavailable in tests.
   if (safe_browsing_service_ && !safe_browsing_url_checker_delegate_) {
     safe_browsing_url_checker_delegate_ =
-        new safe_browsing::UrlCheckerDelegateImpl(
+        base::MakeRefCounted<safe_browsing::UrlCheckerDelegateImpl>(
             safe_browsing_service_->database_manager(),
-            safe_browsing_service_->ui_manager(), io_data);
+            safe_browsing_service_->ui_manager());
   }
 
   return safe_browsing_url_checker_delegate_.get();

chrome/browser/loader/safe_browsing_resource_throttle.cc

@@ -8,34 +8,40 @@
 #include "chrome/browser/safe_browsing/safe_browsing_service.h"
 #include "chrome/browser/safe_browsing/ui_manager.h"
 #include "chrome/browser/safe_browsing/url_checker_delegate_impl.h"
+#include "components/safe_browsing/common/safe_browsing_prefs.h"
 #include "components/safe_browsing/db/database_manager.h"
 #include "components/safe_browsing/db/v4_protocol_manager_util.h"
-#include "net/url_request/url_request.h"
 
 content::ResourceThrottle* MaybeCreateSafeBrowsingResourceThrottle(
     net::URLRequest* request,
     content::ResourceType resource_type,
     safe_browsing::SafeBrowsingService* sb_service,
-    ProfileIOData* io_data) {
+    const ProfileIOData* io_data) {
   if (!sb_service->database_manager()->IsSupported())
     return nullptr;
 
+  // No need to create resource throttle if request url is whitelisted by
+  // enterprise policy.
+  if (io_data &&
+      safe_browsing::IsURLWhitelistedByPolicy(
+          request->url(), io_data->safe_browsing_whitelist_domains())) {
+    return nullptr;
+  }
+
   return new SafeBrowsingParallelResourceThrottle(request, resource_type,
-                                                  sb_service, io_data);
+                                                  sb_service);
 }
 
 SafeBrowsingParallelResourceThrottle::SafeBrowsingParallelResourceThrottle(
     const net::URLRequest* request,
     content::ResourceType resource_type,
-    safe_browsing::SafeBrowsingService* sb_service,
-    ProfileIOData* io_data)
+    safe_browsing::SafeBrowsingService* sb_service)
     : safe_browsing::BaseParallelResourceThrottle(
           request,
           resource_type,
           base::MakeRefCounted<safe_browsing::UrlCheckerDelegateImpl>(
               sb_service->database_manager(),
-              sb_service->ui_manager(),
-              io_data)) {}
+              sb_service->ui_manager())) {}
 
 SafeBrowsingParallelResourceThrottle::~SafeBrowsingParallelResourceThrottle() =
     default;

chrome/browser/loader/safe_browsing_resource_throttle.h

@@ -28,7 +28,7 @@ content::ResourceThrottle* MaybeCreateSafeBrowsingResourceThrottle(
     net::URLRequest* request,
     content::ResourceType resource_type,
     safe_browsing::SafeBrowsingService* sb_service,
-    ProfileIOData* io_data);
+    const ProfileIOData* io_data);
 
 // SafeBrowsingParallelResourceThrottle uses a Chrome-specific
 // safe_browsing::UrlCheckerDelegate implementation with its base class
@@ -40,13 +40,12 @@ class SafeBrowsingParallelResourceThrottle
       net::URLRequest* request,
       content::ResourceType resource_type,
       safe_browsing::SafeBrowsingService* sb_service,
-      ProfileIOData* io_data);
+      const ProfileIOData* io_data);
 
   SafeBrowsingParallelResourceThrottle(
       const net::URLRequest* request,
       content::ResourceType resource_type,
-      safe_browsing::SafeBrowsingService* sb_service,
-      ProfileIOData* io_data);
+      safe_browsing::SafeBrowsingService* sb_service);
 
   ~SafeBrowsingParallelResourceThrottle() override;
 

chrome/browser/safe_browsing/url_checker_delegate_impl.cc

@@ -57,15 +57,14 @@ void StartDisplayingBlockingPage(
 
 UrlCheckerDelegateImpl::UrlCheckerDelegateImpl(
     scoped_refptr<SafeBrowsingDatabaseManager> database_manager,
-    scoped_refptr<SafeBrowsingUIManager> ui_manager,
-    const ProfileIOData* profile_io_data)
+    scoped_refptr<SafeBrowsingUIManager> ui_manager)
     : database_manager_(std::move(database_manager)),
       ui_manager_(std::move(ui_manager)),
       threat_types_(
           CreateSBThreatTypeSet({safe_browsing::SB_THREAT_TYPE_URL_MALWARE,
                                  safe_browsing::SB_THREAT_TYPE_URL_PHISHING,
-                                 safe_browsing::SB_THREAT_TYPE_URL_UNWANTED})),
-      profile_io_data_(profile_io_data) {}
+                                 safe_browsing::SB_THREAT_TYPE_URL_UNWANTED})) {
+}
 
 UrlCheckerDelegateImpl::~UrlCheckerDelegateImpl() = default;
 
@@ -89,10 +88,7 @@ void UrlCheckerDelegateImpl::StartDisplayingBlockingPageHelper(
 }
 
 bool UrlCheckerDelegateImpl::IsUrlWhitelisted(const GURL& url) {
-  return profile_io_data_
-             ? safe_browsing::IsURLWhitelistedByPolicy(
-                   url, profile_io_data_->safe_browsing_whitelist_domains())
-             : false;
+  return false;
 }
 
 bool UrlCheckerDelegateImpl::ShouldSkipRequestCheck(

chrome/browser/safe_browsing/url_checker_delegate_impl.h

@@ -9,7 +9,6 @@
 #include "base/memory/ref_counted.h"
 #include "components/safe_browsing/browser/url_checker_delegate.h"
 
-class ProfileIOData;
 namespace safe_browsing {
 
 class SafeBrowsingUIManager;
@@ -18,8 +17,7 @@ class UrlCheckerDelegateImpl : public UrlCheckerDelegate {
  public:
   UrlCheckerDelegateImpl(
       scoped_refptr<SafeBrowsingDatabaseManager> database_manager,
-      scoped_refptr<SafeBrowsingUIManager> ui_manager,
-      const ProfileIOData* io_data);
+      scoped_refptr<SafeBrowsingUIManager> ui_manager);
 
  private:
   ~UrlCheckerDelegateImpl() override;
@@ -49,7 +47,6 @@ class UrlCheckerDelegateImpl : public UrlCheckerDelegate {
   scoped_refptr<SafeBrowsingDatabaseManager> database_manager_;
   scoped_refptr<SafeBrowsingUIManager> ui_manager_;
   SBThreatTypeSet threat_types_;
-  const ProfileIOData* profile_io_data_;
 
   DISALLOW_COPY_AND_ASSIGN(UrlCheckerDelegateImpl);
 };