[Sampling profiler] Fix incorrect reuse of frames across samples

Frames from deduplicated samples were being left in the current sample
state on the class, resulting in incorrect extra frames being added when
the next sample was recorded. This change eliminates the current sample
state to avoid this problem, and adds a test for this case.

Bug: 905379
Change-Id: I5fe0ccd3e50675669d4c7124fb7f64ebb85fe807
Reviewed-on: https://chromium-review.googlesource.com/c/1336247Reviewed-by: Xi Cheng <chengx@chromium.org>
Commit-Queue: Mike Wittman <wittman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#608160}

components/metrics/legacy_call_stack_profile_builder.cc

@@ -162,7 +162,8 @@ LegacyCallStackProfileBuilder::Sample::Sample(const std::vector<Frame>& frames)
 LegacyCallStackProfileBuilder::LegacyCallStackProfileBuilder(
     const CallStackProfileParams& profile_params,
     base::OnceClosure completed_callback)
-    : profile_params_(profile_params),
+    : process_milestones_(0),
+      profile_params_(profile_params),
       profile_start_time_(base::TimeTicks::Now()) {
   completed_callback_ = std::move(completed_callback);
 }
@@ -173,8 +174,7 @@ void LegacyCallStackProfileBuilder::RecordAnnotations() {
   // The code inside this method must not do anything that could acquire a
   // mutex, including allocating memory (which includes LOG messages) because
   // that mutex could be held by a stopped thread, thus resulting in deadlock.
-  sample_.process_milestones =
-      base::subtle::NoBarrier_Load(&g_process_milestones);
+  process_milestones_ = base::subtle::NoBarrier_Load(&g_process_milestones);
 }
 
 void LegacyCallStackProfileBuilder::OnSampleCompleted(
@@ -185,12 +185,15 @@ void LegacyCallStackProfileBuilder::OnSampleCompleted(
 void LegacyCallStackProfileBuilder::OnSampleCompleted(
     std::vector<base::StackSamplingProfiler::Frame> frames,
     size_t count) {
+  Sample sample;
+  sample.process_milestones = process_milestones_;
+
   // Assemble sample_ from |frames| first.
   for (const auto& frame : frames) {
     const base::ModuleCache::Module& module(frame.module);
     if (!module.is_valid) {
-      sample_.frames.emplace_back(frame.instruction_pointer,
-                                  kUnknownModuleIndex);
+      sample.frames.emplace_back(frame.instruction_pointer,
+                                 kUnknownModuleIndex);
       continue;
     }
 
@@ -202,12 +205,12 @@ void LegacyCallStackProfileBuilder::OnSampleCompleted(
       loc = module_index_.insert(std::make_pair(module.base_address, index))
                 .first;
     }
-    sample_.frames.emplace_back(frame.instruction_pointer, loc->second);
+    sample.frames.emplace_back(frame.instruction_pointer, loc->second);
   }
 
-  // Write CallStackProfile::Sample protocol buffer message based on sample_.
+  // Write CallStackProfile::Sample protocol buffer message based on sample.
   int existing_sample_index = -1;
-  auto location = sample_index_.find(sample_);
+  auto location = sample_index_.find(sample);
   if (location != sample_index_.end())
     existing_sample_index = location->second;
 
@@ -220,17 +223,15 @@ void LegacyCallStackProfileBuilder::OnSampleCompleted(
 
   CallStackProfile::Sample* sample_proto =
       proto_profile_.add_deprecated_sample();
-  CopySampleToProto(sample_, modules_, sample_proto);
+  CopySampleToProto(sample, modules_, sample_proto);
   sample_proto->set_count(count);
-  CopyAnnotationsToProto(sample_.process_milestones & ~milestones_,
+  CopyAnnotationsToProto(sample.process_milestones & ~milestones_,
                          sample_proto);
-  milestones_ = sample_.process_milestones;
+  milestones_ = sample.process_milestones;
 
   sample_index_.insert(std::make_pair(
-      std::move(sample_),
+      std::move(sample),
       static_cast<int>(proto_profile_.deprecated_sample_size()) - 1));
-
-  sample_ = Sample();
 }
 
 // Build a SampledProfile in the protocol buffer message format from the

components/metrics/legacy_call_stack_profile_builder.h

@@ -132,8 +132,8 @@ class LegacyCallStackProfileBuilder
   // The collected stack samples in proto buffer message format.
   CallStackProfile proto_profile_;
 
-  // The current sample being recorded.
-  Sample sample_;
+  // The process milestones to use for the next sample.
+  uint32_t process_milestones_;
 
   // The indexes of samples, indexed by the sample.
   std::map<Sample, int> sample_index_;

components/metrics/legacy_call_stack_profile_builder_unittest.cc

@@ -218,6 +218,7 @@ TEST(LegacyCallStackProfileBuilderTest, SamplesDeduped) {
   ASSERT_TRUE(proto.has_call_stack_profile());
   ASSERT_EQ(1, proto.call_stack_profile().deprecated_sample_size());
   ASSERT_EQ(43, proto.call_stack_profile().deprecated_sample(0).count());
+  EXPECT_EQ(2, proto.call_stack_profile().deprecated_sample(0).frame_size());
 }
 
 TEST(LegacyCallStackProfileBuilderTest, SamplesNotDeduped) {
@@ -263,6 +264,65 @@ TEST(LegacyCallStackProfileBuilderTest, SamplesNotDeduped) {
 
   ASSERT_TRUE(proto.has_call_stack_profile());
   ASSERT_EQ(2, proto.call_stack_profile().deprecated_sample_size());
+  EXPECT_EQ(2, proto.call_stack_profile().deprecated_sample(0).frame_size());
+  EXPECT_EQ(2, proto.call_stack_profile().deprecated_sample(1).frame_size());
+}
+
+TEST(LegacyCallStackProfileBuilderTest, SamplesDedupedAndNotDeduped) {
+  auto profile_builder =
+      std::make_unique<TestingLegacyCallStackProfileBuilder>(kProfileParams);
+
+#if defined(OS_WIN)
+  base::FilePath module_path(L"c:\\some\\path\\to\\chrome.exe");
+#else
+  base::FilePath module_path("/some/path/to/chrome");
+#endif
+
+  const uintptr_t module_base_address1 = 0x1000;
+  Module module1 = {module_base_address1, "1", module_path};
+  Frame frame1 = {module_base_address1 + 0x10, module1};
+
+  const uintptr_t module_base_address2 = 0x1100;
+  Module module2 = {module_base_address2, "2", module_path};
+  Frame frame2 = {module_base_address2 + 0x20, module2};
+
+  std::vector<Frame> frames1 = {frame1, frame2};
+  std::vector<Frame> frames2 = {frame2, frame1};
+
+  profile_builder->RecordAnnotations();
+  profile_builder->OnSampleCompleted(frames1, 42);
+
+  profile_builder->RecordAnnotations();
+  profile_builder->OnSampleCompleted(frames1);
+
+  profile_builder->RecordAnnotations();
+  profile_builder->OnSampleCompleted(frames2);
+
+  profile_builder->OnProfileCompleted(base::TimeDelta(), base::TimeDelta());
+
+  const SampledProfile& proto = profile_builder->sampled_profile();
+
+  EXPECT_TRUE(proto.has_process());
+  EXPECT_EQ(BROWSER_PROCESS, proto.process());
+  EXPECT_TRUE(proto.has_thread());
+  EXPECT_EQ(MAIN_THREAD, proto.thread());
+  EXPECT_TRUE(proto.has_trigger_event());
+  EXPECT_EQ(SampledProfile::PROCESS_STARTUP, proto.trigger_event());
+
+  EXPECT_TRUE(proto.has_call_stack_profile());
+  ASSERT_EQ(2, proto.call_stack_profile().deprecated_sample_size());
+  EXPECT_EQ(43, proto.call_stack_profile().deprecated_sample(0).count());
+  ASSERT_EQ(2, proto.call_stack_profile().deprecated_sample(0).frame_size());
+  EXPECT_EQ(0x10u,
+            proto.call_stack_profile().deprecated_sample(0).frame(0).address());
+  EXPECT_EQ(0x20u,
+            proto.call_stack_profile().deprecated_sample(0).frame(1).address());
+  ASSERT_EQ(2, proto.call_stack_profile().deprecated_sample(1).frame_size());
+  EXPECT_EQ(1, proto.call_stack_profile().deprecated_sample(1).count());
+  EXPECT_EQ(0x20u,
+            proto.call_stack_profile().deprecated_sample(1).frame(0).address());
+  EXPECT_EQ(0x10u,
+            proto.call_stack_profile().deprecated_sample(1).frame(1).address());
 }
 
 TEST(LegacyCallStackProfileBuilderTest, Modules) {