blink/bindings: Add thread annotations to ParkableString.

This CL turns load-bearing comments into thread annotations checked by the clang
static analyzer. Comments were actually out-of-date already (as it wasn't
specified that is_young_ must be protected by the mutex), though the code was
correct.

No behavior change, and besides the annotations, unit tests have to change as
they need locking to access a variable.

Bug: 924164
Change-Id: Iecac23df0c2e61f4d42be105387fa6bf7605d8cf
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1849854Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Benoit L <lizeb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#704533}

third_party/blink/renderer/platform/bindings/parkable_string.cc

@@ -209,12 +209,7 @@ ParkableStringImpl::~ParkableStringImpl() {
   if (!may_be_parked())
     return;
 
-#if DCHECK_IS_ON()
-  {
-    MutexLocker locker(mutex_);
-    DCHECK_EQ(0, lock_depth_);
-  }
-#endif
+  DCHECK_EQ(0, lock_depth_for_testing());
   AsanUnpoisonString(string_);
   DCHECK(state_ == State::kParked || state_ == State::kUnparked);
 
@@ -362,7 +357,7 @@ ParkableStringImpl::AgeOrParkResult ParkableStringImpl::MaybeAgeOrParkString() {
   DCHECK(!is_parked());
 
   Status status = CurrentStatus();
-  if (is_young()) {
+  if (is_young_) {
     if (status == Status::kUnreferencedExternally)
       is_young_ = false;
   } else {
@@ -403,7 +398,7 @@ bool ParkableStringImpl::Park(ParkingMode mode) {
 void ParkableStringImpl::ParkInternal(ParkingMode mode) {
   mutex_.AssertAcquired();
   DCHECK_EQ(State::kUnparked, state_);
-  DCHECK(!is_young());
+  DCHECK(!is_young_);
   DCHECK(CanParkNow());
 
   // Parking can proceed synchronously.
@@ -449,7 +444,7 @@ ParkableStringImpl::Status ParkableStringImpl::CurrentStatus() const {
 }
 
 bool ParkableStringImpl::CanParkNow() const {
-  return CurrentStatus() == Status::kUnreferencedExternally && !is_young();
+  return CurrentStatus() == Status::kUnreferencedExternally && !is_young_;
 }
 
 void ParkableStringImpl::Unpark() {

third_party/blink/renderer/platform/bindings/parkable_string.h

@@ -11,6 +11,7 @@
 #include "base/gtest_prod_util.h"
 #include "base/macros.h"
 #include "base/memory/scoped_refptr.h"
+#include "base/thread_annotations.h"
 #include "third_party/blink/renderer/platform/platform_export.h"
 #include "third_party/blink/renderer/platform/wtf/allocator/allocator.h"
 #include "third_party/blink/renderer/platform/wtf/ref_counted.h"
@@ -105,12 +106,10 @@ class PLATFORM_EXPORT ParkableStringImpl final
     return compressed_->size();
   }
 
-  // A string can either be "young" or "old". It starts young, and transitions
-  // are:
-  // Young -> Old: By calling |MaybeAgeOrParkString()|.
-  // Old -> Young: When the string is accessed, either by |Lock()|-ing it or
-  //               calling |ToString()|.
-  bool is_young() const { return is_young_; }
+  bool is_young_for_testing() {
+    MutexLocker locker(mutex_);
+    return is_young_;
+  }
 
  private:
   enum class State : uint8_t;
@@ -128,12 +127,12 @@ class PLATFORM_EXPORT ParkableStringImpl final
   // May be called from any thread.
   void LockWithoutMakingYoung();
 #endif  // defined(ADDRESS_SANITIZER)
-  // May be called from any thread. Must acquire |mutex_| first.
-  void MakeYoung();
-  // Whether the string is referenced or locked. Must be called with |mutex_|
-  // held, and the return value is valid as long as |mutex_| is held.
-  Status CurrentStatus() const;
-  bool CanParkNow() const;
+  // May be called from any thread.
+  void MakeYoung() EXCLUSIVE_LOCKS_REQUIRED(mutex_);
+  // Whether the string is referenced or locked. The return value is valid as
+  // long as |mutex_| is held.
+  Status CurrentStatus() const EXCLUSIVE_LOCKS_REQUIRED(mutex_);
+  bool CanParkNow() const EXCLUSIVE_LOCKS_REQUIRED(mutex_);
   void ParkInternal(ParkingMode mode);
   void Unpark();
   String UnparkInternal() const;
@@ -151,14 +150,29 @@ class PLATFORM_EXPORT ParkableStringImpl final
   // Background thread.
   static void CompressInBackground(std::unique_ptr<CompressionTaskParams>);
 
-  Mutex mutex_;  // protects lock_depth_.
-  int lock_depth_;
+  int lock_depth_for_testing() {
+    MutexLocker locker_(mutex_);
+    return lock_depth_;
+  }
+
+  Mutex mutex_;
+  int lock_depth_ GUARDED_BY(mutex_);
 
   // Main thread only.
   State state_;
   String string_;
   std::unique_ptr<Vector<uint8_t>> compressed_;
-  bool is_young_ : 1;
+
+  // A string can either be "young" or "old". It starts young, and transitions
+  // are:
+  // Young -> Old: By calling |MaybeAgeOrParkString()|.
+  // Old -> Young: When the string is accessed, either by |Lock()|-ing it or
+  //               calling |ToString()|.
+  //
+  // Thread safety: it is typically not safe to guard only one part of a
+  // bitfield with a mutex, but this is correct here, as the other members are
+  // const (and never change).
+  bool is_young_ : 1 GUARDED_BY(mutex_);
 
   const bool may_be_parked_ : 1;
   const bool is_8bit_ : 1;

third_party/blink/renderer/platform/bindings/parkable_string_test.cc

@@ -362,15 +362,15 @@ TEST_F(ParkableStringTest, Unpark) {
 TEST_F(ParkableStringTest, LockUnlock) {
   ParkableString parkable(MakeLargeString().Impl());
   ParkableStringImpl* impl = parkable.Impl();
-  EXPECT_EQ(0, impl->lock_depth_);
+  EXPECT_EQ(0, impl->lock_depth_for_testing());
 
   parkable.Lock();
-  EXPECT_EQ(1, impl->lock_depth_);
+  EXPECT_EQ(1, impl->lock_depth_for_testing());
   parkable.Lock();
   parkable.Unlock();
-  EXPECT_EQ(1, impl->lock_depth_);
+  EXPECT_EQ(1, impl->lock_depth_for_testing());
   parkable.Unlock();
-  EXPECT_EQ(0, impl->lock_depth_);
+  EXPECT_EQ(0, impl->lock_depth_for_testing());
 
   parkable.Lock();
   EXPECT_FALSE(ParkAndWait(parkable));
@@ -393,12 +393,12 @@ TEST_F(ParkableStringTest, LockParkedString) {
   EXPECT_TRUE(impl->is_parked());
   parkable.ToString();
   EXPECT_FALSE(impl->is_parked());
-  EXPECT_EQ(1, impl->lock_depth_);
+  EXPECT_EQ(1, impl->lock_depth_for_testing());
 
   EXPECT_FALSE(ParkAndWait(parkable));
 
   parkable.Unlock();
-  EXPECT_EQ(0, impl->lock_depth_);
+  EXPECT_EQ(0, impl->lock_depth_for_testing());
   EXPECT_TRUE(ParkAndWait(parkable));
   EXPECT_TRUE(impl->is_parked());
 }
@@ -675,37 +675,37 @@ TEST_F(ParkableStringTest, ReportMemoryDump) {
 
 TEST_F(ParkableStringTest, Aging) {
   ParkableString parkable(MakeLargeString().ReleaseImpl());
-  EXPECT_TRUE(parkable.Impl()->is_young());
+  EXPECT_TRUE(parkable.Impl()->is_young_for_testing());
   WaitForAging();
-  EXPECT_FALSE(parkable.Impl()->is_young());
+  EXPECT_FALSE(parkable.Impl()->is_young_for_testing());
 
   parkable.Lock();
-  EXPECT_TRUE(parkable.Impl()->is_young());
+  EXPECT_TRUE(parkable.Impl()->is_young_for_testing());
   // Locked strings don't age.
   WaitForAging();
-  EXPECT_TRUE(parkable.Impl()->is_young());
+  EXPECT_TRUE(parkable.Impl()->is_young_for_testing());
   parkable.Unlock();
   WaitForAging();
-  EXPECT_FALSE(parkable.Impl()->is_young());
+  EXPECT_FALSE(parkable.Impl()->is_young_for_testing());
 
   parkable.ToString();
-  EXPECT_TRUE(parkable.Impl()->is_young());
+  EXPECT_TRUE(parkable.Impl()->is_young_for_testing());
   // No external reference, can age again.
   WaitForAging();
-  EXPECT_FALSE(parkable.Impl()->is_young());
+  EXPECT_FALSE(parkable.Impl()->is_young_for_testing());
 
   // External references prevent a string from aging.
   String retained = parkable.ToString();
-  EXPECT_TRUE(parkable.Impl()->is_young());
+  EXPECT_TRUE(parkable.Impl()->is_young_for_testing());
   WaitForAging();
-  EXPECT_TRUE(parkable.Impl()->is_young());
+  EXPECT_TRUE(parkable.Impl()->is_young_for_testing());
 }
 
 TEST_F(ParkableStringTest, OldStringsAreParked) {
   ParkableString parkable(MakeLargeString().ReleaseImpl());
-  EXPECT_TRUE(parkable.Impl()->is_young());
+  EXPECT_TRUE(parkable.Impl()->is_young_for_testing());
   WaitForAging();
-  EXPECT_FALSE(parkable.Impl()->is_young());
+  EXPECT_FALSE(parkable.Impl()->is_young_for_testing());
   WaitForAging();
   EXPECT_TRUE(parkable.Impl()->is_parked());