Implement basic support for Windows module emission.

components/tracing/common/process_metrics_memory_dump_provider.cc

 // Copyright 2015 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 "components/tracing/common/process_metrics_memory_dump_provider.h"
 
 #include <fcntl.h>
 #include <stdint.h>
 
 #include <map>
@@ skipping 20 matching lines @@
 #include <mach/shared_region.h>
 #include <sys/param.h>
 
 #include <mach-o/dyld_images.h>
 #include <mach-o/loader.h>
 #include <mach/mach.h>
 
 #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 {
 
 base::LazyInstance<
     std::map<base::ProcessId,
              std::unique_ptr<ProcessMetricsMemoryDumpProvider>>>::Leaky
     g_dump_providers_map = LAZY_INSTANCE_INITIALIZER;
 
 #if defined(OS_LINUX) || defined(OS_ANDROID)
@@ skipping 175 matching lines @@
     base::ScopedFILE smaps_file(fopen(file_name.c_str(), "r"));
     res = ReadLinuxProcSmapsFile(smaps_file.get(), pmd->process_mmaps());
   }
 
   if (res)
     pmd->set_has_process_mmaps();
   return res;
 }
 #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 {
 
 using VMRegion = base::trace_event::ProcessMemoryMaps::VMRegion;
 
 bool IsAddressInSharedRegion(uint64_t address) {
   return address >= SHARED_REGION_BASE_X86_64 &&
          address < (SHARED_REGION_BASE_X86_64 + SHARED_REGION_SIZE_X86_64);
 }
@@ skipping 294 matching lines @@
 
 ProcessMetricsMemoryDumpProvider::~ProcessMetricsMemoryDumpProvider() {}
 
 // Called at trace dump point time. Creates a snapshot of the memory maps for
 // the current process.
 bool ProcessMetricsMemoryDumpProvider::OnMemoryDump(
     const base::trace_event::MemoryDumpArgs& args,
     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;
 }
 
 bool ProcessMetricsMemoryDumpProvider::DumpProcessTotals(
     const base::trace_event::MemoryDumpArgs& args,
     base::trace_event::ProcessMemoryDump* pmd) {
   const uint64_t rss_bytes = rss_bytes_for_testing
                                  ? rss_bytes_for_testing
                                  : process_metrics_->GetWorkingSetSize();
 
@@ skipping 82 matching lines @@
 #endif
 }
 
 void ProcessMetricsMemoryDumpProvider::SuspendFastMemoryPolling() {
 #if defined(OS_LINUX) || defined(OS_ANDROID)
   fast_polling_statm_fd_.reset();
 #endif
 }
 
 }  // namespace tracing