[Subresource Filter] Localize all nav throttle creation in component

Subresource filter navigation throttle creation is currently distributed
between //chrome and //components. This CL moves the //chrome-level
logic into the //components-level logic in anticipation of reusing this
logic in WebLayer's subresource filter bringup. Without this change,
this logic would need to be duplicated unnecessarily in //weblayer.

As part of this change, I add a SubresourceFilterClient method to get
the SafeBrowsingDatabaseManager. Note that the conditional on whether
the safe browsing throttle is created would seem to be slightly
changed: currently it is done if the (//chrome-level)
SafeBrowsingService is not null, whereas in this CL it is changed to be
gated on whether the database manager returned by the
SafeBrowsingService is not null. However, the created throttle already
assumes that the database manager passed in from SafeBrowsingService is
not null (cf. SubresourceFilterSafeBrowsingClient), so the new check is
actually functionally equivalent to the old check (i.e., the database
manager will be null iff the safe browsing service is null).

Bug: 1116095
Change-Id: If71e59db4212482d05eb5eb49e50a7bebfee47cb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2440096
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: Charlie Harrison <csharrison@chromium.org>
Cr-Commit-Position: refs/heads/master@{#812618}

chrome/browser/chrome_content_browser_client.cc

@@ -4021,8 +4021,8 @@ ChromeContentBrowserClient::CreateThrottlesForNavigation(
   content::WebContents* web_contents = handle->GetWebContents();
   if (auto* subresource_filter_client =
           ChromeSubresourceFilterClient::FromWebContents(web_contents)) {
-    subresource_filter_client->MaybeAppendNavigationThrottles(handle,
-                                                              &throttles);
+    subresource_filter_client->GetThrottleManager()
+        ->MaybeAppendNavigationThrottles(handle, &throttles);
   }
 
 #if !defined(OS_ANDROID)

chrome/browser/subresource_filter/chrome_subresource_filter_client.cc

@@ -24,13 +24,10 @@
 #include "components/safe_browsing/core/db/database_manager.h"
 #include "components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.h"
 #include "components/subresource_filter/content/browser/ruleset_service.h"
-#include "components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.h"
 #include "components/subresource_filter/core/browser/subresource_filter_features.h"
 #include "components/subresource_filter/core/common/activation_decision.h"
 #include "components/subresource_filter/core/common/activation_scope.h"
 #include "components/subresource_filter/core/mojom/subresource_filter.mojom.h"
-#include "content/public/browser/browser_task_traits.h"
-#include "content/public/browser/browser_thread.h"
 #include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/render_frame_host.h"
 #include "services/metrics/public/cpp/ukm_source_id.h"
@@ -67,23 +64,6 @@ void ChromeSubresourceFilterClient::DidStartNavigation(
   }
 }
 
-void ChromeSubresourceFilterClient::MaybeAppendNavigationThrottles(
-    content::NavigationHandle* navigation_handle,
-    std::vector<std::unique_ptr<content::NavigationThrottle>>* throttles) {
-  safe_browsing::SafeBrowsingService* safe_browsing_service =
-      g_browser_process->safe_browsing_service();
-  if (navigation_handle->IsInMainFrame() && safe_browsing_service) {
-    throttles->push_back(
-        std::make_unique<subresource_filter::
-                             SubresourceFilterSafeBrowsingActivationThrottle>(
-            navigation_handle, this, content::GetIOThreadTaskRunner({}),
-            safe_browsing_service->database_manager()));
-  }
-
-  throttle_manager_->MaybeAppendNavigationThrottles(navigation_handle,
-                                                    throttles);
-}
-
 void ChromeSubresourceFilterClient::OnReloadRequested() {
   LogAction(SubresourceFilterAction::kAllowlistedSite);
   AllowlistByContentSettings(web_contents()->GetLastCommittedURL());
@@ -185,6 +165,14 @@ void ChromeSubresourceFilterClient::AllowlistByContentSettings(
   profile_context_->settings_manager()->AllowlistSite(top_level_url);
 }
 
+const scoped_refptr<safe_browsing::SafeBrowsingDatabaseManager>
+ChromeSubresourceFilterClient::GetSafeBrowsingDatabaseManager() {
+  safe_browsing::SafeBrowsingService* safe_browsing_service =
+      g_browser_process->safe_browsing_service();
+  return safe_browsing_service ? safe_browsing_service->database_manager()
+                               : nullptr;
+}
+
 void ChromeSubresourceFilterClient::ToggleForceActivationInCurrentWebContents(
     bool force_activation) {
   if (!activated_via_devtools_ && force_activation)
@@ -192,7 +180,7 @@ void ChromeSubresourceFilterClient::ToggleForceActivationInCurrentWebContents(
   activated_via_devtools_ = force_activation;
 }
 
-const subresource_filter::ContentSubresourceFilterThrottleManager*
+subresource_filter::ContentSubresourceFilterThrottleManager*
 ChromeSubresourceFilterClient::GetThrottleManager() const {
   return throttle_manager_.get();
 }

chrome/browser/subresource_filter/chrome_subresource_filter_client.h

@@ -19,7 +19,6 @@ class SubresourceFilterProfileContext;
 
 namespace content {
 class NavigationHandle;
-class NavigationThrottle;
 class WebContents;
 }  // namespace content
 
@@ -64,10 +63,6 @@ class ChromeSubresourceFilterClient
   explicit ChromeSubresourceFilterClient(content::WebContents* web_contents);
   ~ChromeSubresourceFilterClient() override;
 
-  void MaybeAppendNavigationThrottles(
-      content::NavigationHandle* navigation_handle,
-      std::vector<std::unique_ptr<content::NavigationThrottle>>* throttles);
-
   void OnReloadRequested();
 
   // content::WebContentsObserver:
@@ -83,6 +78,8 @@ class ChromeSubresourceFilterClient
   void OnAdsViolationTriggered(
       content::RenderFrameHost* rfh,
       subresource_filter::mojom::AdsViolation triggered_violation) override;
+  const scoped_refptr<safe_browsing::SafeBrowsingDatabaseManager>
+  GetSafeBrowsingDatabaseManager() override;
 
   // Should be called by devtools in response to a protocol command to enable ad
   // blocking in this WebContents. Should only persist while devtools is
@@ -93,7 +90,7 @@ class ChromeSubresourceFilterClient
     return did_show_ui_for_navigation_;
   }
 
-  const subresource_filter::ContentSubresourceFilterThrottleManager*
+  subresource_filter::ContentSubresourceFilterThrottleManager*
   GetThrottleManager() const;
 
   static void LogAction(SubresourceFilterAction action);

components/blocked_content/safe_browsing_triggered_popup_blocker_unittest.cc

@@ -67,6 +67,10 @@ class SafeBrowsingTriggeredPopupBlockerTest
   void OnAdsViolationTriggered(
       content::RenderFrameHost*,
       subresource_filter::mojom::AdsViolation) override {}
+  const scoped_refptr<safe_browsing::SafeBrowsingDatabaseManager>
+  GetSafeBrowsingDatabaseManager() override {
+    return nullptr;
+  }
 
   // content::RenderViewHostTestHarness:
   void SetUp() override {

components/subresource_filter/content/browser/BUILD.gn

@@ -107,6 +107,7 @@ source_set("unit_tests") {
     ":test_support",
     "//base/test:test_support",
     "//components/prefs:test_support",
+    "//components/safe_browsing/core/db:database_manager",
     "//components/safe_browsing/core/db:util",
     "//components/subresource_filter/content/common",
     "//components/subresource_filter/core/browser",

components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc

@@ -19,11 +19,14 @@
 #include "components/subresource_filter/content/browser/async_document_subresource_filter.h"
 #include "components/subresource_filter/content/browser/page_load_statistics.h"
 #include "components/subresource_filter/content/browser/subresource_filter_client.h"
+#include "components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.h"
 #include "components/subresource_filter/content/common/subresource_filter_messages.h"
 #include "components/subresource_filter/content/common/subresource_filter_utils.h"
 #include "components/subresource_filter/content/mojom/subresource_filter_agent.mojom.h"
 #include "components/subresource_filter/core/browser/subresource_filter_constants.h"
 #include "components/subresource_filter/core/common/common_features.h"
+#include "content/public/browser/browser_task_traits.h"
+#include "content/public/browser/browser_thread.h"
 #include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/navigation_throttle.h"
 #include "content/public/browser/render_frame_host.h"
@@ -308,6 +311,15 @@ void ContentSubresourceFilterThrottleManager::MaybeAppendNavigationThrottles(
     content::NavigationHandle* navigation_handle,
     std::vector<std::unique_ptr<content::NavigationThrottle>>* throttles) {
   DCHECK(!navigation_handle->IsSameDocument());
+
+  if (navigation_handle->IsInMainFrame() &&
+      client_->GetSafeBrowsingDatabaseManager()) {
+    throttles->push_back(
+        std::make_unique<SubresourceFilterSafeBrowsingActivationThrottle>(
+            navigation_handle, client_, content::GetIOThreadTaskRunner({}),
+            client_->GetSafeBrowsingDatabaseManager()));
+  }
+
   if (!dealer_handle_)
     return;
   if (auto filtering_throttle =

components/subresource_filter/content/browser/content_subresource_filter_throttle_manager_unittest.cc

@@ -21,6 +21,8 @@
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/time/time.h"
 #include "build/build_config.h"
+#include "components/safe_browsing/core/db/database_manager.h"
+#include "components/safe_browsing/core/db/test_database_manager.h"
 #include "components/subresource_filter/content/browser/async_document_subresource_filter.h"
 #include "components/subresource_filter/content/browser/subframe_navigation_test_utils.h"
 #include "components/subresource_filter/content/browser/subresource_filter_client.h"
@@ -104,6 +106,17 @@ class FakeSubresourceFilterAgent : public mojom::SubresourceFilterAgent {
   mojo::AssociatedReceiver<mojom::SubresourceFilterAgent> receiver_{this};
 };
 
+// Overrides the TestSafeBrowsingDatabaseManager methods that are
+// expected to be called to eliminate error logs.
+class CustomTestSafeBrowsingDatabaseManager
+    : public safe_browsing::TestSafeBrowsingDatabaseManager {
+ public:
+  bool IsSupported() const override { return false; }
+
+ private:
+  ~CustomTestSafeBrowsingDatabaseManager() override = default;
+};
+
 // Simple throttle that sends page-level activation to the manager for a
 // specific set of URLs.
 class MockPageStateActivationThrottle : public content::NavigationThrottle {
@@ -286,7 +299,14 @@ class ContentSubresourceFilterThrottleManagerTest
         navigation_handle, state));
     throttle_manager_->MaybeAppendNavigationThrottles(navigation_handle,
                                                       &throttles);
+
+    created_safe_browsing_throttle_for_last_navigation_ = false;
     for (auto& it : throttles) {
+      if (strcmp(it->GetNameForLogging(),
+                 "SubresourceFilterSafeBrowsingActivationThrottle") == 0) {
+        created_safe_browsing_throttle_for_last_navigation_ = true;
+      }
+
       navigation_handle->RegisterThrottleForTesting(std::move(it));
     }
   }
@@ -313,13 +333,28 @@ class ContentSubresourceFilterThrottleManagerTest
       content::RenderFrameHost* rfh,
       mojom::AdsViolation triggered_violation) override {}
 
+  const scoped_refptr<safe_browsing::SafeBrowsingDatabaseManager>
+  GetSafeBrowsingDatabaseManager() override {
+    return database_manager_;
+  }
+
   ContentSubresourceFilterThrottleManager* throttle_manager() {
     return throttle_manager_.get();
   }
 
+  bool created_safe_browsing_throttle_for_current_navigation() const {
+    return created_safe_browsing_throttle_for_last_navigation_;
+  }
+
+  void CreateSafeBrowsingDatabaseManager() {
+    database_manager_ =
+        base::MakeRefCounted<CustomTestSafeBrowsingDatabaseManager>();
+  }
+
  private:
   testing::TestRulesetCreator test_ruleset_creator_;
   testing::TestRulesetPair test_ruleset_pair_;
+  scoped_refptr<safe_browsing::SafeBrowsingDatabaseManager> database_manager_;
 
   std::unique_ptr<VerifiedRulesetDealer::Handle> dealer_handle_;
 
@@ -331,6 +366,8 @@ class ContentSubresourceFilterThrottleManagerTest
 
   std::unique_ptr<content::NavigationSimulator> navigation_simulator_;
 
+  bool created_safe_browsing_throttle_for_last_navigation_ = false;
+
   // Incremented on every OnFirstSubresourceLoadDisallowed call.
   int disallowed_notification_count_ = 0;
 
@@ -777,6 +814,29 @@ TEST_P(ContentSubresourceFilterThrottleManagerTest,
   EXPECT_EQ(0, disallowed_notification_count());
 }
 
+TEST_F(ContentSubresourceFilterThrottleManagerTest,
+       SafeBrowsingThrottleCreation) {
+  // If no safe browsing database is present, the throttle should not be
+  // created on a navigation.
+  NavigateAndCommitMainFrame(GURL(kTestURLWithNoActivation));
+  EXPECT_FALSE(created_safe_browsing_throttle_for_current_navigation());
+
+  CreateSafeBrowsingDatabaseManager();
+
+  // With a safe browsing database present, the throttle should be created on
+  // a main frame navigation.
+  NavigateAndCommitMainFrame(GURL(kTestURLWithNoActivation));
+  EXPECT_TRUE(created_safe_browsing_throttle_for_current_navigation());
+
+  // However, it still should not be created on a subframe navigation.
+  CreateSubframeWithTestNavigation(
+      GURL("https://www.example.com/disallowed.html"), main_rfh());
+  EXPECT_EQ(content::NavigationThrottle::PROCEED,
+            SimulateStartAndGetResult(navigation_simulator()));
+
+  EXPECT_FALSE(created_safe_browsing_throttle_for_current_navigation());
+}
+
 TEST_F(ContentSubresourceFilterThrottleManagerTest, LogActivation) {
   // This test assumes that we're not in DryRun mode.
   base::test::ScopedFeatureList scoped_feature;

components/subresource_filter/content/browser/subresource_filter_client.h

@@ -5,6 +5,7 @@
 #ifndef COMPONENTS_SUBRESOURCE_FILTER_CONTENT_BROWSER_SUBRESOURCE_FILTER_CLIENT_H_
 #define COMPONENTS_SUBRESOURCE_FILTER_CONTENT_BROWSER_SUBRESOURCE_FILTER_CLIENT_H_
 
+#include "base/memory/scoped_refptr.h"
 #include "components/subresource_filter/content/browser/verified_ruleset_dealer.h"
 #include "components/subresource_filter/core/common/activation_decision.h"
 #include "components/subresource_filter/core/mojom/subresource_filter.mojom.h"
@@ -15,6 +16,10 @@ namespace content {
 class NavigationHandle;
 }  // namespace content
 
+namespace safe_browsing {
+class SafeBrowsingDatabaseManager;
+}
+
 namespace subresource_filter {
 
 class SubresourceFilterClient {
@@ -43,6 +48,11 @@ class SubresourceFilterClient {
   virtual void OnAdsViolationTriggered(
       content::RenderFrameHost* rfh,
       mojom::AdsViolation triggered_violation) = 0;
+
+  // Returns the SafeBrowsingDatabaseManager instance associated with this
+  // client, or null if there is no such instance.
+  virtual const scoped_refptr<safe_browsing::SafeBrowsingDatabaseManager>
+  GetSafeBrowsingDatabaseManager() = 0;
 };
 
 }  // namespace subresource_filter

components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc

@@ -19,6 +19,7 @@
 #include "base/test/test_mock_time_task_runner.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "build/build_config.h"
+#include "components/safe_browsing/core/db/database_manager.h"
 #include "components/safe_browsing/core/db/test_database_manager.h"
 #include "components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.h"
 #include "components/subresource_filter/content/browser/fake_safe_browsing_database_manager.h"
@@ -90,6 +91,9 @@ class MockSubresourceFilterClient : public SubresourceFilterClient {
   MOCK_METHOD2(OnAdsViolationTriggered,
                void(content::RenderFrameHost*,
                     subresource_filter::mojom::AdsViolation));
+  MOCK_METHOD0(
+      GetSafeBrowsingDatabaseManager,
+      const scoped_refptr<safe_browsing::SafeBrowsingDatabaseManager>());
 
   void AllowlistInCurrentWebContents(const GURL& url) {
     ASSERT_TRUE(url.SchemeIsHTTPOrHTTPS());