Browse Source

chore: cherry-pick c244270e23 from chromium. (#26484)

Pedro Pontes 4 years ago
parent
commit
f04721c7c0

+ 1 - 0
patches/chromium/.patches

@@ -140,3 +140,4 @@ cherry-pick-229fdaf8fc05.patch
 cherry-pick-88f263f401b4.patch
 worker_feat_add_hook_to_notify_script_ready.patch
 cherry-pick-8f24f935c903.patch
+ignore_renderframehostimpl_detach_for_speculative_rfhs.patch

+ 165 - 0
patches/chromium/ignore_renderframehostimpl_detach_for_speculative_rfhs.patch

@@ -0,0 +1,165 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: Daniel Cheng <[email protected]>
+Date: Wed, 11 Nov 2020 00:54:41 +0000
+Subject: Ignore RenderFrameHostImpl::Detach() for speculative RFHs.
+
+Currently, this all happens to work by chance, because the speculative
+RFH or the entire FTN happens to be torn down before the browser process
+ever processes a Detach() IPC for a speculative RFH.
+
+However, there are a number of followup CLs that restructure how
+provisional RenderFrames are managed and owned in the renderer process.
+To simplify those CLs, explicitly branch in Detach() based on whether or
+not the RFH is speculative. In the future, additional logic may be added
+to the speculative branch (e.g. cancelling the navigation, if
+appropriate).
+
+(cherry picked from commit cf054220a2e1570a9149220494de8826c2e9d4db)
+
+Bug: 1146709
+Change-Id: I6490a90f7b447422d698676665b52f6f3a6f8ffd
+Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2524280
+Commit-Queue: Daniel Cheng <[email protected]>
+Reviewed-by: Nasko Oskov <[email protected]>
+Cr-Original-Commit-Position: refs/heads/master@{#825903}
+Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2530189
+Reviewed-by: Adrian Taylor <[email protected]>
+Cr-Commit-Position: refs/branch-heads/4240@{#1430}
+Cr-Branched-From: f297677702651916bbf65e59c0d4bbd4ce57d1ee-refs/heads/master@{#800218}
+
+diff --git a/content/browser/frame_host/render_frame_host_impl.cc b/content/browser/frame_host/render_frame_host_impl.cc
+index 8a02ff83c8e40d641530211ef944b62b09a03ac8..3b78a2c33d5ccad8d0a90df895a1f947faab68b4 100644
+--- a/content/browser/frame_host/render_frame_host_impl.cc
++++ b/content/browser/frame_host/render_frame_host_impl.cc
+@@ -2520,6 +2520,9 @@ void RenderFrameHostImpl::UpdateRenderProcessHostFramePriorities() {
+ }
+ 
+ void RenderFrameHostImpl::OnDetach() {
++  if (frame_tree_node_->render_manager()->speculative_frame_host() == this)
++    return;
++
+   if (!parent_) {
+     bad_message::ReceivedBadMessage(GetProcess(),
+                                     bad_message::RFH_DETACH_MAIN_FRAME);
+diff --git a/content/browser/site_per_process_browsertest.cc b/content/browser/site_per_process_browsertest.cc
+index eeaddcacdf714dff2c84cd39e69477b87b2462b4..2d7df6e122bdc5d8270e40d6bbacd68daabe4bd7 100644
+--- a/content/browser/site_per_process_browsertest.cc
++++ b/content/browser/site_per_process_browsertest.cc
+@@ -10347,6 +10347,36 @@ IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest,
+   EXPECT_EQ("opener-ping-reply", response);
+ }
+ 
++IN_PROC_BROWSER_TEST_P(SitePerProcessBrowserTest,
++                       DetachSpeculativeRenderFrameHost) {
++  // Commit a page with one iframe.
++  GURL main_url(embedded_test_server()->GetURL(
++      "a.com", "/cross_site_iframe_factory.html?a(a)"));
++  EXPECT_TRUE(NavigateToURL(shell(), main_url));
++
++  // Start a cross-site navigation.
++  GURL cross_site_url(embedded_test_server()->GetURL("b.com", "/title2.html"));
++  TestNavigationManager nav_manager(shell()->web_contents(), cross_site_url);
++  BeginNavigateIframeToURL(web_contents(), "child-0", cross_site_url);
++
++  // Wait for the request, but don't commit it yet. This should create a
++  // speculative RenderFrameHost.
++  ASSERT_TRUE(nav_manager.WaitForRequestStart());
++  FrameTreeNode* root = web_contents()->GetFrameTree()->root();
++  RenderFrameHostImpl* speculative_rfh = root->current_frame_host()
++                                             ->child_at(0)
++                                             ->render_manager()
++                                             ->speculative_frame_host();
++  EXPECT_TRUE(speculative_rfh);
++
++  // Currently, the browser process never handles an explicit Detach() for a
++  // speculative RFH, since the speculative RFH or the entire FTN is always
++  // destroyed before the renderer sends this IPC.
++  speculative_rfh->Detach();
++
++  // Passes if there is no crash.
++}
++
+ #if defined(OS_ANDROID)
+ 
+ namespace {
+diff --git a/content/public/test/browser_test_utils.cc b/content/public/test/browser_test_utils.cc
+index 98708c6ac228d6bcc969a980c879de20fa3d3269..936f9564a19c3fac2339af7c1fde9946008a8537 100644
+--- a/content/public/test/browser_test_utils.cc
++++ b/content/public/test/browser_test_utils.cc
+@@ -624,17 +624,23 @@ bool NavigateToURL(WebContents* web_contents,
+ }
+ 
+ bool NavigateIframeToURL(WebContents* web_contents,
+-                         std::string iframe_id,
++                         const std::string& iframe_id,
+                          const GURL& url) {
++  TestNavigationObserver load_observer(web_contents);
++  bool result = BeginNavigateIframeToURL(web_contents, iframe_id, url);
++  load_observer.Wait();
++  return result;
++}
++
++bool BeginNavigateIframeToURL(WebContents* web_contents,
++                              const std::string& iframe_id,
++                              const GURL& url) {
+   std::string script = base::StringPrintf(
+       "setTimeout(\""
+       "var iframes = document.getElementById('%s');iframes.src='%s';"
+       "\",0)",
+       iframe_id.c_str(), url.spec().c_str());
+-  TestNavigationObserver load_observer(web_contents);
+-  bool result = ExecuteScript(web_contents, script);
+-  load_observer.Wait();
+-  return result;
++  return ExecuteScript(web_contents, script);
+ }
+ 
+ void NavigateToURLBlockUntilNavigationsComplete(WebContents* web_contents,
+diff --git a/content/public/test/browser_test_utils.h b/content/public/test/browser_test_utils.h
+index 2861386110a0098104d5cf5456cc7507a2cb0ca7..21c15c6af4ea0b3a2be1a4ba61c309231e59578c 100644
+--- a/content/public/test/browser_test_utils.h
++++ b/content/public/test/browser_test_utils.h
+@@ -135,6 +135,12 @@ bool NavigateIframeToURL(WebContents* web_contents,
+                          std::string iframe_id,
+                          const GURL& url);
+ 
++// Similar to |NavigateIframeToURL()| but returns as soon as the navigation is
++// initiated.
++bool BeginNavigateIframeToURL(WebContents* web_contents,
++                              const std::string& iframe_id,
++                              const GURL& url);
++
+ // Generate a URL for a file path including a query string.
+ GURL GetFileUrlWithQuery(const base::FilePath& path,
+                          const std::string& query_string);
+diff --git a/content/test/data/cross_site_iframe_factory.html b/content/test/data/cross_site_iframe_factory.html
+index 2116189a33566bbd241952d276bb82341fd6ecaf..43fb6fb6278e5bbc42797b8206b0546351d46e8e 100644
+--- a/content/test/data/cross_site_iframe_factory.html
++++ b/content/test/data/cross_site_iframe_factory.html
+@@ -10,12 +10,12 @@ Example usage in a browsertest, explained:
+ When you navigate to the above URL, the outer document (on a.com) will create a
+ single iframe:
+ 
+-  <iframe src="http://b.com:1234/cross_site_iframe_factory.html?b(c(),d())">
++  <iframe id="child-0" src="http://b.com:1234/cross_site_iframe_factory.html?b(c(),d())">
+ 
+ Inside of which, then, are created the two leaf iframes:
+ 
+-  <iframe src="http://c.com:1234/cross_site_iframe_factory.html?c()">
+-  <iframe src="http://d.com:1234/cross_site_iframe_factory.html?d()">
++  <iframe id="child-0" src="http://c.com:1234/cross_site_iframe_factory.html?c()">
++  <iframe id="child-1" src="http://d.com:1234/cross_site_iframe_factory.html?d()">
+ 
+ Add iframe options by enclosing them in '{' and '}' characters after the
+ hostname (multiple options can be separated with commas):
+@@ -24,8 +24,8 @@ hostname (multiple options can be separated with commas):
+ 
+ Will create two iframes:
+ 
+-  <iframe src="http://a.com:1234/cross_site_iframe_factory.html?b()" allowfullscreen>
+-  <iframe src="http://c.com:1234/cross_site_iframe_factory.html?c{sandbox-allow-scripts}(d())" sandbox="allow-scripts">
++  <iframe id="child-0" src="http://a.com:1234/cross_site_iframe_factory.html?b()" allowfullscreen>
++  <iframe id="child-1" src="http://c.com:1234/cross_site_iframe_factory.html?c{sandbox-allow-scripts}(d())" sandbox="allow-scripts">
+ 
+ To specify the site for each iframe, you can use a simple identifier like "a"
+ or "b", and ".com" will be automatically appended. You can also specify a port