[win] Create ModuleDatabase and ModuleEventSinkImpl.

chrome/browser/conflicts/module_database_win.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.
+
+#include "chrome/browser/conflicts/module_database_win.h"
+
+#include "base/bind.h"
+#include "base/debug/leak_annotations.h"
+#include "base/lazy_instance.h"
+#include "base/strings/utf_string_conversions.h"
+
+namespace {
+
+// Document the assumptions made on the ProcessType enum in order to convert
+// them to bits.
+static_assert(1 == content::PROCESS_TYPE_UNKNOWN,
+              "assumes unknown process type has value 1");
+static_assert(2 == content::PROCESS_TYPE_BROWSER,
+              "assumes unknown browser type has value 2");
+static constexpr uint32_t kMinProcessType = content::PROCESS_TYPE_BROWSER;
+
+}  // namespace
+
+ModuleDatabase::ModuleDatabase(
+    scoped_refptr<base::SequencedTaskRunner> task_runner)
+    : task_runner_(task_runner), weak_ptr_factory_(this) {}
+
+ModuleDatabase::~ModuleDatabase() = default;
+
+void ModuleDatabase::OnProcessStarted(uint32_t process_id,
+                                      content::ProcessType process_type) {
+  // Messages can arrive from any thread (UI thread for calls over IPC, and
+  // anywhere at all for calls from ModuleWatcher), so bounce if necessary.
+  if (!task_runner_->RunsTasksOnCurrentThread()) {
+    task_runner_->PostTask(
+        FROM_HERE,
+        base::Bind(&ModuleDatabase::OnProcessStarted,
+                   weak_ptr_factory_.GetWeakPtr(), process_id, process_type));
+    return;
+  }
+
+  auto* process_info = CreateProcessInfo(process_id, process_type);
+  // If each client is sending messages in the appropriate order then this
+  // should always be true.
+  DCHECK(process_info);

[sky, 2016/12/14 21:58:08]

Doesn't this really depend upon how fast messages are picked up? If a process id
is reused rapidly then I could see this check being racy, but perhaps OSs don't
recycle that quickly?

[chrisha, 2016/12/19 20:15:36]

Yes, this indeed could depend on the order in which the messages are picked up.

I was betting on process ID reuse being sufficiently spaced out that you'd never
have two at the same time. If we think this is going to be an issue then we
could move to using the (process_id, process_creation_time) pair, as suggested
by grt.

In the meantime, I've reworked the logic to be resistant to out of order
messages.

+}
+
+void ModuleDatabase::OnModuleEvent(uint32_t process_id,
+                                   const mojom::ModuleEvent& event) {
+  // Messages can arrive from any thread (UI thread for calls over IPC, and
+  // anywhere at all for calls from ModuleWatcher), so bounce if necessary.
+  if (!task_runner_->RunsTasksOnCurrentThread()) {
+    task_runner_->PostTask(FROM_HERE, base::Bind(&ModuleDatabase::OnModuleEvent,
+                                                 weak_ptr_factory_.GetWeakPtr(),
+                                                 process_id, event));
+    return;
+  }
+
+  base::FilePath module_path(base::UTF8ToWide(event.module_path));
+  auto* module_info = FindOrCreateModuleInfo(module_path);
+
+  // In theory this should always succeed. However, it is possible for a client
+  // to misbehave and sent out-of-order messages. It is easy to be tolerant of
+  // this case by simply not updating the process info in this case. It's not
+  // worth crashing if this data is slightly out of sync as this is purely

[grt (UTC plus 2), 2016/12/15 09:07:12]

if it's not worth crashing, why is there a DCHECK? will the DCHECK do anything
other than annoy/confuse some other random developer who happens to trip on
this? see
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...
for guidance on DCHECK

[chrisha, 2016/12/19 20:15:36]

Not worth crashing at all. Removed.

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

Maybe, maybe not.... :-)

+  // informational.
+  auto* process_info = GetProcessInfo(process_id);
+  DCHECK(process_info);
+  if (!process_info)
+    return;
+
+  // Update the list of process types that this module has been seen in.
+  uint32_t process_type_bit = ProcessTypeToBit(process_info->process_type);

[grt (UTC plus 2), 2016/12/15 09:07:12]

nit: omit variable

[chrisha, 2016/12/19 20:15:36]

Done.

