[Sampling profiler] Restore V8Unwinder embedded code range

Restores a separate representation of the V8 embedded code range as
compared to runtime-generated code ranges, now that V8 provides access
to this state in the API. This distinction was made in an earlier
version of the V8 unwind API but the current version did not previously
expose the necessary information. Representing the embedded code range
allows profiling of JavaScript builtins which live within the range.

Along the way fixes a bug in StackSamplerImpl where the unwinder's
AddInitialModules was invoked twice if the unwinder was added before
profiling started.

Fixed: 1136256
Change-Id: I6ec1433fcf5b7704cb9022781edc423d027b8da9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2476626
Commit-Queue: Mike Wittman <wittman@chromium.org>
Auto-Submit: Mike Wittman <wittman@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Reviewed-by: Etienne Pierre-Doray <etiennep@chromium.org>
Cr-Commit-Position: refs/heads/master@{#820934}

base/profiler/stack_sampler_impl.cc

@@ -93,10 +93,16 @@ void StackSamplerImpl::Initialize() {
 
   for (const auto& unwinder : unwinders_)
     unwinder->AddInitialModules(module_cache_);
+
+  was_initialized_ = true;
 }
 
 void StackSamplerImpl::AddAuxUnwinder(std::unique_ptr<Unwinder> unwinder) {
-  unwinder->AddInitialModules(module_cache_);
+  // Initialize() invokes AddInitialModules() on the unwinders that are present
+  // at the time. If it hasn't occurred yet, we allow it to add the initial
+  // modules, otherwise we do it here.
+  if (was_initialized_)
+    unwinder->AddInitialModules(module_cache_);
   unwinders_.push_front(std::move(unwinder));
 }
 

base/profiler/stack_sampler_impl.h

@@ -62,6 +62,11 @@ class BASE_EXPORT StackSamplerImpl : public StackSampler {
   ModuleCache* const module_cache_;
   const RepeatingClosure record_sample_callback_;
   StackSamplerTestDelegate* const test_delegate_;
+
+  // True if ownership of the object has been passed to the profiling thread and
+  // initialization has occurred there. If that's the case then any further aux
+  // unwinder that's provided needs to be set up within AddAuxUnwinder().
+  bool was_initialized_ = false;
 };
 
 }  // namespace base

base/profiler/stack_sampling_profiler_unittest.cc

@@ -474,12 +474,19 @@ PROFILER_TEST_F(StackSamplingProfilerTest, MAYBE_Basic) {
 // A simple unwinder that always generates one frame then aborts the stack walk.
 class TestAuxUnwinder : public Unwinder {
  public:
-  TestAuxUnwinder(const Frame& frame_to_report)
-      : frame_to_report_(frame_to_report) {}
+  TestAuxUnwinder(const Frame& frame_to_report,
+                  base::RepeatingClosure add_initial_modules_callback)
+      : frame_to_report_(frame_to_report),
+        add_initial_modules_callback_(std::move(add_initial_modules_callback)) {
+  }
 
   TestAuxUnwinder(const TestAuxUnwinder&) = delete;
   TestAuxUnwinder& operator=(const TestAuxUnwinder&) = delete;
 
+  void AddInitialModules(ModuleCache* module_cache) override {
+    if (add_initial_modules_callback_)
+      add_initial_modules_callback_.Run();
+  }
   bool CanUnwindFrom(const Frame& current_frame) const override { return true; }
 
   UnwindResult TryUnwind(RegisterContext* thread_context,
@@ -492,6 +499,7 @@ class TestAuxUnwinder : public Unwinder {
 
  private:
   const Frame frame_to_report_;
+  base::RepeatingClosure add_initial_modules_callback_;
 };
 
 // Checks that the profiler handles stacks containing dynamically-allocated
@@ -1285,6 +1293,12 @@ PROFILER_TEST_F(StackSamplingProfilerTest, AddAuxUnwinder_BeforeStart) {
 
   UnwindScenario scenario(BindRepeating(&CallWithPlainFunction));
 
+  int add_initial_modules_invocation_count = 0;
+  const auto add_initial_modules_callback =
+      [&add_initial_modules_invocation_count]() {
+        ++add_initial_modules_invocation_count;
+      };
+
   Profile profile;
   WithTargetThread(
       &scenario,
@@ -1303,12 +1317,15 @@ PROFILER_TEST_F(StackSamplingProfilerTest, AddAuxUnwinder_BeforeStart) {
                       sampling_thread_completed.Signal();
                     })),
                 CreateCoreUnwindersFactoryForTesting(module_cache()));
-            profiler.AddAuxUnwinder(
-                std::make_unique<TestAuxUnwinder>(Frame(23, nullptr)));
+            profiler.AddAuxUnwinder(std::make_unique<TestAuxUnwinder>(
+                Frame(23, nullptr),
+                BindLambdaForTesting(add_initial_modules_callback)));
             profiler.Start();
             sampling_thread_completed.Wait();
           }));
 
