BFCache: Do not check IsDOMContentLoaded of a frame if last url

in the frame is invalid

IsDOMContentLoaded can be completed when the url is valid.
If a subframe with an invalid url exists in a page, bfcache does not work because IsDOMContentLoaded of the frame is always false.

This required test updates:
- ChildFrameCaptureContentFirst disable BFCache because the test requires that the frame be evicted
- the data= version of embedded-not-found-expected.txt now passes

This adds WebContentsWaiter a base class for making test-waiters that wait for a WebContentObserver event to occur.

R=toyoshim@chromium.org

Bug: 1366707
Change-Id: I371cd9ad55f342d9c71ba24eaf5b8b5981d818ba
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3912822
Reviewed-by: Xianzhu Wang <wangxianzhu@chromium.org>
Reviewed-by: Rakina Zata Amni <rakina@chromium.org>
Commit-Queue: Fergal Daly <fergal@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1107116}

AUTHORS

@@ -644,6 +644,7 @@ Julien Isorce <j.isorce@samsung.com>
 Julien Racle <jracle@logitech.com>
 Jun Fang <jun_fang@foxitsoftware.com>
 Jun Jiang <jun.a.jiang@intel.com>
+Junbong Eom <jb.eom@samsung.com>
 Jungchang Park <valley84265@gmail.com>
 Junchao Han <junchao.han@intel.com>
 Junghoon Lee <sjh836@gmail.com>

components/content_capture/browser/content_capture_receiver_test.cc

@@ -9,7 +9,9 @@
 #include "base/test/test_mock_time_task_runner.h"
 #include "build/build_config.h"
 #include "components/content_capture/browser/content_capture_test_helper.h"
+#include "content/public/browser/back_forward_cache.h"
 #include "content/public/common/content_features.h"
+#include "content/public/test/back_forward_cache_util.h"
 #include "content/public/test/test_renderer_host.h"
 #include "content/public/test/test_utils.h"
 #include "third_party/blink/public/mojom/favicon/favicon_url.mojom.h"
