graphite: Make viz GraphiteCacheController sequence local

Some cc_unittests run both single-threaded and multi-threaded tests in
the same process which ends up instantiating viz Display and related
resources on different threads - this causes the thread checker in
GetOrCreateGraphiteCacheController to fail.

This CL addresses that by moving the thread/sequence check to the
GraphiteCacheController class and using a sequence local storage slot
to hold the weak pointer. This way all SkiaOutputSurfaceImpl instances
on the same sequence share the same GraphiteCacheController, but not
across threads.

This CL also includes other minor changes like reordering constructor
parameters, using base::SupportsWeakPtr<T>, minor cleanup, etc.

Bug: 1472451
Change-Id: Ia4f1d4939dcbd36ca17492e0a606ec8f2c36dc0e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4815009
Commit-Queue: Peng Huang <penghuang@chromium.org>
Commit-Queue: Sunny Sachanandani <sunnyps@chromium.org>
Reviewed-by: Peng Huang <penghuang@chromium.org>
Auto-Submit: Sunny Sachanandani <sunnyps@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1188637}

components/viz/service/display_embedder/skia_output_surface_impl.cc

@@ -18,7 +18,7 @@
 #include "base/synchronization/waitable_event.h"
 #include "base/system/sys_info.h"
 #include "base/task/single_thread_task_runner.h"
-#include "base/threading/thread_checker.h"
+#include "base/threading/sequence_local_storage_slot.h"
 #include "base/time/time.h"
 #include "base/trace_event/trace_event.h"
 #include "build/build_config.h"
@@ -135,25 +135,19 @@ gpu::ContextUrl& GetActiveUrl() {
 
 scoped_refptr<gpu::raster::GraphiteCacheController>
 GetOrCreateGraphiteCacheController(skgpu::graphite::Recorder* recorder) {
-#if DCHECK_IS_ON()
-  static base::ThreadChecker thread_checker;
-#endif
-  // This method is called on viz thread only.
-  DCHECK_CALLED_ON_VALID_THREAD(thread_checker);
-
-  // All SkiaOutputSurfaceImpl instances share one cache controller, and the
-  // controller will be released when all SkiaOutputSurfaceImpl instances are
-  // released, so we use WeakPtr here.
-  static base::WeakPtr<gpu::raster::GraphiteCacheController>
-      weak_controller_ptr;
-  if (weak_controller_ptr) {
-    return base::WrapRefCounted(weak_controller_ptr.get());
+  // All SkiaOutputSurfaceImpl instances on a thread share one cache controller,
+  // and the controller will be released when all SkiaOutputSurfaceImpl
+  // instances are released, so we use a sequence local WeakPtr here.
+  static base::SequenceLocalStorageSlot<
+      base::WeakPtr<gpu::raster::GraphiteCacheController>>
+      sls_weak_controller;
+  auto& weak_controller = sls_weak_controller.GetOrCreateValue();
+  if (weak_controller) {
+    return base::WrapRefCounted(weak_controller.get());
   }
-
-  auto controller = base::MakeRefCounted<gpu::raster::GraphiteCacheController>(
-      /*context=*/nullptr, recorder);
-  weak_controller_ptr = controller->GetWeakPtr();
-
+  auto controller =
+      base::MakeRefCounted<gpu::raster::GraphiteCacheController>(recorder);
+  weak_controller = controller->AsWeakPtr();
   return controller;
 }
 
@@ -1173,12 +1167,13 @@ void SkiaOutputSurfaceImpl::InitializeOnGpuThread(
   capabilities_ = impl_on_gpu_->capabilities();
   is_displayed_as_overlay_ = impl_on_gpu_->IsDisplayedAsOverlay();
 
-  gr_context_type_ = dependency_->GetSharedContextState()->gr_context_type();
-  if (auto* gr_context = dependency_->GetSharedContextState()->gr_context()) {
+  auto shared_context_state = dependency_->GetSharedContextState();
+  gr_context_type_ = shared_context_state->gr_context_type();
+  if (auto* gr_context = shared_context_state->gr_context()) {
     gr_context_thread_safe_ = gr_context->threadSafeProxy();
   }
-  graphite_recorder_ =
-      dependency_->GetSharedContextState()->viz_compositor_graphite_recorder();
+  graphite_recorder_ = shared_context_state->viz_compositor_graphite_recorder();
+
   *result = true;
 }
 

gpu/command_buffer/service/graphite_cache_controller.cc

@@ -16,22 +16,29 @@ constexpr base::TimeDelta kCleanupDelay = base::Seconds(5);
 }
 
 GraphiteCacheController::GraphiteCacheController(
-    skgpu::graphite::Context* context,
-    skgpu::graphite::Recorder* recorder)
-    : context_(context), recorder_(recorder) {
-  timer_ = std::make_unique<base::RetainingOneShotTimer>(
-      FROM_HERE, kCleanupDelay,
-      base::BindRepeating(&GraphiteCacheController::PerformCleanup,
-                          GetWeakPtr()));
+    skgpu::graphite::Recorder* recorder,
+    skgpu::graphite::Context* context)
+    : recorder_(recorder),
+      context_(context),
+      timer_(std::make_unique<base::RetainingOneShotTimer>(
+          FROM_HERE,
+          kCleanupDelay,
+          base::BindRepeating(&GraphiteCacheController::PerformCleanup,
+                              AsWeakPtr()))) {
+  DETACH_FROM_SEQUENCE(sequence_checker_);
 }
 
-GraphiteCacheController::~GraphiteCacheController() = default;
+GraphiteCacheController::~GraphiteCacheController() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+}
 
 void GraphiteCacheController::ScheduleCleanup() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   timer_->Reset();
 }
 
 void GraphiteCacheController::PerformCleanup() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   if (context_) {
     // TODO(crbug.com/1472451): cleanup resources in context_;
   }