+  ASSERT_EQ(1, add_initial_modules_invocation_count);
+
   // The sample should have one frame from the context values and one from the
   // TestAuxUnwinder.
   ASSERT_EQ(1u, profile.samples.size());
@@ -1326,6 +1343,12 @@ PROFILER_TEST_F(StackSamplingProfilerTest, AddAuxUnwinder_AfterStart) {
 
   UnwindScenario scenario(BindRepeating(&CallWithPlainFunction));
 
+  int add_initial_modules_invocation_count = 0;
+  const auto add_initial_modules_callback =
+      [&add_initial_modules_invocation_count]() {
+        ++add_initial_modules_invocation_count;
+      };
+
   Profile profile;
   WithTargetThread(
       &scenario,
@@ -1345,11 +1368,14 @@ PROFILER_TEST_F(StackSamplingProfilerTest, AddAuxUnwinder_AfterStart) {
                     })),
                 CreateCoreUnwindersFactoryForTesting(module_cache()));
             profiler.Start();
-            profiler.AddAuxUnwinder(
-                std::make_unique<TestAuxUnwinder>(Frame(23, nullptr)));
+            profiler.AddAuxUnwinder(std::make_unique<TestAuxUnwinder>(
+                Frame(23, nullptr),
+                BindLambdaForTesting(add_initial_modules_callback)));
             sampling_thread_completed.Wait();
           }));
 
+  ASSERT_EQ(1, add_initial_modules_invocation_count);
+
   // The sample should have one frame from the context values and one from the
   // TestAuxUnwinder.
   ASSERT_EQ(1u, profile.samples.size());
@@ -1387,8 +1413,8 @@ PROFILER_TEST_F(StackSamplingProfilerTest, AddAuxUnwinder_AfterStop) {
                 CreateCoreUnwindersFactoryForTesting(module_cache()));
             profiler.Start();
             profiler.Stop();
-            profiler.AddAuxUnwinder(
-                std::make_unique<TestAuxUnwinder>(Frame(23, nullptr)));
+            profiler.AddAuxUnwinder(std::make_unique<TestAuxUnwinder>(
+                Frame(23, nullptr), base::RepeatingClosure()));
             sampling_thread_completed.Wait();
           }));
 

chrome/renderer/v8_unwinder.cc

