Reland: 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: I54a13f1d65c2b2e8603883457bdd2e1195a3f9ac
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1943047
Commit-Queue: Jonathan Backer <backer@chromium.org>
Reviewed-by: kylechar <kylechar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#720081}

components/viz/service/display_embedder/skia_output_surface_impl.cc

@@ -54,40 +54,6 @@ 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"));
@@ -672,43 +638,60 @@ bool SkiaOutputSurfaceImpl::Initialize() {
   task_sequence_ = dependency_->CreateSequence();
 
   weak_ptr_ = weak_ptr_factory_.GetWeakPtr();
+
+  // This runner 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 vsync_callback_runner =
+#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
+
   base::WaitableEvent event(base::WaitableEvent::ResetPolicy::MANUAL,
                             base::WaitableEvent::InitialState::NOT_SIGNALED);
   bool result = false;
   auto callback = base::BindOnce(&SkiaOutputSurfaceImpl::InitializeOnGpuThread,
-                                 base::Unretained(this), &event, &result);
+                                 base::Unretained(this), vsync_callback_runner,
+                                 &event, &result);
   ScheduleGpuTask(std::move(callback), {});
   event.Wait();
   return result;
 }
 
-void SkiaOutputSurfaceImpl::InitializeOnGpuThread(base::WaitableEvent* event,
-                                                  bool* result) {
+void SkiaOutputSurfaceImpl::InitializeOnGpuThread(
+    GpuVSyncCallback vsync_callback_runner,
+    base::WaitableEvent* event,
+    bool* result) {
   base::Optional<base::ScopedClosureRunner> scoped_runner;
   if (event) {
     scoped_runner.emplace(
         base::BindOnce(&base::WaitableEvent::Signal, base::Unretained(event)));
   }
 
-  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_));
+  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_);
 
   impl_on_gpu_ = SkiaOutputSurfaceImplOnGpu::Create(
       dependency_.get(), renderer_settings_, task_sequence_->GetSequenceId(),
       std::move(did_swap_buffer_complete_callback),
       std::move(buffer_presented_callback), std::move(context_lost_callback),
-      std::move(gpu_vsync_callback));
+      std::move(vsync_callback_runner));
   if (!impl_on_gpu_) {
     *result = false;
   } else {

components/viz/service/display_embedder/skia_output_surface_impl.h

@@ -161,7 +161,9 @@ class VIZ_SERVICE_EXPORT SkiaOutputSurfaceImpl : public SkiaOutputSurface {
 
  private:
   bool Initialize();
-  void InitializeOnGpuThread(base::WaitableEvent* event, bool* result);
+  void InitializeOnGpuThread(GpuVSyncCallback vsync_callback_runner,
+                             base::WaitableEvent* event,
+                             bool* result);
   SkSurfaceCharacterization CreateSkSurfaceCharacterization(
       const gfx::Size& surface_size,
       ResourceFormat format,

components/viz/service/display_embedder/skia_output_surface_impl_on_gpu.cc

@@ -84,6 +84,25 @@ 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)
@@ -645,14 +664,18 @@ 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);
 }
 
@@ -1297,7 +1320,6 @@ 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()
@@ -1591,7 +1613,7 @@ void SkiaOutputSurfaceImplOnGpu::RenderToOverlay(
 void SkiaOutputSurfaceImplOnGpu::MarkContextLost() {
   context_state_->MarkContextLost();
   if (context_lost_callback_) {
-    std::move(context_lost_callback_).Run();
+    PostTaskToClientThread(std::move(context_lost_callback_));
     if (context_provider_)
       context_provider_->MarkContextLost();
   }

components/viz/service/display_embedder/skia_output_surface_impl_on_gpu.h

@@ -21,6 +21,7 @@
 #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"
@@ -64,7 +65,6 @@ class DawnContextProvider;
 class DirectContextProvider;
 class GLRendererCopier;
 class ImageContextImpl;
-class SkiaOutputSurfaceDependency;
 class TextureDeleter;
 class VulkanContextProvider;
 
@@ -86,6 +86,8 @@ 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,
@@ -201,6 +203,10 @@ class SkiaOutputSurfaceImplOnGpu : public gpu::ImageTransportSurfaceDelegate,
   // gpu::DisplayContext implementation:
   void MarkContextLost() override;
 
+  void PostTaskToClientThread(base::OnceClosure closure) {
+    dependency_->PostTaskToClientThread(std::move(closure));
+  }
+
  private:
   class ScopedPromiseImageAccess;
 
@@ -268,10 +274,10 @@ class SkiaOutputSurfaceImplOnGpu : public gpu::ImageTransportSurfaceDelegate,
   // readback using GLRendererCopier.
   // TODO(samans): Remove |sequence_id| once readback always uses Skia.
   const gpu::SequenceId sequence_id_;
-  const DidSwapBufferCompleteCallback did_swap_buffer_complete_callback_;
-  const BufferPresentedCallback buffer_presented_callback_;
+  DidSwapBufferCompleteCallback did_swap_buffer_complete_callback_;
+  BufferPresentedCallback buffer_presented_callback_;
   ContextLostCallback context_lost_callback_;
-  const GpuVSyncCallback gpu_vsync_callback_;
+  GpuVSyncCallback gpu_vsync_callback_;
 
 #if defined(USE_OZONE)
   // This should outlive gl_surface_ and vulkan_surface_.