gpu/command_buffer/service/graphite_cache_controller.h

@@ -7,9 +7,10 @@
 
 #include <memory>
 
+#include "base/memory/raw_ptr.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
-#include "base/task/single_thread_task_runner.h"
+#include "base/sequence_checker.h"
 #include "gpu/gpu_gles2_export.h"
 
 namespace base {
@@ -23,22 +24,22 @@ class Recorder;
 
 namespace gpu::raster {
 
+// GraphiteCacheController is not thread-safe; it can be created on any thread,
+// but it must be destroyed on the same thread that ScheduleCleanup is called.
 class GPU_GLES2_EXPORT GraphiteCacheController
-    : public base::RefCounted<GraphiteCacheController> {
+    : public base::RefCounted<GraphiteCacheController>,
+      public base::SupportsWeakPtr<GraphiteCacheController> {
  public:
-  // |context| and |recorder| are optional, GraphiteCacheController only purge
-  // resource in non-null |context| and |recorder|.
-  GraphiteCacheController(skgpu::graphite::Context* context,
-                          skgpu::graphite::Recorder* recorder);
+  // |recorder| and |context| are optional, GraphiteCacheController only purges
+  // resources in non-null |recorder| and |context|.
+  explicit GraphiteCacheController(skgpu::graphite::Recorder* recorder,
+                                   skgpu::graphite::Context* context = nullptr);
+
   GraphiteCacheController(const GraphiteCacheController&) = delete;
   GraphiteCacheController& operator=(const GraphiteCacheController&) = delete;
 
-  base::WeakPtr<GraphiteCacheController> GetWeakPtr() {
-    return weak_factory_.GetWeakPtr();
-  }
-
-  // Schedule cleanup for the graphite cache, the cleanup will be performed
-  // until ScheduleCleanup() is called for a while.
+  // Schedule cleanup for the graphite cache; the cleanup will be performed
+  // after ScheduleCleanup() is not called for a while after going idle.
   void ScheduleCleanup();
 
  private:
@@ -47,11 +48,11 @@ class GPU_GLES2_EXPORT GraphiteCacheController
 
   void PerformCleanup();
 
-  const raw_ptr<skgpu::graphite::Context> context_;
   const raw_ptr<skgpu::graphite::Recorder> recorder_;
+  const raw_ptr<skgpu::graphite::Context> context_;
   std::unique_ptr<base::RetainingOneShotTimer> timer_;
 
-  base::WeakPtrFactory<GraphiteCacheController> weak_factory_{this};
+  SEQUENCE_CHECKER(sequence_checker_);
 };
 
 }  // namespace gpu::raster

gpu/command_buffer/service/shared_context_state.cc

@@ -424,7 +424,7 @@ bool SharedContextState::InitializeGraphite(
       MakeGraphiteRecorderWithImageProvider(graphite_context_);
   gpu_main_graphite_cache_controller_ =
       base::MakeRefCounted<raster::GraphiteCacheController>(
-          graphite_context_.get(), gpu_main_graphite_recorder_.get());
+          gpu_main_graphite_recorder_.get(), graphite_context_.get());
 
   viz_compositor_graphite_recorder_ =
       MakeGraphiteRecorderWithImageProvider(graphite_context_);