After remote subframe stops loading, check the parent.

This CL fixes a problem where a GuestView that stops loading (e.g. a
MimeHandlerView for a PDF) would not trigger a re-checking whether a
parent frame can also report that it stopped loading.  The CL fixes this
by adding a |parent->CheckCompleted()| call to
WebRemoteFrameImpl::DidStopLoading.

In a regular-OOPIF case, the check in the parent would be triggered by:
1. OOPIF process: FrameLoader::DidFinishNavigation
                  -> parent->CheckCompleted()
                  -> RemoteFrame::CheckCompleted()
                  -> ...
                  -> RenderFrameProxy::CheckCompleted()
                  -> send FrameHostMsg_CheckCompleted
2. Parent process: receive FrameMsg_CheckCompleted
                   -> LocalFrame::CheckCompleted()
The flow above is not happening for GuestView, because the main frame
inside the GuestView doesn't know it has a parent.

FWIW, the DidStopLoading issue is (a bit remotely) similar to the
old https://crbug.com/779433.

This CL re-enables a portion of the
SavePageOriginalVsSavedComparisonTest.ObjectElementsViaHttp test,
because after this CL, the test no longer hangs when waiting for the
test page to stop loading.  The remainder of the test will be re-enabled
after fixing find-in-page issues that are tracked in
https://crbug.com/965254.

Bug: 964364
Change-Id: Iee165a88cdfb94b3169919393c563b483c145cda
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1621486
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: David Trainor <dtrainor@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Cr-Commit-Position: refs/heads/master@{#665221}

chrome/browser/download/save_page_browsertest.cc

@@ -495,7 +495,7 @@ IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, MAYBE_SaveHTMLOnlyTabDestroy) {
                                         content::SAVE_PAGE_TYPE_AS_ONLY_HTML));
   std::vector<DownloadItem*> items;
   creation_observer.WaitForDownloadItem(&items);
-  ASSERT_TRUE(items.size() == 1);
+  ASSERT_EQ(1u, items.size());
 
   // Close the tab; does this cancel the download?
   GetCurrentTab(browser())->Close();
@@ -1035,7 +1035,8 @@ class SavePageOriginalVsSavedComparisonTest
       const GURL& url,
       int expected_number_of_frames_in_original_page,
       int expected_number_of_frames_in_mhtml_page,
-      const std::vector<std::string>& expected_substrings) {
+      const std::vector<std::string>& expected_substrings,
+      bool skip_find_in_page = false) {
     // Navigate to the test page and verify if test expectations
     // are met (this is mostly a sanity check - a failure to meet
     // expectations would probably mean that there is a test bug
@@ -1044,7 +1045,8 @@ class SavePageOriginalVsSavedComparisonTest
     DLOG(INFO) << "Verifying test expectations for original page... : "
                << GetCurrentTab(browser())->GetLastCommittedURL();
     AssertExpectationsAboutCurrentTab(
-        expected_number_of_frames_in_original_page, expected_substrings);
+        expected_number_of_frames_in_original_page, expected_substrings,
+        skip_find_in_page);
 
     // Save the page.
     base::FilePath full_file_name, dir;
