Revert "[Sampling profiler] Make ModuleCache::Module a reference type"

This reverts commit 8f219b0.

Reason for revert: broke base_unittests on win-asan
First failure
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/win-asan/3559

Original change's description:
> [Sampling profiler] Make ModuleCache::Module a reference type
> 
> Changes ModuleCache::Module from a value type to a reference type,
> with ModuleCache maintaining ownership of the Module. ModuleCache needs
> to own its Modules to properly support Windows, which reference counts
> its modules.
> 
> ModuleCache is retained as a struct to minimize the size of this change,
> but will be changed to a class in a later CL.
> 
> Bug: 931418
> Change-Id: Ifa5bb0e763de14d91c1663ba01aeb3bab09447be
> Reviewed-on: https://chromium-review.googlesource.com/c/1477817
> Reviewed-by: Alexei Filippov <alph@chromium.org>
> Reviewed-by: Charlie Andrews <charliea@chromium.org>
> Reviewed-by: oysteine <oysteine@chromium.org>
> Commit-Queue: Mike Wittman <wittman@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#634408}

TBR=wittman@chromium.org,alph@chromium.org,oysteine@chromium.org,charliea@chromium.org

Change-Id: Iad9daf5933823b3139d4a81d5d2d6ba6b8e655f6
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 931418
Reviewed-on: https://chromium-review.googlesource.com/c/1482894
Commit-Queue: Christos Froussios <cfroussios@chromium.org>
Reviewed-by: Christos Froussios <cfroussios@chromium.org>
Cr-Commit-Position: refs/heads/master@{#634601}

base/profiler/native_stack_sampler_mac.cc

@@ -409,12 +409,11 @@ std::vector<Frame> NativeStackSamplerMac::RecordStackFrames(
     return HasValidRbp(unwind_cursor, new_stack_top);
   };
 
-  WalkStack(
-      thread_state,
-      [&frames](uintptr_t frame_ip, const ModuleCache::Module* module) {
-        frames.emplace_back(frame_ip, module);
-      },
-      continue_predicate);
+  WalkStack(thread_state,
+            [&frames](uintptr_t frame_ip, ModuleCache::Module module) {
+              frames.emplace_back(frame_ip, std::move(module));
+            },
+            continue_predicate);
 
   return frames;
 }
