Reland "Add ModuleCache API contract checks"

Adds checks to ensure early detection of API contract violations that
would lead to use-after-frees.

This is a reland of affe1977, omitting a
DCHECK that flakily failed.

Bug: 1127466
Change-Id: I07aacfb6752fa2f2afc24cb999b08ccbb52f9f0c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2429307
Commit-Queue: Mike Wittman <wittman@chromium.org>
Commit-Queue: Etienne Pierre-Doray <etiennep@chromium.org>
Auto-Submit: Mike Wittman <wittman@chromium.org>
Reviewed-by: Etienne Pierre-Doray <etiennep@chromium.org>
Cr-Commit-Position: refs/heads/master@{#810394}

base/profiler/module_cache.cc

@@ -39,8 +39,10 @@ const ModuleCache::Module* ModuleCache::GetModuleForAddress(uintptr_t address) {
   std::unique_ptr<const Module> new_module = CreateModuleForAddress(address);
   if (!new_module)
     return nullptr;
-  const auto loc = native_modules_.insert(std::move(new_module));
-  return loc.first->get();
+  const auto result = native_modules_.insert(std::move(new_module));
+  // TODO(https://crbug.com/1131769): Reintroduce DCHECK(result.second) after
+  // fixing the issue that is causing it to fail.
+  return result.first->get();
 }
 
 std::vector<const ModuleCache::Module*> ModuleCache::GetModules() const {
@@ -87,12 +89,27 @@ void ModuleCache::UpdateNonNativeModules(
   // Insert the modules to be added. This operation is O((m + a) + a*log(a))
   // where m is the number of current modules and a is the number of modules to
   // be added.
+  const size_t prior_non_native_modules_size = non_native_modules_.size();
   non_native_modules_.insert(std::make_move_iterator(new_modules.begin()),
                              std::make_move_iterator(new_modules.end()));
+  // Every module in |new_modules| should have been moved into
+  // |non_native_modules_|. This guards against use-after-frees if |new_modules|
+  // were to contain any modules equivalent to what's already in
+  // |non_native_modules_|, in which case the module would remain in
+  // |new_modules| and be deleted on return from the function. While this
+  // scenario would be a violation of the API contract, it would present a
+  // difficult-to-track-down crash scenario.
+  CHECK_EQ(prior_non_native_modules_size + new_modules.size(),
+           non_native_modules_.size());
 }
 
 void ModuleCache::AddCustomNativeModule(std::unique_ptr<const Module> module) {
-  native_modules_.insert(std::move(module));
+  const bool was_inserted = native_modules_.insert(std::move(module)).second;
+  // |module| should have been inserted into |native_modules_|, indicating that
+  // there was no equivalent module already present. While this scenario would
+  // be a violation of the API contract, it would present a
+  // difficult-to-track-down crash scenario.
+  CHECK(was_inserted);
 }
 
 const ModuleCache::Module* ModuleCache::GetExistingModuleForAddress(

base/profiler/module_cache.h

@@ -88,7 +88,10 @@ class BASE_EXPORT ModuleCache {
   // specifically they no longer participate in the GetModuleForAddress()
   // lookup. They continue to exist for the lifetime of the ModuleCache,
   // however, so that existing references to them remain valid. Modules in
-  // |new_modules| are added to the set of active non-native modules.
+  // |new_modules| are added to the set of active non-native modules. Modules in
+  // |new_modules| may not overlap with any non-native Modules already present
+  // in ModuleCache, unless those modules are provided in |defunct_modules| in
+  // the same call.
   void UpdateNonNativeModules(
       const std::vector<const Module*>& defunct_modules,
       std::vector<std::unique_ptr<const Module>> new_modules);
@@ -96,6 +99,8 @@ class BASE_EXPORT ModuleCache {
   // Adds a custom native module to the cache. This is intended to support
   // native modules that require custom handling. In general, native modules
   // will be found and added automatically when invoking GetModuleForAddress().
+  // |module| may not overlap with any native Modules already present in
+  // ModuleCache.
   void AddCustomNativeModule(std::unique_ptr<const Module> module);
 
   // Gets the module containing |address| if one already exists, or nullptr