Remove the last SubresourceFilter message by moving it to mojom.

Create a SubresourceFilter interface in mojo. Adjust the tests so
that we can capture the send of the message to the MockRenderProcess.

BUG=993189,820612

Change-Id: I72f5d2ba3305955165fea2555271089812ada8ba
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2633353
Commit-Queue: Dave Tapuska <dtapuska@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Reviewed-by: Charlie Harrison <csharrison@chromium.org>
Cr-Commit-Position: refs/heads/master@{#844893}

components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc

@@ -21,7 +21,6 @@
 #include "components/subresource_filter/content/browser/profile_interaction_manager.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"

components/subresource_filter/content/browser/content_subresource_filter_throttle_manager_unittest.cc

@@ -27,7 +27,6 @@
 #include "components/subresource_filter/content/browser/subframe_navigation_test_utils.h"
 #include "components/subresource_filter/content/browser/subresource_filter_client.h"
 #include "components/subresource_filter/content/browser/subresource_filter_observer_manager.h"
-#include "components/subresource_filter/content/common/subresource_filter_messages.h"
 #include "components/subresource_filter/content/mojom/subresource_filter_agent.mojom.h"
 #include "components/subresource_filter/core/browser/subresource_filter_features.h"
 #include "components/subresource_filter/core/common/common_features.h"

components/subresource_filter/content/browser/ruleset_publisher_impl.cc

@@ -11,34 +11,25 @@
 #include "base/feature_list.h"
 #include "base/files/file_util.h"
 #include "base/location.h"
+#include "base/logging.h"
 #include "base/sequenced_task_runner.h"
 #include "base/task/thread_pool.h"
 #include "base/task_runner_util.h"
 #include "components/subresource_filter/content/browser/ruleset_service.h"
-#include "components/subresource_filter/content/common/subresource_filter_messages.h"
 #include "components/subresource_filter/core/browser/subresource_filter_constants.h"
 #include "components/subresource_filter/core/common/common_features.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/notification_service.h"
 #include "content/public/browser/notification_source.h"
 #include "content/public/browser/notification_types.h"
 #include "content/public/browser/render_process_host.h"
-#include "ipc/ipc_platform_file.h"
 
 namespace subresource_filter {
 
 namespace {
 
-void SendRulesetToRenderProcess(base::File* file,
-                                content::RenderProcessHost* rph) {
-  DCHECK(rph);
-  DCHECK(file);
-  DCHECK(file->IsValid());
-  rph->Send(new SubresourceFilterMsg_SetRulesetForProcess(
-      IPC::TakePlatformFileForTransit(file->Duplicate())));
-}
-
 // The file handle is closed when the argument goes out of scope.
 void CloseFile(base::File) {}
 
@@ -137,4 +128,18 @@ void RulesetPublisherImpl::Observe(
       content::Source<content::RenderProcessHost>(source).ptr());
 }
 
+void RulesetPublisherImpl::SendRulesetToRenderProcess(
+    base::File* file,
+    content::RenderProcessHost* rph) {
+  DCHECK(rph);
+  DCHECK(file);
+  DCHECK(file->IsValid());
+  if (!rph->GetChannel())
+    return;
+  mojo::AssociatedRemote<mojom::SubresourceFilterRulesetObserver>
+      subresource_filter;
+  rph->GetChannel()->GetRemoteAssociatedInterface(&subresource_filter);
+  subresource_filter->SetRulesetForProcess(file->Duplicate());
+}
+
 }  // namespace subresource_filter

components/subresource_filter/content/browser/ruleset_publisher_impl.h

@@ -18,6 +18,7 @@
 #include "components/subresource_filter/content/browser/verified_ruleset_dealer.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
+#include "content/public/browser/render_process_host.h"
 
 namespace subresource_filter {
 
@@ -51,6 +52,10 @@ class RulesetPublisherImpl : public RulesetPublisher,
   void IndexAndStoreAndPublishRulesetIfNeeded(
       const UnindexedRulesetInfo& unindex_ruleset_info);
 
+ protected:
+  virtual void SendRulesetToRenderProcess(base::File* file,
+                                          content::RenderProcessHost* rph);
+
  private:
   // content::NotificationObserver:
   void Observe(int type,

components/subresource_filter/content/browser/ruleset_publisher_impl_unittest.cc

@@ -23,7 +23,6 @@
 #include "base/threading/thread_task_runner_handle.h"
 #include "components/prefs/testing_pref_service.h"
 #include "components/subresource_filter/content/browser/ruleset_service.h"
-#include "components/subresource_filter/content/common/subresource_filter_messages.h"
 #include "components/subresource_filter/core/common/test_ruleset_creator.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/content_browser_client.h"
@@ -64,13 +63,6 @@ std::string ReadFileContentsToString(base::File* file) {
   return contents;
 }
 
-// Extracts and takes ownership of the ruleset file handle in the IPC message.
-base::File ExtractRulesetFromMessage(const IPC::Message* message) {
-  std::tuple<IPC::PlatformFileForTransit> arg;
-  SubresourceFilterMsg_SetRulesetForProcess::Read(message, &arg);
-  return IPC::PlatformFileForTransitToFile(std::get<0>(arg));
-}
-
 }  // namespace
 
 class SubresourceFilterRulesetPublisherImplTest : public ::testing::Test {
@@ -91,15 +83,11 @@ class SubresourceFilterRulesetPublisherImplTest : public ::testing::Test {
     return scoped_temp_dir_.GetPath().AppendASCII("data");
   }
 
-  void AssertSetRulesetForProcessMessageWithContent(
-      const IPC::Message* message,
-      const std::string& expected_contents) {
-    ASSERT_EQ(
-        static_cast<uint32_t>(SubresourceFilterMsg_SetRulesetForProcess::ID),
-        message->type());
-    base::File ruleset_file = ExtractRulesetFromMessage(message);
-    ASSERT_TRUE(ruleset_file.IsValid());
-    ASSERT_EQ(expected_contents, ReadFileContentsToString(&ruleset_file));
+  void AssertSetRulesetFileWithContent(base::File* ruleset_file,
+                                       const std::string& expected_contents) {
+    ASSERT_TRUE(ruleset_file);
+    ASSERT_TRUE(ruleset_file->IsValid());
+    ASSERT_EQ(expected_contents, ReadFileContentsToString(ruleset_file));
   }
 
  private:
@@ -111,13 +99,39 @@ class SubresourceFilterRulesetPublisherImplTest : public ::testing::Test {
   DISALLOW_COPY_AND_ASSIGN(SubresourceFilterRulesetPublisherImplTest);
 };
 
+class MockRulesetPublisherImpl : public RulesetPublisherImpl {
+ public:
+  template <typename... Args>
+  explicit MockRulesetPublisherImpl(Args&&... args)
+      : RulesetPublisherImpl(std::forward<Args>(args)...) {}
+  void SendRulesetToRenderProcess(
+      base::File* file,
+      content::RenderProcessHost* process) override {
+    last_file_[process] = file;
+    sent_count_++;
+  }
+
+  size_t RulesetSent() const { return sent_count_; }
+
+  base::File* RulesetFileForProcess(content::RenderProcessHost* process) {
+    auto it = last_file_.find(process);
+    if (it == last_file_.end())
+      return nullptr;
+    return it->second;
+  }
+
+ private:
+  size_t sent_count_ = 0;
+  std::map<content::RenderProcessHost*, base::File*> last_file_;
+};
+
 TEST_F(SubresourceFilterRulesetPublisherImplTest, NoRuleset_NoIPCMessages) {
   NotifyingMockRenderProcessHost existing_renderer(browser_context());
-  RulesetPublisherImpl service(nullptr, base::ThreadTaskRunnerHandle::Get());
+  MockRulesetPublisherImpl service(nullptr,
+                                   base::ThreadTaskRunnerHandle::Get());
   NotifyingMockRenderProcessHost new_renderer(browser_context());
   base::RunLoop().RunUntilIdle();
-  EXPECT_EQ(0u, existing_renderer.sink().message_count());
-  EXPECT_EQ(0u, new_renderer.sink().message_count());
+  EXPECT_EQ(0u, service.RulesetSent());
 }
 
 TEST_F(SubresourceFilterRulesetPublisherImplTest,
@@ -132,7 +146,8 @@ TEST_F(SubresourceFilterRulesetPublisherImplTest,
 
   NotifyingMockRenderProcessHost existing_renderer(browser_context());
   MockClosureTarget publish_callback_target;
-  RulesetPublisherImpl service(nullptr, base::ThreadTaskRunnerHandle::Get());
+  MockRulesetPublisherImpl service(nullptr,
+                                   base::ThreadTaskRunnerHandle::Get());
   service.SetRulesetPublishedCallbackForTesting(base::BindOnce(
       &MockClosureTarget::Call, base::Unretained(&publish_callback_target)));
   EXPECT_CALL(publish_callback_target, Call()).Times(1);
@@ -140,16 +155,16 @@ TEST_F(SubresourceFilterRulesetPublisherImplTest,
   base::RunLoop().RunUntilIdle();
   ::testing::Mock::VerifyAndClearExpectations(&publish_callback_target);
 
-  ASSERT_EQ(1u, existing_renderer.sink().message_count());
-  ASSERT_NO_FATAL_FAILURE(AssertSetRulesetForProcessMessageWithContent(
-      existing_renderer.sink().GetMessageAt(0), kTestFileContents));
+  ASSERT_EQ(2u, service.RulesetSent());
+  ASSERT_NO_FATAL_FAILURE(AssertSetRulesetFileWithContent(
+      service.RulesetFileForProcess(&existing_renderer), kTestFileContents));
 
   NotifyingMockRenderProcessHost second_renderer(browser_context());
   base::RunLoop().RunUntilIdle();
 
-  ASSERT_EQ(1u, second_renderer.sink().message_count());
-  ASSERT_NO_FATAL_FAILURE(AssertSetRulesetForProcessMessageWithContent(
-      second_renderer.sink().GetMessageAt(0), kTestFileContents));
+  ASSERT_EQ(3u, service.RulesetSent());
+  ASSERT_NO_FATAL_FAILURE(AssertSetRulesetFileWithContent(
+      service.RulesetFileForProcess(&second_renderer), kTestFileContents));
 }
 
 TEST_F(SubresourceFilterRulesetPublisherImplTest,
@@ -192,9 +207,10 @@ TEST_F(SubresourceFilterRulesetPublisherImplTest,
   NotifyingMockRenderProcessHost renderer_host(browser_context());
   base::RunLoop callback_waiter;
   auto content_service =
-      std::make_unique<RulesetPublisherImpl>(nullptr, blocking_task_runner);
+      std::make_unique<MockRulesetPublisherImpl>(nullptr, blocking_task_runner);
   content_service->SetRulesetPublishedCallbackForTesting(
       callback_waiter.QuitClosure());
+  auto* mock_publisher = content_service.get();
 
   // |RulesetService| constructor should read the last indexed ruleset version
   // and post ruleset setup on |blocking_task_runner|.
@@ -211,11 +227,11 @@ TEST_F(SubresourceFilterRulesetPublisherImplTest,
   callback_waiter.Run();
 
   // Check that the ruleset data is delivered to the renderer.
-  EXPECT_EQ(1u, renderer_host.sink().message_count());
+  ASSERT_EQ(2u, mock_publisher->RulesetSent());
   const std::string expected_data(ruleset.indexed.contents.begin(),
                                   ruleset.indexed.contents.end());
-  ASSERT_NO_FATAL_FAILURE(AssertSetRulesetForProcessMessageWithContent(
-      renderer_host.sink().GetMessageAt(0), expected_data));
+  ASSERT_NO_FATAL_FAILURE(AssertSetRulesetFileWithContent(
+      mock_publisher->RulesetFileForProcess(&renderer_host), expected_data));
 
   //
   // |RulesetPublisherImpl| destruction requires additional tricks. Its member

components/subresource_filter/content/browser/ruleset_service.cc

@@ -27,7 +27,6 @@
 #include "components/subresource_filter/content/browser/ruleset_publisher.h"
 #include "components/subresource_filter/content/browser/ruleset_publisher_impl.h"
 #include "components/subresource_filter/content/browser/unindexed_ruleset_stream_generator.h"
-#include "components/subresource_filter/content/common/subresource_filter_messages.h"
 #include "components/subresource_filter/core/browser/copying_file_stream.h"
 #include "components/subresource_filter/core/browser/subresource_filter_constants.h"
 #include "components/subresource_filter/core/browser/subresource_filter_features.h"

components/subresource_filter/content/common/BUILD.gn

@@ -6,9 +6,6 @@ static_library("common") {
   sources = [
     "ruleset_dealer.cc",
     "ruleset_dealer.h",
-    "subresource_filter_message_generator.cc",
-    "subresource_filter_message_generator.h",
-    "subresource_filter_messages.h",
     "subresource_filter_utils.cc",
     "subresource_filter_utils.h",
   ]

components/subresource_filter/content/common/subresource_filter_message_generator.cc

-// Copyright 2016 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.
-
-// Multiply-included file, hence no include guard.
-
-#include "components/subresource_filter/content/common/subresource_filter_message_generator.h"

components/subresource_filter/content/common/subresource_filter_message_generator.h

-// Copyright 2016 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.
-
-// Get basic type definitions.
-#define IPC_MESSAGE_IMPL
-#include "components/subresource_filter/content/common/subresource_filter_messages.h"
-
-// Generate constructors.
-#include "ipc/struct_constructor_macros.h"
-#include "components/subresource_filter/content/common/subresource_filter_messages.h"
-
-// Generate param traits write methods.
-#include "ipc/param_traits_write_macros.h"
-namespace IPC {
-#include "components/subresource_filter/content/common/subresource_filter_messages.h"
-}  // namespace IPC
-
-// Generate param traits read methods.
-#include "ipc/param_traits_read_macros.h"
-namespace IPC {
-#include "components/subresource_filter/content/common/subresource_filter_messages.h"
-}  // namespace IPC
-
-// Generate param traits log methods.
-#include "ipc/param_traits_log_macros.h"
-namespace IPC {
-#include "components/subresource_filter/content/common/subresource_filter_messages.h"
-}  // namespace IPC

components/subresource_filter/content/common/subresource_filter_messages.h

-// Copyright 2016 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.
-
-// Message definition file, included multiple times, hence no include guard.
-// no-include-guard-because-multiply-included
-
-#include "base/time/time.h"
-#include "content/public/common/common_param_traits_macros.h"
-#include "ipc/ipc_message.h"
-#include "ipc/ipc_message_macros.h"
-#include "ipc/ipc_platform_file.h"
-
-#define IPC_MESSAGE_START SubresourceFilterMsgStart
-
-// ----------------------------------------------------------------------------
-// Messages sent from the browser to the renderer.
-// ----------------------------------------------------------------------------
-
-// Sends a read-only mode file handle with the ruleset data to a renderer
-// process, containing the subresource filtering rules to be consulted for all
-// subsequent document loads that have subresource filtering activated.
-IPC_MESSAGE_CONTROL1(SubresourceFilterMsg_SetRulesetForProcess,
-                     IPC::PlatformFileForTransit /* ruleset_file */)

components/subresource_filter/content/renderer/BUILD.gn

@@ -18,6 +18,7 @@ static_library("renderer") {
     "//components/subresource_filter/content/common",
     "//components/subresource_filter/content/mojom",
     "//components/subresource_filter/core/common",
+    "//components/subresource_filter/core/mojom",
     "//content/public/common",
     "//content/public/renderer",
     "//third_party/blink/public:blink",

components/subresource_filter/content/renderer/subresource_filter_agent.cc

@@ -13,7 +13,6 @@
 #include "base/memory/ref_counted.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/time/time.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/renderer/unverified_ruleset_dealer.h"
 #include "components/subresource_filter/content/renderer/web_document_subresource_filter_impl.h"

components/subresource_filter/content/renderer/subresource_filter_agent_unittest.cc

@@ -12,7 +12,6 @@
 #include "base/strings/string_piece.h"
 #include "base/test/metrics/histogram_tester.h"
 #include "base/time/time.h"
-#include "components/subresource_filter/content/common/subresource_filter_messages.h"
 #include "components/subresource_filter/content/renderer/unverified_ruleset_dealer.h"
 #include "components/subresource_filter/core/common/memory_mapped_ruleset.h"
 #include "components/subresource_filter/core/common/scoped_timers.h"

components/subresource_filter/content/renderer/unverified_ruleset_dealer.cc

@@ -4,28 +4,38 @@
 
 #include "components/subresource_filter/content/renderer/unverified_ruleset_dealer.h"
 
-#include "components/subresource_filter/content/common/subresource_filter_messages.h"
-#include "ipc/ipc_message_macros.h"
+#include "third_party/blink/public/common/associated_interfaces/associated_interface_registry.h"
 
 namespace subresource_filter {
 
 UnverifiedRulesetDealer::UnverifiedRulesetDealer() = default;
 UnverifiedRulesetDealer::~UnverifiedRulesetDealer() = default;
 
-bool UnverifiedRulesetDealer::OnControlMessageReceived(
-    const IPC::Message& message) {
-  bool handled = true;
-  IPC_BEGIN_MESSAGE_MAP(UnverifiedRulesetDealer, message)
-    IPC_MESSAGE_HANDLER(SubresourceFilterMsg_SetRulesetForProcess,
-                        OnSetRulesetForProcess)
-    IPC_MESSAGE_UNHANDLED(handled = false)
-  IPC_END_MESSAGE_MAP()
-  return handled;
+void UnverifiedRulesetDealer::RegisterMojoInterfaces(
+    blink::AssociatedInterfaceRegistry* associated_interfaces) {
+  // base::Unretained can be used here because the associated_interfaces
+  // is owned by the RenderThread and will live for the duration of the
+  // RenderThread.
+  associated_interfaces->AddInterface(
+      base::BindRepeating(&UnverifiedRulesetDealer::OnRendererAssociatedRequest,
+                          base::Unretained(this)));
 }
 
-void UnverifiedRulesetDealer::OnSetRulesetForProcess(
-    const IPC::PlatformFileForTransit& platform_file) {
-  SetRulesetFile(IPC::PlatformFileForTransitToFile(platform_file));
+void UnverifiedRulesetDealer::UnregisterMojoInterfaces(
+    blink::AssociatedInterfaceRegistry* associated_interfaces) {
+  associated_interfaces->RemoveInterface(
+      mojom::SubresourceFilterRulesetObserver::Name_);
+}
+
+void UnverifiedRulesetDealer::SetRulesetForProcess(base::File ruleset_file) {
+  SetRulesetFile(std::move(ruleset_file));
+}
+
+void UnverifiedRulesetDealer::OnRendererAssociatedRequest(
+    mojo::PendingAssociatedReceiver<mojom::SubresourceFilterRulesetObserver>
+        receiver) {
+  receiver_.reset();
+  receiver_.Bind(std::move(receiver));
 }
 
 }  // namespace subresource_filter

components/subresource_filter/content/renderer/unverified_ruleset_dealer.h

@@ -7,12 +7,9 @@
 
 #include "base/macros.h"
 #include "components/subresource_filter/content/common/ruleset_dealer.h"
+#include "components/subresource_filter/core/mojom/subresource_filter.mojom.h"
 #include "content/public/renderer/render_thread_observer.h"
-#include "ipc/ipc_platform_file.h"
-
-namespace IPC {
-class Message;
-}  // namespace IPC
+#include "mojo/public/cpp/bindings/associated_receiver.h"
 
 namespace subresource_filter {
 
@@ -26,15 +23,28 @@ class MemoryMappedRuleset;
 // See RulesetDealerBase for details on the lifetime of MemoryMappedRuleset, and
 // the distribution pipeline diagram in content_ruleset_service.h.
 class UnverifiedRulesetDealer : public RulesetDealer,
-                                public content::RenderThreadObserver {
+                                public content::RenderThreadObserver,
+                                public mojom::SubresourceFilterRulesetObserver {
  public:
   UnverifiedRulesetDealer();
   ~UnverifiedRulesetDealer() override;
 
  private:
-  // content::RenderThreadObserver:
-  bool OnControlMessageReceived(const IPC::Message& message) override;
-  void OnSetRulesetForProcess(const IPC::PlatformFileForTransit& file);
+  // content::RenderThreadObserver overrides:
+  void RegisterMojoInterfaces(
+      blink::AssociatedInterfaceRegistry* associated_interfaces) override;
+  void UnregisterMojoInterfaces(
+      blink::AssociatedInterfaceRegistry* associated_interfaces) override;
+
+  // mojom::SubresourceFilterRulesetObserver overrides:
+  void SetRulesetForProcess(base::File ruleset_file) override;
+
+  void OnRendererAssociatedRequest(
+      mojo::PendingAssociatedReceiver<mojom::SubresourceFilterRulesetObserver>
+          receiver);
+
+  mojo::AssociatedReceiver<mojom::SubresourceFilterRulesetObserver> receiver_{
+      this};
 
   DISALLOW_COPY_AND_ASSIGN(UnverifiedRulesetDealer);
 };

components/subresource_filter/core/mojom/subresource_filter.mojom

@@ -4,6 +4,7 @@
 
 module subresource_filter.mojom;
 
+import "mojo/public/mojom/base/file.mojom";
 import "mojo/public/mojom/base/time.mojom";
 
 enum ActivationLevel {
@@ -94,3 +95,11 @@ interface SubresourceFilterHost {
   // ads violations within the ads intervention manager.
   OnAdsViolationTriggered(AdsViolation violation);
 };
+
+// Subresource filter implemented by the renderer.
+interface SubresourceFilterRulesetObserver {
+  // Sends a read-only mode file handle with the ruleset data to a renderer
+  // process, containing the subresource filtering rules to be consulted for all
+  // subsequent document loads that have subresource filtering activated.
+  SetRulesetForProcess(mojo_base.mojom.File ruleset_file);
+};

ipc/ipc_message_start.h

@@ -32,7 +32,6 @@ enum IPCMessageStart {
   GuestViewMsgStart,
   MediaPlayerDelegateMsgStart,
   ExtensionWorkerMsgStart,
-  SubresourceFilterMsgStart,
   LastIPCMsgStart  // Must come last.
 };
 

tools/ipc_fuzzer/message_lib/all_messages.h

@@ -22,7 +22,6 @@
 #include "components/nacl/common/nacl_host_messages.h"
 #endif
 #include "components/guest_view/common/guest_view_message_generator.h"
-#include "components/subresource_filter/content/common/subresource_filter_messages.h"
 #include "content/common/all_messages.h"
 #include "extensions/common/extension_message_generator.h"
 #include "gpu/ipc/common/gpu_message_generator.h"