gpu: Make MemoryTypeTracker usage thread safe

SharedImageBacking attributes its allocation to the MemoryTypeTracker of
its first SharedImageRepresentation which is considered its owning ref.
When the owning ref changes, the backing will switch the allocation to
the new owning ref's tracker.

This can happen on a different thread than the one the representation
was created on, and in general calling the underlying MemoryTracker is
not thread safe e.g. dereferencing the peak memory monitor's WeakPtr on
a different thread. This issue was discovered while testing the DrDC
prototype which makes the display's gpu work run on the viz thread
instead of the gpu main thread.

This CL fixes the above problem by making MemoryTypeTracker forward its
calls to its original sequence (and thus thread).  If a sequenced task
runner isn't set e.g. on the render thread on Android Webview, then the
calls are made synchronously.

Bug: 1114888
Change-Id: I6833139c96de3824f450b09333f755560dff62c1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2350398
Commit-Queue: Sunny Sachanandani <sunnyps@chromium.org>
Reviewed-by: vikas soni <vikassoni@chromium.org>
Reviewed-by: Jonathan Ross <jonross@chromium.org>
Cr-Commit-Position: refs/heads/master@{#797841}

gpu/command_buffer/service/BUILD.gn

@@ -50,6 +50,7 @@ target(link_target_type, "service_sources") {
     "image_manager.cc",
     "image_manager.h",
     "mailbox_manager.h",
+    "memory_tracking.cc",
     "memory_tracking.h",
     "scheduler.cc",
     "scheduler.h",

gpu/command_buffer/service/buffer_manager.h

@@ -17,6 +17,7 @@
 #include "base/check_op.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
+#include "base/trace_event/memory_dump_provider.h"
 #include "gpu/command_buffer/common/buffer.h"
 #include "gpu/command_buffer/service/gl_utils.h"
 #include "gpu/command_buffer/service/memory_tracking.h"

gpu/command_buffer/service/memory_tracking.cc

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "gpu/command_buffer/service/memory_tracking.h"
+
+#include "base/threading/sequenced_task_runner_handle.h"
+
+namespace gpu {
+
+// This can be created on the render thread on Andorid Webview which is managed
+// by the OS and doesn't have a task runner associated with it in which case
+// base::SequencedTaskRunnerHandle::Get() will trigger a DCHECK.
+MemoryTypeTracker::MemoryTypeTracker(MemoryTracker* memory_tracker)
+    : MemoryTypeTracker(memory_tracker,
+                        base::SequencedTaskRunnerHandle::IsSet()
+                            ? base::SequencedTaskRunnerHandle::Get()
+                            : nullptr) {}
+
+MemoryTypeTracker::MemoryTypeTracker(
+    MemoryTracker* memory_tracker,
+    scoped_refptr<base::SequencedTaskRunner> task_runner)
+    : memory_tracker_(memory_tracker),
+      task_runner_(std::move(task_runner)),
+      weak_ptr_factory_(this) {}
+
+MemoryTypeTracker::~MemoryTypeTracker() {
+  DCHECK_EQ(mem_represented_, 0u);
+}
+
+void MemoryTypeTracker::TrackMemAlloc(size_t bytes) {
+  {
+    base::AutoLock auto_lock(lock_);
+    DCHECK(bytes >= 0);
+    mem_represented_ += bytes;
+  }
+  // Notify memory tracker outside the lock to prevent reentrancy deadlock.
+  if (memory_tracker_ && bytes)
+    TrackMemoryAllocatedChange(bytes);
+}
+
+void MemoryTypeTracker::TrackMemFree(size_t bytes) {
+  {
+    base::AutoLock auto_lock(lock_);
+    DCHECK(bytes >= 0 && bytes <= mem_represented_);
+    mem_represented_ -= bytes;
+  }
+  // Notify memory tracker outside the lock to prevent reentrancy deadlock.
+  if (memory_tracker_ && bytes)
+    TrackMemoryAllocatedChange(-static_cast<int64_t>(bytes));
+}
+
+size_t MemoryTypeTracker::GetMemRepresented() const {
+  base::AutoLock auto_lock(lock_);
+  return mem_represented_;
+}
+
+void MemoryTypeTracker::TrackMemoryAllocatedChange(int64_t delta) {
+  DCHECK(memory_tracker_);
+  if (task_runner_ && !task_runner_->RunsTasksInCurrentSequence()) {
+    task_runner_->PostTask(
+        FROM_HERE,
+        base::BindOnce(&MemoryTypeTracker::TrackMemoryAllocatedChange,
+                       weak_ptr_factory_.GetWeakPtr(), delta));
+  } else {
+    memory_tracker_->TrackMemoryAllocatedChange(delta);
+  }
+}
+
+}  // namespace gpu

gpu/command_buffer/service/memory_tracking.h

@@ -5,22 +5,25 @@
 #ifndef GPU_COMMAND_BUFFER_SERVICE_MEMORY_TRACKING_H_
 #define GPU_COMMAND_BUFFER_SERVICE_MEMORY_TRACKING_H_
 
-#include <stddef.h>
 #include <stdint.h>
 
-#include <string>
-#include "base/check.h"
 #include "base/macros.h"
-#include "base/memory/ref_counted.h"
-#include "base/trace_event/trace_event.h"
+#include "base/memory/scoped_refptr.h"
+#include "base/memory/weak_ptr.h"
+#include "base/synchronization/lock.h"
 #include "gpu/command_buffer/common/command_buffer_id.h"
+#include "gpu/gpu_export.h"
 #include "gpu/ipc/common/gpu_peak_memory.h"
 
+namespace base {
+class SequencedTaskRunner;
+}  // namespace base
+
 namespace gpu {
 
 // A MemoryTracker is used to propagate per-ContextGroup memory usage
 // statistics to the global GpuMemoryManager.
-class MemoryTracker {
+class GPU_EXPORT MemoryTracker {
  public:
   // Observe all changes in memory notified to this MemoryTracker.
   class Observer {
@@ -55,34 +58,32 @@ class MemoryTracker {
 
 // A MemoryTypeTracker tracks the use of a particular type of memory (buffer,
 // texture, or renderbuffer) and forward the result to a specified
-// MemoryTracker.
-class MemoryTypeTracker {
+// MemoryTracker. MemoryTypeTracker is thread-safe, but it must not outlive the
+// MemoryTracker which will be notified on the sequence the MemoryTypeTracker
+// was created on (if base::SequencedTaskRunnerHandle::IsSet()), or on the task
+// runner specified (for testing).
+class GPU_EXPORT MemoryTypeTracker {
  public:
-  explicit MemoryTypeTracker(MemoryTracker* memory_tracker)
-      : memory_tracker_(memory_tracker) {}
+  explicit MemoryTypeTracker(MemoryTracker* memory_tracker);
+  // For testing.
+  MemoryTypeTracker(MemoryTracker* memory_tracker,
+                    scoped_refptr<base::SequencedTaskRunner> task_runner);
+  ~MemoryTypeTracker();
 
-  ~MemoryTypeTracker() { DCHECK(!mem_represented_); }
+  void TrackMemAlloc(size_t bytes);
+  void TrackMemFree(size_t bytes);
+  size_t GetMemRepresented() const;
 
-  void TrackMemAlloc(size_t bytes) {
-    DCHECK(bytes >= 0);
-    mem_represented_ += bytes;
-    if (memory_tracker_ && bytes)
-      memory_tracker_->TrackMemoryAllocatedChange(bytes);
-  }
+ private:
+  void TrackMemoryAllocatedChange(int64_t delta);
 
-  void TrackMemFree(size_t bytes) {
-    DCHECK(bytes >= 0 && bytes <= mem_represented_);
-    mem_represented_ -= bytes;
-    if (memory_tracker_ && bytes) {
-      memory_tracker_->TrackMemoryAllocatedChange(-static_cast<int64_t>(bytes));
-    }
-  }
+  MemoryTracker* const memory_tracker_;
 
-  size_t GetMemRepresented() const { return mem_represented_; }
+  size_t mem_represented_ GUARDED_BY(lock_) = 0;
+  mutable base::Lock lock_;
 
- private:
-  MemoryTracker* memory_tracker_;
-  size_t mem_represented_ = 0;
+  scoped_refptr<base::SequencedTaskRunner> task_runner_;
+  base::WeakPtrFactory<MemoryTypeTracker> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(MemoryTypeTracker);
 };

gpu/command_buffer/service/renderbuffer_manager.h

@@ -15,6 +15,7 @@
 #include "base/containers/flat_set.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
+#include "base/trace_event/memory_dump_provider.h"
 #include "gpu/command_buffer/service/gl_utils.h"
 #include "gpu/command_buffer/service/memory_tracking.h"
 #include "gpu/gpu_gles2_export.h"

gpu/command_buffer/service/shared_image_manager_unittest.cc

@@ -4,6 +4,9 @@
 
 #include "gpu/command_buffer/service/shared_image_manager.h"
 
+#include "base/task/task_traits.h"
+#include "base/task/thread_pool.h"
+#include "base/task/thread_pool/thread_pool_instance.h"
 #include "gpu/command_buffer/common/gpu_memory_buffer_support.h"
 #include "gpu/command_buffer/common/mailbox.h"
 #include "gpu/command_buffer/common/shared_image_usage.h"
@@ -138,5 +141,87 @@ TEST(SharedImageManagerTest, TransferRefNewTracker) {
   EXPECT_EQ(0u, tracker2->GetMemRepresented());
 }
 
+class SequenceValidatingMemoryTracker : public MemoryTracker {
+ public:
+  SequenceValidatingMemoryTracker()
+      : task_runner_(base::ThreadPool::CreateSequencedTaskRunner({})) {}
+
+  ~SequenceValidatingMemoryTracker() override { EXPECT_EQ(size_, 0u); }
+
+  scoped_refptr<base::SequencedTaskRunner> task_runner() const {
+    return task_runner_;
+  }
+
+  void TrackMemoryAllocatedChange(int64_t delta) override {
+    EXPECT_TRUE(task_runner_->RunsTasksInCurrentSequence());
+    size_ += delta;
+  }
+
+  uint64_t GetSize() const override { return size_; }
+  int ClientId() const override { return 0; }
+  uint64_t ClientTracingId() const override { return 0; }
+  uint64_t ContextGroupTracingId() const override { return 0; }
+
+ private:
+  scoped_refptr<base::SequencedTaskRunner> task_runner_;
+  int64_t size_ = 0;
+};
+
+TEST(SharedImageManagerTest, TransferRefCrossThread) {
+  const size_t kSizeBytes = 1024;
+  SharedImageManager manager;
+  SequenceValidatingMemoryTracker memory_tracker1;
+  SequenceValidatingMemoryTracker memory_tracker2;
+
+  auto memory_type_tracker1 = std::make_unique<MemoryTypeTracker>(
+      &memory_tracker1, memory_tracker1.task_runner());
+  auto memory_type_tracker2 = std::make_unique<MemoryTypeTracker>(
+      &memory_tracker2, memory_tracker2.task_runner());
+
+  auto mailbox = Mailbox::GenerateForSharedImage();
+  auto format = viz::ResourceFormat::RGBA_8888;
+  gfx::Size size(256, 256);
+  auto color_space = gfx::ColorSpace::CreateSRGB();
+  auto surface_origin = kTopLeft_GrSurfaceOrigin;
+  auto alpha_type = kPremul_SkAlphaType;
+  uint32_t usage = SHARED_IMAGE_USAGE_GLES2;
+
+  auto backing = std::make_unique<TestSharedImageBacking>(
+      mailbox, format, size, color_space, surface_origin, alpha_type, usage,
+      kSizeBytes);
+
+  auto factory_ref =
+      manager.Register(std::move(backing), memory_type_tracker1.get());
+  EXPECT_EQ(kSizeBytes, memory_type_tracker1->GetMemRepresented());
+  base::ThreadPoolInstance::Get()->FlushForTesting();
+  EXPECT_EQ(kSizeBytes, memory_tracker1.GetSize());
+
+  // Take an additional ref/representation with a new tracker. Memory should
+  // stay accounted to the original tracker.
+  auto gl_representation =
+      manager.ProduceGLTexture(mailbox, memory_type_tracker2.get());
+  EXPECT_EQ(kSizeBytes, memory_type_tracker1->GetMemRepresented());
+  EXPECT_EQ(0u, memory_type_tracker2->GetMemRepresented());
+  base::ThreadPoolInstance::Get()->FlushForTesting();
+  EXPECT_EQ(kSizeBytes, memory_tracker1.GetSize());
+  EXPECT_EQ(0u, memory_tracker2.GetSize());
+
+  // Releasing the original should transfer memory to the new tracker.
+  factory_ref.reset();
+  EXPECT_EQ(0u, memory_type_tracker1->GetMemRepresented());
+  EXPECT_EQ(kSizeBytes, memory_type_tracker2->GetMemRepresented());
+  base::ThreadPoolInstance::Get()->FlushForTesting();
+  EXPECT_EQ(0u, memory_tracker1.GetSize());
+  EXPECT_EQ(kSizeBytes, memory_tracker2.GetSize());
+
+  // We can now safely destroy the original tracker.
+  memory_type_tracker1.reset();
+
+  gl_representation.reset();
+  EXPECT_EQ(0u, memory_type_tracker2->GetMemRepresented());
+  base::ThreadPoolInstance::Get()->FlushForTesting();
+  EXPECT_EQ(0u, memory_tracker2.GetSize());
+}
+
 }  // anonymous namespace
 }  // namespace gpu