Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: cherry-pick c244270e23 from chromium. #26477

Merged
merged 2 commits into from Nov 13, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions patches/chromium/.patches
Expand Up @@ -152,3 +152,4 @@ cherry-pick-1ed869ad4bb3.patch
cherry-pick-8f24f935c903.patch
crashpad-initialize-logging.patch
make_macos_os_version_numbers_consistent.patch
ignore_renderframehostimpl_detach_for_speculative_rfhs.patch
@@ -0,0 +1,162 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Daniel Cheng <dcheng@chromium.org>
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 <dcheng@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#825903}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2530189
Reviewed-by: Adrian Taylor <adetaylor@chromium.org>
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 8a3afc59f96e0f29997d0e239726217d490189d8..0c71cf19a2d8587e3e341d963da72c03a092b453 100644
--- a/content/browser/frame_host/render_frame_host_impl.cc
+++ b/content/browser/frame_host/render_frame_host_impl.cc
@@ -2403,6 +2403,9 @@ void RenderFrameHostImpl::UpdateRenderProcessHostFramePriorities() {
}

void RenderFrameHostImpl::OnDetach() {
+ if (frame_tree_node_->render_manager()->speculative_frame_host() == this)
+ return;
nornagon marked this conversation as resolved.
Show resolved Hide resolved
+
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 1e8f9b19e4bdeb0b6a371e384e30e10b7986137e..7770582317986869ff38658e2abfd4ba836c5bca 100644
--- a/content/browser/site_per_process_browsertest.cc
+++ b/content/browser/site_per_process_browsertest.cc
@@ -10362,6 +10362,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 d7f88a819505305d690e35b3b78862c1b9fd8f5e..9b2f0bb265293549b84d37279ba1e2e7c314a737 100644
--- a/content/public/test/browser_test_utils.cc
+++ b/content/public/test/browser_test_utils.cc
@@ -624,15 +624,21 @@ bool NavigateToURL(WebContents* web_contents,
bool NavigateIframeToURL(WebContents* web_contents,
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 8c2e904462c655c9a587e5243572e60b8327ad1c..d4b16a127f92559469f386b734ffa1e6d20eb51e 100644
--- a/content/public/test/browser_test_utils.h
+++ b/content/public/test/browser_test_utils.h
@@ -137,6 +137,12 @@ bool NavigateIframeToURL(WebContents* web_contents,
const 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 959f45a6be7f233082e364f90d6875d125ae6fe6..e4807d1ad3f7526d7b21843ba8f49e50f7ed8d7e 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