@@ -1072,7 +1074,7 @@ class SavePageOriginalVsSavedComparisonTest
         expected_number_of_frames_in_mhtml_page :
         expected_number_of_frames_in_original_page;
     AssertExpectationsAboutCurrentTab(expected_number_of_frames_in_saved_page,
-                                      expected_substrings);
+                                      expected_substrings, skip_find_in_page);
 
     if (GetParam() == content::SAVE_PAGE_TYPE_AS_MHTML) {
       std::set<url::Origin> origins;
@@ -1089,21 +1091,26 @@ class SavePageOriginalVsSavedComparisonTest
     chrome::GoBack(browser(), WindowOpenDisposition::CURRENT_TAB);
     content::WaitForLoadStop(GetCurrentTab(browser()));
     AssertExpectationsAboutCurrentTab(expected_number_of_frames_in_saved_page,
-                                      expected_substrings);
+                                      expected_substrings, skip_find_in_page);
   }
 
   // Helper method to deduplicate some code across 2 tests.
   void RunObjectElementsTest(GURL url) {
     content::SavePageType save_page_type = GetParam();
 
-    // 7 comes from:
+    // The |expected_number_of_frames| comes from:
     // - main frame (frames-objects.htm)
     // - object with frame-nested.htm + 2 subframes (frames-nested2.htm + b.htm)
     // - iframe with a.htm
     // - object with svg.svg
     // - object with text.txt
-    // (pdf and png objects do not get a separate frame)
+    // - (in presence of MimeHandlerViewMode::UsesCrossProcessFrame) object with
+    //   pdf.pdf is responsible for presence of 2 extra frames (about:blank +
+    //   one frame for the actual pdf.pdf).  These frames are an implementation
+    //   detail and are not web-exposed (e.g. via window.frames).
     int expected_number_of_frames = 7;
+    if (content::MimeHandlerViewMode::UsesCrossProcessFrame())
+      expected_number_of_frames = 9;
 
     std::vector<std::string> expected_substrings = {
         "frames-objects.htm: 8da13db4-a512-4d9b-b1c5-dc1c134234b9",
@@ -1119,19 +1126,31 @@ class SavePageOriginalVsSavedComparisonTest
     if (save_page_type == content::SAVE_PAGE_TYPE_AS_MHTML)
       return;
 
+    // TODO(lukasza): https://crbug.com/965254: Re-enable find-in-page aspect of
+    // the test.
+    bool skip_find_in_page =
+        content::MimeHandlerViewMode::UsesCrossProcessFrame();
+
     TestOriginalVsSavedPage(save_page_type, url, expected_number_of_frames,
-                            expected_number_of_frames, expected_substrings);
+                            expected_number_of_frames, expected_substrings,
+                            skip_find_in_page);
   }
 
  private:
   void AssertExpectationsAboutCurrentTab(
       int expected_number_of_frames,
-      const std::vector<std::string>& expected_substrings) {
+      const std::vector<std::string>& expected_substrings,
+      bool skip_find_in_page) {
     int actual_number_of_frames = 0;
     GetCurrentTab(browser())->ForEachFrame(base::BindRepeating(
         &IncrementInteger, base::Unretained(&actual_number_of_frames)));
     EXPECT_EQ(expected_number_of_frames, actual_number_of_frames);
 
+    // TODO(lukasza): https://crbug.com/965254: Re-enable find-in-page aspect of
+    // the test.
+    if (skip_find_in_page)
+      return;
+
     for (const auto& expected_substring : expected_substrings) {
       int actual_number_of_matches = ui_test_utils::FindInPage(
           GetCurrentTab(browser()), base::UTF8ToUTF16(expected_substring),
@@ -1205,10 +1224,6 @@ IN_PROC_BROWSER_TEST_P(SavePageOriginalVsSavedComparisonTest, CrossSite) {
 // (see crbug.com/553478).
 IN_PROC_BROWSER_TEST_P(SavePageOriginalVsSavedComparisonTest,
                        ObjectElementsViaHttp) {
-  // TODO(lukasza): https://crbug.com/964364: Re-enable the test.
-  if (content::MimeHandlerViewMode::UsesCrossProcessFrame())
-    return;
-
   GURL url(
       embedded_test_server()->GetURL("a.com", "/save_page/frames-objects.htm"));
 

chrome/browser/pdf/pdf_extension_test.cc

@@ -19,8 +19,10 @@
 #include "base/run_loop.h"
 #include "base/strings/pattern.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/synchronization/lock.h"
 #include "base/task/post_task.h"
 #include "base/test/test_timeouts.h"
+#include "base/thread_annotations.h"
 #include "base/threading/thread_restrictions.h"
 #include "build/build_config.h"
 #include "chrome/browser/chrome_notification_types.h"
@@ -71,6 +73,7 @@
 #include "content/public/test/browser_test_utils.h"
 #include "content/public/test/hit_test_region_observer.h"
 #include "content/public/test/test_navigation_observer.h"
+#include "content/public/test/url_loader_interceptor.h"
 #include "extensions/browser/extension_registry.h"
 #include "extensions/common/manifest_handlers/mime_types_handler.h"
 #include "extensions/test/result_catcher.h"
@@ -2175,3 +2178,92 @@ IN_PROC_BROWSER_TEST_F(PDFExtensionTest, MAYBE_EmbeddedPdfGetsFocus) {
     run_loop.Run();
   }
 }
+
+// A helper for waiting for the first request for |url_to_intercept|.
+class RequestWaiter {
+ public:
+  // Start intercepting requests to |url_to_intercept|.
+  explicit RequestWaiter(const GURL& url_to_intercept)
+      : url_to_intercept_(url_to_intercept),
+        interceptor_(base::BindRepeating(&RequestWaiter::InterceptorCallback,
+                                         base::Unretained(this))) {
+    DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+    DCHECK(url_to_intercept.is_valid());
+  }
+
+  void WaitForRequest() {
+    DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+    if (!IsAlreadyIntercepted())
+      run_loop_.Run();
+    DCHECK(IsAlreadyIntercepted());
+  }
+
+ private:
+  bool InterceptorCallback(
+      content::URLLoaderInterceptor::RequestParams* params) {
+    // This method may be called either on the IO or UI thread.
+    DCHECK(params);
+
+    base::AutoLock lock(lock_);
+    if (url_to_intercept_ != params->url_request.url || already_intercepted_)
+      return false;
+
+    already_intercepted_ = true;
+    run_loop_.Quit();
+    return false;
+  }
+
+  bool IsAlreadyIntercepted() {
+    base::AutoLock lock(lock_);
+    return already_intercepted_;
+  }
+
+  const GURL url_to_intercept_;
+  content::URLLoaderInterceptor interceptor_;
+  base::RunLoop run_loop_;
+
+  base::Lock lock_;
+  bool already_intercepted_ GUARDED_BY(lock_) = false;
+
+  DISALLOW_COPY_AND_ASSIGN(RequestWaiter);
+};
+
+// This is a regression test for a problem where DidStopLoading didn't get
+// propagated from a remote frame into the main frame.  See also
+// https://crbug.com/964364.
+IN_PROC_BROWSER_TEST_F(PDFExtensionTest, DidStopLoading) {
+  // Prepare to wait for requests for the main page of the MimeHandlerView for
+  // PDFs.
+  RequestWaiter interceptor(
+      GURL("chrome-extension://mhjfbmdgcfjbbpaeojofohoefgiehjai/index.html"));
+
+  // Navigate to a page with:
+  //   <embed type="application/pdf" src="test.pdf"></embed>
+  //   <iframe src="/hung"></iframe>
+  // Afterwards, the main page should be still loading because of the hung
+  // subframe (but the subframe for the OOPIF-based PDF MimeHandlerView might or
+  // might not be created at this point).
+  GURL url = embedded_test_server()->GetURL(
+      "/pdf/pdf_embed_with_hung_sibling_subframe.html");
+  ui_test_utils::NavigateToURLWithDisposition(
+      browser(), url, WindowOpenDisposition::CURRENT_TAB,
+      ui_test_utils::BROWSER_TEST_NONE);  // Don't wait for completion.
+
+  // Wait for the request for the MimeHandlerView extension.  Afterwards, the
+  // main page should be still loading because of
+  // 1) the MimeHandlerView frame is loading
+  // 2) the hung subframe is loading.
+  interceptor.WaitForRequest();
+
+  // Remove the hung subframe.  Afterwards the main page should stop loading as
+  // soon as the MimeHandlerView frame stops loading (assumming we have not bugs
+  // similar to https://crbug.com/964364).
+  content::WebContents* web_contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  ASSERT_TRUE(content::ExecJs(
+      web_contents, "document.getElementById('hung_subframe').remove();"));
+
+  // MAIN VERIFICATION: Wait for the main frame to report that is has stopped
+  // loading.
+  content::WaitForLoadStop(web_contents);
+}

chrome/test/data/pdf/pdf_embed_with_hung_sibling_subframe.html

+<!doctype html>
+<embed type="application/pdf" src="test.pdf"></embed>
+<iframe id="hung_subframe" src="/hung"></iframe>

third_party/blink/renderer/core/exported/web_remote_frame_impl.cc

@@ -4,6 +4,8 @@
 
 #include "third_party/blink/renderer/core/exported/web_remote_frame_impl.h"
 
+#include <utility>
+
 #include "third_party/blink/public/common/feature_policy/feature_policy.h"
 #include "third_party/blink/public/platform/web_float_rect.h"
 #include "third_party/blink/public/platform/web_intrinsic_sizing_info.h"
@@ -326,6 +328,14 @@ void WebRemoteFrameImpl::DidStartLoading() {
 
 void WebRemoteFrameImpl::DidStopLoading() {
   GetFrame()->SetIsLoading(false);
+
+  // When a subframe finishes loading, the parent should check if *all*
+  // subframes have finished loading (which may mean that the parent can declare
+  // that the parent itself has finished loading).  This remote-subframe-focused
+  // code has a local-subframe equivalent in FrameLoader::DidFinishNavigation.
+  Frame* parent = GetFrame()->Tree().Parent();
+  if (parent)
+    parent->CheckCompleted();
 }
 
 bool WebRemoteFrameImpl::IsIgnoredForHitTest() const {

third_party/blink/renderer/core/loader/frame_loader.cc

@@ -37,6 +37,8 @@
 #include "third_party/blink/renderer/core/loader/frame_loader.h"
 
 #include <memory>
+#include <utility>
+
 #include "base/auto_reset.h"
 #include "base/unguessable_token.h"
 #include "third_party/blink/public/common/features.h"
@@ -417,6 +419,11 @@ void FrameLoader::DidFinishNavigation() {
     frame_->FinishedLoading();
   }
 
+  // When a subframe finishes loading, the parent should check if *all*
+  // subframes have finished loading (which may mean that the parent can declare
+  // that the parent itself has finished loading).  This local-subframe-focused
+  // code has a remote-subframe equivalent in
+  // WebRemoteFrameImpl::DidStopLoading.
   Frame* parent = frame_->Tree().Parent();
   if (parent)
     parent->CheckCompleted();