Add shared-memory support to Activity Tracker.

This is to aid access to non-persistent breadcrumbs created by
subprocesses.

Turns out that the analyzer required read/write access to take
snapshots because of the "unchanged" indicator.  That's undesirable
(and sometimes simply infeasible) so I've changed "unchanged" in
favor of a "data version" that is incremented.  Comparing before/after
versions will indicate if anything has changed without requiring write
ability by the analyzer.

If the data-version increments 4B times during an analyzer copy, we
have an ABA problem but I think that can be safely discounted.

Bug: 620813
Change-Id: Ic2455dcd3a70f8ec92c4878d7680ce890f16c071
Reviewed-on: https://chromium-review.googlesource.com/773159
Commit-Queue: Brian White <bcwhite@chromium.org>
Reviewed-by: Sigurður Ásgeirsson <siggi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#518963}

base/debug/activity_analyzer.cc

@@ -24,7 +24,6 @@ namespace {
 // An empty snapshot that can be returned when there otherwise is none.
 LazyInstance<ActivityUserData::Snapshot>::Leaky g_empty_user_data_snapshot;
 
-#if !defined(OS_NACL)
 // DO NOT CHANGE VALUES. This is logged persistently in a histogram.
 enum AnalyzerCreationError {
   kInvalidMemoryMappedFile,
@@ -39,7 +38,6 @@ void LogAnalyzerCreationError(AnalyzerCreationError error) {
   UMA_HISTOGRAM_ENUMERATION("ActivityTracker.Collect.AnalyzerCreationError",
                             error, kAnalyzerCreationErrorMax);
 }
-#endif  // !defined(OS_NACL)
 
 }  // namespace
 
@@ -91,6 +89,28 @@ GlobalActivityAnalyzer::GlobalActivityAnalyzer(
 
 GlobalActivityAnalyzer::~GlobalActivityAnalyzer() {}
 
+// static
+std::unique_ptr<GlobalActivityAnalyzer>
+GlobalActivityAnalyzer::CreateWithAllocator(
+    std::unique_ptr<PersistentMemoryAllocator> allocator) {
+  if (allocator->GetMemoryState() ==
+      PersistentMemoryAllocator::MEMORY_UNINITIALIZED) {
+    LogAnalyzerCreationError(kPmaUninitialized);
+    return nullptr;
+  }
+  if (allocator->GetMemoryState() ==
+      PersistentMemoryAllocator::MEMORY_DELETED) {
+    LogAnalyzerCreationError(kPmaDeleted);
+    return nullptr;
+  }
+  if (allocator->IsCorrupt()) {
+    LogAnalyzerCreationError(kPmaCorrupt);
+    return nullptr;
+  }
+
+  return WrapUnique(new GlobalActivityAnalyzer(std::move(allocator)));
+}
+
 #if !defined(OS_NACL)
 // static
 std::unique_ptr<GlobalActivityAnalyzer> GlobalActivityAnalyzer::CreateWithFile(
@@ -109,27 +129,34 @@ std::unique_ptr<GlobalActivityAnalyzer> GlobalActivityAnalyzer::CreateWithFile(
     return nullptr;
   }
 
-  std::unique_ptr<FilePersistentMemoryAllocator> allocator(
-      new FilePersistentMemoryAllocator(std::move(mmfile), 0, 0,
-                                        base::StringPiece(), true));
-  if (allocator->GetMemoryState() ==
-      PersistentMemoryAllocator::MEMORY_UNINITIALIZED) {
-    LogAnalyzerCreationError(kPmaUninitialized);
-    return nullptr;
-  }
-  if (allocator->GetMemoryState() ==
-      PersistentMemoryAllocator::MEMORY_DELETED) {
-    LogAnalyzerCreationError(kPmaDeleted);
-    return nullptr;
-  }
-  if (allocator->IsCorrupt()) {
-    LogAnalyzerCreationError(kPmaCorrupt);
+  return CreateWithAllocator(std::make_unique<FilePersistentMemoryAllocator>(
+      std::move(mmfile), 0, 0, StringPiece(), /*readonly=*/true));
+}
+#endif  // !defined(OS_NACL)
+
+// static
+std::unique_ptr<GlobalActivityAnalyzer>
+GlobalActivityAnalyzer::CreateWithSharedMemory(
+    std::unique_ptr<SharedMemory> shm) {
+  if (shm->mapped_size() == 0 ||
+      !SharedPersistentMemoryAllocator::IsSharedMemoryAcceptable(*shm)) {
     return nullptr;
   }
+  return CreateWithAllocator(std::make_unique<SharedPersistentMemoryAllocator>(
+      std::move(shm), 0, StringPiece(), /*readonly=*/true));
+}
 
-  return WrapUnique(new GlobalActivityAnalyzer(std::move(allocator)));
+// static
+std::unique_ptr<GlobalActivityAnalyzer>
+GlobalActivityAnalyzer::CreateWithSharedMemoryHandle(
+    const SharedMemoryHandle& handle,
+    size_t size) {
+  std::unique_ptr<SharedMemory> shm(
+      new SharedMemory(handle, /*readonly=*/true));
+  if (!shm->Map(size))
+    return nullptr;
+  return CreateWithSharedMemory(std::move(shm));
 }
-#endif  // !defined(OS_NACL)
 
 int64_t GlobalActivityAnalyzer::GetFirstProcess() {
   PrepareAllAnalyzers();

base/debug/activity_analyzer.h

@@ -137,13 +137,27 @@ class BASE_EXPORT GlobalActivityAnalyzer {
 
   ~GlobalActivityAnalyzer();
 
+  // Creates a global analyzer using a given persistent-memory |allocator|.
+  static std::unique_ptr<GlobalActivityAnalyzer> CreateWithAllocator(
+      std::unique_ptr<PersistentMemoryAllocator> allocator);
+
 #if !defined(OS_NACL)
   // Creates a global analyzer using the contents of a file given in
   // |file_path|.
   static std::unique_ptr<GlobalActivityAnalyzer> CreateWithFile(
       const FilePath& file_path);
 #endif  // !defined(OS_NACL)
 
+  // Like above but accesses an allocator in a mapped shared-memory segment.
+  static std::unique_ptr<GlobalActivityAnalyzer> CreateWithSharedMemory(
+      std::unique_ptr<SharedMemory> shm);
+
+  // Like above but takes a handle to an existing shared memory segment and
+  // maps it before creating the tracker.
+  static std::unique_ptr<GlobalActivityAnalyzer> CreateWithSharedMemoryHandle(
+      const SharedMemoryHandle& handle,
+      size_t size);
+
   // Iterates over all known valid processes and returns their PIDs or zero
   // if there are no more. Calls to GetFirstProcess() will perform a global
   // snapshot in order to provide a relatively consistent state across the

base/debug/activity_analyzer_unittest.cc

@@ -216,6 +216,35 @@ TEST_F(ActivityAnalyzerTest, GlobalAnalyzerConstruction) {
   EXPECT_EQ("bar", data_snapshot.at("foo").GetString());
 }
 
+TEST_F(ActivityAnalyzerTest, GlobalAnalyzerFromSharedMemory) {
+  SharedMemoryHandle handle1;
+  SharedMemoryHandle handle2;
+
+  {
+    std::unique_ptr<SharedMemory> shmem(new SharedMemory());
+    ASSERT_TRUE(shmem->CreateAndMapAnonymous(kMemorySize));
+    handle1 = shmem->handle().Duplicate();
+    ASSERT_TRUE(handle1.IsValid());
+    handle2 = shmem->handle().Duplicate();
+    ASSERT_TRUE(handle2.IsValid());
+  }
+
+  GlobalActivityTracker::CreateWithSharedMemoryHandle(handle1, kMemorySize, 0,
+                                                      "", 3);
+  GlobalActivityTracker::Get()->process_data().SetString("foo", "bar");
+
+  std::unique_ptr<GlobalActivityAnalyzer> analyzer =
+      GlobalActivityAnalyzer::CreateWithSharedMemoryHandle(handle2,
+                                                           kMemorySize);
+
+  const int64_t pid = analyzer->GetFirstProcess();
+  ASSERT_NE(0, pid);
+  const ActivityUserData::Snapshot& data_snapshot =
+      analyzer->GetProcessDataSnapshot(pid);
+  ASSERT_LE(1U, data_snapshot.size());
+  EXPECT_EQ("bar", data_snapshot.at("foo").GetString());
+}
+
 TEST_F(ActivityAnalyzerTest, UserDataSnapshotTest) {
   GlobalActivityTracker::CreateWithLocalMemory(kMemorySize, 0, "", 3, 0);
   ThreadActivityAnalyzer::Snapshot tracker_snapshot;

base/debug/activity_tracker.cc

@@ -638,13 +638,11 @@ struct ThreadActivityTracker::Header {
 
   // A memory location used to indicate if changes have been made to the data
   // that would invalidate an in-progress read of its contents. The active
-  // tracker will zero the value whenever something gets popped from the
-  // stack. A monitoring tracker can write a non-zero value here, copy the
-  // stack contents, and read the value to know, if it is still non-zero, that
-  // the contents didn't change while being copied. This can handle concurrent
-  // snapshot operations only if each snapshot writes a different bit (which
-  // is not the current implementation so no parallel snapshots allowed).
-  std::atomic<uint32_t> data_unchanged;
+  // tracker will increment the value whenever something gets popped from the
+  // stack. A monitoring tracker can check the value before and after access
+  // to know, if it's still the same, that the contents didn't change while
+  // being copied.
+  std::atomic<uint32_t> data_version;
 
   // The last "exception" activity. This can't be stored on the stack because
   // that could get popped as things unwind.
@@ -728,7 +726,7 @@ ThreadActivityTracker::ThreadActivityTracker(void* base, size_t size)
     DCHECK_EQ(0, header_->start_ticks);
     DCHECK_EQ(0U, header_->stack_slots);
     DCHECK_EQ(0U, header_->current_depth.load(std::memory_order_relaxed));
-    DCHECK_EQ(0U, header_->data_unchanged.load(std::memory_order_relaxed));
+    DCHECK_EQ(0U, header_->data_version.load(std::memory_order_relaxed));
     DCHECK_EQ(0, stack_[0].time_internal);
     DCHECK_EQ(0U, stack_[0].origin_address);
     DCHECK_EQ(0U, stack_[0].call_stack[0]);
@@ -840,12 +838,11 @@ void ThreadActivityTracker::PopActivity(ActivityId id) {
          CalledOnValidThread());
 
   // The stack has shrunk meaning that some other thread trying to copy the
-  // contents for reporting purposes could get bad data. That thread would
-  // have written a non-zero value into |data_unchanged|; clearing it here
-  // will let that thread detect that something did change. This needs to
+  // contents for reporting purposes could get bad data. Increment the data
+  // version so that it con tell that things have changed. This needs to
   // happen after the atomic |depth| operation above so a "release" store
   // is required.
-  header_->data_unchanged.store(0, std::memory_order_release);
+  header_->data_version.fetch_add(1, std::memory_order_release);
 }
 
 std::unique_ptr<ActivityUserData> ThreadActivityTracker::GetUserData(
@@ -894,7 +891,7 @@ void ThreadActivityTracker::RecordExceptionActivity(const void* program_counter,
 
   // The data has changed meaning that some other thread trying to copy the
   // contents for reporting purposes could get bad data.
-  header_->data_unchanged.store(0, std::memory_order_relaxed);
+  header_->data_version.fetch_add(1, std::memory_order_relaxed);
 }
 
 bool ThreadActivityTracker::IsValid() const {
@@ -940,12 +937,13 @@ bool ThreadActivityTracker::CreateSnapshot(Snapshot* output_snapshot) const {
     const int64_t starting_process_id = header_->owner.process_id;
     const int64_t starting_thread_id = header_->thread_ref.as_id;
 
-    // Write a non-zero value to |data_unchanged| so it's possible to detect
-    // at the end that nothing has changed since copying the data began. A
-    // "cst" operation is required to ensure it occurs before everything else.
-    // Using "cst" memory ordering is relatively expensive but this is only
-    // done during analysis so doesn't directly affect the worker threads.
-    header_->data_unchanged.store(1, std::memory_order_seq_cst);
+    // Note the current |data_version| so it's possible to detect at the end
+    // that nothing has changed since copying the data began. A "cst" operation
+    // is required to ensure it occurs before everything else. Using "cst"
+    // memory ordering is relatively expensive but this is only done during
+    // analysis so doesn't directly affect the worker threads.
+    const uint32_t pre_version =
+        header_->data_version.load(std::memory_order_seq_cst);
 
     // Fetching the current depth also "acquires" the contents of the stack.
     depth = header_->current_depth.load(std::memory_order_acquire);
@@ -965,7 +963,7 @@ bool ThreadActivityTracker::CreateSnapshot(Snapshot* output_snapshot) const {
 
     // Retry if something changed during the copy. A "cst" operation ensures
     // it must happen after all the above operations.
-    if (!header_->data_unchanged.load(std::memory_order_seq_cst))
+    if (header_->data_version.load(std::memory_order_seq_cst) != pre_version)
       continue;
 
     // Stack copied. Record it's full depth.
@@ -1025,12 +1023,8 @@ const void* ThreadActivityTracker::GetBaseAddress() {
   return header_;
 }
 
-void ThreadActivityTracker::ClearDataChangedForTesting() {
-  header_->data_unchanged.store(2, std::memory_order_relaxed);
-}
-
-bool ThreadActivityTracker::WasDataChangedForTesting() {
-  return !header_->data_unchanged.load(std::memory_order_relaxed);
+uint32_t ThreadActivityTracker::GetDataVersionForTesting() {
+  return header_->data_version.load(std::memory_order_relaxed);
 }
 
 void ThreadActivityTracker::SetOwningProcessIdForTesting(int64_t pid,
@@ -1307,6 +1301,36 @@ bool GlobalActivityTracker::CreateWithLocalMemory(size_t size,
   return true;
 }
 
+// static
+bool GlobalActivityTracker::CreateWithSharedMemory(
+    std::unique_ptr<SharedMemory> shm,
+    uint64_t id,
+    StringPiece name,
+    int stack_depth) {
+  if (shm->mapped_size() == 0 ||
+      !SharedPersistentMemoryAllocator::IsSharedMemoryAcceptable(*shm)) {
+    return false;
+  }
+  CreateWithAllocator(std::make_unique<SharedPersistentMemoryAllocator>(
+                          std::move(shm), id, name, false),
+                      stack_depth, 0);
+  return true;
+}
+
+// static
+bool GlobalActivityTracker::CreateWithSharedMemoryHandle(
+    const SharedMemoryHandle& handle,
+    size_t size,
+    uint64_t id,
+    StringPiece name,
+    int stack_depth) {
+  std::unique_ptr<SharedMemory> shm(
+      new SharedMemory(handle, /*readonly=*/false));
+  if (!shm->Map(size))
+    return false;
+  return CreateWithSharedMemory(std::move(shm), id, name, stack_depth);
+}
+
 // static
 void GlobalActivityTracker::SetForTesting(
     std::unique_ptr<GlobalActivityTracker> tracker) {

base/debug/activity_tracker.h

@@ -27,6 +27,7 @@
 #include "base/compiler_specific.h"
 #include "base/gtest_prod_util.h"
 #include "base/location.h"
+#include "base/memory/shared_memory.h"
 #include "base/metrics/persistent_memory_allocator.h"
 #include "base/process/process_handle.h"
 #include "base/strings/string_piece.h"
@@ -713,10 +714,9 @@ class BASE_EXPORT ThreadActivityTracker {
   // Gets the base memory address used for storing data.
   const void* GetBaseAddress();
 
-  // Access the "data unchanged" flag so tests can determine if an activity
+  // Access the "data version" value so tests can determine if an activity
   // was pushed and popped in a single call.
-  void ClearDataChangedForTesting();
-  bool WasDataChangedForTesting();
+  uint32_t GetDataVersionForTesting();
 
   // Explicitly sets the process ID.
   void SetOwningProcessIdForTesting(int64_t pid, int64_t stamp);
@@ -905,6 +905,21 @@ class BASE_EXPORT GlobalActivityTracker {
                                     int stack_depth,
                                     int64_t process_id);
 
+  // Like above but internally creates an allocator using a shared-memory
+  // segment. The segment must already be mapped into the local memory space.
+  static bool CreateWithSharedMemory(std::unique_ptr<SharedMemory> shm,
+                                     uint64_t id,
+                                     StringPiece name,
+                                     int stack_depth);
+
+  // Like above but takes a handle to an existing shared memory segment and
+  // maps it before creating the tracker.
+  static bool CreateWithSharedMemoryHandle(const SharedMemoryHandle& handle,
+                                           size_t size,
+                                           uint64_t id,
+                                           StringPiece name,
+                                           int stack_depth);
+
   // Gets the global activity-tracker or null if none exists.
   static GlobalActivityTracker* Get() {
     return reinterpret_cast<GlobalActivityTracker*>(

base/debug/activity_tracker_unittest.cc

@@ -265,11 +265,11 @@ class SimpleLockThread : public SimpleThread {
   void Run() override {
     ThreadActivityTracker* tracker =
         GlobalActivityTracker::Get()->GetOrCreateTrackerForCurrentThread();
-    tracker->ClearDataChangedForTesting();
+    uint32_t pre_version = tracker->GetDataVersionForTesting();
 
     is_running_.store(true, std::memory_order_relaxed);
     lock_->Acquire();
-    data_changed_ = tracker->WasDataChangedForTesting();
+    data_changed_ = tracker->GetDataVersionForTesting() != pre_version;
     lock_->Release();
     is_running_.store(false, std::memory_order_relaxed);
   }
@@ -297,14 +297,13 @@ TEST_F(ActivityTrackerTest, LockTest) {
   ASSERT_EQ(0U, GetGlobalUserDataMemoryCacheUsed());
 
   Lock lock;
-  tracker->ClearDataChangedForTesting();
-  ASSERT_FALSE(tracker->WasDataChangedForTesting());
+  uint32_t pre_version = tracker->GetDataVersionForTesting();
 
   // Check no activity when only "trying" a lock.
   EXPECT_TRUE(lock.Try());
-  EXPECT_FALSE(tracker->WasDataChangedForTesting());
+  EXPECT_EQ(pre_version, tracker->GetDataVersionForTesting());
   lock.Release();
-  EXPECT_FALSE(tracker->WasDataChangedForTesting());
+  EXPECT_EQ(pre_version, tracker->GetDataVersionForTesting());
 
   // Check no activity when acquiring a free lock.
   SimpleLockThread t1("locker1", &lock);