[win] Create ModuleDatabase and ModuleEventSinkImpl.

chrome/common/conflicts/module_watcher_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/common/conflicts/module_watcher_win.h"
 
 #include <windows.h>
 #include <tlhelp32.h>
 #include <winternl.h>  // For UNICODE_STRING.
 
@@ skipping 78 matching lines @@
 // Global pointer to the singleton ModuleWatcher, if one exists. Under
 // |module_watcher_lock|.
 ModuleWatcher* g_module_watcher_instance = nullptr;
 
 // Names of the DLL notification registration functions. These are exported by
 // ntdll.
 constexpr wchar_t kNtDll[] = L"ntdll.dll";
 constexpr char kLdrRegisterDllNotification[] = "LdrRegisterDllNotification";
 constexpr char kLdrUnregisterDllNotification[] = "LdrUnregisterDllNotification";
 
-// Helper function for converting a UNICODE_STRING to a UTF8 std::string.
-std::string ToString(const UNICODE_STRING* str) {
-  std::string s;
-  base::WideToUTF8(str->Buffer, str->Length / sizeof(wchar_t), &s);
-  return s;
+// Helper function for converting a UNICODE_STRING to a FilePath.
+base::FilePath ToFilePath(const UNICODE_STRING* str) {
+  return base::FilePath(
+      base::string16(str->Buffer, str->Length / sizeof(wchar_t)));

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

nit: base::StringPiece16(str->Buffer, str->Length / sizeof(wchar_t)) rather than
base::string16 (and #include "base/strings/string_piece.h")

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

Done.

 }
 
 template <typename NotificationDataType>
-void OnModuleEvent(mojom::ModuleEventType event_type,
+void OnModuleEvent(const mojom::ModuleEventType& event_type,

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

passing an enum by constref?

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

Errr.... oops?

Done.

                    const NotificationDataType& notification_data,
                    const ModuleWatcher::OnModuleEventCallback& callback) {
-  mojom::ModuleEvent event;
-  event.event_type = event_type;
-  event.module_path = ToString(notification_data.FullDllName);
-  event.load_address = reinterpret_cast<uintptr_t>(notification_data.DllBase);
-  event.size = notification_data.SizeOfImage;
+  ModuleWatcher::ModuleEvent event(
+      event_type, ToFilePath(notification_data.FullDllName),
+      notification_data.DllBase, notification_data.SizeOfImage);
   callback.Run(event);
 }
 
 }  // namespace
 
+ModuleWatcher::ModuleEvent::ModuleEvent(mojom::ModuleEventType event_type,

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

is this sufficiently trivial that it could be inline?

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

Done.

+                                        const base::FilePath& module_path,
+                                        void* module_load_address,
+                                        size_t module_size)
+    : event_type(event_type),
+      module_path(module_path),
+      module_load_address(module_load_address),
+      module_size(module_size) {}
+
 // static
 std::unique_ptr<ModuleWatcher> ModuleWatcher::Create(
     const OnModuleEventCallback& callback) {
   // If a ModuleWatcher already exists then bail out.
   base::AutoLock lock(g_module_watcher_lock.Get());
   if (g_module_watcher_instance)
     return nullptr;
 
   // This thread acquired the right to create a ModuleWatcher, so do so.
   g_module_watcher_instance = new ModuleWatcher(callback);
@@ skipping 41 matching lines @@
   }
   if (!snap.IsValid())
     return;
 
   // Walk the module list.
   MODULEENTRY32 module = {sizeof(module)};
   std::string path;
   for (BOOL result = ::Module32First(snap.Get(), &module); result != FALSE;
        result = ::Module32Next(snap.Get(), &module)) {
     base::WideToUTF8(module.szExePath, ::wcslen(module.szExePath), &path);
-    mojom::ModuleEvent event;
-    event.event_type = mojom::ModuleEventType::MODULE_ALREADY_LOADED;
-    event.module_path = path;
-    event.load_address = reinterpret_cast<uintptr_t>(module.modBaseAddr);
-    event.size = module.modBaseSize;
+    ModuleEvent event(mojom::ModuleEventType::MODULE_ALREADY_LOADED,
+                      base::FilePath(base::UTF8ToWide(path)),

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

while i loves me a good conversion, how about using module.szExePath directly
rather than Wide->UTF8->Wide?

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

Oops, remnants of when this was being stored as as std::string.

Done.

+                      module.modBaseAddr, module.modBaseSize);
     callback_.Run(event);

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

nit: i'd be inclined to inline the ModuleEvent, but that's just me. do as you
will.

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

I lean towards verbosity, as I personally find it easier to read. Doing too much
in one statement irks me :)

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

Acknowledged.

   }
 
   return;
 }
 
 // static
 ModuleWatcher::OnModuleEventCallback ModuleWatcher::GetCallbackForContext(
     void* context) {
   base::AutoLock lock(g_module_watcher_lock.Get());
   if (context != g_module_watcher_instance)
@@ skipping 21 matching lines @@
                     notification_data->Unloaded, callback);
       break;
 
     default:
       // This is unexpected, but not a reason to crash.
       NOTREACHED() << "Unknown LDR_DLL_NOTIFICATION_REASON: "
                    << notification_reason;
   }
 }
 
 ModuleWatcher::ModuleWatcher(const OnModuleEventCallback& callback)

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

i just recently read this guidance on callbacks "Pass Callback objects by value
if ownership is transferred; otherwise, pass it by const-reference."
(https://chromium.googlesource.com/chromium/src/+/master/docs/callback.md#Memo...).
should this (or others be by-value (with std::move where appropriate) rather
than by-ref? how about other places in this CL?

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

Interesting. In the context of ModuleWatcher ownership is clearly maintained by
the ModuleWatcher, as the callback is synchronous. IMO, a ref makes sense here.

(FWIW, this is the only callback that passes around an object; everything is
effectively ints, and is already passed by value.)

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

Are you saying that ModuleWatcher takes ownership of |callback|? If so, this
should be pass-by-value.

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

Err, no. I was walking about ownership of the arguments passed to the callback.
My reading comprehension is clearly amazing.

Moved to pass by value.

     : callback_(callback) {
   RegisterDllNotificationCallback();
   EnumerateAlreadyLoadedModules();
 }