Fix data races when returning OffscreenCanvas placeholder frames.

Before this change, it was possible for the placeholder canvas to
return a CanvasResource object to the canvas resource dispatcher
when there are still outstanding references to it.  With this
change, the ref count falling to zero is used as a trigger for
returning the resource, which guarantees that a resource will
not be destroyed or recycled while there are still pending
accesses to its data.

Bug: 1348616
Change-Id: I6c4018bf50b03b699926cbffb71e5973eab5d9ab
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3826420
Reviewed-by: Fernando Serboncini <fserb@chromium.org>
Commit-Queue: Justin Novosad <junov@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1035103}

third_party/blink/renderer/modules/canvas/offscreencanvas/offscreen_canvas_test.cc

@@ -149,7 +149,7 @@ TEST_P(OffscreenCanvasTest, CompositorFrameOpacity) {
 
   const bool context_alpha = GetParam().alpha;
 
-  const auto canvas_resource = CanvasResourceSharedBitmap::Create(
+  auto canvas_resource = CanvasResourceSharedBitmap::Create(
       SkImageInfo::MakeN32Premul(offscreen_canvas().Size().width(),
                                  offscreen_canvas().Size().height()),
       nullptr /* provider */, cc::PaintFlags::FilterQuality::kLow);
@@ -175,7 +175,7 @@ TEST_P(OffscreenCanvasTest, CompositorFrameOpacity) {
                                SkIRect::MakeWH(10, 10));
   platform->RunUntilIdle();
 
-  const auto canvas_resource2 = CanvasResourceSharedBitmap::Create(
+  auto canvas_resource2 = CanvasResourceSharedBitmap::Create(
       SkImageInfo::MakeN32Premul(offscreen_canvas().Size().width(),
                                  offscreen_canvas().Size().height()),
       nullptr /* provider */, cc::PaintFlags::FilterQuality::kLow);

third_party/blink/renderer/platform/BUILD.gn

@@ -2625,6 +2625,7 @@ source_set("unit_tests") {
     "graphics/gpu/drawing_buffer_software_rendering_test.cc",
     "graphics/image_decoding_store_test.cc",
     "graphics/image_frame_generator_test.cc",
+    "graphics/offscreen_canvas_placeholder_test.cc",
     "graphics/paint_worklet_paint_dispatcher_test.cc",
     "graphics/test/stub_image.h",
 

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

@@ -71,6 +71,25 @@ void CanvasResource::OnDestroy() {
 #endif
 }
 
+void CanvasResource::Release() {
+  if (last_unref_callback_ && HasOneRef()) {
+    // "this" will not be destroyed if last_unref_callback_ retains the
+    // reference.
+#if DCHECK_IS_ON()
+    auto last_ref = base::WrapRefCounted(this);
+    WTF::ThreadSafeRefCounted<CanvasResource>::Release();  // does not destroy.
+#else
+    // In a DCHECK build, AdoptRef would fail because it is only supposed to be
+    // used on new objects.  Nonetheless, we prefer to use AdoptRef "illegally"
+    // in non-DCHECK builds to avoid unnecessary atomic operations.
+    auto last_ref = base::AdoptRef(this);
+#endif
+    std::move(last_unref_callback_).Run(std::move(last_ref));
+  } else {
+    WTF::ThreadSafeRefCounted<CanvasResource>::Release();
+  }
+}
+
 gpu::InterfaceBase* CanvasResource::InterfaceBase() const {
   if (!ContextProviderWrapper())
     return nullptr;
@@ -120,7 +139,8 @@ static void ReleaseFrameResources(
   // resource.
   if (lost_resource)
     resource->NotifyResourceLost();
-  if (resource_provider && !lost_resource && resource->IsRecycleable())
+  if (resource_provider && !lost_resource && resource->IsRecycleable() &&
+      resource->HasOneRef())
     resource_provider->RecycleResource(std::move(resource));
 }
 
@@ -1073,11 +1093,19 @@ scoped_refptr<ExternalCanvasResource> ExternalCanvasResource::Create(
 }
 
 ExternalCanvasResource::~ExternalCanvasResource() {
+  // Should always be destroyed on thread of origin.
+  DCHECK(!is_cross_thread());
   OnDestroy();
 }
 
 bool ExternalCanvasResource::IsValid() const {
-  return context_provider_wrapper_ && HasGpuMailbox();
+  // On same thread we need to make sure context was not dropped, but
+  // in the cross-thread case, checking a WeakPtr in not thread safe, not
+  // to mention that we will use a shared context rather than the context
+  // of origin to access the resource. In that case we will find out
+  // whether the resource was dropped later, when we attempt to access the
+  // mailbox.
+  return (is_cross_thread() || context_provider_wrapper_) && HasGpuMailbox();
 }
 
 void ExternalCanvasResource::Abandon() {
@@ -1150,6 +1178,8 @@ const gpu::SyncToken ExternalCanvasResource::GetSyncToken() {
 
 base::WeakPtr<WebGraphicsContext3DProviderWrapper>
 ExternalCanvasResource::ContextProviderWrapper() const {
+  // The context provider is not thread-safe, nor is the WeakPtr that holds it.
+  DCHECK(!is_cross_thread());
   return context_provider_wrapper_;
 }
 

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

@@ -70,12 +70,29 @@ class PLATFORM_EXPORT CanvasResource
       const gpu::SyncToken& sync_token,
       bool is_lost)>;
 
+  using LastUnrefCallback = base::OnceCallback<void(
+      scoped_refptr<blink::CanvasResource> canvas_resource)>;
+
   virtual ~CanvasResource();
 
+  // Non-virtual override of ThreadSafeRefCounted::Release
+  void Release();
+
+  // Set a callback that will be invoked as the last outstanding reference to
+  // this CanvasResource goes out of scope.  This provides a last chance hook
+  // to intercept a canvas before it get destroyed. For resources that need to
+  // be destroyed on their thread of origin, this hook can be used to return
+  // resources to their creators.
+  void SetLastUnrefCallback(LastUnrefCallback callback) {
+    last_unref_callback_ = std::move(callback);
+  }
+
   // We perform a lazy copy on write if the canvas content needs to be updated
   // while its current resource is in use. In order to avoid re-allocating
   // resources, its preferable to reuse a resource if its no longer in use.
-  // This API indicates whether a resource can be recycled.
+  // This API indicates whether a resource can be recycled.  This method does
+  // not however check whether the resource is still in use (e.g. has
+  // outstanding references).
   virtual bool IsRecycleable() const = 0;
 
   // Returns true if rendering to the resource is accelerated.
@@ -231,6 +248,7 @@ class PLATFORM_EXPORT CanvasResource
   base::WeakPtr<CanvasResourceProvider> provider_;
   SkColorInfo info_;
   cc::PaintFlags::FilterQuality filter_quality_;
+  LastUnrefCallback last_unref_callback_;
 #if DCHECK_IS_ON()
   bool did_call_on_destroy_ = false;
 #endif

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

@@ -63,7 +63,9 @@ class PLATFORM_EXPORT CanvasResourceDispatcher
                      const SkIRect& damage_rect,
                      bool needs_vertical_flip,
                      bool is_opaque);
-  void ReclaimResource(viz::ResourceId, scoped_refptr<CanvasResource>&&);
+  // virtual for mocking
+  virtual void ReclaimResource(viz::ResourceId,
+                               scoped_refptr<CanvasResource>&&);
   void DispatchFrameSync(scoped_refptr<CanvasResource>&&,
                          base::TimeTicks commit_start_time,
                          const SkIRect& damage_rect,
@@ -95,6 +97,7 @@ class PLATFORM_EXPORT CanvasResourceDispatcher
   void SetPlaceholderCanvasDispatcher(int placeholder_canvas_id);
 
  private:
+  friend class OffscreenCanvasPlaceholderTest;
   friend class CanvasResourceDispatcherTest;
   struct FrameResource;
 

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

@@ -56,14 +56,53 @@ OffscreenCanvasPlaceholder::~OffscreenCanvasPlaceholder() {
   UnregisterPlaceholderCanvas();
 }
 
+namespace {
+
+// This function gets called when the last outstanding reference to a
+// CanvasResource is released.  This callback is only registered on
+// resources received via SetOffscreenCanvasResource(). When the resource
+// is received, its ref count may be 2 because the CanvasResourceProvider
+// that created it may be holding a cached snapshot that will be released when
+// copy-on-write kicks in. This is okay even if the resource provider is on a
+// different thread because concurrent read access is safe. By the time the
+// next frame is received by OffscreenCanvasPlaceholder, the reference held by
+// CanvasResourceProvider will have been released (otherwise there wouldn't be
+// a new frame). This means that all outstanding references are held on the
+// same thread as the OffscreenCanvasPlaceholder at the time when
+// 'placeholder_frame_' is assigned a new value.  Therefore, when the last
+// reference is released, we need to temporarily keep the object alive and send
+// it back to its thread of origin, where it can be safely destroyed or
+// recycled.
+void FrameLastUnrefCallback(
+    base::WeakPtr<CanvasResourceDispatcher> frame_dispatcher,
+    scoped_refptr<base::SingleThreadTaskRunner> frame_dispatcher_task_runner,
+    viz::ResourceId placeholder_frame_resource_id,
+    scoped_refptr<CanvasResource> placeholder_frame) {
+  DCHECK(placeholder_frame);
+  DCHECK(placeholder_frame->HasOneRef());
+  DCHECK(frame_dispatcher_task_runner);
+  placeholder_frame->Transfer();
+  PostCrossThreadTask(
+      *frame_dispatcher_task_runner, FROM_HERE,
+      CrossThreadBindOnce(ReleaseFrameToDispatcher, frame_dispatcher,
+                          std::move(placeholder_frame),
+                          placeholder_frame_resource_id));
+}
+
+}  // unnamed namespace
+
 void OffscreenCanvasPlaceholder::SetOffscreenCanvasResource(
     scoped_refptr<CanvasResource>&& new_frame,
     viz::ResourceId resource_id) {
   DCHECK(IsOffscreenCanvasRegistered());
   DCHECK(new_frame);
-  ReleaseOffscreenCanvasFrame();
+  // The following implicitly returns placeholder_frame_ to its
+  // CanvasResourceDispatcher, via FrameLastUnrefCallback if it was
+  // the last outstanding reference on this thread.
   placeholder_frame_ = std::move(new_frame);
-  placeholder_frame_resource_id_ = resource_id;
+  placeholder_frame_->SetLastUnrefCallback(
+      base::BindOnce(FrameLastUnrefCallback, frame_dispatcher_,
+                     frame_dispatcher_task_runner_, resource_id));
 
   if (animation_state_ == kShouldSuspendAnimation) {
     bool success = PostSetSuspendAnimationToOffscreenCanvasThread(true);
@@ -92,20 +131,6 @@ void OffscreenCanvasPlaceholder::SetOffscreenCanvasDispatcher(
   }
 }
 
-void OffscreenCanvasPlaceholder::ReleaseOffscreenCanvasFrame() {
-  DCHECK(IsOffscreenCanvasRegistered());
-  if (!placeholder_frame_)
-    return;
-
-  DCHECK(frame_dispatcher_task_runner_);
-  placeholder_frame_->Transfer();
-  PostCrossThreadTask(
-      *frame_dispatcher_task_runner_, FROM_HERE,
-      CrossThreadBindOnce(ReleaseFrameToDispatcher, frame_dispatcher_,
-                          std::move(placeholder_frame_),
-                          placeholder_frame_resource_id_));
-}
-
 void OffscreenCanvasPlaceholder::UpdateOffscreenCanvasFilterQuality(
     cc::PaintFlags::FilterQuality filter_quality) {
   DCHECK(IsOffscreenCanvasRegistered());

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

@@ -31,8 +31,6 @@ class PLATFORM_EXPORT OffscreenCanvasPlaceholder {
       base::WeakPtr<CanvasResourceDispatcher>,
       scoped_refptr<base::SingleThreadTaskRunner>);
 
-  void ReleaseOffscreenCanvasFrame();
-
   void SetSuspendOffscreenCanvasAnimation(bool);
 
   static OffscreenCanvasPlaceholder* GetPlaceholderCanvasById(
@@ -60,7 +58,6 @@ class PLATFORM_EXPORT OffscreenCanvasPlaceholder {
   scoped_refptr<CanvasResource> placeholder_frame_;
   base::WeakPtr<CanvasResourceDispatcher> frame_dispatcher_;
   scoped_refptr<base::SingleThreadTaskRunner> frame_dispatcher_task_runner_;
-  viz::ResourceId placeholder_frame_resource_id_ = viz::kInvalidResourceId;
 
   enum {
     kNoPlaceholderId = -1,