Fix memory leak and use LocalUpdatableMaps for libunwindstack unwinder

We had a memory leak in our usage of libunwindstack unwinder. While
reparsing maps we never clear() the |maps_| vector. Which should have been
cleared given Maps::Parse() only adds new elements to the vector.

Switching to LocalUpdatableMaps should fix the memory leak and it should
be more efficient than clearing the |maps_| vector, since it will not
invalidate the existing decompresssed unwind info.
Cons of using LocalUpdatableMaps:
- Uses locks which is not required for our use case in Chromium since we
  just unwind on StackSamplingProfiler thread only.
- No rate limiting on re parsing maps.
  -- I plan to add an UMA Histogram to keep track of count of reparses
     in a session.

Bug: 1418485, b/269239545
Change-Id: Iaac5d1a6fca0f1215c9286f214d39bdcb2273383
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4295821
Reviewed-by: Mike Wittman <wittman@chromium.org>
Reviewed-by: Tushar Agarwal <agarwaltushar@google.com>
Commit-Queue: Kartar Singh <kartarsingh@google.com>
Reviewed-by: Tommy Nyquist <nyquist@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1112113}

base/BUILD.gn

@@ -2805,6 +2805,20 @@ if ((is_win && (current_cpu == "x64" || current_cpu == "arm64")) || is_mac ||
   }
 }
 
