Revert "SkiaRenderer: Fix subtle use after free with WeakPtr"

This reverts commit 2d5b633a.

Reason for revert: Causes this to fire: [1028:3416:1127/151100.479:FATAL:skia_output_surface_impl.cc(770)] Check failed: (thread_checker_).CalledOnValidThread(). 

Original change's description:
> SkiaRenderer: Fix subtle use after free with WeakPtr
> 
> SkiaOutputSurfaceImpl owns the SkiaOutputSurfaceDependency. After Impl
> goes out of scope, there is no more Dependency. We reference the
> Dependency when we call CreateSafe{Once,Repeating}Callback for things
> like presentation feedback callbacks. It is possible for presentation
> feedback to occur after Impl is gone, in which case we can no longer use
> Dependency.
> 
> Bug: 1020699
> Change-Id: Iedb37a444358e4c543afbb8d8da5907d6b290b7b
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1936889
> Commit-Queue: Jonathan Backer <backer@chromium.org>
> Reviewed-by: kylechar <kylechar@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#719744}

TBR=backer@chromium.org,kylechar@chromium.org

Change-Id: Ia092cdbd191648549381843080a01e53f53f1c31
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1020699
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1942342Reviewed-by: Corentin Wallez <cwallez@chromium.org>
Commit-Queue: Corentin Wallez <cwallez@chromium.org>
Cr-Commit-Position: refs/heads/master@{#719980}

components/viz/service/display_embedder/skia_output_surface_impl.cc

@@ -54,6 +54,40 @@ sk_sp<SkPromiseImageTexture> Fulfill(void* texture_context) {
 
 void DoNothing(void* texture_context) {}
 
+template <typename... Args>
+void PostAsyncTaskRepeatedly(
+    SkiaOutputSurfaceDependency* dependency,
+    const base::RepeatingCallback<void(Args...)>& callback,
+    Args... args) {
+  dependency->PostTaskToClientThread(base::BindOnce(callback, args...));
+}
+
+template <typename... Args>
+base::RepeatingCallback<void(Args...)> CreateSafeRepeatingCallback(
+    SkiaOutputSurfaceDependency* dependency,
+    const base::RepeatingCallback<void(Args...)>& callback) {
+  DCHECK(dependency);
+  return base::BindRepeating(&PostAsyncTaskRepeatedly<Args...>, dependency,
+                             callback);
+}
+
+template <typename... Args>
+void PostAsyncTaskOnce(SkiaOutputSurfaceDependency* dependency,
+                       base::OnceCallback<void(Args...)> callback,
+                       Args... args) {
+  dependency->PostTaskToClientThread(
+      base::BindOnce(std::move(callback), args...));
+}
+
+template <typename... Args>
+base::OnceCallback<void(Args...)> CreateSafeOnceCallback(
+    SkiaOutputSurfaceDependency* dependency,
+    base::OnceCallback<void(Args...)> callback) {
+  DCHECK(dependency);
+  return base::BindOnce(&PostAsyncTaskOnce<Args...>, dependency,
+                        std::move(callback));
+}
+
 gpu::ContextUrl& GetActiveUrl() {
   static base::NoDestructor<gpu::ContextUrl> active_url(
       GURL("chrome://gpu/SkiaRenderer"));
@@ -659,32 +693,19 @@ void SkiaOutputSurfaceImpl::InitializeOnGpuThread(base::WaitableEvent* event,
         base::BindOnce(&base::WaitableEvent::Signal, base::Unretained(event)));
   }
 
-  auto did_swap_buffer_complete_callback = base::BindRepeating(
-      &SkiaOutputSurfaceImpl::DidSwapBuffersComplete, weak_ptr_);
-  auto buffer_presented_callback =
-      base::BindRepeating(&SkiaOutputSurfaceImpl::BufferPresented, weak_ptr_);
-  auto context_lost_callback =
-      base::BindOnce(&SkiaOutputSurfaceImpl::ContextLost, weak_ptr_);
-
-  // This callback could be called from vsync or GPU thread after |this| is
-  // destroyed. We post directly to display compositor thread to check
-  // |weak_ptr_| as |dependency_| may have been destroyed.
-  GpuVSyncCallback gpu_vsync_callback =
-#if defined(OS_ANDROID)
-      // Callback is never used on Android. Doesn't work with WebView because
-      // calling it bypasses SkiaOutputSurfaceDependency.
-      base::DoNothing();
-#else
-      base::BindRepeating(
-          [](scoped_refptr<base::SingleThreadTaskRunner> runner,
-             base::WeakPtr<SkiaOutputSurfaceImpl> weak_ptr,
-             base::TimeTicks timebase, base::TimeDelta interval) {
-            runner->PostTask(FROM_HERE,
-                             base::BindOnce(&SkiaOutputSurfaceImpl::OnGpuVSync,
-                                            weak_ptr, timebase, interval));
-          },
-          base::ThreadTaskRunnerHandle::Get(), weak_ptr_);
-#endif
+  auto did_swap_buffer_complete_callback = CreateSafeRepeatingCallback(
+      dependency_.get(),
+      base::BindRepeating(&SkiaOutputSurfaceImpl::DidSwapBuffersComplete,
+                          weak_ptr_));
+  auto buffer_presented_callback = CreateSafeRepeatingCallback(
+      dependency_.get(),
+      base::BindRepeating(&SkiaOutputSurfaceImpl::BufferPresented, weak_ptr_));
+  auto context_lost_callback = CreateSafeOnceCallback(
+      dependency_.get(),
+      base::BindOnce(&SkiaOutputSurfaceImpl::ContextLost, weak_ptr_));
+  auto gpu_vsync_callback = CreateSafeRepeatingCallback(
+      dependency_.get(),
+      base::BindRepeating(&SkiaOutputSurfaceImpl::OnGpuVSync, weak_ptr_));
 
   impl_on_gpu_ = SkiaOutputSurfaceImplOnGpu::Create(
       dependency_.get(), renderer_settings_, task_sequence_->GetSequenceId(),

components/viz/service/display_embedder/skia_output_surface_impl_on_gpu.cc

@@ -84,25 +84,6 @@ namespace viz {
 
 namespace {
 
-template <typename... Args>
-void PostAsyncTaskRepeatedly(
-    base::WeakPtr<SkiaOutputSurfaceImplOnGpu> impl_on_gpu,
-    const base::RepeatingCallback<void(Args...)>& callback,
-    Args... args) {
-  // Callbacks generated by this function may be executed asynchronously
-  // (e.g. by presentation feedback) after |impl_on_gpu| has been destroyed.
-  if (impl_on_gpu)
-    impl_on_gpu->PostTaskToClientThread(base::BindOnce(callback, args...));
-}
-
-template <typename... Args>
-base::RepeatingCallback<void(Args...)> CreateSafeRepeatingCallback(
-    base::WeakPtr<SkiaOutputSurfaceImplOnGpu> impl_on_gpu,
-    const base::RepeatingCallback<void(Args...)>& callback) {
-  return base::BindRepeating(&PostAsyncTaskRepeatedly<Args...>, impl_on_gpu,
-                             callback);
-}
-
 struct ReadPixelsContext {
   ReadPixelsContext(std::unique_ptr<CopyOutputRequest> request,
                     const gfx::Rect& result_rect)
@@ -664,18 +645,14 @@ SkiaOutputSurfaceImplOnGpu::SkiaOutputSurfaceImplOnGpu(
       dawn_context_provider_(dependency_->GetDawnContextProvider()),
       renderer_settings_(renderer_settings),
       sequence_id_(sequence_id),
+      did_swap_buffer_complete_callback_(
+          std::move(did_swap_buffer_complete_callback)),
+      buffer_presented_callback_(std::move(buffer_presented_callback)),
       context_lost_callback_(std::move(context_lost_callback)),
       gpu_vsync_callback_(std::move(gpu_vsync_callback)),
       gpu_preferences_(dependency_->GetGpuPreferences()),
       copier_active_url_(GURL("chrome://gpu/SkiaRendererGLRendererCopier")) {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
-
-  weak_ptr_ = weak_ptr_factory_.GetWeakPtr();
-  did_swap_buffer_complete_callback_ = CreateSafeRepeatingCallback(
-      weak_ptr_, std::move(did_swap_buffer_complete_callback));
-  buffer_presented_callback_ = CreateSafeRepeatingCallback(
-      weak_ptr_, std::move(buffer_presented_callback));
-
   dependency_->RegisterDisplayContext(this);
 }
 
@@ -1320,6 +1297,7 @@ bool SkiaOutputSurfaceImplOnGpu::Initialize() {
   TRACE_EVENT1("viz", "SkiaOutputSurfaceImplOnGpu::Initialize",
                "is_using_vulkan", is_using_vulkan());
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+  weak_ptr_ = weak_ptr_factory_.GetWeakPtr();
 #if defined(USE_OZONE)
   window_surface_ =
       ui::OzonePlatform::GetInstance()
@@ -1613,7 +1591,7 @@ void SkiaOutputSurfaceImplOnGpu::RenderToOverlay(
 void SkiaOutputSurfaceImplOnGpu::MarkContextLost() {
   context_state_->MarkContextLost();
   if (context_lost_callback_) {
-    PostTaskToClientThread(std::move(context_lost_callback_));
+    std::move(context_lost_callback_).Run();
     if (context_provider_)
       context_provider_->MarkContextLost();
   }

components/viz/service/display_embedder/skia_output_surface_impl_on_gpu.h

@@ -21,7 +21,6 @@
 #include "components/viz/service/display/output_surface_frame.h"
 #include "components/viz/service/display/overlay_processor.h"
 #include "components/viz/service/display_embedder/skia_output_device.h"
-#include "components/viz/service/display_embedder/skia_output_surface_dependency.h"
 #include "gpu/command_buffer/common/mailbox.h"
 #include "gpu/command_buffer/common/sync_token.h"
 #include "gpu/command_buffer/service/shared_context_state.h"
@@ -65,6 +64,7 @@ class DawnContextProvider;
 class DirectContextProvider;
 class GLRendererCopier;
 class ImageContextImpl;
+class SkiaOutputSurfaceDependency;
 class TextureDeleter;
 class VulkanContextProvider;
 
@@ -84,8 +84,6 @@ class SkiaOutputSurfaceImplOnGpu : public gpu::ImageTransportSurfaceDelegate,
       base::RepeatingCallback<void(const gfx::PresentationFeedback& feedback)>;
   using ContextLostCallback = base::OnceClosure;
 
-  // |gpu_vsync_callback| must be safe to call on any thread. The other
-  // callbacks will only be called via |deps->PostTaskToClientThread|.
   static std::unique_ptr<SkiaOutputSurfaceImplOnGpu> Create(
       SkiaOutputSurfaceDependency* deps,
       const RendererSettings& renderer_settings,
@@ -203,10 +201,6 @@ class SkiaOutputSurfaceImplOnGpu : public gpu::ImageTransportSurfaceDelegate,
   // gpu::DisplayContext implementation:
   void MarkContextLost() override;
 
-  void PostTaskToClientThread(base::OnceClosure closure) {
-    dependency_->PostTaskToClientThread(std::move(closure));
-  }
-
  private:
   class ScopedPromiseImageAccess;
 
@@ -254,10 +248,10 @@ class SkiaOutputSurfaceImplOnGpu : public gpu::ImageTransportSurfaceDelegate,
   // readback using GLRendererCopier.
   // TODO(samans): Remove |sequence_id| once readback always uses Skia.
   const gpu::SequenceId sequence_id_;
-  DidSwapBufferCompleteCallback did_swap_buffer_complete_callback_;
-  BufferPresentedCallback buffer_presented_callback_;
+  const DidSwapBufferCompleteCallback did_swap_buffer_complete_callback_;
+  const BufferPresentedCallback buffer_presented_callback_;
   ContextLostCallback context_lost_callback_;
-  GpuVSyncCallback gpu_vsync_callback_;
+  const GpuVSyncCallback gpu_vsync_callback_;
 
 #if defined(USE_OZONE)
   // This should outlive gl_surface_ and vulkan_surface_.