Reland "Reland "[Paint Preview] Post task captured callback""

This is a reland of 2cda8338

Test is Flaky on Mac, so relanding with Mac disabled.

Original change's description:
> 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/+/2596697
> Reviewed-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}

Bug: 1146573
Bug: 1160608
Change-Id: Ib26af42633d55e49752623930ce63245b6f4482e
Cq-Include-Trybots: luci.chromium.try:linux-dcheck-off-rel
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2615724
Auto-Submit: Fergal Daly <fergal@chromium.org>
Commit-Queue: Calder Kitagawa <ckitagawa@chromium.org>
Reviewed-by: Calder Kitagawa <ckitagawa@chromium.org>
Cr-Commit-Position: refs/heads/master@{#841489}

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,79 @@ 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.
+// Flaky on Mac. TODO(https://crbug.com/1160608): Enabled this test.
+#if defined(OS_MAC)
+#define MAYBE_DontReloadInRenderProcessExit \
+  DISABLED_DontReloadInRenderProcessExit
+#else
+#define MAYBE_DontReloadInRenderProcessExit DontReloadInRenderProcessExit
+#endif
+IN_PROC_BROWSER_TEST_P(PaintPreviewBrowserTest,
+                       MAYBE_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,