blink: Add thread annotation to platform.

Replaces comments with annotations for variables protected by mutexes in
blink/platform. This allows the static analysis pass in clang to check locking
at compile time.

Changes are:
- Annotate member variables with GUARDED_BY()
- Add a few Mutex::AssertAcquired() assertions. These mark the mutex as acquired
  to the checker, and do not generate any code when DCHECK()s are off.
- Add required locking to some unit tests only methods. The code wasn't
  incorrect before as these are typically single-threaded, but this is still
  required in principle.

While in the neighborhood, also fix a few "git cl lint warnings": missing
include guards comments, explicit constructors, and include what you use.

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

third_party/blink/renderer/platform/audio/push_pull_fifo.cc

@@ -4,7 +4,9 @@
 
 #include "third_party/blink/renderer/platform/audio/push_pull_fifo.h"
 
+#include <algorithm>
 #include <memory>
+
 #include "build/build_config.h"
 #include "third_party/blink/renderer/platform/audio/audio_utilities.h"
 #include "third_party/blink/renderer/platform/instrumentation/histogram.h"
@@ -207,7 +209,8 @@ size_t PushPullFIFO::Pull(AudioBus* output_bus, size_t frames_requested) {
       : 0;
 }
 
-const PushPullFIFOStateForTest PushPullFIFO::GetStateForTest() const {
+const PushPullFIFOStateForTest PushPullFIFO::GetStateForTest() {
+  MutexLocker locker(lock_);
   return {length(),     NumberOfChannels(), frames_available_, index_read_,
           index_write_, overflow_count_,    underflow_count_};
 }

third_party/blink/renderer/platform/audio/push_pull_fifo.h

@@ -67,15 +67,21 @@ class PLATFORM_EXPORT PushPullFIFO {
   size_t Pull(AudioBus* output_bus, size_t frames_requested);
 
   size_t length() const { return fifo_length_; }
-  unsigned NumberOfChannels() const { return fifo_bus_->NumberOfChannels(); }
+  unsigned NumberOfChannels() const {
+    lock_.AssertAcquired();
+    return fifo_bus_->NumberOfChannels();
+  }
 
   // TODO(hongchan): For single thread unit test only. Consider refactoring.
-  AudioBus* GetFIFOBusForTest() const { return fifo_bus_.get(); }
+  AudioBus* GetFIFOBusForTest() {
+    MutexLocker locker(lock_);
+    return fifo_bus_.get();
+  }
 
   // For single thread unit test only. Get the current configuration that
   // consists of FIFO length, number of channels, read/write index position and
   // under/overflow count.
-  const PushPullFIFOStateForTest GetStateForTest() const;
+  const PushPullFIFOStateForTest GetStateForTest();
 
  private:
   // The size of the FIFO.
@@ -86,13 +92,12 @@ class PLATFORM_EXPORT PushPullFIFO {
   unsigned overflow_count_ = 0;
   unsigned underflow_count_ = 0;
 
-  // This lock protects variables below.
   Mutex lock_;
   // The number of frames in the FIFO actually available for pulling.
-  size_t frames_available_ = 0;
-  size_t index_read_ = 0;
-  size_t index_write_ = 0;
-  scoped_refptr<AudioBus> fifo_bus_;
+  size_t frames_available_ GUARDED_BY(lock_) = 0;
+  size_t index_read_ GUARDED_BY(lock_) = 0;
+  size_t index_write_ GUARDED_BY(lock_) = 0;
+  scoped_refptr<AudioBus> fifo_bus_ GUARDED_BY(lock_);
 
   DISALLOW_COPY_AND_ASSIGN(PushPullFIFO);
 };

third_party/blink/renderer/platform/audio/push_pull_fifo_multithread_test.cc

@@ -30,7 +30,8 @@ class FIFOClient {
  public:
   FIFOClient(PushPullFIFO* fifo, size_t bus_length, size_t jitter_range_ms)
       : fifo_(fifo),
-        bus_(AudioBus::Create(fifo->NumberOfChannels(), bus_length)),
+        bus_(AudioBus::Create(fifo->GetStateForTest().number_of_channels,
+                              bus_length)),
         client_thread_(Platform::Current()->CreateThread(
             ThreadCreationParams(WebThreadType::kTestThread)
                 .SetThreadNameForTest("FIFOClientThread"))),

third_party/blink/renderer/platform/graphics/image_decoding_store.cc

@@ -240,6 +240,7 @@ template <class T, class U, class V>
 void ImageDecodingStore::InsertCacheInternal(std::unique_ptr<T> cache_entry,
                                              U* cache_map,
                                              V* identifier_map) {
+  mutex_.AssertAcquired();
   const size_t cache_entry_bytes = cache_entry->MemoryUsageInBytes();
   heap_memory_usage_in_bytes_ += cache_entry_bytes;
 
@@ -266,6 +267,7 @@ void ImageDecodingStore::RemoveFromCacheInternal(
     U* cache_map,
     V* identifier_map,
     Vector<std::unique_ptr<CacheEntry>>* deletion_list) {
+  mutex_.AssertAcquired();
   DCHECK_EQ(cache_entry->UseCount(), 0);
 
   const size_t cache_entry_bytes = cache_entry->MemoryUsageInBytes();
@@ -307,6 +309,7 @@ void ImageDecodingStore::RemoveCacheIndexedByGeneratorInternal(
     V* identifier_map,
     const ImageFrameGenerator* generator,
     Vector<std::unique_ptr<CacheEntry>>* deletion_list) {
+  mutex_.AssertAcquired();
   typename V::iterator iter = identifier_map->find(generator);
   if (iter == identifier_map->end())
     return;
@@ -327,6 +330,7 @@ void ImageDecodingStore::RemoveCacheIndexedByGeneratorInternal(
 
 void ImageDecodingStore::RemoveFromCacheListInternal(
     const Vector<std::unique_ptr<CacheEntry>>& deletion_list) {
+  mutex_.AssertAcquired();
   for (size_t i = 0; i < deletion_list.size(); ++i)
     ordered_cache_list_.Remove(deletion_list[i].get());
 }

third_party/blink/renderer/platform/graphics/image_decoding_store.h

@@ -322,35 +322,31 @@ class PLATFORM_EXPORT ImageDecodingStore final {
   // This is used for eviction of old entries.
   // Head of this list is the least recently used cache entry.
   // Tail of this list is the most recently used cache entry.
-  DoublyLinkedList<CacheEntry> ordered_cache_list_;
+  DoublyLinkedList<CacheEntry> ordered_cache_list_ GUARDED_BY(mutex_);
 
   // A lookup table for all decoder cache objects. Owns all decoder cache
   // objects.
   typedef HashMap<DecoderCacheKey, std::unique_ptr<DecoderCacheEntry>>
       DecoderCacheMap;
-  DecoderCacheMap decoder_cache_map_;
+  DecoderCacheMap decoder_cache_map_ GUARDED_BY(mutex_);
 
   // A lookup table to map ImageFrameGenerator to all associated
   // decoder cache keys.
   typedef HashSet<DecoderCacheKey> DecoderCacheKeySet;
   typedef HashMap<const ImageFrameGenerator*, DecoderCacheKeySet>
       DecoderCacheKeyMap;
-  DecoderCacheKeyMap decoder_cache_key_map_;
+  DecoderCacheKeyMap decoder_cache_key_map_ GUARDED_BY(mutex_);
 
-  size_t heap_limit_in_bytes_;
-  size_t heap_memory_usage_in_bytes_;
+  size_t heap_limit_in_bytes_ GUARDED_BY(mutex_);
+  size_t heap_memory_usage_in_bytes_ GUARDED_BY(mutex_);
 
   // A listener to global memory pressure events.
   base::MemoryPressureListener memory_pressure_listener_;
 
-  // Protect concurrent access to these members:
-  //   m_orderedCacheList
-  //   m_decoderCacheMap and all CacheEntrys stored in it
-  //   m_decoderCacheKeyMap
-  //   m_heapLimitInBytes
-  //   m_heapMemoryUsageInBytes
-  // This mutex also protects calls to underlying skBitmap's
-  // lockPixels()/unlockPixels() as they are not threadsafe.
+  // Also protects:
+  // - the CacheEntry in |decoder_cache_map_|.
+  // - calls to underlying skBitmap's LockPixels()/UnlockPixels() as they are
+  //   not threadsafe.
   Mutex mutex_;
 
   DISALLOW_COPY_AND_ASSIGN(ImageDecodingStore);
@@ -358,4 +354,4 @@ class PLATFORM_EXPORT ImageDecodingStore final {
 
 }  // namespace blink
 
-#endif
+#endif  // THIRD_PARTY_BLINK_RENDERER_PLATFORM_GRAPHICS_IMAGE_DECODING_STORE_H_

third_party/blink/renderer/platform/graphics/image_frame_generator.h

@@ -27,6 +27,7 @@
 #define THIRD_PARTY_BLINK_RENDERER_PLATFORM_GRAPHICS_IMAGE_FRAME_GENERATOR_H_
 
 #include <memory>
+#include <utility>
 
 #include "base/macros.h"
 #include "base/memory/scoped_refptr.h"
@@ -150,13 +151,11 @@ class PLATFORM_EXPORT ImageFrameGenerator final
   const bool is_multi_frame_;
   const Vector<SkISize> supported_sizes_;
 
-  // Prevents concurrent access to all variables below.
   mutable Mutex generator_mutex_;
-
-  bool decode_failed_ = false;
-  bool yuv_decoding_failed_ = false;
-  size_t frame_count_ = 0u;
-  Vector<bool> has_alpha_;
+  bool decode_failed_ GUARDED_BY(generator_mutex_) = false;
+  bool yuv_decoding_failed_ GUARDED_BY(generator_mutex_) = false;
+  size_t frame_count_ GUARDED_BY(generator_mutex_) = 0u;
+  Vector<bool> has_alpha_ GUARDED_BY(generator_mutex_);
 
   struct ClientMutex {
     int ref_count = 0;
@@ -170,7 +169,7 @@ class PLATFORM_EXPORT ImageFrameGenerator final
           std::unique_ptr<ClientMutex>,
           WTF::IntHash<cc::PaintImage::GeneratorClientId>,
           WTF::UnsignedWithZeroKeyHashTraits<cc::PaintImage::GeneratorClientId>>
-      mutex_map_;
+      mutex_map_ GUARDED_BY(generator_mutex_);
 
   std::unique_ptr<ImageDecoderFactory> image_decoder_factory_;
 
@@ -179,4 +178,4 @@ class PLATFORM_EXPORT ImageFrameGenerator final
 
 }  // namespace blink
 
-#endif
+#endif  // THIRD_PARTY_BLINK_RENDERER_PLATFORM_GRAPHICS_IMAGE_FRAME_GENERATOR_H_

third_party/blink/renderer/platform/image-decoders/segment_reader.cc

@@ -4,6 +4,8 @@
 
 #include "third_party/blink/renderer/platform/image-decoders/segment_reader.h"
 
+#include <utility>
+
 #include "base/containers/span.h"
 #include "base/macros.h"
 #include "base/memory/scoped_refptr.h"
@@ -20,7 +22,7 @@ namespace blink {
 // Interface for ImageDecoder to read a SharedBuffer.
 class SharedBufferSegmentReader final : public SegmentReader {
  public:
-  SharedBufferSegmentReader(scoped_refptr<SharedBuffer>);
+  explicit SharedBufferSegmentReader(scoped_refptr<SharedBuffer>);
   size_t size() const override;
   size_t GetSomeData(const char*& data, size_t position) const override;
   sk_sp<SkData> GetAsSkData() const override;
@@ -66,7 +68,7 @@ sk_sp<SkData> SharedBufferSegmentReader::GetAsSkData() const {
 // Interface for ImageDecoder to read an SkData.
 class DataSegmentReader final : public SegmentReader {
  public:
-  DataSegmentReader(sk_sp<SkData>);
+  explicit DataSegmentReader(sk_sp<SkData>);
   size_t size() const override;
   size_t GetSomeData(const char*& data, size_t position) const override;
   sk_sp<SkData> GetAsSkData() const override;
@@ -101,7 +103,7 @@ sk_sp<SkData> DataSegmentReader::GetAsSkData() const {
 
 class ROBufferSegmentReader final : public SegmentReader {
  public:
-  ROBufferSegmentReader(sk_sp<SkROBuffer>);
+  explicit ROBufferSegmentReader(sk_sp<SkROBuffer>);
 
   size_t size() const override;
   size_t GetSomeData(const char*& data, size_t position) const override;
@@ -109,11 +111,10 @@ class ROBufferSegmentReader final : public SegmentReader {
 
  private:
   sk_sp<SkROBuffer> ro_buffer_;
-  // Protects access to mutable fields.
   mutable Mutex read_mutex_;
   // Position of the first char in the current block of iter_.
-  mutable size_t position_of_block_;
-  mutable SkROBuffer::Iter iter_;
+  mutable size_t position_of_block_ GUARDED_BY(read_mutex_);
+  mutable SkROBuffer::Iter iter_ GUARDED_BY(read_mutex_);
 
   DISALLOW_COPY_AND_ASSIGN(ROBufferSegmentReader);
 };

third_party/blink/renderer/platform/instrumentation/memory_pressure_listener.h

@@ -69,7 +69,7 @@ class PLATFORM_EXPORT MemoryPressureListenerRegistry final
   static bool is_low_end_device_;
 
   HeapHashSet<WeakMember<MemoryPressureListener>> clients_;
-  HashSet<Thread*> threads_;
+  HashSet<Thread*> threads_ GUARDED_BY(threads_mutex_);
   Mutex threads_mutex_;
 
   DISALLOW_COPY_AND_ASSIGN(MemoryPressureListenerRegistry);

third_party/blink/renderer/platform/mediastream/media_stream_source.cc

@@ -189,7 +189,10 @@ void MediaStreamSource::Trace(blink::Visitor* visitor) {
 }
 
 void MediaStreamSource::Dispose() {
+  {
+    MutexLocker locker(audio_consumers_lock_);
     audio_consumers_.clear();
+  }
   platform_source_.reset();
   constraints_.Reset();
 }

third_party/blink/renderer/platform/mediastream/media_stream_source.h

@@ -135,7 +135,8 @@ class PLATFORM_EXPORT MediaStreamSource final
   bool requires_consumer_;
   HeapHashSet<WeakMember<Observer>> observers_;
   Mutex audio_consumers_lock_;
-  HashSet<AudioDestinationConsumer*> audio_consumers_;
+  HashSet<AudioDestinationConsumer*> audio_consumers_
+      GUARDED_BY(audio_consumers_lock_);
   std::unique_ptr<WebPlatformMediaStreamSource> platform_source_;
   WebMediaConstraints constraints_;
   WebMediaStreamSource::Capabilities capabilities_;

third_party/blink/renderer/platform/network/network_state_notifier.h

@@ -362,9 +362,9 @@ class PLATFORM_EXPORT NetworkStateNotifier {
   double GetRandomMultiplier(const String& host) const;
 
   mutable Mutex mutex_;
-  NetworkState state_;
-  bool has_override_;
-  NetworkState override_;
+  NetworkState state_ GUARDED_BY(mutex_);
+  bool has_override_ GUARDED_BY(mutex_);
+  NetworkState override_ GUARDED_BY(mutex_);
 
   ObserverListMap connection_observers_;
   ObserverListMap on_line_state_observers_;