/
ignore_renderframehostimpl_detach_for_speculative_rfhs.patch
162 lines (145 loc) · 7.9 KB
/
ignore_renderframehostimpl_detach_for_speculative_rfhs.patch
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
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