[Sampling profiler] Fix ModuleCache module partitioning

std::remove_if may not preserve removed elements if they aren't already
at the end of the container. Use std::stable_partition instead to
get this behavior and avoid destroying modules prematurely.

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

base/profiler/module_cache.cc

@@ -54,38 +54,41 @@ std::vector<const ModuleCache::Module*> ModuleCache::GetModules() const {
 }
 
 void ModuleCache::UpdateNonNativeModules(
-    const std::vector<const Module*>& to_remove,
-    std::vector<std::unique_ptr<const Module>> to_add) {
+    const std::vector<const Module*>& defunct_modules,
+    std::vector<std::unique_ptr<const Module>> new_modules) {
   // Insert the modules to remove into a set to support O(log(n)) lookup below.
-  flat_set<const Module*> to_remove_set(to_remove.begin(), to_remove.end());
+  flat_set<const Module*> defunct_modules_set(defunct_modules.begin(),
+                                              defunct_modules.end());
 
   // Reorder the modules to be removed to the last slots in the set, then move
   // them to the inactive modules, then erase the moved-from modules from the
-  // set. The flat_set docs endorse using base::EraseIf() which performs the
-  // same operations -- exclusive of the moves -- so this is OK even though it
-  // might seem like we're messing with the internal set representation.
+  // set. This is a variation on the standard erase-remove idiom, which is
+  // explicitly endorsed for implementing erase behavior on flat_sets.
   //
-  // remove_if is O(m*log(r)) where m is the number of current modules and r is
-  // the number of modules to remove. insert and erase are both O(r).
-  auto first_module_to_remove = std::remove_if(
+  // stable_partition is O(m*log(r)) where m is the number of current modules
+  // and r is the number of modules to remove. insert and erase are both O(r).
+  auto first_module_defunct_modules = std::stable_partition(
       non_native_modules_.begin(), non_native_modules_.end(),
-      [&to_remove_set](const std::unique_ptr<const Module>& module) {
-        return to_remove_set.find(module.get()) != to_remove_set.end();
+      [&defunct_modules_set](const std::unique_ptr<const Module>& module) {
+        return defunct_modules_set.find(module.get()) ==
+               defunct_modules_set.end();
       });
   // All modules requested to be removed should have been found.
-  DCHECK_EQ(static_cast<ptrdiff_t>(to_remove.size()),
-            std::distance(first_module_to_remove, non_native_modules_.end()));
+  DCHECK_EQ(
+      static_cast<ptrdiff_t>(defunct_modules.size()),
+      std::distance(first_module_defunct_modules, non_native_modules_.end()));
   inactive_non_native_modules_.insert(
       inactive_non_native_modules_.end(),
-      std::make_move_iterator(first_module_to_remove),
+      std::make_move_iterator(first_module_defunct_modules),
       std::make_move_iterator(non_native_modules_.end()));
-  non_native_modules_.erase(first_module_to_remove, non_native_modules_.end());
+  non_native_modules_.erase(first_module_defunct_modules,
+                            non_native_modules_.end());
 
   // 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.
-  non_native_modules_.insert(std::make_move_iterator(to_add.begin()),
-                             std::make_move_iterator(to_add.end()));
+  non_native_modules_.insert(std::make_move_iterator(new_modules.begin()),
+                             std::make_move_iterator(new_modules.end()));
 }
 
 void ModuleCache::AddCustomNativeModule(std::unique_ptr<const Module> module) {

base/profiler/module_cache.h

@@ -84,14 +84,14 @@ class BASE_EXPORT ModuleCache {
   // GetModuleForAddress() will return the non-native module rather than the
   // native module for the memory region it occupies.
   //
-  // Modules in |to_remove| are removed from the set of active modules;
+  // Modules in |defunct_modules| are removed from the set of active modules;
   // 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
-  // |to_add| are added to the set of active non-native modules.
+  // |new_modules| are added to the set of active non-native modules.
   void UpdateNonNativeModules(
-      const std::vector<const Module*>& to_remove,
-      std::vector<std::unique_ptr<const Module>> to_add);
+      const std::vector<const Module*>& defunct_modules,
+      std::vector<std::unique_ptr<const Module>> new_modules);
 
   // Adds a custom native module to the cache. This is intended to support
   // native modules that require custom handling. In general, native modules

base/profiler/module_cache_unittest.cc

@@ -226,6 +226,28 @@ MAYBE_TEST(ModuleCacheTest, UpdateNonNativeModulesRemoveModuleIsNotDestroyed) {
   EXPECT_TRUE(was_destroyed);
 }
 
+// Regression test to validate that when modules are partitioned into modules to
+// keep and modules to remove, the modules to remove are not destroyed.
+// https://crbug.com/1127466 case 2.
+MAYBE_TEST(ModuleCacheTest, UpdateNonNativeModulesPartitioning) {
+  int destroyed_count = 0;
+  const auto record_destroyed = [&destroyed_count]() { ++destroyed_count; };
+  {
+    ModuleCache cache;
+    std::vector<std::unique_ptr<const ModuleCache::Module>> modules;
+    modules.push_back(std::make_unique<FakeModule>(
+        1, 1, false, BindLambdaForTesting(record_destroyed)));
+    const ModuleCache::Module* module1 = modules.back().get();
+    modules.push_back(std::make_unique<FakeModule>(
+        2, 1, false, BindLambdaForTesting(record_destroyed)));
+    cache.UpdateNonNativeModules({}, std::move(modules));
+    cache.UpdateNonNativeModules({module1}, {});
+
+    EXPECT_EQ(0, destroyed_count);
+  }
+  EXPECT_EQ(2, destroyed_count);
+}
+
 MAYBE_TEST(ModuleCacheTest, UpdateNonNativeModulesReplace) {
   ModuleCache cache;
   // Replace a module with another larger module at the same base address.