@@ -445,8 +444,8 @@ bool NativeStackSamplerMac::WalkStackFromContext(
     // libunwind adds the expected stack size, it will look for the return
     // address in the wrong place. This check should ensure that we bail before
     // trying to deref a bad IP obtained this way in the previous frame.
-    const ModuleCache::Module* module = module_cache_->GetModuleForAddress(rip);
-    if (!module->is_valid)
+    const ModuleCache::Module& module = module_cache_->GetModuleForAddress(rip);
+    if (!module.is_valid)
       return false;
 
     callback(static_cast<uintptr_t>(rip), module);

base/profiler/native_stack_sampler_win.cc

@@ -19,7 +19,6 @@
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
-#include "base/no_destructor.h"
 #include "base/profiler/win32_stack_frame_unwinder.h"
 #include "base/sampling_heap_profiler/module_cache.h"
 #include "base/stl_util.h"
@@ -434,7 +433,7 @@ class NativeStackSamplerWin : public NativeStackSampler {
   const void* const thread_stack_base_address_;
 
   // The module objects, indexed by the module handle.
-  std::map<HMODULE, std::unique_ptr<ModuleCache::Module>> module_cache_;
+  std::map<HMODULE, ModuleCache::Module> module_cache_;
 
   DISALLOW_COPY_AND_ASSIGN(NativeStackSamplerWin);
 };
@@ -486,8 +485,6 @@ std::vector<Frame> NativeStackSamplerWin::CreateFrames(
   TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("cpu_profiler.debug"),
                "NativeStackSamplerWin::CreateFrames");
 
-  static NoDestructor<ModuleCache::Module> invalid_module;
-
   std::vector<Frame> frames;
   frames.reserve(stack.size());
 
@@ -496,22 +493,22 @@ std::vector<Frame> NativeStackSamplerWin::CreateFrames(
 
     HMODULE module_handle = frame.module.Get();
     if (!module_handle) {
-      frames.emplace_back(frame_ip, invalid_module.get());
+      frames.emplace_back(frame_ip, ModuleCache::Module());
       continue;
     }
 
     auto loc = module_cache_.find(module_handle);
     if (loc != module_cache_.end()) {
-      frames.emplace_back(frame_ip, loc->second.get());
+      frames.emplace_back(frame_ip, loc->second);
       continue;
     }
 
-    std::unique_ptr<ModuleCache::Module> module =
+    ModuleCache::Module module =
         ModuleCache::CreateModuleForHandle(module_handle);
-    frames.emplace_back(frame_ip,
-                        module->is_valid ? module.get() : invalid_module.get());
-    if (module->is_valid)
-      module_cache_.insert(std::make_pair(module_handle, std::move(module)));
+    if (module.is_valid)
+      module_cache_.insert(std::make_pair(module_handle, module));
+
+    frames.emplace_back(frame_ip, std::move(module));
   }
 
   return frames;

base/profiler/stack_sampling_profiler.cc

@@ -50,8 +50,8 @@ const int kNullProfilerId = -1;
 // StackSamplingProfiler::Frame -------------------------------------
 
 StackSamplingProfiler::Frame::Frame(uintptr_t instruction_pointer,
-                                    const ModuleCache::Module* module)
-    : instruction_pointer(instruction_pointer), module(module) {}
+                                    ModuleCache::Module module)
+    : instruction_pointer(instruction_pointer), module(std::move(module)) {}
 
 StackSamplingProfiler::Frame::~Frame() = default;
 

base/profiler/stack_sampling_profiler.h

@@ -58,14 +58,14 @@ class BASE_EXPORT StackSamplingProfiler {
   // This struct is only used for sampling data transfer from NativeStackSampler
   // to ProfileBuilder.
   struct BASE_EXPORT Frame {
-    Frame(uintptr_t instruction_pointer, const ModuleCache::Module* module);
+    Frame(uintptr_t instruction_pointer, ModuleCache::Module module);
     ~Frame();
 
     // The sampled instruction pointer within the function.
     uintptr_t instruction_pointer;
 
     // The module information.
-    const ModuleCache::Module* module;
+    ModuleCache::Module module;
   };
 
   // Represents parameters that configure the sampling.

base/profiler/stack_sampling_profiler_unittest.cc

@@ -557,7 +557,7 @@ std::string FormatSampleForDiagnosticOutput(const Frames& frames) {
   for (const auto& frame : frames) {
     output += StringPrintf(
         "0x%p %s\n", reinterpret_cast<const void*>(frame.instruction_pointer),
-        frame.module->filename.AsUTF8Unsafe().c_str());
+        frame.module.filename.AsUTF8Unsafe().c_str());
   }
   return output;
 }