+  module_info->process_types |= process_type_bit;
+
+  // Update the module lists for this process.
+  switch (event.event_type) {
+    case mojom::ModuleEventType::MODULE_ALREADY_LOADED:
+    case mojom::ModuleEventType::MODULE_LOADED:
+      process_info->loaded_module_ids.insert(module_info->module_id);
+      break;
+    case mojom::ModuleEventType::MODULE_UNLOADED:
+      process_info->loaded_module_ids.erase(module_info->module_id);
+      process_info->unloaded_module_ids.insert(module_info->module_id);
+      break;
+    default:

[grt (UTC plus 2), 2016/12/15 09:07:12]

remove this so that compilation will fail if a new type is ever added

[chrisha, 2016/12/19 20:15:36]

Done.

+      NOTREACHED() << "Unknown ModuleEventType: " << event.event_type;
+  }
+}
+
+void ModuleDatabase::OnProcessEnded(uint32_t process_id) {
+  // Messages can arrive from any thread (UI thread for calls over IPC, and
+  // anywhere at all for calls from ModuleWatcher), so bounce if necessary.
+  if (!task_runner_->RunsTasksOnCurrentThread()) {
+    task_runner_->PostTask(
+        FROM_HERE, base::Bind(&ModuleDatabase::OnProcessEnded,
+                              weak_ptr_factory_.GetWeakPtr(), process_id));
+    return;
+  }
+
+  bool deleted = DeleteProcessInfo(process_id);
+  // If each client is sending messages in the appropriate order then this
+  // should always be true.
+  DCHECK(deleted);
+}
+
+// static
+uint32_t ModuleDatabase::ProcessTypeToBit(content::ProcessType process_type) {
+  uint32_t bit_index = static_cast<uint32_t>(process_type) - kMinProcessType;
+  DCHECK_LE(0u, bit_index);

[grt (UTC plus 2), 2016/12/15 09:07:12]

nit: i find these more natural the other way around since that's the way we
write general comparisons (i.e., "if (0 <= bit_index)" is backwards).

[chrisha, 2016/12/19 20:15:36]

I'm impartial, but over the years had reviewers drill into me this "backwards"
way of doing things (that the constant should be first).

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

I apologize if I was a driller in the past, but I believe that you were
improperly drilled. The guidance for general comparisons in C++ (let's forget
about GTest for the time being) is that the constant comes last, for example:
  if (bit_index > 31)  // woah there, cowboy!
Then Google Test came around with a quad of {EXPECT,ASSERT}_{EQ,NE} macros where
the expected value came first
(https://cs.chromium.org/chromium/src/testing/gtest/include/gtest/gtest.h?q=%2...).
This put a wrench in the works, but we adapted by following this convention for
these four macros (and their {,D}CHECK counterparts for consistency). We didn't
carry this over to the use of the other EXPECT/ASSERT macros. Here's one thread
where this was discussed:
https://groups.google.com/a/chromium.org/d/topic/chromium-dev/QuN9McxZpts/dis....

Also worth mentioning: GTest no longer has this convention:
https://github.com/google/googletest/commit/f364e188372e489230ef4e44e1aec6bcb...,
but Chromium hasn't picked that up yet.

+  DCHECK_GE(31u, bit_index);
+  uint32_t bit = (1 << bit_index);
+  return bit;
+}
+
+// static
+content::ProcessType ModuleDatabase::BitIndexToProcessType(uint32_t bit_index) {
+  DCHECK_LE(0u, bit_index);
+  DCHECK_GE(31u, bit_index);
+  return static_cast<content::ProcessType>(bit_index + kMinProcessType);
+}
+
+ModuleDatabase::ModuleInfo::ModuleInfo(const base::FilePath& module_path,

[grt (UTC plus 2), 2016/12/15 09:07:12]

nit: keep class function definitions grouped by class (so either move these
above or below the ModuleDatabase functions). i'm partial to adding comments
like:

// ModuleDatabase::ModuleInfo --------------------------------------------------

to separate the blocks of functions.

[chrisha, 2016/12/19 20:15:36]

I had kept the declarations and definitions in the same order. I'll move the
inline declarations out of line (move to forward declares) in the header, and
also maintain a consistent order of definitions.

(So, done.)

+                                       uint32_t module_id)
+    : module_path(module_path), module_id(module_id), process_types(0) {}
+
+bool ModuleDatabase::ModuleInfoKeyComparator::operator()(
+    const ModuleInfo& mi1,
+    const ModuleInfo& mi2) const {
+  return mi1.module_path < mi2.module_path;
+}
+
+ModuleDatabase::ProcessInfo::ProcessInfo(uint32_t process_id,
+                                         content::ProcessType process_type)
+    : process_id(process_id), process_type(process_type) {}
+
+bool ModuleDatabase::ProcessInfoKeyComparator::operator()(
+    const ProcessInfo& pi1,
+    const ProcessInfo& pi2) const {
+  return pi1.process_id < pi2.process_id;
+}
+
+ModuleDatabase::ModuleInfo* ModuleDatabase::FindOrCreateModuleInfo(
+    const base::FilePath& module_path) {
+  ModuleInfo key(module_path, modules_.size());
+  auto result = modules_.insert(key);
+  return const_cast<ModuleInfo*>(&(*result.first));
+}
+
+ModuleDatabase::ProcessInfo* ModuleDatabase::GetProcessInfo(
+    uint32_t process_id) {
+  ProcessInfo key(process_id, content::PROCESS_TYPE_UNKNOWN);
+  auto it = processes_.find(key);
+  if (it == processes_.end())
+    return nullptr;
+  return const_cast<ProcessInfo*>(&(*it));
+}
+
+const ModuleDatabase::ProcessInfo* ModuleDatabase::CreateProcessInfo(
+    uint32_t process_id,
+    content::ProcessType process_type) {
+  ProcessInfo key(process_id, process_type);
+  auto result = processes_.insert(key);
+
+  // If the element was inserted then return it.
+  if (result.second)
+    return &(*result.first);
+  // Otherwise it already existed, so return nullptr.
+  return nullptr;
+}
+
+bool ModuleDatabase::DeleteProcessInfo(uint32_t process_id) {
+  ProcessInfo key(process_id, content::PROCESS_TYPE_UNKNOWN);
+  auto it = processes_.find(key);
+  if (it == processes_.end())
+    return false;
+  processes_.erase(it);
+  return true;
+}