@@ -12,8 +12,10 @@ namespace {
 
 class V8Module : public base::ModuleCache::Module {
  public:
-  explicit V8Module(const v8::MemoryRange& memory_range)
-      : memory_range_(memory_range) {}
+  enum CodeRangeType { kEmbedded, kNonEmbedded };
+
+  V8Module(const v8::MemoryRange& memory_range, CodeRangeType code_range_type)
+      : memory_range_(memory_range), code_range_type_(code_range_type) {}
 
   V8Module(const V8Module&) = delete;
   V8Module& operator=(const V8Module&) = delete;
@@ -24,13 +26,15 @@ class V8Module : public base::ModuleCache::Module {
   }
 
   std::string GetId() const override {
-    // We don't want to distinguish V8 code by memory region so we use the same
-    // synthetic build id for all V8Modules.
-    return V8Unwinder::kV8CodeRangeBuildId;
+    return code_range_type_ == kEmbedded
+               ? V8Unwinder::kV8EmbeddedCodeRangeBuildId
+               : V8Unwinder::kV8CodeRangeBuildId;
   }
 
   base::FilePath GetDebugBasename() const override {
-    return base::FilePath().AppendASCII("V8 Code Range");
+    return base::FilePath().AppendASCII(code_range_type_ == kEmbedded
+                                            ? "V8 Embedded Code Range"
+                                            : "V8 Code Range");
   }
 
   size_t GetSize() const override { return memory_range_.length_in_bytes; }
@@ -39,6 +43,7 @@ class V8Module : public base::ModuleCache::Module {
 
  private:
   const v8::MemoryRange memory_range_;
+  const CodeRangeType code_range_type_;
 };
 
 // Heterogeneous comparator for MemoryRanges and Modules. Compares on both
@@ -65,13 +70,33 @@ struct MemoryRangeModuleCompare {
   }
 };
 
+v8::MemoryRange GetEmbeddedCodeRange(v8::Isolate* isolate) {
+  v8::MemoryRange range;
+  isolate->GetEmbeddedCodeRange(&range.start, &range.length_in_bytes);
+  return range;
+}
+
 }  // namespace
 
 V8Unwinder::V8Unwinder(v8::Isolate* isolate)
-    : isolate_(isolate), js_entry_stubs_(isolate->GetJSEntryStubs()) {}
+    : isolate_(isolate),
+      js_entry_stubs_(isolate->GetJSEntryStubs()),
+      embedded_code_range_(GetEmbeddedCodeRange(isolate)) {}
 
 V8Unwinder::~V8Unwinder() = default;
 
+void V8Unwinder::AddInitialModules(base::ModuleCache* module_cache) {
+  // This function must be called only once.
+  DCHECK(modules_.empty());
+
+  // Add a module for the embedded code range.
+  std::vector<std::unique_ptr<const base::ModuleCache::Module>> new_module;
+  new_module.push_back(
+      std::make_unique<V8Module>(embedded_code_range_, V8Module::kEmbedded));
+  modules_.insert(new_module.front().get());
+  module_cache->UpdateNonNativeModules({}, std::move(new_module));
+}
+
 // IMPORTANT NOTE: to avoid deadlock this function must not invoke any
 // non-reentrant code that is also invoked by the target thread. In particular,
 // no heap allocation or deallocation is permitted, including indirectly via use
@@ -83,9 +108,20 @@ void V8Unwinder::OnStackCapture() {
       std::min(required_code_ranges_capacity_, code_ranges_.capacity()));
 }
 