@@ -756,7 +756,7 @@ PROFILER_TEST_F(StackSamplingProfilerTest, MAYBE_Basic) {
 
   // Check that all the modules are valid.
   for (const auto& frame : frames)
-    EXPECT_TRUE(frame.module->is_valid);
+    EXPECT_TRUE(frame.module.is_valid);
 
   // Check that the stack contains a frame for
   // TargetThread::SignalAndWaitUntilSignaled().

base/sampling_heap_profiler/module_cache.cc

@@ -30,29 +30,29 @@ ModuleCache::Module::~Module() = default;
 ModuleCache::ModuleCache() = default;
 ModuleCache::~ModuleCache() = default;
 
-const ModuleCache::Module* ModuleCache::GetModuleForAddress(uintptr_t address) {
+const ModuleCache::Module& ModuleCache::GetModuleForAddress(uintptr_t address) {
   static NoDestructor<Module> invalid_module;
   auto it = modules_cache_map_.upper_bound(address);
   if (it != modules_cache_map_.begin()) {
     DCHECK(!modules_cache_map_.empty());
     --it;
-    const Module* module = it->second.get();
-    if (address < module->base_address + module->size)
+    Module& module = it->second;
+    if (address < module.base_address + module.size)
       return module;
   }
 
-  std::unique_ptr<Module> module = CreateModuleForAddress(address);
-  if (!module->is_valid)
-    return invalid_module.get();
-  return modules_cache_map_.emplace(module->base_address, std::move(module))
-      .first->second.get();
+  auto module = CreateModuleForAddress(address);
+  if (!module.is_valid)
+    return *invalid_module;
+  return modules_cache_map_.emplace(module.base_address, std::move(module))
+      .first->second;
 }
 
 std::vector<const ModuleCache::Module*> ModuleCache::GetModules() const {
   std::vector<const Module*> result;
   result.reserve(modules_cache_map_.size());
   for (const auto& it : modules_cache_map_)
-    result.push_back(it.second.get());
+    result.push_back(&it.second);
   return result;
 }
 

base/sampling_heap_profiler/module_cache.h

@@ -6,7 +6,6 @@
 #define BASE_SAMPLING_HEAP_PROFILER_MODULE_CACHE_H_
 
 #include <map>
-#include <memory>
 #include <string>
 #include <vector>
 
@@ -36,9 +35,6 @@ class BASE_EXPORT ModuleCache {
            size_t size);
     ~Module();
 
-    Module(const Module&) = delete;
-    Module& operator=(const Module&) = delete;
-
     // Points to the base address of the module.
     uintptr_t base_address;
 
@@ -64,7 +60,7 @@ class BASE_EXPORT ModuleCache {
   ModuleCache();
   ~ModuleCache();
 
-  const Module* GetModuleForAddress(uintptr_t address);
+  const Module& GetModuleForAddress(uintptr_t address);
   std::vector<const Module*> GetModules() const;
 
  private:
@@ -73,7 +69,7 @@ class BASE_EXPORT ModuleCache {
 
   // Creates a Module object for the specified memory address. If the address
   // does not belong to a module returns an invalid module.
-  static std::unique_ptr<Module> CreateModuleForAddress(uintptr_t address);
+  static Module CreateModuleForAddress(uintptr_t address);
   friend class NativeStackSamplerMac;
 
 #if defined(OS_MACOSX)
@@ -84,11 +80,11 @@ class BASE_EXPORT ModuleCache {
 #endif
 
 #if defined(OS_WIN)
-  static std::unique_ptr<Module> CreateModuleForHandle(HMODULE module_handle);
+  static Module CreateModuleForHandle(HMODULE module_handle);
   friend class NativeStackSamplerWin;
 #endif
 
-  std::map<uintptr_t, std::unique_ptr<Module>> modules_cache_map_;
+  std::map<uintptr_t, Module> modules_cache_map_;
 };
 
 }  // namespace base

base/sampling_heap_profiler/module_cache_mac.cc

@@ -64,15 +64,13 @@ std::string GetUniqueId(const void* module_addr) {
 }  // namespace
 
 // static
-std::unique_ptr<ModuleCache::Module> ModuleCache::CreateModuleForAddress(
-    uintptr_t address) {
+ModuleCache::Module ModuleCache::CreateModuleForAddress(uintptr_t address) {
   Dl_info inf;
   if (!dladdr(reinterpret_cast<const void*>(address), &inf))
-    return std::make_unique<Module>();
+    return Module();
   auto base_module_address = reinterpret_cast<uintptr_t>(inf.dli_fbase);
-  return std::make_unique<Module>(
-      base_module_address, GetUniqueId(inf.dli_fbase), FilePath(inf.dli_fname),
-      GetModuleTextSize(inf.dli_fbase));
+  return Module(base_module_address, GetUniqueId(inf.dli_fbase),
+                FilePath(inf.dli_fname), GetModuleTextSize(inf.dli_fbase));
 }
 
 size_t ModuleCache::GetModuleTextSize(const void* module_addr) {

base/sampling_heap_profiler/module_cache_posix.cc

@@ -7,10 +7,9 @@
 namespace base {
 
 // static
-std::unique_ptr<ModuleCache::Module> ModuleCache::CreateModuleForAddress(
-    uintptr_t address) {
+ModuleCache::Module ModuleCache::CreateModuleForAddress(uintptr_t address) {
   // TODO(alph): Implement it.
-  return std::make_unique<Module>();
+  return Module();
 }
 
 }  // namespace base

base/sampling_heap_profiler/module_cache_unittest.cc

@@ -28,28 +28,28 @@ TEST_F(ModuleCacheTest, MAYBE_ModuleCache) {
   uintptr_t ptr1 = reinterpret_cast<uintptr_t>(&AFunctionForTest);
   uintptr_t ptr2 = ptr1 + 1;
   ModuleCache cache;
-  const ModuleCache::Module* module1 = cache.GetModuleForAddress(ptr1);
-  const ModuleCache::Module* module2 = cache.GetModuleForAddress(ptr2);
-  EXPECT_EQ(module1, module2);
-  EXPECT_TRUE(module1->is_valid);
-  EXPECT_GT(module1->size, 0u);
-  EXPECT_LE(module1->base_address, ptr1);
-  EXPECT_GT(module1->base_address + module1->size, ptr2);
+  const ModuleCache::Module& module1 = cache.GetModuleForAddress(ptr1);
+  const ModuleCache::Module& module2 = cache.GetModuleForAddress(ptr2);
+  EXPECT_EQ(&module1, &module2);
+  EXPECT_TRUE(module1.is_valid);
+  EXPECT_GT(module1.size, 0u);
+  EXPECT_LE(module1.base_address, ptr1);
+  EXPECT_GT(module1.base_address + module1.size, ptr2);
 }
 
 TEST_F(ModuleCacheTest, MAYBE_ModulesList) {
   ModuleCache cache;
   uintptr_t ptr = reinterpret_cast<uintptr_t>(&AFunctionForTest);
-  const ModuleCache::Module* module = cache.GetModuleForAddress(ptr);
-  EXPECT_TRUE(module->is_valid);
+  const ModuleCache::Module& module = cache.GetModuleForAddress(ptr);
+  EXPECT_TRUE(module.is_valid);
   EXPECT_EQ(1u, cache.GetModules().size());
-  EXPECT_EQ(module, cache.GetModules().front());
+  EXPECT_EQ(&module, cache.GetModules().front());
 }
 
 TEST_F(ModuleCacheTest, InvalidModule) {
   ModuleCache cache;
-  const ModuleCache::Module* invalid_module = cache.GetModuleForAddress(1);
-  EXPECT_FALSE(invalid_module->is_valid);
+  const ModuleCache::Module& invalid_module = cache.GetModuleForAddress(1);
+  EXPECT_FALSE(invalid_module.is_valid);
 }
 
 }  // namespace

base/sampling_heap_profiler/module_cache_win.cc

@@ -66,38 +66,35 @@ void GetDebugInfoForModule(HMODULE module_handle,
 }  // namespace
 
 // static
-std::unique_ptr<ModuleCache::Module> ModuleCache::CreateModuleForAddress(
-    uintptr_t address) {
+ModuleCache::Module ModuleCache::CreateModuleForAddress(uintptr_t address) {
   HMODULE module_handle = nullptr;
   if (!::GetModuleHandleEx(GET_MODULE_HANDLE_EX_FLAG_FROM_ADDRESS,
                            reinterpret_cast<LPCTSTR>(address),
                            &module_handle)) {
     DCHECK_EQ(ERROR_MOD_NOT_FOUND, static_cast<int>(::GetLastError()));
-    return std::make_unique<Module>();
+    return Module();
   }
-  std::unique_ptr<Module> module = CreateModuleForHandle(module_handle);
+  Module module = CreateModuleForHandle(module_handle);
   ::CloseHandle(module_handle);
   return module;
 }
 
 // static
-std::unique_ptr<ModuleCache::Module> ModuleCache::CreateModuleForHandle(
-    HMODULE module_handle) {
+ModuleCache::Module ModuleCache::CreateModuleForHandle(HMODULE module_handle) {
   FilePath pdb_name;
   std::string build_id;
   GetDebugInfoForModule(module_handle, &build_id, &pdb_name);
   if (build_id.empty())
-    return std::make_unique<Module>();
+    return Module();
 
   MODULEINFO module_info;
   if (!::GetModuleInformation(GetCurrentProcessHandle(), module_handle,
                               &module_info, sizeof(module_info))) {
-    return std::make_unique<Module>();
+    return Module();
   }
 
-  return std::make_unique<Module>(
-      reinterpret_cast<uintptr_t>(module_info.lpBaseOfDll), build_id, pdb_name,
-      module_info.SizeOfImage);
+  return Module(reinterpret_cast<uintptr_t>(module_info.lpBaseOfDll), build_id,
+                pdb_name, module_info.SizeOfImage);
 }
 
 }  // namespace base

chrome/common/heap_profiler_controller.cc

@@ -130,7 +130,7 @@ void HeapProfilerController::RetrieveAndSendSnapshot() {
     frames.reserve(sample.stack.size());
     for (const void* frame : sample.stack) {
       uintptr_t address = reinterpret_cast<uintptr_t>(frame);
-      const base::ModuleCache::Module* module =
+      const base::ModuleCache::Module& module =
           module_cache.GetModuleForAddress(address);
       frames.emplace_back(address, module);
     }

components/metrics/call_stack_profile_builder.cc

@@ -91,22 +91,22 @@ void CallStackProfileBuilder::OnSampleCompleted(
     // keep the frame information even if its module is invalid so we have
     // visibility into how often this issue is happening on the server.
     CallStackProfile::Location* location = stack.add_frame();
-    if (!frame.module->is_valid)
+    if (!frame.module.is_valid)
       continue;
 
     // Dedup modules.
-    const base::ModuleCache::Module* module = frame.module;
-    auto module_loc = module_index_.find(module->base_address);
+    const base::ModuleCache::Module& module = frame.module;
+    auto module_loc = module_index_.find(module.base_address);
     if (module_loc == module_index_.end()) {
       modules_.push_back(module);
       size_t index = modules_.size() - 1;
-      module_loc = module_index_.emplace(module->base_address, index).first;
+      module_loc = module_index_.emplace(module.base_address, index).first;
     }
 
     // Write CallStackProfile::Location protobuf message.
     ptrdiff_t module_offset =
         reinterpret_cast<const char*>(frame.instruction_pointer) -
-        reinterpret_cast<const char*>(module->base_address);
+        reinterpret_cast<const char*>(module.base_address);
     DCHECK_GE(module_offset, 0);
     location->set_address(static_cast<uint64_t>(module_offset));
     location->set_module_id_index(module_loc->second);
@@ -165,11 +165,11 @@ void CallStackProfileBuilder::OnProfileCompleted(
   call_stack_profile->set_sampling_period_ms(sampling_period.InMilliseconds());
 
   // Write CallStackProfile::ModuleIdentifier protobuf message.
-  for (const auto* module : modules_) {
+  for (const auto& module : modules_) {
     CallStackProfile::ModuleIdentifier* module_id =
         call_stack_profile->add_module_id();
-    module_id->set_build_id(module->id);
-    module_id->set_name_md5_prefix(HashModuleFilename(module->filename));
+    module_id->set_build_id(module.id);
+    module_id->set_name_md5_prefix(HashModuleFilename(module.filename));
   }
 
   PassProfilesToMetricsProvider(std::move(sampled_profile_));