@@ -371,6 +373,9 @@ TEST_P(ContentCaptureReceiverTest, TitleUpdateTaskDelay) {
 #define MAYBE_ChildFrameCaptureContentFirst ChildFrameCaptureContentFirst
 #endif
 TEST_P(ContentCaptureReceiverTest, MAYBE_ChildFrameCaptureContentFirst) {
+  // This test performs navigations, expecting the frames to be destroyed.
+  content::DisableBackForwardCacheForTesting(
+      web_contents(), content::BackForwardCache::TEST_REQUIRES_NO_CACHING);
   // Simulate add child frame.
   SetupChildFrame();
   // Simulate to capture the content from child frame.

content/browser/back_forward_cache_basics_browsertest.cc

@@ -5,9 +5,12 @@
 #include "content/browser/back_forward_cache_browsertest.h"
 
 #include "content/browser/web_contents/web_contents_impl.h"
+#include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/site_isolation_policy.h"
+#include "content/public/browser/web_contents.h"
 #include "content/public/common/url_constants.h"
 #include "content/public/test/browser_test.h"
+#include "content/public/test/browser_test_utils.h"
 #include "content/public/test/content_browser_test_utils.h"
 #include "content/public/test/navigation_handle_observer.h"
 #include "content/public/test/test_navigation_observer.h"
@@ -32,6 +35,19 @@ using testing::UnorderedElementsAreArray;
 
 namespace content {
 
+namespace {
+void InsertSubFrameWithUrl(RenderFrameHost* rfh, std::string url) {
+  std::string insert_script = base::StringPrintf(
+      R"(
+    const iframeElement = document.createElement("iframe");
+    iframeElement.src = "%s";
+    document.body.appendChild(iframeElement);
+  )",
+      url.c_str());
+  ASSERT_TRUE(ExecJs(rfh, insert_script));
+}
+}  // namespace
+
 using NotRestoredReason = BackForwardCacheMetrics::NotRestoredReason;
 using NotRestoredReasons =
     BackForwardCacheCanStoreDocumentResult::NotRestoredReasons;
@@ -575,125 +591,172 @@ IN_PROC_BROWSER_TEST_F(BackForwardCacheBrowserTest,
   }
 }
 
-IN_PROC_BROWSER_TEST_F(BackForwardCacheBrowserTest,
+class BackForwardCacheStillLoadingBrowserTest
-                       DoesNotCacheIfMainFrameStillLoading) {
+    : public BackForwardCacheBrowserTest,
+      public ::testing::WithParamInterface<TestFrameType> {
+ protected:
+  std::string GetMainFramePath() {
+    switch (GetParam()) {
+      case TestFrameType::kMainFrame:
+        return "/controlled";
+      case TestFrameType::kSubFrame:
+        return "/back_forward_cache/controllable_subframe.html";
+      case TestFrameType::kSubFrameOfSubframe:
+        return "/back_forward_cache/controllable_subframe_of_subframe.html";
+    }
+  }
+
+  int GetNavigationCount() {
+    switch (GetParam()) {
+      case TestFrameType::kMainFrame:
+        return 1;
+      case TestFrameType::kSubFrame:
+        return 2;
+      case TestFrameType::kSubFrameOfSubframe:
+        return 3;
+    }
+  }
+};
+
+INSTANTIATE_TEST_SUITE_P(All,
+                         BackForwardCacheStillLoadingBrowserTest,
+                         ::testing::Values(TestFrameType::kMainFrame,
+                                           TestFrameType::kSubFrame,
+                                           TestFrameType::kSubFrameOfSubframe));
+
+IN_PROC_BROWSER_TEST_P(BackForwardCacheStillLoadingBrowserTest,
+                       DoesNotCacheIfFrameStillLoading) {
+  std::string controlled_path("/controlled");
   net::test_server::ControllableHttpResponse response(embedded_test_server(),
-                                                      "/main_document");
+                                                      controlled_path);
   ASSERT_TRUE(embedded_test_server()->Start());
 
-  // 1) Navigate to a page that doesn't finish loading.
+  bool testing_main_frame = GetParam() == TestFrameType::kMainFrame;
-  GURL url(embedded_test_server()->GetURL("a.com", "/main_document"));
-  TestNavigationManager navigation_manager(shell()->web_contents(), url);
-  shell()->LoadURL(url);
 
-  // The navigation starts.
+  GURL main_frame_url(
-  EXPECT_TRUE(navigation_manager.WaitForRequestStart());
+      embedded_test_server()->GetURL("a.com", GetMainFramePath()));
-  navigation_manager.ResumeNavigation();
 
-  // The server sends the first part of the response and waits.
+  // 1) Navigate to a page with a frame that loads partially but never
+  // completes. We need the navigation of the partial frame to complete to avoid
+  // extra blocking reasons from occurring.
+  TestNavigationObserver observer(web_contents(), GetNavigationCount());
+  observer.set_wait_event(
+      TestNavigationObserver::WaitEvent::kNavigationFinished);
+  shell()->LoadURL(main_frame_url);
   response.WaitForRequest();
   response.Send(
       "HTTP/1.1 200 OK\r\n"
       "Content-Type: text/html; charset=utf-8\r\n"
       "\r\n"
-      "<html><body> ... ");
+      "<html><body>...");
-
+  observer.WaitForNavigationFinished();
-  // The navigation finishes while the body is still loading.
-  ASSERT_TRUE(navigation_manager.WaitForNavigationFinished());
-  RenderFrameDeletedObserver delete_observer_rfh_a(current_frame_host());
 
   // 2) Navigate away.
+  RenderFrameHostImplWrapper rfh_a(current_frame_host());
   shell()->LoadURL(embedded_test_server()->GetURL("b.com", "/title1.html"));
+  ASSERT_TRUE(WaitForLoadStop(web_contents()));
 
-  // The page was still loading when we navigated away, so it shouldn't have
+  // The page should not have been added to cache, since it had a subframe
-  // been cached.
+  // that was still loading at the time it was navigated away from.
-  delete_observer_rfh_a.WaitUntilDeleted();
+  ASSERT_TRUE(rfh_a.WaitUntilRenderFrameDeleted());
 
-  // 3) Go back.
+  // 3) Go back. If this is the main frame, then going back will get a 404.
-  web_contents()->GetController().GoBack();
+  ASSERT_NE(HistoryGoBack(web_contents()), testing_main_frame);
-  EXPECT_FALSE(WaitForLoadStop(shell()->web_contents()));
   ExpectNotRestored({NotRestoredReason::kLoading}, {}, {}, {}, {}, FROM_HERE);
 }
 
