[Clank SSM]: Release read_lock only after thread resumes.

This CL removes StackCopier::Delegate::OnThreadResume and manually resets
MetadataProvider in StackSamplerImpl::RecordStackFrames, and enables
RecordMetadata() on android (more specifically StackCopiedSignal).

|read_lock_| is only acquired:
- before CopyStack() on the sampling thread. Since there is only 1 sampling
  thread per process, this won't have any contention.
- in TryReclaimInactiveSlots() with |read_lock_.Try()|. Failing to acquire
  delays ReclaimInactiveSlots().
Since read_lock_ is only held during stack capture (compared to stack walking
which takes ~3x as long), starvation is unlikely to affect TryReclaimInactiveSlots().

Bug: 1056283
Change-Id: I4f0ef3e8c3641ec7b7258da519c3fba519fb096b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2154738
Commit-Queue: Etienne Pierre-Doray <etiennep@chromium.org>
Reviewed-by: Mike Wittman <wittman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#763420}

base/profiler/metadata_recorder.cc

@@ -107,23 +107,16 @@ void MetadataRecorder::Remove(uint64_t name_hash, Optional<int64_t> key) {
 MetadataRecorder::MetadataProvider::MetadataProvider(
     MetadataRecorder* metadata_recorder)
     : metadata_recorder_(metadata_recorder),
-      auto_lock_(&metadata_recorder->read_lock_) {}
+      auto_lock_(metadata_recorder->read_lock_) {}
 
 MetadataRecorder::MetadataProvider::~MetadataProvider() = default;
 
-// This function is marked as NO_THREAD_SAFETY_ANALYSIS because the analyzer
-// doesn't understand that the lock is acquired in the constructor initializer
-// list and can therefore be safely released here.
-size_t MetadataRecorder::MetadataProvider::GetItems(ItemArray* items)
-    NO_THREAD_SAFETY_ANALYSIS {
-  size_t item_count = metadata_recorder_->GetItems(items);
-  auto_lock_.Release();
-  return item_count;
-}
-
-std::unique_ptr<MetadataRecorder::MetadataProvider>
-MetadataRecorder::CreateMetadataProvider() {
-  return std::make_unique<MetadataRecorder::MetadataProvider>(this);
+size_t MetadataRecorder::MetadataProvider::GetItems(
+    ItemArray* const items) const {
+  // Assertion is only necessary so that thread annotations recognize that
+  // |read_lock_| is acquired.
+  metadata_recorder_->read_lock_.AssertAcquired();
+  return metadata_recorder_->GetItems(items);
 }
 
 size_t MetadataRecorder::GetItems(ItemArray* const items) const {

base/profiler/metadata_recorder.h

@@ -53,11 +53,11 @@ namespace base {
 //        allows readers to preallocate the data structure that we pass back
 //        the metadata in.
 //
-// C) We shouldn't guard writes with a lock that also guards reads. It can take
-//    ~30us from the time that the sampling thread requests that a thread be
-//    suspended and the time that it actually happens. If all metadata writes
-//    block their thread during that time, we're very likely to block all Chrome
-//    threads for an additional 30us per sample.
+// C) We shouldn't guard writes with a lock that also guards reads, since the
+//    read lock is held from the time that the sampling thread requests that a
+//    thread be suspended up to the time that the thread is resumed. If all
+//    metadata writes block their thread during that time, we're very likely to
+//    block all Chrome threads.
 //
 //      Ramifications:
 //
@@ -93,8 +93,8 @@ namespace base {
 //
 // - No thread is using the recorder.
 //
-// - A single writer is writing into the recorder without a simultaneous
-//   read. The write will succeed.
+// - A single writer is writing into the recorder without a simultaneous read.
+//   The write will succeed.
 //
 // - A reader is reading from the recorder without a simultaneous write. The
 //   read will succeed.
@@ -154,16 +154,15 @@ class BASE_EXPORT MetadataRecorder {
   void Remove(uint64_t name_hash, Optional<int64_t> key);
 
   // An object that provides access to a MetadataRecorder's items and holds the
-  // necessary exclusive read lock until either GetItems() is called or the
-  // object is destroyed. Reclaiming of inactive slots in the recorder
-  // can't occur while this object lives, so it should be created as soon before
-  // it's needed as possible. Calling GetItems() releases the lock held by the
-  // object and can therefore only be called once during the object's lifetime.
+  // necessary exclusive read lock until the object is destroyed. Reclaiming of
+  // inactive slots in the recorder can't occur while this object lives, so it
+  // should be created as soon before it's needed as possible and released as
+  // soon as possible.
   //
-  // This object should be created *before* suspending the target
-  // thread. Otherwise, that thread might be suspended while reclaiming inactive
-  // slots and holding the read lock, which would cause the sampling thread to
-  // deadlock.
+  // This object should be created *before* suspending the target thread and
+  // destroyed after resuming the target thread. Otherwise, that thread might be
+  // suspended while reclaiming inactive slots and holding the read lock, which
+  // would cause the sampling thread to deadlock.
   //
   // Example usage:
   //
@@ -178,7 +177,7 @@ class BASE_EXPORT MetadataRecorder {
   class SCOPED_LOCKABLE BASE_EXPORT MetadataProvider {
    public:
     // Acquires an exclusive read lock on the metadata recorder which is held
-    // until either GetItems() is called or the object is destroyed.
+    // until the object is destroyed.
     explicit MetadataProvider(MetadataRecorder* metadata_recorder)
         EXCLUSIVE_LOCK_FUNCTION(metadata_recorder_->read_lock_);
     ~MetadataProvider() UNLOCK_FUNCTION();
@@ -189,21 +188,13 @@ class BASE_EXPORT MetadataRecorder {
     // copies them into |items|, returning the number of metadata items that
     // were copied. To ensure that all items can be copied, |available slots|
     // should be greater than or equal to |MAX_METADATA_COUNT|.
-    //
-    // This function releases the lock held by the object and can therefore only
-    // be called once during the object's lifetime.
-    size_t GetItems(ItemArray* items);
+    size_t GetItems(ItemArray* const items) const;
 
    private:
     const MetadataRecorder* const metadata_recorder_;
-    base::ReleasableAutoLock auto_lock_;
+    base::AutoLock auto_lock_;
   };
 
-  // Creates a MetadataProvider object for the recorder, which acquires the
-  // necessary exclusive read lock and provides access to the recorder's items
-  // via its GetItems() function.
-  std::unique_ptr<MetadataProvider> CreateMetadataProvider();
-
  private:
   // TODO(charliea): Support large quantities of metadata efficiently.
   struct ItemInternal {

base/profiler/profile_builder.h

@@ -36,7 +36,7 @@ class BASE_EXPORT ProfileBuilder {
   // implementations should simply atomically copy metadata state to be
   // associated with the sample.
   virtual void RecordMetadata(
-      MetadataRecorder::MetadataProvider* metadata_provider) {}
+      const MetadataRecorder::MetadataProvider& metadata_provider) {}
 
   // Applies the specified metadata |item| to samples collected in the range
   // [period_start, period_end), iff the profile already captured execution that

base/profiler/stack_copier.h

@@ -35,9 +35,6 @@ class BASE_EXPORT StackCopier {
     // deallocation, including indirectly via use of DCHECK/CHECK or other
     // logging statements.
     virtual void OnStackCopy() = 0;
-
-    // Invoked after the stack has been copied and the target thread resumed.
-    virtual void OnThreadResume() = 0;
   };
 
   virtual ~StackCopier();

base/profiler/stack_copier_signal.cc

@@ -229,8 +229,6 @@ bool StackCopierSignal::CopyStack(StackBuffer* stack_buffer,
     }
   }
 
-  delegate->OnThreadResume();
-
   const uintptr_t bottom = RegisterContextStackPointer(params.context);
   for (uintptr_t* reg :
        thread_delegate_->GetRegistersToRewrite(thread_context)) {

base/profiler/stack_copier_signal_unittest.cc

@@ -63,25 +63,13 @@ class TargetThread : public SimpleThread {
 class TestStackCopierDelegate : public StackCopier::Delegate {
  public:
   void OnStackCopy() override {
-    // We can't EXPECT_FALSE(on_thread_resume_was_invoked_) here because that
-    // invocation is not reentrant.
     on_stack_copy_was_invoked_ = true;
   }
 
-  void OnThreadResume() override {
-    EXPECT_TRUE(on_stack_copy_was_invoked_);
-    on_thread_resume_was_invoked_ = true;
-  }
-
   bool on_stack_copy_was_invoked() const { return on_stack_copy_was_invoked_; }
 
-  bool on_thread_resume_was_invoked() const {
-    return on_thread_resume_was_invoked_;
-  }
-
  private:
   bool on_stack_copy_was_invoked_ = false;
-  bool on_thread_resume_was_invoked_ = false;
 };
 
 }  // namespace
@@ -179,7 +167,6 @@ TEST(StackCopierSignalTest, MAYBE_CopyStackDelegateInvoked) {
   ASSERT_TRUE(result);
 
   EXPECT_TRUE(stack_copier_delegate.on_stack_copy_was_invoked());
-  EXPECT_TRUE(stack_copier_delegate.on_thread_resume_was_invoked());
 }
 
 // Limit to 32-bit Android, which is the platform we care about for this

base/profiler/stack_copier_suspend.cc

@@ -64,8 +64,6 @@ bool StackCopierSuspend::CopyStack(StackBuffer* stack_buffer,
         StackBuffer::kPlatformStackAlignment, stack_buffer->buffer());
   }
 
-  delegate->OnThreadResume();
-
   *stack_top = reinterpret_cast<uintptr_t>(stack_copy_bottom) + (top - bottom);
 
   for (uintptr_t* reg :

base/profiler/stack_copier_suspend_unittest.cc

@@ -85,25 +85,13 @@ class TestSuspendableThreadDelegate : public SuspendableThreadDelegate {
 class TestStackCopierDelegate : public StackCopier::Delegate {
  public:
   void OnStackCopy() override {
-    // We can't EXPECT_FALSE(on_thread_resume_was_invoked_) here because that
-    // invocation is not reentrant.
     on_stack_copy_was_invoked_ = true;
   }
 
-  void OnThreadResume() override {
-    EXPECT_TRUE(on_stack_copy_was_invoked_);
-    on_thread_resume_was_invoked_ = true;
-  }
-
   bool on_stack_copy_was_invoked() const { return on_stack_copy_was_invoked_; }
 
-  bool on_thread_resume_was_invoked() const {
-    return on_thread_resume_was_invoked_;
-  }
-
  private:
   bool on_stack_copy_was_invoked_ = false;
-  bool on_thread_resume_was_invoked_ = false;
 };
 
 }  // namespace
@@ -218,7 +206,6 @@ TEST(StackCopierSuspendTest, CopyStackDelegateInvoked) {
                                  &register_context, &stack_copier_delegate);
 
   EXPECT_TRUE(stack_copier_delegate.on_stack_copy_was_invoked());
-  EXPECT_TRUE(stack_copier_delegate.on_thread_resume_was_invoked());
 }
 
 TEST(StackCopierSuspendTest, RewriteRegisters) {

base/profiler/stack_sampler_impl.cc

@@ -8,6 +8,7 @@
 
 #include "base/check.h"
 #include "base/compiler_specific.h"
+#include "base/logging.h"
 #include "base/profiler/metadata_recorder.h"
 #include "base/profiler/profile_builder.h"
 #include "base/profiler/sample_metadata.h"
@@ -32,16 +33,14 @@ namespace {
 // the thread is suspended.
 class StackCopierDelegate : public StackCopier::Delegate {
  public:
-  StackCopierDelegate(ModuleCache* module_cache,
-                      Unwinder* native_unwinder,
+  StackCopierDelegate(Unwinder* native_unwinder,
                       Unwinder* aux_unwinder,
-                      ProfileBuilder* profile_builder)
-      : module_cache_(module_cache),
-        native_unwinder_(native_unwinder),
+                      ProfileBuilder* profile_builder,
+                      MetadataRecorder::MetadataProvider* metadata_provider)
+      : native_unwinder_(native_unwinder),
         aux_unwinder_(aux_unwinder),
         profile_builder_(profile_builder),
-        metadata_provider_(std::make_unique<MetadataRecorder::MetadataProvider>(
-            GetSampleMetadataRecorder())) {}
+        metadata_provider_(metadata_provider) {}
 
   StackCopierDelegate(const StackCopierDelegate&) = delete;
   StackCopierDelegate& operator=(const StackCopierDelegate&) = delete;
@@ -56,31 +55,14 @@ class StackCopierDelegate : public StackCopier::Delegate {
     if (aux_unwinder_)
       aux_unwinder_->OnStackCapture();
 
-#if !defined(OS_POSIX) || defined(OS_MACOSX)
-    profile_builder_->RecordMetadata(metadata_provider_.get());
-#else
-    // TODO(https://crbug.com/1056283): Support metadata recording on POSIX
-    // platforms.
-    ALLOW_UNUSED_LOCAL(profile_builder_);
-#endif
-  }
-
-  void OnThreadResume() override {
-    // Reset this as soon as possible because it may hold a lock on the
-    // metadata.
-    metadata_provider_.reset();
-
-    native_unwinder_->UpdateModules(module_cache_);
-    if (aux_unwinder_)
-      aux_unwinder_->UpdateModules(module_cache_);
+    profile_builder_->RecordMetadata(*metadata_provider_);
   }
 
  private:
-  ModuleCache* const module_cache_;
   Unwinder* const native_unwinder_;
   Unwinder* const aux_unwinder_;
   ProfileBuilder* const profile_builder_;
-  std::unique_ptr<MetadataRecorder::MetadataProvider> metadata_provider_;
+  const MetadataRecorder::MetadataProvider* const metadata_provider_;
 };
 
 }  // namespace
@@ -108,12 +90,21 @@ void StackSamplerImpl::RecordStackFrames(StackBuffer* stack_buffer,
   RegisterContext thread_context;
   uintptr_t stack_top;
   TimeTicks timestamp;
-  StackCopierDelegate delegate(module_cache_, native_unwinder_.get(),
-                               aux_unwinder_.get(), profile_builder);
-  bool success = stack_copier_->CopyStack(stack_buffer, &stack_top, &timestamp,
-                                          &thread_context, &delegate);
-  if (!success)
-    return;
+  {
+    // Make this scope as small as possible because |metadata_provider| is
+    // holding a lock.
+    MetadataRecorder::MetadataProvider metadata_provider(
+        GetSampleMetadataRecorder());
+    StackCopierDelegate delegate(native_unwinder_.get(), aux_unwinder_.get(),
+                                 profile_builder, &metadata_provider);
+    bool success = stack_copier_->CopyStack(
+        stack_buffer, &stack_top, &timestamp, &thread_context, &delegate);
+    if (!success)
+      return;
+  }
+  native_unwinder_->UpdateModules(module_cache_);
+  if (aux_unwinder_)
+    aux_unwinder_->UpdateModules(module_cache_);
 
   if (test_delegate_)
     test_delegate_->OnPreStackWalk();

base/profiler/stack_sampler_impl_unittest.cc

@@ -37,7 +37,7 @@ class TestProfileBuilder : public ProfileBuilder {
   // ProfileBuilder
   ModuleCache* GetModuleCache() override { return module_cache_; }
   void RecordMetadata(
-      MetadataRecorder::MetadataProvider* metadata_provider) override {}
+      const MetadataRecorder::MetadataProvider& metadata_provider) override {}
 
   void OnSampleCompleted(std::vector<Frame> frames,
                          TimeTicks sample_timestamp) override {
@@ -97,7 +97,6 @@ class DelegateInvokingStackCopier : public StackCopier {
                  RegisterContext* thread_context,
                  Delegate* delegate) override {
     delegate->OnStackCopy();
-    delegate->OnThreadResume();
     return true;
   }
 };

base/profiler/stack_sampling_profiler_test_util.cc

@@ -36,7 +36,7 @@ class TestProfileBuilder : public ProfileBuilder {
   // ProfileBuilder:
   ModuleCache* GetModuleCache() override { return module_cache_; }
   void RecordMetadata(
-      MetadataRecorder::MetadataProvider* metadata_provider) override {}
+      const MetadataRecorder::MetadataProvider& metadata_provider) override {}
 
   void OnSampleCompleted(std::vector<Frame> sample,
                          TimeTicks sample_timestamp) override {

base/profiler/stack_sampling_profiler_unittest.cc

@@ -165,7 +165,7 @@ class TestProfileBuilder : public ProfileBuilder {
   // ProfileBuilder:
   ModuleCache* GetModuleCache() override;
   void RecordMetadata(
-      MetadataRecorder::MetadataProvider* metadata_provider) override;
+      const MetadataRecorder::MetadataProvider& metadata_provider) override;
   void ApplyMetadataRetrospectively(
       TimeTicks period_start,
       TimeTicks period_end,
@@ -204,7 +204,7 @@ ModuleCache* TestProfileBuilder::GetModuleCache() {
 }
 
 void TestProfileBuilder::RecordMetadata(
-    MetadataRecorder::MetadataProvider* metadata_provider) {
+    const MetadataRecorder::MetadataProvider& metadata_provider) {
   ++record_metadata_count_;
 }
 

components/metrics/call_stack_profile_builder.cc

@@ -72,7 +72,7 @@ base::ModuleCache* CallStackProfileBuilder::GetModuleCache() {
 // suspended so must not take any locks, including indirectly through use of
 // heap allocation, LOG, CHECK, or DCHECK.
 void CallStackProfileBuilder::RecordMetadata(
-    base::MetadataRecorder::MetadataProvider* metadata_provider) {
+    const base::MetadataRecorder::MetadataProvider& metadata_provider) {
   if (work_id_recorder_) {
     unsigned int work_id = work_id_recorder_->RecordWorkId();
     // A work id of 0 indicates that the message loop has not yet started.

components/metrics/call_stack_profile_builder.h

@@ -72,8 +72,8 @@ class CallStackProfileBuilder : public base::ProfileBuilder {
 
   // base::ProfileBuilder:
   base::ModuleCache* GetModuleCache() override;
-  void RecordMetadata(
-      base::MetadataRecorder::MetadataProvider* metadata_provider) override;
+  void RecordMetadata(const base::MetadataRecorder::MetadataProvider&
+                          metadata_provider) override;
   void ApplyMetadataRetrospectively(
       base::TimeTicks period_start,
       base::TimeTicks period_end,

components/metrics/call_stack_profile_builder_unittest.cc

@@ -102,6 +102,7 @@ TEST(CallStackProfileBuilderTest, ProfilingCompleted) {
 
   auto profile_builder = std::make_unique<TestingCallStackProfileBuilder>(
       kProfileParams, nullptr, mock_closure.Get());
+  base::MetadataRecorder metadata_recorder;
 
 #if defined(OS_WIN)
   uint64_t module_md5 = 0x46C3E4166659AC02ULL;
@@ -127,10 +128,10 @@ TEST(CallStackProfileBuilderTest, ProfilingCompleted) {
   std::vector<base::Frame> frames2 = {frame3};
 
   profile_builder->RecordMetadata(
-      base::MetadataRecorder().CreateMetadataProvider().get());
+      base::MetadataRecorder::MetadataProvider(&metadata_recorder));
   profile_builder->OnSampleCompleted(frames1, base::TimeTicks());
   profile_builder->RecordMetadata(
-      base::MetadataRecorder().CreateMetadataProvider().get());
+      base::MetadataRecorder::MetadataProvider(&metadata_recorder));
   profile_builder->OnSampleCompleted(frames2, base::TimeTicks());
   profile_builder->OnProfileCompleted(base::TimeDelta::FromMilliseconds(500),
                                       base::TimeDelta::FromMilliseconds(100));
@@ -216,6 +217,7 @@ TEST(CallStackProfileBuilderTest, CustomWeightsAndCounts) {
 TEST(CallStackProfileBuilderTest, StacksDeduped) {
   auto profile_builder =
       std::make_unique<TestingCallStackProfileBuilder>(kProfileParams);
+  base::MetadataRecorder metadata_recorder;
 
   TestModule module1;
   base::Frame frame1 = {0x10, &module1};
@@ -228,10 +230,10 @@ TEST(CallStackProfileBuilderTest, StacksDeduped) {
   // Two stacks are completed with the same frames therefore they are deduped
   // to one.
   profile_builder->RecordMetadata(
-      base::MetadataRecorder().CreateMetadataProvider().get());
+      base::MetadataRecorder::MetadataProvider(&metadata_recorder));
   profile_builder->OnSampleCompleted(frames, base::TimeTicks());
   profile_builder->RecordMetadata(
-      base::MetadataRecorder().CreateMetadataProvider().get());
+      base::MetadataRecorder::MetadataProvider(&metadata_recorder));
   profile_builder->OnSampleCompleted(frames, base::TimeTicks());
 
   profile_builder->OnProfileCompleted(base::TimeDelta(), base::TimeDelta());
@@ -256,6 +258,7 @@ TEST(CallStackProfileBuilderTest, StacksDeduped) {
 TEST(CallStackProfileBuilderTest, StacksNotDeduped) {
   auto profile_builder =
       std::make_unique<TestingCallStackProfileBuilder>(kProfileParams);
+  base::MetadataRecorder metadata_recorder;
 
   TestModule module1;
   base::Frame frame1 = {0x10, &module1};
@@ -268,10 +271,10 @@ TEST(CallStackProfileBuilderTest, StacksNotDeduped) {
 
   // Two stacks are completed with the different frames therefore not deduped.
   profile_builder->RecordMetadata(
-      base::MetadataRecorder().CreateMetadataProvider().get());
+      base::MetadataRecorder::MetadataProvider(&metadata_recorder));
   profile_builder->OnSampleCompleted(frames1, base::TimeTicks());
   profile_builder->RecordMetadata(
-      base::MetadataRecorder().CreateMetadataProvider().get());
+      base::MetadataRecorder::MetadataProvider(&metadata_recorder));
   profile_builder->OnSampleCompleted(frames2, base::TimeTicks());
 
   profile_builder->OnProfileCompleted(base::TimeDelta(), base::TimeDelta());
@@ -296,6 +299,7 @@ TEST(CallStackProfileBuilderTest, StacksNotDeduped) {
 TEST(CallStackProfileBuilderTest, Modules) {
   auto profile_builder =
       std::make_unique<TestingCallStackProfileBuilder>(kProfileParams);
+  base::MetadataRecorder metadata_recorder;
 
   // A frame with no module.
   base::Frame frame1 = {0x1010, nullptr};
@@ -314,7 +318,7 @@ TEST(CallStackProfileBuilderTest, Modules) {
   std::vector<base::Frame> frames = {frame1, frame2};
 
   profile_builder->RecordMetadata(
-      base::MetadataRecorder().CreateMetadataProvider().get());
+      base::MetadataRecorder::MetadataProvider(&metadata_recorder));
   profile_builder->OnSampleCompleted(frames, base::TimeTicks());
   profile_builder->OnProfileCompleted(base::TimeDelta(), base::TimeDelta());
 
@@ -347,6 +351,7 @@ TEST(CallStackProfileBuilderTest, Modules) {
 TEST(CallStackProfileBuilderTest, DedupModules) {
   auto profile_builder =
       std::make_unique<TestingCallStackProfileBuilder>(kProfileParams);
+  base::MetadataRecorder metadata_recorder;
 
   const uintptr_t module_base_address = 0x1000;
 
@@ -365,7 +370,7 @@ TEST(CallStackProfileBuilderTest, DedupModules) {
   std::vector<base::Frame> frames = {frame1, frame2};
 
   profile_builder->RecordMetadata(
-      base::MetadataRecorder().CreateMetadataProvider().get());
+      base::MetadataRecorder::MetadataProvider(&metadata_recorder));
   profile_builder->OnSampleCompleted(frames, base::TimeTicks());
   profile_builder->OnProfileCompleted(base::TimeDelta(), base::TimeDelta());
 
@@ -410,6 +415,7 @@ TEST(CallStackProfileBuilderTest, WorkIds) {
   TestWorkIdRecorder work_id_recorder;
   auto profile_builder = std::make_unique<TestingCallStackProfileBuilder>(
       kProfileParams, &work_id_recorder);
+  base::MetadataRecorder metadata_recorder;
 
   TestModule module;
   base::Frame frame = {0x10, &module};
@@ -417,25 +423,25 @@ TEST(CallStackProfileBuilderTest, WorkIds) {
   // Id 0 means the message loop hasn't been started yet, so the sample should
   // not have continued_work set.
   profile_builder->RecordMetadata(
-      base::MetadataRecorder().CreateMetadataProvider().get());
+      base::MetadataRecorder::MetadataProvider(&metadata_recorder));
   profile_builder->OnSampleCompleted({frame}, base::TimeTicks());
 
   // The second sample with the same id should have continued_work set.
   work_id_recorder.current_id = 1;
   profile_builder->RecordMetadata(
-      base::MetadataRecorder().CreateMetadataProvider().get());
+      base::MetadataRecorder::MetadataProvider(&metadata_recorder));
   profile_builder->OnSampleCompleted({frame}, base::TimeTicks());
   profile_builder->RecordMetadata(
-      base::MetadataRecorder().CreateMetadataProvider().get());
+      base::MetadataRecorder::MetadataProvider(&metadata_recorder));
   profile_builder->OnSampleCompleted({frame}, base::TimeTicks());
 
   // Ids are in general non-contiguous across multiple samples.
   work_id_recorder.current_id = 10;
   profile_builder->RecordMetadata(
-      base::MetadataRecorder().CreateMetadataProvider().get());
+      base::MetadataRecorder::MetadataProvider(&metadata_recorder));
   profile_builder->OnSampleCompleted({frame}, base::TimeTicks());
   profile_builder->RecordMetadata(
-      base::MetadataRecorder().CreateMetadataProvider().get());
+      base::MetadataRecorder::MetadataProvider(&metadata_recorder));
   profile_builder->OnSampleCompleted({frame}, base::TimeTicks());
 
   profile_builder->OnProfileCompleted(base::TimeDelta::FromMilliseconds(500),
@@ -484,7 +490,7 @@ TEST(CallStackProfileBuilderTest, RecordMetadata) {
 
   metadata_recorder.Set(100, base::nullopt, 10);
   profile_builder->RecordMetadata(
-      metadata_recorder.CreateMetadataProvider().get());
+      base::MetadataRecorder::MetadataProvider(&metadata_recorder));
   profile_builder->OnSampleCompleted({frame}, base::TimeTicks());
 
   profile_builder->OnProfileCompleted(base::TimeDelta::FromMilliseconds(500),
@@ -521,21 +527,21 @@ TEST(CallStackProfileBuilderTest, ApplyMetadataRetrospectively_Basic) {
   base::TimeDelta sample_time_delta = base::TimeDelta::FromSeconds(1);
 
   profile_builder->RecordMetadata(
-      metadata_recorder.CreateMetadataProvider().get());
+      base::MetadataRecorder::MetadataProvider(&metadata_recorder));
   profile_builder->OnSampleCompleted({frame}, profile_start_time);
 
   profile_builder->RecordMetadata(
-      metadata_recorder.CreateMetadataProvider().get());
+      base::MetadataRecorder::MetadataProvider(&metadata_recorder));
   profile_builder->OnSampleCompleted({frame},
                                      profile_start_time + sample_time_delta);
 
   profile_builder->RecordMetadata(
-      metadata_recorder.CreateMetadataProvider().get());
+      base::MetadataRecorder::MetadataProvider(&metadata_recorder));
   profile_builder->OnSampleCompleted(
       {frame}, profile_start_time + 2 * sample_time_delta);
 
   profile_builder->RecordMetadata(
-      metadata_recorder.CreateMetadataProvider().get());
+      base::MetadataRecorder::MetadataProvider(&metadata_recorder));
   profile_builder->OnSampleCompleted(
       {frame}, profile_start_time + 3 * sample_time_delta);
 
@@ -586,21 +592,21 @@ TEST(CallStackProfileBuilderTest,
   base::TimeDelta sample_time_delta = base::TimeDelta::FromSeconds(1);
 
   profile_builder->RecordMetadata(
-      metadata_recorder.CreateMetadataProvider().get());
+      base::MetadataRecorder::MetadataProvider(&metadata_recorder));
   profile_builder->OnSampleCompleted({frame}, profile_start_time);
 
   profile_builder->RecordMetadata(
-      metadata_recorder.CreateMetadataProvider().get());
+      base::MetadataRecorder::MetadataProvider(&metadata_recorder));
   profile_builder->OnSampleCompleted({frame},
                                      profile_start_time + sample_time_delta);
 
   profile_builder->RecordMetadata(
-      metadata_recorder.CreateMetadataProvider().get());
+      base::MetadataRecorder::MetadataProvider(&metadata_recorder));
   profile_builder->OnSampleCompleted(
       {frame}, profile_start_time + 2 * sample_time_delta);
 
   profile_builder->RecordMetadata(
-      metadata_recorder.CreateMetadataProvider().get());
+      base::MetadataRecorder::MetadataProvider(&metadata_recorder));
   profile_builder->OnSampleCompleted(
       {frame}, profile_start_time + 3 * sample_time_delta);
 

components/metrics/call_stack_profile_metadata.cc

@@ -107,8 +107,8 @@ CallStackProfileMetadata::~CallStackProfileMetadata() = default;
 // suspended so must not take any locks, including indirectly through use of
 // heap allocation, LOG, CHECK, or DCHECK.
 void CallStackProfileMetadata::RecordMetadata(
-    base::MetadataRecorder::MetadataProvider* metadata_provider) {
-  metadata_item_count_ = metadata_provider->GetItems(&metadata_items_);
+    const base::MetadataRecorder::MetadataProvider& metadata_provider) {
+  metadata_item_count_ = metadata_provider.GetItems(&metadata_items_);
 }
 
 google::protobuf::RepeatedPtrField<CallStackProfile::MetadataItem>

components/metrics/call_stack_profile_metadata.h

@@ -28,7 +28,7 @@ class CallStackProfileMetadata {
 
   // Records the metadata for the next sample.
   void RecordMetadata(
-      base::MetadataRecorder::MetadataProvider* metadata_provider);
+      const base::MetadataRecorder::MetadataProvider& metadata_provider);
 
   // Creates MetadataItems for the currently active metadata, adding new name
   // hashes to |metadata_name_hashes| if necessary. The same