Fix *StaticBitmapImage ThreadChecker and unaccelerated SkImage destroy

- AcceleratedStaticBitmapImage was misusing ThreadChecker by having its
own detach logic. Using proper DetachThread is simpler, cleaner and
correct.
- UnacceleratedStaticBitmapImage didn't destroy the SkImage in the
proper thread, leading to GrContext/SkSp problems.

Bug: 890576
Change-Id: Ic71e7f7322b0b851774628247aa5256664bc0723
Reviewed-on: https://chromium-review.googlesource.com/c/1307775Reviewed-by: Gabriel Charette <gab@chromium.org>
Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Commit-Queue: Fernando Serboncini <fserb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#604427}

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

@@ -53,7 +53,6 @@ AcceleratedStaticBitmapImage::AcceleratedStaticBitmapImage(
   CHECK(image && image->isTextureBacked());
   texture_holder_ = std::make_unique<SkiaTextureHolder>(
       std::move(image), std::move(context_provider_wrapper));
-  thread_checker_.DetachFromThread();
 }
 
 AcceleratedStaticBitmapImage::AcceleratedStaticBitmapImage(
@@ -67,7 +66,6 @@ AcceleratedStaticBitmapImage::AcceleratedStaticBitmapImage(
   texture_holder_ = std::make_unique<MailboxTextureHolder>(
       mailbox, sync_token, texture_id, std::move(context_provider_wrapper),
       mailbox_size);
-  thread_checker_.DetachFromThread();
 }
 
 namespace {
@@ -89,20 +87,21 @@ void DestroySkImageOnOriginalThread(
     // In case texture was used by compositor, which may have changed params.
     image->getTexture()->textureParamsModified();
   }
-  // destroy by letting |image| go out of scope
+  image.reset();
 }
 
 }  // unnamed namespace
 
 AcceleratedStaticBitmapImage::~AcceleratedStaticBitmapImage() {
+  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+
   // If the original SkImage was retained, it must be destroyed on the thread
   // where it came from. In the same thread case, there is nothing to do because
   // the regular destruction flow is fine.
   if (original_skia_image_) {
     std::unique_ptr<gpu::SyncToken> sync_token =
         base::WrapUnique(new gpu::SyncToken(texture_holder_->GetSyncToken()));
-    if (original_skia_image_thread_id_ !=
-        Platform::Current()->CurrentThread()->ThreadId()) {
+    if (!original_skia_image_task_runner_->BelongsToCurrentThread()) {
       PostCrossThreadTask(
           *original_skia_image_task_runner_, FROM_HERE,
           CrossThreadBind(
@@ -119,12 +118,14 @@ AcceleratedStaticBitmapImage::~AcceleratedStaticBitmapImage() {
 }
 
 void AcceleratedStaticBitmapImage::RetainOriginalSkImage() {
+  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+
   DCHECK(texture_holder_->IsSkiaTextureHolder());
   original_skia_image_ = texture_holder_->GetSkImage();
   original_skia_image_context_provider_wrapper_ = ContextProviderWrapper();
   DCHECK(original_skia_image_);
+
   Thread* thread = Platform::Current()->CurrentThread();
-  original_skia_image_thread_id_ = thread->ThreadId();
   original_skia_image_task_runner_ = thread->GetTaskRunner();
 }
 
@@ -151,7 +152,7 @@ bool AcceleratedStaticBitmapImage::CopyToTexture(
     bool unpack_flip_y,
     const IntPoint& dest_point,
     const IntRect& source_sub_rectangle) {
-  CheckThread();
+  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
   if (!IsValid())
     return false;
   // This method should only be used for cross-context copying, otherwise it's
@@ -191,14 +192,13 @@ bool AcceleratedStaticBitmapImage::CopyToTexture(
 PaintImage AcceleratedStaticBitmapImage::PaintImageForCurrentFrame() {
   // TODO(ccameron): This function should not ignore |colorBehavior|.
   // https://crbug.com/672306
-  CheckThread();
+  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
   if (!IsValid())
     return PaintImage();
 
   sk_sp<SkImage> image;
   if (original_skia_image_ &&
-      original_skia_image_thread_id_ ==
-          Platform::Current()->CurrentThread()->ThreadId()) {
+      original_skia_image_task_runner_->BelongsToCurrentThread()) {
     // We need to avoid consuming the mailbox in the context where it
     // originated.  This avoids swapping back and forth between TextureHolder
     // types.
@@ -221,6 +221,7 @@ void AcceleratedStaticBitmapImage::Draw(cc::PaintCanvas* canvas,
                                         RespectImageOrientationEnum,
                                         ImageClampingMode image_clamping_mode,
                                         ImageDecodingMode decode_mode) {
+  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
   auto paint_image = PaintImageForCurrentFrame();
   if (!paint_image)
     return;
@@ -253,6 +254,7 @@ AcceleratedStaticBitmapImage::ContextProviderWrapper() const {
 }
 
 void AcceleratedStaticBitmapImage::CreateImageFromMailboxIfNeeded() {
+  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
   if (texture_holder_->IsSkiaTextureHolder())
     return;
   texture_holder_ =
@@ -261,6 +263,7 @@ void AcceleratedStaticBitmapImage::CreateImageFromMailboxIfNeeded() {
 
 void AcceleratedStaticBitmapImage::EnsureMailbox(MailboxSyncMode mode,
                                                  GLenum filter) {
+  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
   if (!texture_holder_->IsMailboxTextureHolder()) {
     TRACE_EVENT0("blink", "AcceleratedStaticBitmapImage::EnsureMailbox");
 
@@ -278,24 +281,17 @@ void AcceleratedStaticBitmapImage::EnsureMailbox(MailboxSyncMode mode,
 }
 
 void AcceleratedStaticBitmapImage::Transfer() {
-  CheckThread();
+  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
   EnsureMailbox(kVerifiedSyncToken, GL_NEAREST);
-  detach_thread_at_next_check_ = true;
+  DETACH_FROM_THREAD(thread_checker_);
 }
 
 bool AcceleratedStaticBitmapImage::CurrentFrameKnownToBeOpaque() {
   return texture_holder_->CurrentFrameKnownToBeOpaque();
 }
 
-void AcceleratedStaticBitmapImage::CheckThread() {
-  if (detach_thread_at_next_check_) {
-    thread_checker_.DetachFromThread();
-    detach_thread_at_next_check_ = false;
-  }
-  CHECK(thread_checker_.CalledOnValidThread());
-}
-
 void AcceleratedStaticBitmapImage::Abandon() {
+  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
   texture_holder_->Abandon();
 }
 

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

@@ -104,20 +104,17 @@ class PLATFORM_EXPORT AcceleratedStaticBitmapImage final
       IntSize mailbox_size);
 
   void CreateImageFromMailboxIfNeeded();
-  void CheckThread();
   void WaitSyncTokenIfNeeded();
   void RetainOriginalSkImage();
 
   std::unique_ptr<TextureHolder> texture_holder_;
 
-  base::ThreadChecker thread_checker_;
-  bool detach_thread_at_next_check_ = false;
+  THREAD_CHECKER(thread_checker_);
   PaintImage::ContentId paint_image_content_id_;
 
   // For RetainOriginalSkImageForCopyOnWrite()
   sk_sp<SkImage> original_skia_image_;
   scoped_refptr<base::SingleThreadTaskRunner> original_skia_image_task_runner_;
-  PlatformThreadId original_skia_image_thread_id_;
   base::WeakPtr<WebGraphicsContext3DProviderWrapper>
       original_skia_image_context_provider_wrapper_;
 };

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

@@ -5,9 +5,13 @@
 #include "third_party/blink/renderer/platform/graphics/unaccelerated_static_bitmap_image.h"
 
 #include "components/viz/common/gpu/context_provider.h"
+#include "third_party/blink/public/platform/platform.h"
 #include "third_party/blink/public/platform/web_graphics_context_3d_provider.h"
+#include "third_party/blink/renderer/platform/cross_thread_functional.h"
 #include "third_party/blink/renderer/platform/graphics/accelerated_static_bitmap_image.h"
 #include "third_party/blink/renderer/platform/graphics/web_graphics_context_3d_provider_wrapper.h"
+#include "third_party/blink/renderer/platform/scheduler/public/thread.h"
+#include "third_party/blink/renderer/platform/web_task_runner.h"
 #include "third_party/skia/include/core/SkImage.h"
 
 namespace blink {
@@ -22,7 +26,6 @@ UnacceleratedStaticBitmapImage::UnacceleratedStaticBitmapImage(
     sk_sp<SkImage> image) {
   CHECK(image);
   DCHECK(!image->isLazyGenerated());
-
   paint_image_ =
       CreatePaintImageBuilder()
           .set_image(std::move(image), cc::PaintImage::GetNextContentId())
@@ -39,7 +42,20 @@ UnacceleratedStaticBitmapImage::UnacceleratedStaticBitmapImage(PaintImage image)
   CHECK(paint_image_.GetSkImage());
 }
 
-UnacceleratedStaticBitmapImage::~UnacceleratedStaticBitmapImage() = default;
+UnacceleratedStaticBitmapImage::~UnacceleratedStaticBitmapImage() {
+  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+  if (!original_skia_image_)
+    return;
+
+  if (!original_skia_image_task_runner_->BelongsToCurrentThread()) {
+    PostCrossThreadTask(
+        *original_skia_image_task_runner_, FROM_HERE,
+        CrossThreadBind([](sk_sp<SkImage> image) { image.reset(); },
+                        std::move(original_skia_image_)));
+  } else {
+    original_skia_image_.reset();
+  }
+}
 
 IntSize UnacceleratedStaticBitmapImage::Size() const {
   return IntSize(paint_image_.width(), paint_image_.height());
@@ -53,6 +69,8 @@ bool UnacceleratedStaticBitmapImage::IsPremultiplied() const {
 scoped_refptr<StaticBitmapImage>
 UnacceleratedStaticBitmapImage::MakeAccelerated(
     base::WeakPtr<WebGraphicsContext3DProviderWrapper> context_wrapper) {
+  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+
   if (!context_wrapper)
     return nullptr;  // Can happen if the context is lost.
 
@@ -81,6 +99,7 @@ void UnacceleratedStaticBitmapImage::Draw(cc::PaintCanvas* canvas,
                                           RespectImageOrientationEnum,
                                           ImageClampingMode clamp_mode,
                                           ImageDecodingMode) {
+  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
   StaticBitmapImage::DrawHelper(canvas, flags, dst_rect, src_rect, clamp_mode,
                                 PaintImageForCurrentFrame());
 }
@@ -89,4 +108,12 @@ PaintImage UnacceleratedStaticBitmapImage::PaintImageForCurrentFrame() {
   return paint_image_;
 }
 
+void UnacceleratedStaticBitmapImage::Transfer() {
+  DETACH_FROM_THREAD(thread_checker_);
+
+  original_skia_image_ = paint_image_.GetSkImage();
+  Thread* thread = Platform::Current()->CurrentThread();
+  original_skia_image_task_runner_ = thread->GetTaskRunner();
+}
+
 }  // namespace blink

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

@@ -6,7 +6,10 @@
 #define THIRD_PARTY_BLINK_RENDERER_PLATFORM_GRAPHICS_UNACCELERATED_STATIC_BITMAP_IMAGE_H_
 
 #include "base/memory/weak_ptr.h"
+#include "base/single_thread_task_runner.h"
+#include "base/threading/thread_checker.h"
 #include "third_party/blink/renderer/platform/graphics/static_bitmap_image.h"
+#include "third_party/blink/renderer/platform/scheduler/public/thread.h"
 
 namespace blink {
 
@@ -34,11 +37,17 @@ class PLATFORM_EXPORT UnacceleratedStaticBitmapImage final
 
   PaintImage PaintImageForCurrentFrame() override;
 
+  void Transfer() final;
+
  private:
   UnacceleratedStaticBitmapImage(sk_sp<SkImage>);
   UnacceleratedStaticBitmapImage(PaintImage);
 
   PaintImage paint_image_;
+  THREAD_CHECKER(thread_checker_);
+
+  sk_sp<SkImage> original_skia_image_;
+  scoped_refptr<base::SingleThreadTaskRunner> original_skia_image_task_runner_;
 };
 
 }  // namespace blink