+// Update the modules based on what was recorded in |code_ranges_|. The singular
+// embedded code range was already added in in AddInitialModules(). It is
+// preserved by the algorithm below, which is why kNonEmbedded is
+// unconditionally passed when creating new modules.
 void V8Unwinder::UpdateModules(base::ModuleCache* module_cache) {
   MemoryRangeModuleCompare less_than;
 
+  const auto is_embedded_code_range_module =
+      [this](const base::ModuleCache::Module* module) {
+        return module->GetBaseAddress() ==
+                   reinterpret_cast<uintptr_t>(embedded_code_range_.start) &&
+               module->GetSize() == embedded_code_range_.length_in_bytes;
+      };
+
   std::vector<std::unique_ptr<const base::ModuleCache::Module>> new_modules;
   std::vector<const base::ModuleCache::Module*> defunct_modules;
 
@@ -97,14 +133,23 @@ void V8Unwinder::UpdateModules(base::ModuleCache* module_cache) {
   DCHECK(std::is_sorted(code_ranges_start, code_ranges_end, less_than));
   v8::MemoryRange* range_it = code_ranges_start;
   auto modules_it = modules_.begin();
+
   while (range_it != code_ranges_end && modules_it != modules_.end()) {
     if (less_than(*range_it, *modules_it)) {
-      new_modules.push_back(std::make_unique<V8Module>(*range_it));
+      new_modules.push_back(
+          std::make_unique<V8Module>(*range_it, V8Module::kNonEmbedded));
       modules_.insert(modules_it, new_modules.back().get());
       ++range_it;
     } else if (less_than(*modules_it, *range_it)) {
-      defunct_modules.push_back(*modules_it);
-      modules_it = modules_.erase(modules_it);
+      // Avoid deleting the embedded code range module if it wasn't provided in
+      // |code_ranges_|. This could happen if |code_ranges_| had insufficient
+      // capacity when the code pages were copied.
+      if (!is_embedded_code_range_module(*modules_it)) {
+        defunct_modules.push_back(*modules_it);
+        modules_it = modules_.erase(modules_it);
+      } else {
+        ++modules_it;
+      }
     } else {
       // The range already has a module, so there's nothing to do.
       ++range_it;
@@ -113,14 +158,19 @@ void V8Unwinder::UpdateModules(base::ModuleCache* module_cache) {
   }
 
   while (range_it != code_ranges_end) {
-    new_modules.push_back(std::make_unique<V8Module>(*range_it));
+    new_modules.push_back(
+        std::make_unique<V8Module>(*range_it, V8Module::kNonEmbedded));
     modules_.insert(modules_it, new_modules.back().get());
     ++range_it;
   }
 
   while (modules_it != modules_.end()) {
-    defunct_modules.push_back(*modules_it);
-    modules_it = modules_.erase(modules_it);
+    if (!is_embedded_code_range_module(*modules_it)) {
+      defunct_modules.push_back(*modules_it);
+      modules_it = modules_.erase(modules_it);
+    } else {
+      ++modules_it;
+    }
   }
 
   module_cache->UpdateNonNativeModules(defunct_modules, std::move(new_modules));
@@ -179,9 +229,12 @@ size_t V8Unwinder::CopyCodePages(size_t capacity, v8::MemoryRange* code_pages) {
   return isolate_->CopyCodePages(capacity, code_pages);
 }
 
-// Synthetic build id to use for V8 modules.
-const char V8Unwinder::kV8CodeRangeBuildId[] =
+// Synthetic build ids to use for V8 modules. The difference is in the digit
+// after the leading 5's.
+const char V8Unwinder::kV8EmbeddedCodeRangeBuildId[] =
     "5555555507284E1E874EFA4EB754964B999";
+const char V8Unwinder::kV8CodeRangeBuildId[] =
+    "5555555517284E1E874EFA4EB754964B999";
 
 V8Unwinder::MemoryRanges::MemoryRanges()
     : capacity_(v8::Isolate::kMinCodePagesBufferSize),

chrome/renderer/v8_unwinder.h

@@ -22,6 +22,7 @@ class V8Unwinder : public base::Unwinder {
   V8Unwinder& operator=(const V8Unwinder&) = delete;
 
   // Unwinder:
+  void AddInitialModules(base::ModuleCache* module_cache) override;
   void OnStackCapture() override;
   void UpdateModules(base::ModuleCache* module_cache) override;
   bool CanUnwindFrom(const base::Frame& current_frame) const override;
@@ -31,6 +32,7 @@ class V8Unwinder : public base::Unwinder {
                                std::vector<base::Frame>* stack) const override;
 
   // Build ids generated by the unwinder. Exposed for test use.
+  static const char kV8EmbeddedCodeRangeBuildId[];
   static const char kV8CodeRangeBuildId[];
 
  protected:
@@ -84,6 +86,7 @@ class V8Unwinder : public base::Unwinder {
 
   v8::Isolate* const isolate_;
   const v8::JSEntryStubs js_entry_stubs_;
+  const v8::MemoryRange embedded_code_range_;
 
   // Code ranges recorded for the current sample.
   MemoryRanges code_ranges_;