-IN_PROC_BROWSER_TEST_F(BackForwardCacheBrowserTest,
+class BackForwardCacheStillNavigatingBrowserTest
-                       DoesNotCacheLoadingSubframe) {
+    : public BackForwardCacheBrowserTest,
+      public ::testing::WithParamInterface<TestFrameType> {
+ protected:
+  std::string GetMainFramePath() {
+    switch (GetParam()) {
+      case TestFrameType::kMainFrame:
+        NOTREACHED();
+        return "";
+      case TestFrameType::kSubFrame:
+        return "/back_forward_cache/controllable_subframe.html";
+      case TestFrameType::kSubFrameOfSubframe:
+        return "/back_forward_cache/controllable_subframe_of_subframe.html";
+    }
+  }
+};
+
+INSTANTIATE_TEST_SUITE_P(All,
+                         BackForwardCacheStillNavigatingBrowserTest,
+                         ::testing::Values(TestFrameType::kSubFrame,
+                                           TestFrameType::kSubFrameOfSubframe));
+
+IN_PROC_BROWSER_TEST_P(BackForwardCacheStillNavigatingBrowserTest,
+                       DoesNotCacheNavigatingSubframe) {
   net::test_server::ControllableHttpResponse response(embedded_test_server(),
                                                       "/controlled");
   ASSERT_TRUE(embedded_test_server()->Start());
 
   // 1) Navigate to a page with an iframe that loads forever.
-  GURL url(embedded_test_server()->GetURL(
+  GURL url(embedded_test_server()->GetURL("a.com", GetMainFramePath()));
-      "a.com", "/back_forward_cache/controllable_subframe.html"));
+
-  TestNavigationManager navigation_manager(shell()->web_contents(), url);
   shell()->LoadURL(url);
-
-  // The navigation finishes while the iframe is still loading.
-  ASSERT_TRUE(navigation_manager.WaitForNavigationFinished());
-
   // Wait for the iframe request to arrive, and leave it hanging with no
   // response.
   response.WaitForRequest();
 
-  RenderFrameHostImpl* rfh_a = current_frame_host();
+  RenderFrameHostImplWrapper rfh_a(current_frame_host());
-  RenderFrameDeletedObserver delete_observer_rfh_a(rfh_a);
+  // If the "DOMContentLoaded" event has not fired, it will cause BFCache to be
+  // blocked. We are not
+  ASSERT_EQ(42, EvalJs(rfh_a.get(), "domContentLoaded"));
 
   // 2) Navigate away.
   shell()->LoadURL(embedded_test_server()->GetURL("b.com", "/title1.html"));
-  EXPECT_TRUE(WaitForLoadStop(shell()->web_contents()));
+  EXPECT_TRUE(WaitForLoadStop(web_contents()));
 
   // The page should not have been added to cache, since it had a subframe that
   // was still loading at the time it was navigated away from.
-  delete_observer_rfh_a.WaitUntilDeleted();
+  ASSERT_TRUE(rfh_a.WaitUntilRenderFrameDeleted());
 
   // 3) Go back.
   ASSERT_TRUE(HistoryGoBack(web_contents()));
-  ExpectNotRestored(
+  ExpectNotRestored({NotRestoredReason::kSubframeIsNavigating}, {}, {}, {}, {},
-      {
+                    FROM_HERE);
-          NotRestoredReason::kLoading,
-          NotRestoredReason::kSubframeIsNavigating,
-      },
-      {}, {}, {}, {}, FROM_HERE);
 }
 
+// Check that a frame with an invalid url doesn't affect the back-forward cache
+// usage.
 IN_PROC_BROWSER_TEST_F(BackForwardCacheBrowserTest,
-                       DoesNotCacheLoadingSubframeOfSubframe) {
+                       FrameWithInvalidURLDoesntAffectCache) {
-  net::test_server::ControllableHttpResponse response(embedded_test_server(),
-                                                      "/controlled");
   ASSERT_TRUE(embedded_test_server()->Start());
 
-  // 1) Navigate to a page with an iframe that contains yet another iframe, that
+  GURL url_a(embedded_test_server()->GetURL("a.com", "/title1.html"));
-  // hangs while loading.
+  GURL url_b(embedded_test_server()->GetURL("b.com", "/title2.html"));
-  GURL url(embedded_test_server()->GetURL(
-      "a.com", "/back_forward_cache/controllable_subframe_of_subframe.html"));
-  TestNavigationManager navigation_manager(shell()->web_contents(), url);
-  shell()->LoadURL(url);
-
-  // The navigation finishes while the iframe within an iframe is still loading.
-  ASSERT_TRUE(navigation_manager.WaitForNavigationFinished());
-
-  // Wait for the innermost iframe request to arrive, and leave it hanging with
-  // no response.
-  response.WaitForRequest();
 
+  // 1) Navigate to A.
+  EXPECT_TRUE(NavigateToURL(shell(), url_a));
   RenderFrameHostImpl* rfh_a = current_frame_host();
-  RenderFrameDeletedObserver delete_rfh_a(rfh_a);
 
-  // 2) Navigate away.
+  // 2) Create some subframes which have an invalid URL
-  shell()->LoadURL(embedded_test_server()->GetURL("b.com", "/title1.html"));
+  // and thus won't commit a document.
-  EXPECT_TRUE(WaitForLoadStop(shell()->web_contents()));
+  InsertSubFrameWithUrl(rfh_a, "javascript:false");
+  InsertSubFrameWithUrl(rfh_a, "blob:");
+  InsertSubFrameWithUrl(rfh_a, "file:///");
+  // wrongly typed scheme
+  InsertSubFrameWithUrl(rfh_a, "htt://");
+  for (size_t i = 0; i < rfh_a->child_count(); i++) {
+    RenderFrameHostImpl* rfh_subframe =
+        rfh_a->child_at(i)->current_frame_host();
+    EXPECT_FALSE(rfh_subframe->IsDOMContentLoaded());
+    EXPECT_FALSE(rfh_subframe->has_committed_any_navigation());
+  }
 
-  // The page should not have been added to the cache, since it had an iframe
+  // 3) Navigate to B.
-  // that was still loading at the time it was navigated away from.
+  EXPECT_TRUE(NavigateToURL(shell(), url_b));
-  delete_rfh_a.WaitUntilDeleted();
+  // The page A should be stored in the back-forward cache.
+  EXPECT_TRUE(rfh_a->IsInBackForwardCache());
 
-  // 3) Go back.
+  // 4) Go back.
   ASSERT_TRUE(HistoryGoBack(web_contents()));
-  ExpectNotRestored(
+
-      {
+  // The page A should be restored from the back-forward cache.
-          NotRestoredReason::kLoading,
+  ExpectRestored(FROM_HERE);
-          NotRestoredReason::kSubframeIsNavigating,
-      },
-      {}, {}, {}, {}, FROM_HERE);
 }
 
 IN_PROC_BROWSER_TEST_F(BackForwardCacheBrowserTest, DoesNotCacheIfHttpError) {
@@ -998,11 +1061,6 @@ IN_PROC_BROWSER_TEST_F(BackForwardCacheBrowserTest,
   EXPECT_EQ(num_messages_received, 2);
 }
 
-enum class TestFrameType {
-  kMainFrame,
-  kSubFrame,
-};
-
 enum class StickinessType {
   kSticky,
   kNonSticky,

content/browser/back_forward_cache_browsertest.h

@@ -52,6 +52,12 @@ MATCHER(Deleted, "") {
 std::initializer_list<RenderFrameHostImpl*> Elements(
     std::initializer_list<RenderFrameHostImpl*> t);
 
+enum class TestFrameType {
+  kMainFrame,
+  kSubFrame,
+  kSubFrameOfSubframe,
+};
+
 // Test about the BackForwardCache.
 class BackForwardCacheBrowserTest
     : public ContentBrowserTest,

content/browser/renderer_host/back_forward_cache_impl.cc

@@ -940,8 +940,11 @@ void BackForwardCacheImpl::NotRestoredReasonBuilder::
         RenderFrameHostImpl* rfh,
         RequestedFeatures requested_features) {
   DCHECK_NE(requested_features, RequestedFeatures::kOnlySticky);
-  if (!rfh->IsDOMContentLoaded())
+  // The DOM content must have finished loading, except when there is
+  // no DOM content to load when the RFH has not committed any navigation.
+  if (!rfh->IsDOMContentLoaded() && rfh->has_committed_any_navigation()) {
     result.No(BackForwardCacheMetrics::NotRestoredReason::kLoading);
+  }
 
   // Check for non-sticky features that are present at the moment.
   WebSchedulerTrackedFeatures banned_features =

content/test/content_unittests_bundle_data.filelist

@@ -130,6 +130,7 @@
 //content/test/data/back_forward_cache/controllable_subframe_of_subframe.html
 //content/test/data/back_forward_cache/dedicated_worker_in_subframe.html
 //content/test/data/back_forward_cache/disallowed_path.html
+//content/test/data/back_forward_cache/dom_content_loaded_promise.js
 //content/test/data/back_forward_cache/empty.html
 //content/test/data/back_forward_cache/empty.js
 //content/test/data/back_forward_cache/page_with_broadcastchannel.html

content/test/data/back_forward_cache/controllable_subframe.html

@@ -1,2 +1,3 @@
 <!-- /controlled routes to a ControllableHttpResponse driven by the tests. -->
+<script src="dom_content_loaded_promise.js"></script>
 <iframe src="/controlled"></iframe>

content/test/data/back_forward_cache/controllable_subframe_of_subframe.html

@@ -1 +1,2 @@
+<script src="dom_content_loaded_promise.js"></script>
 <iframe src="/back_forward_cache/controllable_subframe.html"></iframe>

content/test/data/back_forward_cache/dom_content_loaded_promise.js

@@ -0,0 +1,8 @@
+// Copyright 2023 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Set up a promise that will be resolve when DOMContentLoaded is fired.
+var domContentLoaded = new Promise(resolve => {
+  document.addEventListener("DOMContentLoaded", () => resolve(42));
+});

third_party/blink/web_tests/external/wpt/html/semantics/embedded-content/bfcache/embedded-not-found-expected.txt

@@ -1,5 +0,0 @@
-This is a testharness.js-based test.
-PASS Page with <embed type=image/png src=/404.png>
-NOTRUN Page with <object type=image/png data=/404.png> Could have been BFCached but actually wasn't
-Harness: the test ran to completion.
-