Implement basic support for Windows module emission.

components/tracing/common/process_metrics_memory_dump_provider.cc

Index: components/tracing/common/process_metrics_memory_dump_provider.cc
diff --git a/components/tracing/common/process_metrics_memory_dump_provider.cc b/components/tracing/common/process_metrics_memory_dump_provider.cc
index 9bfe0a43acb0d9aee1a80b62c7a69da2a90ce532..cbf89c5dbdbc69259924fd191f137db5a0a1a6ed 100644
--- a/components/tracing/common/process_metrics_memory_dump_provider.cc
+++ b/components/tracing/common/process_metrics_memory_dump_provider.cc
@@ -38,6 +38,14 @@
 #include "base/numerics/safe_math.h"
 #endif  // defined(OS_MACOSX)
 
+#if defined(OS_WIN)
+#include <psapi.h>
+#include <tchar.h>
+#include <windows.h>
+
+#include <base/strings/sys_string_conversions.h>
+#endif
+
 namespace tracing {
 
 namespace {
@@ -233,6 +241,50 @@ bool ProcessMetricsMemoryDumpProvider::DumpProcessMemoryMaps(
 }
 #endif  // defined(OS_LINUX) || defined(OS_ANDROID)
 
+#if defined(OS_WIN)
+bool ProcessMetricsMemoryDumpProvider::DumpProcessMemoryMaps(
+    const base::trace_event::MemoryDumpArgs& args,
+    base::trace_event::ProcessMemoryDump* pmd) {
+  DWORD module_count = 1024;

[Wez, 2017/02/23 04:15:11]

I would suggest including a brief comment to introduce each of the two blocks of
work here, e.g:

// Fetch handles to each of the modules loaded in this process. The process
itself retains ownership of the returned handles.

and

// Query the base address for each module, and attach it to the dump.

[erikchen, 2017/02/23 21:00:14]

Done.

[awong, 2017/02/23 22:05:44]

Also, please comment on why 1024 makes sense as a magic number.

+  std::vector<HMODULE> modules;
+  while (true) {
+    modules.resize(module_count);
+    DWORD output_size;

[Wez, 2017/02/23 04:15:11]

nit: Even though you're about to overwrite it, please initialize this.

If you move this outside the loop then you can have requested_hmodule_bytes and
result_hmodule_bytes, and only assign one to the other if (result > requested) -
the rest of the code can just use result_hmodule_bytes. WDYT?

(Or equally with counts, modulo some bit shifting ;)

[erikchen, 2017/02/23 21:00:14]

Done.

+    BOOL result = ::EnumProcessModules(::GetCurrentProcess(), modules.data(),
+                                       module_count, &output_size);

[Wez, 2017/02/23 04:15:11]

You're passing |module_count| here, where the API expects a size in bytes - I'm
surprised this doesn't cause the code to tight-loop, actually!

Note that this function is racey wrt the module table, as is the querying code
below, so it's quite possible to have this, or the other calls, fail even though
the process is in a good state, just because something got loaded or unloaded;
might be worth noting that in a comment in case we start seeing flakiness.

[erikchen, 2017/02/23 21:00:14]

fascinating, fixed. Also added a comment + buffer.

+    if (!result)
+      return false;
+    DWORD actual_module_count = output_size / sizeof(HMODULE);
+    if (actual_module_count > module_count) {
+      module_count = actual_module_count;
+      continue;
+    }
+    module_count = actual_module_count;
+    break;

[Wez, 2017/02/23 04:15:11]

Can we re-arrange this to read as:

while (true) {
  ...
  if (condition)
    break
  ...
}

It reads oddly to have break here, and continue in the conditional.

[erikchen, 2017/02/23 21:00:14]

Done.

+  }
+  for (unsigned int i = 0; i < module_count; ++i) {

[Wez, 2017/02/23 04:15:11]

nit: Since |module_count|'s range is defined by DWORD, use a DWORD as the
iterator.

[erikchen, 2017/02/23 21:00:14]

Done.

+    TCHAR module_name[MAX_PATH];

[Wez, 2017/02/23 04:15:10]

wchar_t

[erikchen, 2017/02/23 21:00:14]

Done.

+    BOOL result = GetModuleFileName(modules[i], module_name, MAX_PATH);

[Wez, 2017/02/23 04:15:10]

nit: You used :: prefix for EnumProcessModules(), so be consistent with this
call too.

Do we need the filename in any sort of "normalized" form, or is this primarily
for pretty-printing purposes?

[erikchen, 2017/02/23 21:00:14]

Switched to :: prefixes. The filename is necessary for converting PCs ->
symbols, but the format/path isn't particularly important.

[Wez, 2017/02/23 23:35:03]

Ah OK; I wondered if it might be important to get an absolute path, for that
reason, for example.

+    if (!result)
+      return false;

[Wez, 2017/02/23 04:15:11]

This means failing to resolve any module's filename will prevent any mmap info
being output; perhaps continue here, instead?

[erikchen, 2017/02/23 21:00:14]

Done.

+
+    MODULEINFO module_info;
+    result = GetModuleInformation(::GetCurrentProcess(), modules[i],
+                                  &module_info, sizeof(MODULEINFO));
+    if (!result)
+      return false;

[Wez, 2017/02/23 04:15:11]

As above, consider continue'ing here rather than failing.

[erikchen, 2017/02/23 21:00:14]

Done.

+    base::trace_event::ProcessMemoryMaps::VMRegion region;
+    region.size_in_bytes = module_info.SizeOfImage;
+    region.mapped_file = base::SysWideToNativeMB(module_name);
+    region.start_address = reinterpret_cast<uint64_t>(module_info.lpBaseOfDll);
+    pmd->process_mmaps()->AddVMRegion(region);
+  }
+
+  pmd->set_has_process_mmaps();

[Wez, 2017/02/23 04:15:11]

If you follow my suggestion to use continue's then you might want to make this
conditional on there being >0 regions successfully queried. :)

Is there any way to indicate that we know we failed to gather some regions?

[erikchen, 2017/02/23 21:00:14]

Done
No

+  return true;
+}
+#endif  // defined(OS_WIN)
+
 #if defined(OS_MACOSX)
 
 namespace {
@@ -547,11 +599,13 @@ bool ProcessMetricsMemoryDumpProvider::OnMemoryDump(
     base::trace_event::ProcessMemoryDump* pmd) {
   bool res = DumpProcessTotals(args, pmd);
 
-#if defined(OS_LINUX) || defined(OS_ANDROID) || defined(OS_MACOSX)
+#if defined(OS_LINUX) || defined(OS_ANDROID) || defined(OS_MACOSX) || \
+    defined(OS_WIN)

[Wez, 2017/02/23 04:15:11]

nit: Are there any other platforms? Or is this no longer conditional?

[erikchen, 2017/02/23 21:00:14]

Nope, fixed.

   if (args.level_of_detail ==
       base::trace_event::MemoryDumpLevelOfDetail::DETAILED)
     res &= DumpProcessMemoryMaps(args, pmd);
-#endif  // defined(OS_LINUX) || defined(OS_ANDROID) || defined(OS_MACOSX)
+#endif  // defined(OS_LINUX) || defined(OS_ANDROID) || defined(OS_MACOSX) ||
+        // defined(OS_WIN)
   return res;
 }