[win] Create ModuleDatabase and ModuleEventSinkImpl.

chrome/browser/conflicts/module_event_sink_impl_win.cc

Index: chrome/browser/conflicts/module_event_sink_impl_win.cc
diff --git a/chrome/browser/conflicts/module_event_sink_impl_win.cc b/chrome/browser/conflicts/module_event_sink_impl_win.cc
new file mode 100644
index 0000000000000000000000000000000000000000..a981be392280bc8a5a17d8d3ba84d48efda489a1
--- /dev/null
+++ b/chrome/browser/conflicts/module_event_sink_impl_win.cc
@@ -0,0 +1,95 @@
+// 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.
+
+#include "chrome/browser/conflicts/module_event_sink_impl_win.h"
+
+#include <windows.h>
+#include <psapi.h>

[grt (UTC plus 2), 2016/12/20 11:11:34]

should we set PSAPI_VERSION=2 explicitly somewhere? do we already?

[chrisha, 2016/12/20 19:46:24]

We want PSAPI_VERSION=1, and we set it globally in our build config.

[grt (UTC plus 2), 2016/12/20 21:09:53]

Why 1? My brief read of MSDN made me think that version 2 means that PSAPI is
implemented in kernel32, so we avoid loading an extra dll that just jumps from
elsewhere into kernel32. I don't know what the other implications are, but that
seemed like a good thing to me. What does version 1 do for us now that we only
care about Win7+?

[chrisha, 2016/12/21 20:14:59]

I think this is a discussion for the mailing list. We may well want to update to
PSAPI_VERSION=2 now that we've dropped older OSes. I'm not going to touch that
in this CL, however.

+
+#include "base/bind.h"
+#include "base/files/file_path.h"
+#include "base/memory/ptr_util.h"
+#include "chrome/browser/conflicts/module_database_win.h"
+#include "chrome/common/conflicts/module_watcher_win.h"
+#include "content/public/browser/browser_thread.h"
+#include "mojo/public/cpp/bindings/strong_binding.h"
+
+ModuleEventSinkImpl::ModuleEventSinkImpl(base::ProcessHandle process,
+                                         content::ProcessType process_type,
+                                         ModuleDatabase* module_database)
+    : process_(process), module_database_(module_database), process_id_(0) {
+  process_id_ = ::GetProcessId(process_);
+  module_database->OnProcessStarted(process_id_, process_type);
+}
+
+ModuleEventSinkImpl::~ModuleEventSinkImpl() = default;
+
+// static
+void ModuleEventSinkImpl::Create(base::ProcessHandle process,
+                                 content::ProcessType process_type,
+                                 ModuleDatabase* module_database,
+                                 mojom::ModuleEventSinkRequest request) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  std::unique_ptr<ModuleEventSinkImpl> module_event_sink_impl =

[grt (UTC plus 2), 2016/12/20 11:11:34]

nit: i think "auto" is appropriate here since the type is clear

[chrisha, 2016/12/20 19:46:24]

Done.

+      base::MakeUnique<ModuleEventSinkImpl>(process, process_type,
+                                            module_database);
+  base::Closure error_handler = base::Bind(&ModuleDatabase::OnProcessEnded,

[grt (UTC plus 2), 2016/12/20 11:11:34]

#include "base/callback.h" for base::Closure (which is actually forward decl'd
in callback_forward.h, but you need the full definition here).

[chrisha, 2016/12/20 19:46:24]

Done.

+                                           base::Unretained(module_database),
+                                           module_event_sink_impl->process_id_);
+  auto binding = mojo::MakeStrongBinding(std::move(module_event_sink_impl),
+                                         std::move(request));
+  binding->set_connection_error_handler(error_handler);
+}
+
+void ModuleEventSinkImpl::OnModuleEvent(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.
+  if (load_address == 0 || load_address % (64 * 1024) != 0)
+    return;
+
+  switch (event_type) {
+    case mojom::ModuleEventType::MODULE_ALREADY_LOADED:
+    case mojom::ModuleEventType::MODULE_LOADED:
+      return OnModuleLoad(event_type, load_address);
+
+    case mojom::ModuleEventType::MODULE_UNLOADED:
+      return OnModuleUnload(load_address);
+
+    default:

[grt (UTC plus 2), 2016/12/20 11:11:34]

remove default case so that compilation fails if/when a new type is added

[chrisha, 2016/12/20 19:46:24]

Done.

+      NOTREACHED() << "Unknown ModuleEventType: " << event_type;
+  }
+}
+
+void ModuleEventSinkImpl::OnModuleLoad(mojom::ModuleEventType event_type,
+                                       uint64_t load_address) {
+  DCHECK(event_type == mojom::ModuleEventType::MODULE_ALREADY_LOADED ||
+         event_type == mojom::ModuleEventType::MODULE_LOADED);
+
+  // Convert the |load_address| to a module handle.
+  HMODULE module =
+      reinterpret_cast<HMODULE>(static_cast<uintptr_t>(load_address));
+
+  // Get the module path.
+  WCHAR temp_path[MAX_PATH] = {};

[grt (UTC plus 2), 2016/12/20 11:11:35]

nit: wchar_t
consider not zero-initialzing the whole buffer. i don't feel it's needed. if you
want, you could use "temp_path[0] = L'\0';" below to make it a valid zero-length
string before using it.

[chrisha, 2016/12/20 19:46:24]

Done.

+  if (!::GetModuleFileNameEx(process_, module, temp_path, MAX_PATH))

[grt (UTC plus 2), 2016/12/20 11:11:34]

MAX_PATH -> arraysize(temp_path)  (and #include "base/macros.h")
capture the return value for use below

[chrisha, 2016/12/20 19:46:24]

Done.

+    return;
+
+  // Get the module information.
+  MODULEINFO info = {};
+  if (!::GetModuleInformation(process_, module, &info, sizeof(info)))
+    return;
+
+  // Convert this all to a ModuleEvent and forward this on to the module
+  // database.
+  ModuleWatcher::ModuleEvent event(event_type, base::FilePath(temp_path),

[grt (UTC plus 2), 2016/12/20 11:11:34]

temp_path -> base::StringPiece16(temp_path, len-from-GetModuleFileNameEx)

[chrisha, 2016/12/20 19:46:24]

Good catch with the potentially missing trailing nul.

[grt (UTC plus 2), 2016/12/20 21:09:53]

I didn't check whether |length| would include the string terminator or not. MSDN
didn't really say. Is it not included?

[chrisha, 2016/12/21 20:15:00]

It could be omitted in the case that the buffer is too short. But otherwise,
it's not clear. Better safe than sorry.

+                                   module, info.SizeOfImage);
+  module_database_->OnModuleEvent(process_id_, event);
+}
+
+void ModuleEventSinkImpl::OnModuleUnload(uint64_t load_address) {
+  module_database_->OnModuleUnload(process_id_,
+                                   static_cast<uintptr_t>(load_address));
+}