[Paint Preview] Post task captured callback

This CL attempts to fix a bug where calling
WebContents::DecrementCapturerCount in the callback passed to
PaintPreviewClient can result in reloading a crashed render frame.
This is problematic and crashes if it occurs inside RenderFrameDeleted
and PaintPreviewClient::OnFinished is invoked. By posting the callback
it should be invoked after RenderFrameDeleted cleanup is done resulting
in DecrementCapturerCount no longer being called inside
RenderFrameDeleted.

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

chrome/browser/paint_preview/paint_preview_browsertest.cc

@@ -24,17 +24,44 @@
 #include "components/paint_preview/common/test_utils.h"
 #include "components/ukm/test_ukm_recorder.h"
 #include "content/public/browser/notification_types.h"
+#include "content/public/browser/render_process_host.h"
 #include "content/public/common/content_switches.h"
 #include "content/public/test/browser_test.h"
 #include "net/dns/mock_host_resolver.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
 #include "services/metrics/public/cpp/ukm_builders.h"
+#include "third_party/blink/public/common/associated_interfaces/associated_interface_provider.h"
 #include "third_party/skia/include/core/SkPicture.h"
 #include "third_party/skia/include/core/SkStream.h"
 #include "url/gurl.h"
 
 namespace paint_preview {
 
+class NoOpPaintPreviewRecorder : public mojom::PaintPreviewRecorder {
+ public:
+  NoOpPaintPreviewRecorder() = default;
+  ~NoOpPaintPreviewRecorder() override = default;
+
+  NoOpPaintPreviewRecorder(const NoOpPaintPreviewRecorder&) = delete;
+  NoOpPaintPreviewRecorder& operator=(const NoOpPaintPreviewRecorder&) = delete;
+
+  void CapturePaintPreview(
+      mojom::PaintPreviewCaptureParamsPtr params,
+      mojom::PaintPreviewRecorder::CapturePaintPreviewCallback callback)
+      override {
+    callback_ = std::move(callback);
+  }
+
+  void BindRequest(mojo::ScopedInterfaceEndpointHandle handle) {
+    binding_.Bind(mojo::PendingAssociatedReceiver<mojom::PaintPreviewRecorder>(
+        std::move(handle)));
+  }
+
+ private:
+  mojom::PaintPreviewRecorder::CapturePaintPreviewCallback callback_;
+  mojo::AssociatedReceiver<mojom::PaintPreviewRecorder> binding_{this};
+};
+
 // Test harness for a integration test of paint previews. In this test:
 // - Each RenderFrame has an instance of PaintPreviewRecorder attached.
 // - Each WebContents has an instance of PaintPreviewClient attached.
@@ -46,6 +73,9 @@ class PaintPreviewBrowserTest
   PaintPreviewBrowserTest() = default;
   ~PaintPreviewBrowserTest() override = default;
 
+  PaintPreviewBrowserTest(const PaintPreviewBrowserTest&) = delete;
+  PaintPreviewBrowserTest& operator=(const PaintPreviewBrowserTest&) = delete;
+
   void SetUp() override {
     ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
 
@@ -92,6 +122,15 @@ class PaintPreviewBrowserTest
     return params;
   }
 
+  void OverrideInterface(NoOpPaintPreviewRecorder* service) {
+    blink::AssociatedInterfaceProvider* remote_interfaces =
+        GetWebContents()->GetMainFrame()->GetRemoteAssociatedInterfaces();
+    remote_interfaces->OverrideBinderForTesting(
+        mojom::PaintPreviewRecorder::Name_,
+        base::BindRepeating(&NoOpPaintPreviewRecorder::BindRequest,
+                            base::Unretained(service)));
+  }
+
   void WaitForLoadStopWithoutSuccessCheck() {
     // In many cases, the load may have finished before we get here.  Only wait
     // if the tab still has a pending navigation.
@@ -134,10 +173,6 @@ class PaintPreviewBrowserTest
   base::ScopedTempDir temp_dir_;
   net::EmbeddedTestServer http_server_;
   net::EmbeddedTestServer http_server_different_origin_;
-
- private:
-  PaintPreviewBrowserTest(const PaintPreviewBrowserTest&) = delete;
-  PaintPreviewBrowserTest& operator=(const PaintPreviewBrowserTest&) = delete;
 };
 
 IN_PROC_BROWSER_TEST_P(PaintPreviewBrowserTest, CaptureFrame) {
@@ -324,6 +359,57 @@ IN_PROC_BROWSER_TEST_P(PaintPreviewBrowserTest,
   loop.Run();
 }
 
+// https://crbug.com/1146573. If a renderer crashes,
+// WebContentsObserver::RenderFrameDeleted is called quite early on.
+// DecrementCapturerCount can cause the WebContents to be reloaded. This should
+// be deferred using PostTask.
+IN_PROC_BROWSER_TEST_P(PaintPreviewBrowserTest, DontReloadInRenderProcessExit) {
+  LoadPage(http_server_.GetURL("a.com", "/hello.html"));
+
+  content::WebContents* web_contents = GetWebContents();
+
+  // Override remote interfaces with a no-op.
+  NoOpPaintPreviewRecorder noop_recorder;
+  OverrideInterface(&noop_recorder);
+
+  CreateClient();
+  auto* client = PaintPreviewClient::FromWebContents(web_contents);
+  // Do this twice to simulate conditions for crash.
+  web_contents->IncrementCapturerCount(gfx::Size(), true);
+  web_contents->IncrementCapturerCount(gfx::Size(), true);
+
+  base::RunLoop loop;
+  auto params = MakeParams();
+  client->CapturePaintPreview(
+      params, web_contents->GetMainFrame(),
+      // This callback is now posted so it shouldn't cause a crash.
+      base::BindOnce(
+          [](content::WebContents* web_contents, base::UnguessableToken guid,
+             mojom::PaintPreviewStatus status,
+             std::unique_ptr<CaptureResult> result) {
+            EXPECT_EQ(status, mojom::PaintPreviewStatus::kFailed);
+            EXPECT_EQ(result, nullptr);
+            web_contents->DecrementCapturerCount(true);
+            web_contents->DecrementCapturerCount(true);
+          },
+          web_contents)
+          .Then(loop.QuitClosure()));
+  // Crash the renderer.
+  {
+    base::ScopedAllowBlockingForTesting scope;
+    content::RenderProcessHost* process =
+        GetWebContents()->GetMainFrame()->GetProcess();
+    content::RenderProcessHostWatcher crash_observer(
+        process, content::RenderProcessHostWatcher::WATCH_FOR_PROCESS_EXIT);
+    process->Shutdown(0);
+    crash_observer.Wait();
+  }
+
+  // The browser would have crashed before the loop exited if the callback was
+  // not posted.
+  loop.Run();
+}
+
 INSTANTIATE_TEST_SUITE_P(All,
                          PaintPreviewBrowserTest,
                          testing::Values(RecordingPersistence::kFileSystem,

components/paint_preview/browser/paint_preview_client.cc

@@ -560,16 +560,18 @@ void PaintPreviewClient::OnFinished(
     // At a minimum one frame was captured successfully, it is up to the
     // caller to decide if a partial success is acceptable based on what is
     // contained in the proto.
-    std::move(document_data->callback)
-        .Run(guid,
+    base::SequencedTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE,
+        base::BindOnce(std::move(document_data->callback), guid,
                        document_data->had_error
                            ? mojom::PaintPreviewStatus::kPartialSuccess
                            : mojom::PaintPreviewStatus::kOk,
-             std::move(*document_data).IntoCaptureResult());
+                       std::move(*document_data).IntoCaptureResult()));
   } else {
     // A proto could not be created indicating all frames failed to capture.
-    std::move(document_data->callback)
-        .Run(guid, mojom::PaintPreviewStatus::kFailed, {});
+    base::SequencedTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE, base::BindOnce(std::move(document_data->callback), guid,
+                                  mojom::PaintPreviewStatus::kFailed, nullptr));
   }
   all_document_data_.erase(guid);
 }