Handle same-document navigations in ThumbnailReadinessTracker

Same-document navigations (such as history.pushState() calls) cause
associated DidStartNavigation() and DidFinishNavigation()
calls. However, since this isn't considered a load,
DocumentOnLoadCompletedInMainFrame() is sensibly not called.

ThumbnailReadinessTracker assumed DidStartNavigation eventually lead
to a DocumentOnLoadCompletedInMainFrame call (or an error). This meant
that pages using history.pushState(), such as YouTube, had
never-ending thumbnail capture.

This CL fixes the never-ending capture.

Bug: 1120940
Change-Id: Ia298d384b44bcc2b860260826952898972daf480
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2382426Reviewed-by: Dana Fried <dfried@chromium.org>
Commit-Queue: Collin Baker <collinbaker@chromium.org>
Cr-Commit-Position: refs/heads/master@{#803294}

chrome/browser/ui/thumbnails/thumbnail_readiness_tracker.cc

@@ -8,6 +8,27 @@
 
 #include "content/public/browser/navigation_handle.h"
 
+namespace {
+
+bool NavigationShouldInvalidateThumbnail(
+    content::NavigationHandle* navigation) {
+  // Ignore subframe navigations.
+  if (!navigation->IsInMainFrame())
+    return false;
+
+  // Some navigations change the tab URL but don't create a new
+  // document. They aren't considered loading; onload is never triggered
+  // after they complete. They shouldn't affect the thumbnail.
+  //
+  // See crbug.com/1120940 for why this is necessary.
+  if (navigation->IsSameDocument())
+    return false;
+
+  return true;
+}
+
+}  // namespace
+
 ThumbnailReadinessTracker::ThumbnailReadinessTracker(
     content::WebContents* web_contents,
     ReadinessChangeCallback callback)
@@ -20,15 +41,23 @@ ThumbnailReadinessTracker::~ThumbnailReadinessTracker() = default;
 
 void ThumbnailReadinessTracker::DidStartNavigation(
     content::NavigationHandle* navigation_handle) {
-  if (!navigation_handle->IsInMainFrame())
+  if (!NavigationShouldInvalidateThumbnail(navigation_handle))
     return;
+
+  pending_navigation_ = navigation_handle;
   UpdateReadiness(Readiness::kNotReady);
 }
 
 void ThumbnailReadinessTracker::DidFinishNavigation(
     content::NavigationHandle* navigation_handle) {
-  if (!navigation_handle->IsInMainFrame())
+  // Only respond to the last navigation handled by DidStartNavigation.
+  // Others may be unimportant or outdated.
+  if (navigation_handle != pending_navigation_)
     return;
+
+  pending_navigation_ = nullptr;
+  UpdateReadiness(Readiness::kReadyForInitialCapture);
+
   if (last_readiness_ > Readiness::kReadyForInitialCapture)
     return;
   UpdateReadiness(Readiness::kReadyForInitialCapture);
@@ -39,6 +68,7 @@ void ThumbnailReadinessTracker::DocumentOnLoadCompletedInMainFrame() {
 }
 
 void ThumbnailReadinessTracker::WebContentsDestroyed() {
+  pending_navigation_ = nullptr;
   UpdateReadiness(Readiness::kNotReady);
 }
 

chrome/browser/ui/thumbnails/thumbnail_readiness_tracker.h

@@ -48,6 +48,10 @@ class ThumbnailReadinessTracker : public content::WebContentsObserver {
 
   ReadinessChangeCallback callback_;
   Readiness last_readiness_ = Readiness::kNotReady;
+
+  // The last navigation that reset the thumbnail. When this navigation
+  // finishes, the page is considered ready for capture.
+  content::NavigationHandle* pending_navigation_ = nullptr;
 };
 
 #endif  // CHROME_BROWSER_UI_THUMBNAILS_THUMBNAIL_READINESS_TRACKER_H_

chrome/browser/ui/thumbnails/thumbnail_readiness_tracker_browsertest.cc

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/ui/thumbnails/thumbnail_readiness_tracker.h"
+
+#include <memory>
+
+#include "base/test/mock_callback.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "content/public/browser/web_contents.h"
+#include "content/public/test/browser_test.h"
+#include "content/public/test/browser_test_utils.h"
+#include "net/test/embedded_test_server/embedded_test_server.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "url/gurl.h"
+
+using ::testing::AnyNumber;
+using ::testing::AtLeast;
+using ::testing::InSequence;
+
+class ThumbnailReadinessTrackerBrowserTest : public InProcessBrowserTest {
+ public:
+  void SetUpOnMainThread() override {
+    ASSERT_TRUE(embedded_test_server()->Start());
+    contents_ = content::WebContents::Create(
+        content::WebContents::CreateParams(browser()->profile()));
+    readiness_tracker_ = std::make_unique<ThumbnailReadinessTracker>(
+        contents_.get(), readiness_callback_.Get());
+  }
+
+  void TearDownOnMainThread() override {
+    readiness_tracker_.reset();
+    contents_.reset();
+  }
+
+ protected:
+  std::unique_ptr<content::WebContents> contents_;
+
+  base::MockCallback<ThumbnailReadinessTracker::ReadinessChangeCallback>
+      readiness_callback_;
+  std::unique_ptr<ThumbnailReadinessTracker> readiness_tracker_;
+};
+
+IN_PROC_BROWSER_TEST_F(ThumbnailReadinessTrackerBrowserTest,
+                       NavigateAfterOnload) {
+  const GURL url = embedded_test_server()->GetURL(
+      "/thumbnail_capture/navigate_after_onload.html");
+  const GURL redirect_url =
+      embedded_test_server()->GetURL("/thumbnail_capture/redirect_target.html");
+
+  {
+    InSequence _s;
+
+    // The first page fully loads, completing its onload handler.
+    EXPECT_CALL(
+        readiness_callback_,
+        Run(ThumbnailReadinessTracker::Readiness::kReadyForInitialCapture));
+    EXPECT_CALL(
+        readiness_callback_,
+        Run(ThumbnailReadinessTracker::Readiness::kReadyForFinalCapture));
+
+    // After onload completes, it redirects.
+    EXPECT_CALL(readiness_callback_,
+                Run(ThumbnailReadinessTracker::Readiness::kNotReady));
+    EXPECT_CALL(
+        readiness_callback_,
+        Run(ThumbnailReadinessTracker::Readiness::kReadyForInitialCapture));
+    EXPECT_CALL(
+        readiness_callback_,
+        Run(ThumbnailReadinessTracker::Readiness::kReadyForFinalCapture));
+  }
+
+  content::NavigateToURLBlockUntilNavigationsComplete(contents_.get(), url, 2);
+  EXPECT_EQ(contents_->GetLastCommittedURL(), redirect_url);
+  ASSERT_TRUE(content::WaitForLoadStop(contents_.get()));
+}
+
+IN_PROC_BROWSER_TEST_F(ThumbnailReadinessTrackerBrowserTest,
+                       NavigateIframeAfterOnload) {
+  const GURL url = embedded_test_server()->GetURL(
+      "/thumbnail_capture/navigate_iframe_after_onload.html");
+  const GURL iframe_url =
+      embedded_test_server()->GetURL("/thumbnail_capture/iframe_2.html");
+
+  {
+    InSequence _s;
+
+    EXPECT_CALL(
+        readiness_callback_,
+        Run(ThumbnailReadinessTracker::Readiness::kReadyForInitialCapture));
+    EXPECT_CALL(
+        readiness_callback_,
+        Run(ThumbnailReadinessTracker::Readiness::kReadyForFinalCapture));
+  }
+
+  ASSERT_TRUE(content::NavigateToURL(contents_.get(), url));
+  ASSERT_TRUE(content::WaitForLoadStop(contents_.get()));
+  content::NavigateIframeToURL(contents_.get(), "if", iframe_url);
+}
+
+// Regression test for crbug.com/1120940.
+//
+// Artifical navigations like history.pushState() shouldn't invalidate
+// our thumbnail.
+IN_PROC_BROWSER_TEST_F(ThumbnailReadinessTrackerBrowserTest,
+                       PushStateAfterOnload) {
+  const GURL url = embedded_test_server()->GetURL(
+      "/thumbnail_capture/push_state_after_onload.html");
+
+  {
+    InSequence _s;
+
+    // The page should become ready for capture. The pushState() call
+    // shouldn't change anything.
+    EXPECT_CALL(
+        readiness_callback_,
+        Run(ThumbnailReadinessTracker::Readiness::kReadyForInitialCapture));
+    EXPECT_CALL(
+        readiness_callback_,
+        Run(ThumbnailReadinessTracker::Readiness::kReadyForFinalCapture));
+  }
+
+  content::NavigateToURLBlockUntilNavigationsComplete(contents_.get(), url, 2);
+  ASSERT_TRUE(content::WaitForLoadStop(contents_.get()));
+}

chrome/test/BUILD.gn

@@ -1381,6 +1381,7 @@ if (!is_android) {
       "../browser/ui/test/test_browser_ui.h",
       "../browser/ui/test/test_infobar.cc",
       "../browser/ui/test/test_infobar.h",
+      "../browser/ui/thumbnails/thumbnail_readiness_tracker_browsertest.cc",
       "../browser/ui/thumbnails/thumbnail_tab_helper_browsertest.cc",
       "../browser/ui/toolbar/browser_actions_bar_browsertest.cc",
       "../browser/ui/toolbar/browser_actions_bar_browsertest.h",

chrome/test/data/thumbnail_capture/iframe_1.html

+<html>
+    <body>
+        <h1>Foo</h1>
+    </body>
+</html>

chrome/test/data/thumbnail_capture/iframe_2.html

+<html>
+    <body>
+        <h1>Bar</h1>
+    </body>
+</html>

chrome/test/data/thumbnail_capture/navigate_after_onload.html

+<html>
+    <body>
+        <script type="text/javascript" >
+         window.onload = function () {
+             // Redirect after onload completes.
+             setTimeout(redirect);
+         };
+
+         function redirect() {
+             window.location.href = "redirect_target.html";
+         }
+        </script>
+    </body>
+</html>

chrome/test/data/thumbnail_capture/navigate_iframe_after_onload.html

+<html>
+    <body>
+        <iframe id="if" src="iframe_1.html"></iframe>
+    </body>
+</html>

chrome/test/data/thumbnail_capture/push_state_after_onload.html

+<html>
+    <body>
+        <script type="text/javascript">
+         window.onload = function () {
+             setTimeout(push);
+         };
+
+         function push() {
+             window.history.pushState({}, "", "foo.html");
+         }
+        </script>
+    </body>
+</html>

chrome/test/data/thumbnail_capture/redirect_target.html

+<html>
+    <body>
+        test
+    </body>
+</html>