[Paint Preview] Ensure WeakPtr stays valid

The |service_| weak pointer in PaintPreviewCompositorClientImpl appears
to be getting invalidated on a wrong thread in some in the wild crash
reports. This CL moves the deleter of PaintPreviewCompositorClient and
PaintPreviewCompositorService to always run on the default task runner
this should ensure the WeakPtr is invalidated on the right thread.

Bug: 1120313
Change-Id: I35f766c58c72537f96022131029008d2a0f6c603
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2368462Reviewed-by: Mehran Mahmoudi <mahmoudi@chromium.org>
Commit-Queue: Calder Kitagawa <ckitagawa@chromium.org>
Cr-Commit-Position: refs/heads/master@{#800604}

chrome/browser/paint_preview/paint_preview_compositor_browsertest.cc

@@ -35,13 +35,15 @@ namespace {
 // methods to validate internal state.
 
 std::unique_ptr<PaintPreviewCompositorServiceImpl> ToCompositorServiceImpl(
-    std::unique_ptr<PaintPreviewCompositorService> service) {
+    std::unique_ptr<PaintPreviewCompositorService, base::OnTaskRunnerDeleter>
+        service) {
   return std::unique_ptr<PaintPreviewCompositorServiceImpl>(
       reinterpret_cast<PaintPreviewCompositorServiceImpl*>(service.release()));
 }
 
 std::unique_ptr<PaintPreviewCompositorClientImpl> ToCompositorClientImpl(
-    std::unique_ptr<PaintPreviewCompositorClient> client) {
+    std::unique_ptr<PaintPreviewCompositorClient, base::OnTaskRunnerDeleter>
+        client) {
   return std::unique_ptr<PaintPreviewCompositorClientImpl>(
       reinterpret_cast<PaintPreviewCompositorClientImpl*>(client.release()));
 }

components/paint_preview/browser/compositor_utils.cc

@@ -32,8 +32,8 @@ void BindDiscardableSharedMemoryManagerOnIOThread(
 
 }  // namespace
 
-std::unique_ptr<PaintPreviewCompositorService> StartCompositorService(
-    base::OnceClosure disconnect_handler) {
+std::unique_ptr<PaintPreviewCompositorService, base::OnTaskRunnerDeleter>
+StartCompositorService(base::OnceClosure disconnect_handler) {
   // Create a dedicated sequence for communicating with the compositor. This
   // sequence will handle message serialization/deserialization of bitmaps so it
   // affects user visible elements. This is an implementation detail and the
@@ -52,9 +52,12 @@ std::unique_ptr<PaintPreviewCompositorService> StartCompositorService(
       base::BindOnce(&CreateCompositorCollectionPending,
                      pending_remote.InitWithNewPipeAndPassReceiver()));
 
-  return std::make_unique<PaintPreviewCompositorServiceImpl>(
-      std::move(pending_remote), compositor_task_runner,
-      std::move(disconnect_handler));
+  return std::unique_ptr<PaintPreviewCompositorServiceImpl,
+                         base::OnTaskRunnerDeleter>(
+      new PaintPreviewCompositorServiceImpl(std::move(pending_remote),
+                                            compositor_task_runner,
+                                            std::move(disconnect_handler)),
+      base::OnTaskRunnerDeleter(base::SequencedTaskRunnerHandle::Get()));
 }
 
 mojo::Remote<mojom::PaintPreviewCompositorCollection>

components/paint_preview/browser/compositor_utils.h

@@ -12,8 +12,8 @@
 namespace paint_preview {
 
 // Starts the compositor service in a utility process.
-std::unique_ptr<PaintPreviewCompositorService> StartCompositorService(
-    base::OnceClosure disconnect_handler);
+std::unique_ptr<PaintPreviewCompositorService, base::OnTaskRunnerDeleter>
+StartCompositorService(base::OnceClosure disconnect_handler);
 
 // Creates a utility process via the service manager that is sandboxed and
 // running an instance of the PaintPreviewCompositorCollectionImpl. This can be

components/paint_preview/browser/paint_preview_compositor_service_impl.cc

@@ -57,15 +57,19 @@ PaintPreviewCompositorServiceImpl::PaintPreviewCompositorServiceImpl(
 
 // The destructor for the |compositor_service_| will automatically result in any
 // active compositors being killed.
-PaintPreviewCompositorServiceImpl::~PaintPreviewCompositorServiceImpl() =
-    default;
+PaintPreviewCompositorServiceImpl::~PaintPreviewCompositorServiceImpl() {
+  DCHECK(default_task_runner_->RunsTasksInCurrentSequence());
+}
 
-std::unique_ptr<PaintPreviewCompositorClient>
+std::unique_ptr<PaintPreviewCompositorClient, base::OnTaskRunnerDeleter>
 PaintPreviewCompositorServiceImpl::CreateCompositor(
     base::OnceClosure connected_closure) {
   DCHECK(default_task_runner_->RunsTasksInCurrentSequence());
-  auto compositor = std::make_unique<PaintPreviewCompositorClientImpl>(
-      compositor_task_runner_, weak_ptr_factory_.GetWeakPtr());
+  std::unique_ptr<PaintPreviewCompositorClientImpl, base::OnTaskRunnerDeleter>
+      compositor(
+          new PaintPreviewCompositorClientImpl(compositor_task_runner_,
+                                               weak_ptr_factory_.GetWeakPtr()),
+          base::OnTaskRunnerDeleter(base::SequencedTaskRunnerHandle::Get()));
 
   compositor_task_runner_->PostTask(
       FROM_HERE,

components/paint_preview/browser/paint_preview_compositor_service_impl.h

@@ -34,8 +34,8 @@ class PaintPreviewCompositorServiceImpl : public PaintPreviewCompositorService {
   ~PaintPreviewCompositorServiceImpl() override;
 
   // PaintPreviewCompositorService Implementation.
-  std::unique_ptr<PaintPreviewCompositorClient> CreateCompositor(
-      base::OnceClosure connected_closure) override;
+  std::unique_ptr<PaintPreviewCompositorClient, base::OnTaskRunnerDeleter>
+  CreateCompositor(base::OnceClosure connected_closure) override;
 
   bool HasActiveClients() const override;
 

components/paint_preview/player/player_compositor_delegate.cc

@@ -103,7 +103,11 @@ PlayerCompositorDelegate::PlayerCompositorDelegate(
     : compositor_error_(std::move(compositor_error)),
       paint_preview_service_(paint_preview_service),
       key_(key),
-      compress_on_close_(true) {
+      compress_on_close_(true),
+      paint_preview_compositor_service_(nullptr,
+                                        base::OnTaskRunnerDeleter(nullptr)),
+      paint_preview_compositor_client_(nullptr,
+                                       base::OnTaskRunnerDeleter(nullptr)) {
   if (skip_service_launch) {
     paint_preview_service_->GetCapturedPaintPreviewProto(
         key, base::BindOnce(&PlayerCompositorDelegate::OnProtoAvailable,

components/paint_preview/player/player_compositor_delegate.h

@@ -76,9 +76,9 @@ class PlayerCompositorDelegate {
   PaintPreviewBaseService* paint_preview_service_;
   DirectoryKey key_;
   bool compress_on_close_;
-  std::unique_ptr<PaintPreviewCompositorService>
+  std::unique_ptr<PaintPreviewCompositorService, base::OnTaskRunnerDeleter>
       paint_preview_compositor_service_;
-  std::unique_ptr<PaintPreviewCompositorClient>
+  std::unique_ptr<PaintPreviewCompositorClient, base::OnTaskRunnerDeleter>
       paint_preview_compositor_client_;
   base::flat_map<base::UnguessableToken, std::unique_ptr<HitTester>>
       hit_testers_;

components/paint_preview/public/paint_preview_compositor_service.h

@@ -8,6 +8,7 @@
 #include <memory>
 
 #include "base/callback_forward.h"
+#include "base/sequenced_task_runner.h"
 #include "components/paint_preview/public/paint_preview_compositor_client.h"
 
 namespace paint_preview {
@@ -26,8 +27,9 @@ class PaintPreviewCompositorService {
 
   // Creates a compositor instance tied to the service. |connected_closure| is
   // run once the compositor is started.
-  virtual std::unique_ptr<PaintPreviewCompositorClient> CreateCompositor(
-      base::OnceClosure connected_closure) = 0;
+  virtual std::unique_ptr<PaintPreviewCompositorClient,
+                          base::OnTaskRunnerDeleter>
+  CreateCompositor(base::OnceClosure connected_closure) = 0;
 
   // Returns whether there are any active clients. This can be used to
   // check if killing this service is safe (i.e. won't drop messages).