Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
chore: cherry-pick c244270e23 from chromium.
- Loading branch information
Showing
18 changed files
with
239 additions
and
76 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
162 changes: 162 additions & 0 deletions
162
patches/chromium/ignore_renderframehostimpl_detach_for_speculative_rfhs.patch
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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; | ||
| + | ||
| 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 |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.