Retrieve module information on a background task in ModuleEventSinkImpl

Retrieving this information requires IPC to the remote process, which
is blocking and thus must not be done on the UI thread.

But the ModuleEventSinkImpl instance must still live on the UI thread,
because thats where the RenderProcessHost instance lives. The
RenderProcessHost must be accessed to get the handle to the remote
process inside the ModuleEventSinkImpl::Create() factory function.

To make the process handle available from the background task, it is
now duplicated and passed to the task via the PostTaskWithTraits()
call.

Bug: 832286
Change-Id: I3948a585f3d7645d706b0e8977cdf3182f212306
Reviewed-on: https://chromium-review.googlesource.com/1058339Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Patrick Monette <pmonette@chromium.org>
Cr-Commit-Position: refs/heads/master@{#558758}

chrome/browser/chrome_browser_main_win.cc

@@ -345,7 +345,7 @@ void OnModuleEvent(const ModuleWatcher::ModuleEvent& event) {
         // task.
         base::PostTaskWithTraits(
             FROM_HERE,
-            {base::MayBlock(),
+            {base::MayBlock(), base::TaskPriority::BACKGROUND,
              base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN},
             base::Bind(&HandleModuleLoadEventWithoutTimeDateStamp,
                        event.module_path, event.module_size, load_address));

chrome/browser/chrome_content_browser_client.cc

@@ -3288,14 +3288,14 @@ void ChromeContentBrowserClient::ExposeInterfacesToRenderer(
     // Add the ModuleEventSink interface. This is the interface used by renderer
     // processes to notify the browser of modules in their address space. The
     // process handle is not yet available at this point so pass in a callback
-    // to allow it to be retrieved at the time the interface is actually
+    // to allow to retrieve a duplicate at the time the interface is actually
     // created. It is safe to pass a raw pointer to |render_process_host|: the
     // callback will be invoked in the context of ModuleDatabase::GetInstance,
     // which is invoked by Mojo initialization, which occurs while the
     // |render_process_host| is alive.
     auto get_process = base::BindRepeating(
-        [](content::RenderProcessHost* host) -> base::ProcessHandle {
-          return host->GetProcess().Handle();
+        [](content::RenderProcessHost* host) -> base::Process {
+          return host->GetProcess().Duplicate();
         },
         base::Unretained(render_process_host));
     // The ModuleDatabase is a global singleton so passing an unretained pointer

chrome/browser/conflicts/module_event_sink_impl_win.cc

@@ -16,6 +16,7 @@
 #include "base/files/file_path.h"
 #include "base/macros.h"
 #include "base/strings/string_piece.h"
+#include "base/task_scheduler/post_task.h"
 #include "chrome/browser/conflicts/module_database_win.h"
 #include "chrome/common/conflicts/module_watcher_win.h"
 #include "content/public/browser/browser_thread.h"
@@ -94,33 +95,14 @@ bool GetModuleTimeDateStamp(base::ProcessHandle process,
   return true;
 }
 
-}  // namespace
-
-ModuleEventSinkImpl::ModuleEventSinkImpl(base::ProcessHandle process,
-                                         content::ProcessType process_type,
-                                         ModuleDatabase* module_database)
-    : process_(process),
-      module_database_(module_database),
-      process_type_(process_type) {}
-
-ModuleEventSinkImpl::~ModuleEventSinkImpl() = default;
-
-// static
-void ModuleEventSinkImpl::Create(
-    GetProcessHandleCallback get_process_handle,
-    content::ProcessType process_type,
-    ModuleDatabase* module_database,
-    mojom::ModuleEventSinkRequest request) {
-  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  base::ProcessHandle process = get_process_handle.Run();
-  auto module_event_sink_impl = std::make_unique<ModuleEventSinkImpl>(
-      process, process_type, module_database);
-  mojo::MakeStrongBinding(std::move(module_event_sink_impl),
-                          std::move(request));
-}
-
-void ModuleEventSinkImpl::OnModuleEvent(mojom::ModuleEventType event_type,
-                                        uint64_t load_address) {
+// Handles the module event on a background task. Looks up the path, size and
+// time date stamp of the remote process and forwards the event to the
+// ModuleDatabase.
+void HandleModuleEvent(ModuleDatabase* module_database,
+                       base::Process process,
+                       content::ProcessType process_type,
+                       mojom::ModuleEventType event_Type,
+                       uint64_t load_address) {
   // Mojo takes care of validating |event_type|, so only |load_address| needs to
   // be checked. Load addresses must be aligned with the allocation granularity
   // which is at least 64KB on any supported Windows OS.
@@ -138,18 +120,54 @@ void ModuleEventSinkImpl::OnModuleEvent(mojom::ModuleEventType event_type,
   // Look up the various pieces of module metadata in the remote process.
 
   base::FilePath module_path;
-  if (!GetModulePath(process_, module, &module_path))
+  if (!GetModulePath(process.Handle(), module, &module_path))
     return;
 
   uint32_t module_size = 0;
-  if (!GetModuleSize(process_, module, &module_size))
+  if (!GetModuleSize(process.Handle(), module, &module_size))
     return;
 
   uint32_t module_time_date_stamp = 0;
-  if (!GetModuleTimeDateStamp(process_, load_address, &module_time_date_stamp))
+  if (!GetModuleTimeDateStamp(process.Handle(), load_address,
+                              &module_time_date_stamp))
     return;
 
   // Forward this to the module database.
-  module_database_->OnModuleLoad(process_type_, module_path, module_size,
-                                 module_time_date_stamp, load_address);
+  module_database->OnModuleLoad(process_type, module_path, module_size,
+                                module_time_date_stamp, load_address);
+}
+
+}  // namespace
+
+ModuleEventSinkImpl::ModuleEventSinkImpl(base::Process process,
+                                         content::ProcessType process_type,
+                                         ModuleDatabase* module_database)
+    : process_(std::move(process)),
+      module_database_(module_database),
+      process_type_(process_type) {}
+
+ModuleEventSinkImpl::~ModuleEventSinkImpl() = default;
+
+// static
+void ModuleEventSinkImpl::Create(GetProcessCallback get_process,
+                                 content::ProcessType process_type,
+                                 ModuleDatabase* module_database,
+                                 mojom::ModuleEventSinkRequest request) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  base::Process process = get_process.Run();
+  auto module_event_sink_impl = std::make_unique<ModuleEventSinkImpl>(
+      std::move(process), process_type, module_database);
+  mojo::MakeStrongBinding(std::move(module_event_sink_impl),
+                          std::move(request));
+}
+
+void ModuleEventSinkImpl::OnModuleEvent(mojom::ModuleEventType event_type,
+                                        uint64_t load_address) {
+  // Handle the event on a background sequence.
+  base::PostTaskWithTraits(
+      FROM_HERE,
+      {base::TaskPriority::BACKGROUND,
+       base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN, base::MayBlock()},
+      base::BindOnce(&HandleModuleEvent, module_database_, process_.Duplicate(),
+                     process_type_, event_type, load_address));
 }

chrome/browser/conflicts/module_event_sink_impl_win.h

@@ -8,7 +8,7 @@
 #include <stdint.h>
 
 #include "base/callback_forward.h"
-#include "base/process/process_handle.h"
+#include "base/process/process.h"
 #include "chrome/common/conflicts/module_event_sink_win.mojom.h"
 #include "content/public/common/process_type.h"
 
@@ -21,12 +21,12 @@ class ModuleEventSinkImpl : public mojom::ModuleEventSink {
  public:
   // Callback for retrieving the handle associated with a process. This is used
   // by "Create" to get a handle to the remote process.
-  using GetProcessHandleCallback = base::Callback<base::ProcessHandle()>;
+  using GetProcessCallback = base::Callback<base::Process()>;
 
   // Creates a service endpoint that forwards notifications from the remote
   // |process| of the provided |process_type| to the provided |module_database|.
   // The |module_database| must outlive this object.
-  ModuleEventSinkImpl(base::ProcessHandle process,
+  ModuleEventSinkImpl(base::Process process,
                       content::ProcessType process_type,
                       ModuleDatabase* module_database);
   ~ModuleEventSinkImpl() override;
@@ -35,7 +35,7 @@ class ModuleEventSinkImpl : public mojom::ModuleEventSink {
   // creates a concrete implementation of mojom::ModuleDatabase interface in the
   // current process, for the remote process represented by the provided
   // |request|. This should only be called on the UI thread.
-  static void Create(GetProcessHandleCallback get_process_handle,
+  static void Create(GetProcessCallback get_process,
                      content::ProcessType process_type,
                      ModuleDatabase* module_database,
                      mojom::ModuleEventSinkRequest request);
@@ -48,7 +48,7 @@ class ModuleEventSinkImpl : public mojom::ModuleEventSink {
   friend class ModuleEventSinkImplTest;
 
   // A handle to the process on the other side of the pipe.
-  base::ProcessHandle process_;
+  base::Process process_;
 
   // The module database this forwards events to. The |module_database| must
   // outlive this object.

chrome/browser/conflicts/module_event_sink_impl_win_unittest.cc

@@ -34,10 +34,21 @@ class ModuleEventSinkImplTest : public testing::Test {
         module_database_(std::make_unique<ModuleDatabase>(
             base::SequencedTaskRunnerHandle::Get())) {}
 
-  void CreateModuleSinkImpl() {
+  bool CreateModuleSinkImpl() {
+    HANDLE process_handle = 0;
+    if (!::DuplicateHandle(::GetCurrentProcess(), ::GetCurrentProcess(),
+                           ::GetCurrentProcess(), &process_handle,
+
+                                       0,
+                                       FALSE,
+                                       DUPLICATE_SAME_ACCESS)) {
+      return false;
+    }
+
     module_event_sink_impl_ = std::make_unique<ModuleEventSinkImpl>(
-        ::GetCurrentProcess(), content::PROCESS_TYPE_BROWSER,
+        base::Process(process_handle), content::PROCESS_TYPE_BROWSER,
         module_database_.get());
+    return true;
   }
 
   const ModuleDatabase::ModuleMap& modules() {
@@ -59,16 +70,18 @@ TEST_F(ModuleEventSinkImplTest, CallsForwardedAsExpected) {
 
   EXPECT_EQ(0u, modules().size());
 
-  CreateModuleSinkImpl();
+  ASSERT_TRUE(CreateModuleSinkImpl());
   EXPECT_EQ(0u, modules().size());
 
   // An invalid load event should not cause a module entry.
   module_event_sink_impl_->OnModuleEvent(
       mojom::ModuleEventType::MODULE_ALREADY_LOADED, kInvalidLoadAddress);
+  test_browser_thread_bundle_.RunUntilIdle();
   EXPECT_EQ(0u, modules().size());
 
   // A valid load event should cause a module entry.
   module_event_sink_impl_->OnModuleEvent(mojom::ModuleEventType::MODULE_LOADED,
                                          kValidLoadAddress);
+  test_browser_thread_bundle_.RunUntilIdle();
   EXPECT_EQ(1u, modules().size());
 }