[Paint Preview] Make onHidden work reliably

Deflake the use of onHidden in PaintPreviewTabService by treating the
WebContents as being in capture mode. This prevents the renderer from
going away until after the capture is completed.

Follow up work may be required to ensure the TabImpl#hide hiding the
WebContents prior to calling the observers doesn't cause rare races.
I've run 40 times locally without issue, but it could be an issue.

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

chrome/android/java/src/org/chromium/chrome/browser/paint_preview/services/PaintPreviewTabService.java

@@ -9,6 +9,7 @@ import org.chromium.base.annotations.CalledByNative;
 import org.chromium.base.annotations.JNINamespace;
 import org.chromium.base.annotations.NativeMethods;
 import org.chromium.chrome.browser.tab.Tab;
+import org.chromium.chrome.browser.tab.TabHidingType;
 import org.chromium.chrome.browser.tabmodel.TabModel;
 import org.chromium.chrome.browser.tabmodel.TabModelSelector;
 import org.chromium.chrome.browser.tabmodel.TabModelSelectorTabObserver;
@@ -36,14 +37,8 @@ public class PaintPreviewTabService implements NativePaintPreviewServiceProvider
             mTabService = tabService;
         }
 
-        /**
-         * TODO(crbug/1061190): Ideally we would use {@link TabObserver#onHidden(Tab, int)};
-         * however, that was flaky due to renderers being killed before capture was completed. For
-         * now we will use {@link TabObserver#onPageLoadFinished(Tab, String)}, but this should be
-         * revisted.
-         */
         @Override
-        public void onPageLoadFinished(Tab tab, String url) {
+        public void onHidden(Tab tab, @TabHidingType int reason) {
             if (qualifiesForCapture(tab)) {
                 mTabService.captureTab(tab, success -> {
                     if (!success) {

chrome/android/javatests/src/org/chromium/chrome/browser/paint_preview/services/PaintPreviewTabServiceTest.java

@@ -27,6 +27,8 @@ import org.chromium.content_public.browser.test.util.CriteriaHelper;
 import org.chromium.content_public.browser.test.util.TestThreadUtils;
 import org.chromium.net.test.EmbeddedTestServer;
 
+import java.util.concurrent.TimeUnit;
+
 /** Tests for the Paint Preview Tab Manager. */
 @RunWith(ChromeJUnit4ClassRunner.class)
 @CommandLineFlags.Add({ChromeSwitches.DISABLE_FIRST_RUN_EXPERIENCE})
@@ -67,6 +69,11 @@ public class PaintPreviewTabServiceTest {
             mPaintPreviewTabService.onRestoreCompleted(mTabModelSelector);
             mTab.loadUrl(new LoadUrlParams(url));
         });
+        // Give the tab time to complete layout before hiding.
+        TimeUnit.SECONDS.sleep(1);
+
+        // This will hide mTab so that a capture occurs.
+        mActivityTestRule.loadUrlInNewTab(url);
 
         int tabId = mTab.getId();
         CriteriaHelper.pollUiThread(() -> {

chrome/browser/paint_preview/services/paint_preview_tab_service.cc

@@ -78,6 +78,12 @@ void PaintPreviewTabService::CaptureTab(int tab_id,
                                         content::WebContents* contents,
                                         FinishedCallback callback) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
+  // Mark |contents| as being captured so that the renderer doesn't go away
+  // until the capture is finished. This is done even before a file is created
+  // to ensure the renderer doesn't go away while that happens.
+  contents->IncrementCapturerCount(gfx::Size(), true);
+
   auto file_manager = GetFileManager();
   auto key = file_manager->CreateKey(tab_id);
   GetTaskRunner()->PostTaskAndReplyWithResult(
@@ -197,19 +203,26 @@ void PaintPreviewTabService::CaptureTabInternal(
     std::move(callback).Run(Status::kWebContentsGone);
     return;
   }
-  CapturePaintPreview(contents, file_path.value(), gfx::Rect(0, 0, 0, 0),
-                      base::BindOnce(&PaintPreviewTabService::OnCaptured,
-                                     weak_ptr_factory_.GetWeakPtr(), tab_id,
-                                     key, std::move(callback)));
+  CapturePaintPreview(
+      contents, file_path.value(), gfx::Rect(0, 0, 0, 0),
+      base::BindOnce(&PaintPreviewTabService::OnCaptured,
+                     weak_ptr_factory_.GetWeakPtr(), tab_id, key,
+                     frame_tree_node_id, std::move(callback)));
 }
 
 void PaintPreviewTabService::OnCaptured(
     int tab_id,
     const DirectoryKey& key,
+    int frame_tree_node_id,
     FinishedCallback callback,
     PaintPreviewBaseService::CaptureStatus status,
     std::unique_ptr<PaintPreviewProto> proto) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  auto* web_contents =
+      content::WebContents::FromFrameTreeNodeId(frame_tree_node_id);
+  if (web_contents)
+    web_contents->DecrementCapturerCount(true);
+
   if (status != PaintPreviewBaseService::CaptureStatus::kOk || !proto) {
     std::move(callback).Run(Status::kCaptureFailed);
     return;

chrome/browser/paint_preview/services/paint_preview_tab_service.h

@@ -108,6 +108,7 @@ class PaintPreviewTabService : public PaintPreviewBaseService {
 
   void OnCaptured(int tab_id,
                   const DirectoryKey& key,
+                  int frame_tree_node_id,
                   FinishedCallback callback,
                   PaintPreviewBaseService::CaptureStatus status,
                   std::unique_ptr<PaintPreviewProto>);

components/paint_preview/renderer/paint_preview_recorder_impl.cc

@@ -102,6 +102,15 @@ void PaintPreviewRecorderImpl::CapturePaintPreviewInternal(
   gfx::Rect bounds;
   if (is_main_frame_ || params->clip_rect == gfx::Rect(0, 0, 0, 0)) {
     auto size = frame->DocumentSize();
+
+    // |size| may be 0 if a tab is captured prior to layout finishing. This
+    // shouldn't occur often, if at all, in normal usage. However, this may
+    // occur during tests. Capturing prior to layout is non-sensical as the
+    // canvas size cannot be deremined so just abort.
+    if (size.height == 0 || size.width == 0) {
+      *status = mojom::PaintPreviewStatus::kCaptureFailed;
+      return;
+    }
     bounds = gfx::Rect(0, 0, size.width, size.height);
   } else {
     bounds = gfx::Rect(params->clip_rect.size());