[PartitionAlloc] Add more threading annotations to PartitionRoot.

Bug: 1013082, 998048
Change-Id: I9b44c0da86f9fd93046a57fdd7f397122b90e3f6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2424218
Commit-Queue: Benoit L <lizeb@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Cr-Commit-Position: refs/heads/master@{#810009}

base/allocator/partition_allocator/memory_reclaimer_unittest.cc

@@ -73,31 +73,33 @@ TEST_F(PartitionAllocMemoryReclaimerTest, Simple) {
 TEST_F(PartitionAllocMemoryReclaimerTest, FreesMemory) {
   PartitionRoot<internal::ThreadSafe>* root = allocator_->root();
 
-  size_t committed_initially = root->total_size_of_committed_pages;
+  size_t committed_initially =
+      root->total_size_of_committed_pages_for_testing();
   AllocateAndFree();
-  size_t committed_before = root->total_size_of_committed_pages;
+  size_t committed_before = root->total_size_of_committed_pages_for_testing();
 
   EXPECT_GT(committed_before, committed_initially);
 
   StartReclaimer();
   task_environment_.FastForwardBy(
       task_environment_.NextMainThreadPendingTaskDelay());
-  size_t committed_after = root->total_size_of_committed_pages;
+  size_t committed_after = root->total_size_of_committed_pages_for_testing();
   EXPECT_LT(committed_after, committed_before);
   EXPECT_LE(committed_initially, committed_after);
 }
 
 TEST_F(PartitionAllocMemoryReclaimerTest, Reclaim) {
   PartitionRoot<internal::ThreadSafe>* root = allocator_->root();
-  size_t committed_initially = root->total_size_of_committed_pages;
+  size_t committed_initially =
+      root->total_size_of_committed_pages_for_testing();
 
   {
     AllocateAndFree();
 
-    size_t committed_before = root->total_size_of_committed_pages;
+    size_t committed_before = root->total_size_of_committed_pages_for_testing();
     EXPECT_GT(committed_before, committed_initially);
     PartitionAllocMemoryReclaimer::Instance()->Reclaim();
-    size_t committed_after = root->total_size_of_committed_pages;
+    size_t committed_after = root->total_size_of_committed_pages_for_testing();
 
     EXPECT_LT(committed_after, committed_before);
     EXPECT_LE(committed_initially, committed_after);

base/allocator/partition_allocator/partition_alloc.cc

@@ -28,8 +28,13 @@ NOINLINE void PartitionRoot<thread_safe>::OutOfMemory(size_t size) {
 #if !defined(ARCH_CPU_64_BITS)
   // Check whether this OOM is due to a lot of super pages that are allocated
   // but not committed, probably due to http://crbug.com/421387.
-  if (total_size_of_super_pages + total_size_of_direct_mapped_pages -
-          total_size_of_committed_pages >
+  //
+  // Reading values without locking is fine here, we are going to crash anyway,
+  // this is used for reporting only, and concurrent successful allocations are
+  // unlikely.
+  if (TS_UNCHECKED_READ(total_size_of_super_pages) +
+          TS_UNCHECKED_READ(total_size_of_direct_mapped_pages) -
+          TS_UNCHECKED_READ(total_size_of_committed_pages) >
       kReasonableSizeOfUnusedPages) {
     internal::PartitionOutOfMemoryWithLotsOfUncommitedPages(size);
   }

base/allocator/partition_allocator/partition_alloc.h

@@ -384,9 +384,9 @@ struct BASE_EXPORT PartitionRoot {
   // Invariant: total_size_of_committed_pages <=
   //                total_size_of_super_pages +
   //                total_size_of_direct_mapped_pages.
-  size_t total_size_of_committed_pages = 0;
-  size_t total_size_of_super_pages = 0;
-  size_t total_size_of_direct_mapped_pages = 0;
+  size_t total_size_of_committed_pages GUARDED_BY(lock_) = 0;
+  size_t total_size_of_super_pages GUARDED_BY(lock_) = 0;
+  size_t total_size_of_direct_mapped_pages GUARDED_BY(lock_) = 0;
   bool is_thread_safe = thread_safe;
   // TODO(bartekn): Consider size of added extras (cookies and/or tag, or
   // nothing) instead of true|false, so that we can just add or subtract the
@@ -440,8 +440,10 @@ struct BASE_EXPORT PartitionRoot {
   ALWAYS_INLINE static bool IsValidPage(Page* page);
   ALWAYS_INLINE static PartitionRoot* FromPage(Page* page);
 
-  ALWAYS_INLINE void IncreaseCommittedPages(size_t len);
-  ALWAYS_INLINE void DecreaseCommittedPages(size_t len);
+  ALWAYS_INLINE void IncreaseCommittedPages(size_t len)
+      EXCLUSIVE_LOCKS_REQUIRED(lock_);
+  ALWAYS_INLINE void DecreaseCommittedPages(size_t len)
+      EXCLUSIVE_LOCKS_REQUIRED(lock_);
   ALWAYS_INLINE void DecommitSystemPages(void* address, size_t length)
       EXCLUSIVE_LOCKS_REQUIRED(lock_);
   ALWAYS_INLINE void RecommitSystemPages(void* address, size_t length)
@@ -508,6 +510,10 @@ struct BASE_EXPORT PartitionRoot {
   internal::ThreadCache* thread_cache_for_testing() const {
     return with_thread_cache ? internal::ThreadCache::Get() : nullptr;
   }
+  size_t total_size_of_committed_pages_for_testing() {
+    ScopedGuard guard{lock_};
+    return total_size_of_committed_pages;
+  }
 
  private:
   // Allocates memory, without any cookies / tags.

base/allocator/partition_allocator/partition_alloc_unittest.cc

@@ -1357,7 +1357,7 @@ TEST_F(PartitionAllocTest, MappingCollision) {
 
 // Tests that pages in the free page cache do get freed as appropriate.
 TEST_F(PartitionAllocTest, FreeCache) {
-  EXPECT_EQ(0U, allocator.root()->total_size_of_committed_pages);
+  EXPECT_EQ(0U, allocator.root()->total_size_of_committed_pages_for_testing());
 
   size_t big_size = 1000 - kExtraAllocSize;
   size_t bucket_index = SizeToIndex(big_size + kExtraAllocSize);
@@ -1373,7 +1373,7 @@ TEST_F(PartitionAllocTest, FreeCache) {
   EXPECT_EQ(1, page->num_allocated_slots);
   size_t expected_committed_size = PartitionPageSize();
   EXPECT_EQ(expected_committed_size,
-            allocator.root()->total_size_of_committed_pages);
+            allocator.root()->total_size_of_committed_pages_for_testing());
   allocator.root()->Free(ptr);
   EXPECT_EQ(0, page->num_allocated_slots);
   EXPECT_NE(-1, page->empty_cache_index);
@@ -1390,7 +1390,8 @@ TEST_F(PartitionAllocTest, FreeCache) {
   size_t expected_size =
       cycle_free_cache_bucket->num_system_pages_per_slot_span *
       SystemPageSize();
-  EXPECT_EQ(expected_size, allocator.root()->total_size_of_committed_pages);
+  EXPECT_EQ(expected_size,
+            allocator.root()->total_size_of_committed_pages_for_testing());
 
   // Check that an allocation works ok whilst in this state (a free'd page
   // as the active pages head).
@@ -1407,7 +1408,7 @@ TEST_F(PartitionAllocTest, FreeCache) {
     EXPECT_TRUE(page->freelist_head);
   }
   EXPECT_EQ(expected_committed_size,
-            allocator.root()->total_size_of_committed_pages);
+            allocator.root()->total_size_of_committed_pages_for_testing());
 }
 
 // Tests for a bug we had with losing references to free pages.

base/allocator/partition_allocator/partition_bucket.cc

@@ -25,10 +25,9 @@ namespace internal {
 namespace {
 
 template <bool thread_safe>
-ALWAYS_INLINE PartitionPage<thread_safe>* PartitionDirectMap(
-    PartitionRoot<thread_safe>* root,
-    int flags,
-    size_t raw_size) {
+ALWAYS_INLINE PartitionPage<thread_safe>*
+PartitionDirectMap(PartitionRoot<thread_safe>* root, int flags, size_t raw_size)
+    EXCLUSIVE_LOCKS_REQUIRED(root->lock_) {
   size_t size = PartitionBucket<thread_safe>::get_direct_map_size(raw_size);
 
   // Because we need to fake looking like a super page, we need to allocate

base/allocator/partition_allocator/partition_page.cc

@@ -24,6 +24,7 @@ template <bool thread_safe>
 ALWAYS_INLINE DeferredUnmap
 PartitionDirectUnmap(PartitionPage<thread_safe>* page) {
   PartitionRoot<thread_safe>* root = PartitionRoot<thread_safe>::FromPage(page);
+  root->lock_.AssertAcquired();
   const PartitionDirectMapExtent<thread_safe>* extent =
       PartitionDirectMapExtent<thread_safe>::FromPage(page);
   size_t unmap_size = extent->map_size;
@@ -109,6 +110,10 @@ PartitionPage<thread_safe>* PartitionPage<thread_safe>::get_sentinel_page() {
 
 template <bool thread_safe>
 DeferredUnmap PartitionPage<thread_safe>::FreeSlowPath() {
+#if DCHECK_IS_ON()
+  auto* root = PartitionRoot<thread_safe>::FromPage(this);
+  root->lock_.AssertAcquired();
+#endif
   PA_DCHECK(this != get_sentinel_page());
   if (LIKELY(num_allocated_slots == 0)) {
     // Page became fully unused.

third_party/blink/renderer/platform/wtf/DEPS

@@ -23,6 +23,7 @@ include_rules = [
     "+base/third_party/double_conversion",
     "+base/template_util.h",
     "+base/test/metrics/histogram_tester.h",
+    "+base/thread_annotations.h",
     "+base/threading",
     "+base/time/time.h",
     "+base/tuple.h",

third_party/blink/renderer/platform/wtf/allocator/partitions.cc

@@ -35,6 +35,7 @@
 #include "base/allocator/partition_allocator/page_allocator.h"
 #include "base/debug/alias.h"
 #include "base/strings/safe_sprintf.h"
+#include "base/thread_annotations.h"
 #include "components/crash/core/common/crash_key.h"
 #include "third_party/blink/renderer/platform/wtf/allocator/partition_allocator.h"
 #include "third_party/blink/renderer/platform/wtf/wtf.h"
@@ -145,10 +146,15 @@ class LightPartitionStatsDumperImpl : public base::PartitionStatsDumper {
 size_t Partitions::TotalSizeOfCommittedPages() {
   DCHECK(initialized_);
   size_t total_size = 0;
-  total_size += FastMallocPartition()->total_size_of_committed_pages;
-  total_size += ArrayBufferPartition()->total_size_of_committed_pages;
-  total_size += BufferPartition()->total_size_of_committed_pages;
-  total_size += LayoutPartition()->total_size_of_committed_pages;
+  // Racy reads below: this is fine to collect statistics.
+  total_size +=
+      TS_UNCHECKED_READ(FastMallocPartition()->total_size_of_committed_pages);
+  total_size +=
+      TS_UNCHECKED_READ(ArrayBufferPartition()->total_size_of_committed_pages);
+  total_size +=
+      TS_UNCHECKED_READ(BufferPartition()->total_size_of_committed_pages);
+  total_size +=
+      TS_UNCHECKED_READ(LayoutPartition()->total_size_of_committed_pages);
   return total_size;
 }