+if (is_android && (current_cpu == "arm" || current_cpu == "arm64")) {
+  # Use separate library for
+  # |LibunwindstackUnwinderAndroidTest.ReparsesMapsOnNewDynamicLibraryLoad|
+  # testcase. We can't use the existing `base_profiler_test_support_library`
+  # library for this test since this gets loaded by other tests and unloading
+  # a library in Android doesn't guarantee it will actually be unloaded.
+  # And in the test we would like to observe the change in /proc/self/maps
+  # on loading a dynamic library.
+  loadable_module("base_profiler_reparsing_test_support_library") {
+    testonly = true
+    sources = [ "profiler/test_support_library.cc" ]
+  }
+}
+
 if (is_android) {
   source_set("native_unwinder_android") {
     # This target is intended to be used only within the stack_unwinder dynamic
@@ -3568,6 +3582,7 @@ test("base_unittests") {
         "profiler/native_unwinder_android_unittest.cc",
       ]
       deps += [
+        ":base_profiler_reparsing_test_support_library",
         ":base_profiler_test_support_java",
         ":base_profiler_test_support_jni_headers",
         ":base_profiler_test_support_library",

base/profiler/libunwindstack_unwinder_android.cc

@@ -34,14 +34,6 @@
 
 namespace base {
 namespace {
-// How frequently we're willing to try and reparse maps. Sometimes, dynamic
-// libraries get added and can cause unwinding to fail which can be solved by
-// reparsing maps. However reparsing maps is an expensive operation and we don't
-// want to cause churn if there is for some reason some map consistently
-// failing.
-//
-// if we're sampling every 50ms, 1200 samples is 1 minute.
-const int kMinSamplesBeforeNextMapsParse = 1200;
 
 class NonElfModule : public ModuleCache::Module {
  public:
@@ -82,10 +74,12 @@ std::unique_ptr<unwindstack::Regs> CreateFromRegisterContext(
   return nullptr;
 #endif  // #if defined(ARCH_CPU_ARM_FAMILY) && defined(ARCH_CPU_32_BITS)
 }
+
 }  // namespace
 
 LibunwindstackUnwinderAndroid::LibunwindstackUnwinderAndroid()
-    : memory_regions_map_(NativeUnwinderAndroid::CreateMemoryRegionsMap()),
+    : memory_regions_map_(NativeUnwinderAndroid::CreateMemoryRegionsMap(
+          /*use_updatable_maps=*/true)),
       process_memory_(std::shared_ptr<unwindstack::Memory>(
           memory_regions_map_->TakeMemory().release())) {
   TRACE_EVENT_INSTANT(
@@ -139,46 +133,21 @@ UnwindResult LibunwindstackUnwinderAndroid::TryUnwind(
     std::vector<unwindstack::FrameData> frames;
   };
 
-  auto attempt_unwind = [&]() {
-    // regs will get clobbered by each attempt, so if it fails we have to
-    // start fresh from the initial context.
-    std::unique_ptr<unwindstack::Regs> regs =
-        CreateFromRegisterContext(thread_context);
-    DCHECK(regs);
-    unwindstack::Unwinder unwinder(kMaxFrames, memory_regions_map_->GetMaps(),
-                                   regs.get(), process_memory_);
-
-    unwinder.SetJitDebug(GetOrCreateJitDebug(regs->Arch()));
-    unwinder.SetDexFiles(GetOrCreateDexFiles(regs->Arch()));
-
-    unwinder.Unwind(/*initial_map_names_to_skip=*/nullptr,
-                    /*map_suffixes_to_ignore=*/nullptr);
-    ++samples_since_last_maps_parse_;
-    // Currently libunwindstack doesn't support warnings.
-    return UnwindValues{unwinder.LastErrorCode(), /*unwinder.warnings()*/ 0,
-                        unwinder.ConsumeFrames()};
-  };
-
-  // We now proceed with the first unwind.
-  UnwindValues values = attempt_unwind();
-
-  // If our maps are invalid and we haven't reparsed in awhile then attempt to
-  // reparse the maps and reunwind the stack to recover from the error.
-  bool should_retry =
-      values.error_code == unwindstack::ERROR_INVALID_MAP &&
-      samples_since_last_maps_parse_ > kMinSamplesBeforeNextMapsParse;
-  if (should_retry) {
-    TRACE_EVENT(TRACE_DISABLED_BY_DEFAULT("cpu_profiler.debug"),
-                "TryUnwind Reparsing Maps");
-    samples_since_last_maps_parse_ = 0;
-    memory_regions_map_->GetMaps()->Parse();
-    jit_debug_.reset();
-    dex_files_.reset();
-
-    // Our second attempt will override our first attempt when we check the
-    // result later.
-    values = attempt_unwind();
-  }
+  std::unique_ptr<unwindstack::Regs> regs =
+      CreateFromRegisterContext(thread_context);
+  DCHECK(regs);
+  unwindstack::Unwinder unwinder(kMaxFrames, memory_regions_map_->GetMaps(),
+                                 regs.get(), process_memory_);
+
+  unwinder.SetJitDebug(GetOrCreateJitDebug(regs->Arch()));
+  unwinder.SetDexFiles(GetOrCreateDexFiles(regs->Arch()));
+
+  unwinder.Unwind(/*initial_map_names_to_skip=*/nullptr,
+                  /*map_suffixes_to_ignore=*/nullptr);
+  // Currently libunwindstack doesn't support warnings.
+  UnwindValues values =
+      UnwindValues{unwinder.LastErrorCode(), /*unwinder.warnings()*/ 0,
+                   unwinder.ConsumeFrames()};
 
   // Check the result of either the first or second unwind. If we were
   // successful transfer from libunwindstack format into base::Unwinder format.

base/profiler/libunwindstack_unwinder_android.h

@@ -42,7 +42,6 @@ class LibunwindstackUnwinderAndroid : public Unwinder {
   unwindstack::JitDebug* GetOrCreateJitDebug(unwindstack::ArchEnum arch);
   unwindstack::DexFiles* GetOrCreateDexFiles(unwindstack::ArchEnum arch);
 
-  int samples_since_last_maps_parse_ = 0;
   std::unique_ptr<NativeUnwinderAndroidMemoryRegionsMap> memory_regions_map_;
   // libunwindstack::Unwinder requires that process_memory be provided as a
   // std::shared_ptr. Since this is a third_party library this exception to

base/profiler/libunwindstack_unwinder_android_unittest.cc

@@ -12,6 +12,8 @@
 
 #include "base/android/build_info.h"
 #include "base/functional/bind.h"
+#include "base/native_library.h"
+#include "base/path_service.h"
 #include "base/profiler/register_context.h"
 #include "base/profiler/stack_buffer.h"
 #include "base/profiler/stack_copier_signal.h"
@@ -21,6 +23,7 @@
 #include "base/profiler/thread_delegate_posix.h"
 #include "base/test/bind.h"
 #include "build/build_config.h"
+#include "stack_sampling_profiler_test_util.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace base {
@@ -217,4 +220,34 @@ TEST(LibunwindstackUnwinderAndroidTest, MAYBE_JavaFunction) {
                                     "callWithJavaFunction"});
 }
 
+TEST(LibunwindstackUnwinderAndroidTest, ReparsesMapsOnNewDynamicLibraryLoad) {
+  // The current version of /proc/self/maps is used to create
+  // memory_regions_map_ object.
+  auto unwinder = std::make_unique<LibunwindstackUnwinderAndroid>();
+  ModuleCache module_cache;
+  unwinder->Initialize(&module_cache);
+
+  // Dynamically loading a library should update maps and a reparse is required
+  // to actually unwind through functions involving this library.
+  NativeLibrary dynamic_library =
+      LoadTestLibrary("base_profiler_reparsing_test_support_library");
+  UnwindScenario scenario(
+      BindRepeating(&CallThroughOtherLibrary, Unretained(dynamic_library)));
+
+  auto sample =
+      CaptureScenario(&scenario, &module_cache,
+                      BindLambdaForTesting([&](RegisterContext* thread_context,
+                                               uintptr_t stack_top,
+                                               std::vector<Frame>* sample) {
+                        ASSERT_TRUE(unwinder->CanUnwindFrom(sample->back()));
+                        UnwindResult result = unwinder->TryUnwind(
+                            thread_context, stack_top, sample);
+                        EXPECT_EQ(UnwindResult::kCompleted, result);
+                      }));
+
+  ExpectStackContains(sample, {scenario.GetWaitForSampleAddressRange(),
+                               scenario.GetSetupFunctionAddressRange(),
+                               scenario.GetOuterFunctionAddressRange()});
+}
+
 }  // namespace base

base/profiler/native_unwinder_android.cc

@@ -131,8 +131,13 @@ size_t UnwindStackMemoryAndroid::Read(uint64_t addr, void* dst, size_t size) {
 
 // static
 std::unique_ptr<NativeUnwinderAndroidMemoryRegionsMap>
-NativeUnwinderAndroid::CreateMemoryRegionsMap() {
-  auto maps = std::make_unique<unwindstack::LocalMaps>();
+NativeUnwinderAndroid::CreateMemoryRegionsMap(bool use_updatable_maps) {
+  std::unique_ptr<unwindstack::Maps> maps;
+  if (use_updatable_maps) {
+    maps = std::make_unique<unwindstack::LocalUpdatableMaps>();
+  } else {
+    maps = std::make_unique<unwindstack::LocalMaps>();
+  }
   const bool success = maps->Parse();
   DCHECK(success);
 

base/profiler/native_unwinder_android.h

@@ -39,8 +39,16 @@ class NativeUnwinderAndroid : public Unwinder,
   // Creates maps object from /proc/self/maps for use by NativeUnwinderAndroid.
   // Since this is an expensive call, the maps object should be re-used across
   // all profiles in a process.
+  // Set |use_updatable_maps| to true to use unwindstack::LocalUpdatableMaps,
+  // instead of unwindstack::LocalMaps. LocalUpdatableMaps might be preferable
+  // when the frames come from dynamically added ELFs like JITed ELFs, or
+  // dynamically loaded libraries. With LocalMaps the frames corresponding to
+  // newly loaded ELFs don't get unwound since the existing maps structure
+  // fails to find a map for the given pc while LocalUpdatableMaps reparses
+  // /proc/self/maps when it fails to find a map for the given pc and then can
+  // successfully unwind through newly loaded ELFs as well.
   static std::unique_ptr<NativeUnwinderAndroidMemoryRegionsMap>
-  CreateMemoryRegionsMap();
+  CreateMemoryRegionsMap(bool use_updatable_maps = false);
 
   // |exclude_module_with_base_address| is used to exclude a specific module and
   // let another unwinder take control. TryUnwind() will exit with

base/profiler/stack_sampling_profiler_test_util.cc

@@ -424,31 +424,35 @@ void ExpectStackDoesNotContain(
   }
 }
 
-NativeLibrary LoadOtherLibrary() {
+NativeLibrary LoadTestLibrary(StringPiece library_name) {
   // The lambda gymnastics works around the fact that we can't use ASSERT_*
   // macros in a function returning non-null.
-  const auto load = [](NativeLibrary* library) {
-    FilePath other_library_path;
+  const auto load = [&](NativeLibrary* library) {
+    FilePath library_path;
 #if BUILDFLAG(IS_FUCHSIA)
     // TODO(crbug.com/1262430): Find a solution that works across platforms.
-    ASSERT_TRUE(PathService::Get(DIR_ASSETS, &other_library_path));
+    ASSERT_TRUE(PathService::Get(DIR_ASSETS, &library_path));
 #else
     // The module is next to the test module rather than with test data.
-    ASSERT_TRUE(PathService::Get(DIR_MODULE, &other_library_path));
+    ASSERT_TRUE(PathService::Get(DIR_MODULE, &library_path));
 #endif  // BUILDFLAG(IS_FUCHSIA)
-    other_library_path = other_library_path.AppendASCII(
-        GetLoadableModuleName("base_profiler_test_support_library"));
+    library_path =
+        library_path.AppendASCII(GetLoadableModuleName(library_name));
     NativeLibraryLoadError load_error;
-    *library = LoadNativeLibrary(other_library_path, &load_error);
-    ASSERT_TRUE(*library) << "error loading " << other_library_path.value()
-                          << ": " << load_error.ToString();
+    *library = LoadNativeLibrary(library_path, &load_error);
+    ASSERT_TRUE(*library) << "error loading " << library_path.value() << ": "
+                          << load_error.ToString();
   };
 
   NativeLibrary library = nullptr;
   load(&library);
   return library;
 }
 
+NativeLibrary LoadOtherLibrary() {
+  return LoadTestLibrary("base_profiler_test_support_library");
+}
+
 uintptr_t GetAddressInOtherLibrary(NativeLibrary library) {
   EXPECT_TRUE(library);
   uintptr_t address = reinterpret_cast<uintptr_t>(

base/profiler/stack_sampling_profiler_test_util.h

@@ -17,6 +17,7 @@
 #include "base/profiler/frame.h"
 #include "base/profiler/sampling_profiler_thread_token.h"
 #include "base/profiler/stack_sampling_profiler.h"
+#include "base/strings/string_piece.h"
 #include "base/synchronization/waitable_event.h"
 #include "base/threading/platform_thread.h"
 
@@ -179,6 +180,9 @@ void ExpectStackDoesNotContain(
     const std::vector<Frame>& stack,
     const std::vector<FunctionAddressRange>& functions);
 
+// Load test library with given name.
+NativeLibrary LoadTestLibrary(StringPiece library_name);
+
 // Loads the other library, which defines a function to be called in the
 // WITH_OTHER_LIBRARY configuration.
 NativeLibrary LoadOtherLibrary();

testing/android/native_test/java/src/org/chromium/native_test/NativeUnitTest.java

@@ -52,6 +52,12 @@ public void preCreate(Activity activity) {
     private void loadLibraries() {
         LibraryLoader.setEnvForNative();
         for (String library : NativeLibraries.LIBRARIES) {
+            // Do not load this library early so that
+            // |LibunwindstackUnwinderAndroidTest.ReparsesMapsOnNewDynamicLibraryLoad| test can
+            // observe the change in /proc/self/maps before and after loading the library.
+            if (library.equals("base_profiler_reparsing_test_support_library")) {
+                continue;
+            }
             Log.i(TAG, "loading: %s", library);
             System.loadLibrary(library);
             Log.i(TAG, "loaded: %s", library);