Reland "[Paint Preview] Post task captured callback"

This is a reland of c669ac6b

This is the test only, the fix was landed in https://crrev.com/c/2598087

- the previous test did not reproduce the crash, this one has crash a
  crash that matches the trace in the wild
  https://ci.chromium.org/p/chromium/builders/try/linux-dcheck-off-rel/16863?
  It differs from the previous test by
  - calling SetNeedsReload (emulating Clank's autorefresh)
  - navigating to another frame afterwards, triggering Unload

- the previous test timed out on some builders. It's possible that
  the callback was already called before the test's own RunlLoop
  started, which would mean that the loop never exits. This is avoided
  by recording whether it has run or not.

Original change's description:
> [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}

Bug: 1146573, 1160608


Cq-Include-Trybots: luci.chromium.try:linux-dcheck-off-rel
Change-Id: I3ef0a81bb4fa99e2f42c6278ba53931aa934793e
Cq-Do-Not-Cancel-Tryjobs: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2596697Reviewed-by: Calder Kitagawa <ckitagawa@chromium.org>
Commit-Queue: Fergal Daly <fergal@chromium.org>
Auto-Submit: Fergal Daly <fergal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#840497}

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.
@@ -42,6 +69,10 @@ namespace paint_preview {
 class PaintPreviewBrowserTest
     : public InProcessBrowserTest,
       public testing::WithParamInterface<RecordingPersistence> {
+ public:
+  PaintPreviewBrowserTest(const PaintPreviewBrowserTest&) = delete;
+  PaintPreviewBrowserTest& operator=(const PaintPreviewBrowserTest&) = delete;
+
  protected:
   PaintPreviewBrowserTest() = default;
   ~PaintPreviewBrowserTest() override = default;
@@ -92,6 +123,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 +174,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 +360,72 @@ IN_PROC_BROWSER_TEST_P(PaintPreviewBrowserTest,
   loop.Run();
 }
 
+// https://crbug.com/1146573 reproduction. If a renderer crashes,
+// WebContentsObserver::RenderFrameDeleted. Paint preview implements this in an
+// observer which in turn calls DecrementCapturerCount which can cause the
+// WebContents to be reloaded on Android where we have auto-reload. This reload
+// occurs *during* crash handling, leaving the frame in an invalid state and
+// leading to a crash when it subsequently unloaded.
+// This is fixed by deferring it to a PostTask.
+// This also
+IN_PROC_BROWSER_TEST_P(PaintPreviewBrowserTest, DontReloadInRenderProcessExit) {
+  LoadPage(http_server_.GetURL("a.com", "/title1.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);
+
+  // A callback that causes the frame to reload and end up in an invalid state
+  // if it is allowed to run during crash handling.
+  base::RunLoop loop;
+  auto params = MakeParams();
+  bool did_run = false;
+  client->CapturePaintPreview(
+      params, web_contents->GetMainFrame(),
+      // This callback is now posted so it shouldn't cause a crash.
+      base::BindOnce(
+          [](content::WebContents* web_contents, bool* did_run_ptr,
+             base::UnguessableToken guid, mojom::PaintPreviewStatus status,
+             std::unique_ptr<CaptureResult> result) {
+            EXPECT_EQ(status, mojom::PaintPreviewStatus::kFailed);
+            EXPECT_EQ(result, nullptr);
+            // On Android crashed frames are marked as needing reload.
+            web_contents->GetController().SetNeedsReload();
+            web_contents->DecrementCapturerCount(true);
+            web_contents->DecrementCapturerCount(true);
+            *did_run_ptr = true;
+          },
+          web_contents, &did_run)
+          .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.
+  if (!did_run)
+    loop.Run();
+
+  // Now navigate away and ensure that the frame unloads successfully.
+  LoadPage(http_server_.GetURL("a.com", "/title2.html"));
+}
+
 INSTANTIATE_TEST_SUITE_P(All,
                          PaintPreviewBrowserTest,
                          testing::Values(RecordingPersistence::kFileSystem,