Always reset RFH before `NavigationRequest::OnRequestFailedInternal()`
When a NavigationRequest fails, the `render_frame_host_` member in the
NavigationRequest might have been chosen previously. If the
NavigationRequest is needed for an error page commit, a new value of
`render_frame_host_` will be recomputed, which will delete the
previously picked speculative RFH.
In some cases, the `render_frame_host_` member in the
NavigationRequest is not reset before `OnRequestFailedInternal()`,
and it will cause the destruction of the NavigationRequest when the
RFH is destroyed. Since the NavigationRequest is used for the error
page commit, there will be a potential UaF case.
This CL ensures the `render_frame_host_` is always set to null before
`OnRequestFailedInternal()`, it also removes one of the
NavigationRequest reset logic (which we believe should not happen) from
the RFH's destructor. A browser test is added for the situation
mentioned above.
Bug: 1487944
Change-Id: I295dc4fbb95e82bcc494b0b872d4648341aa6177
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4924175
Reviewed-by: Rakina Zata Amni <rakina@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: Mingyu Lei <leimy@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1208754}
diff --git a/content/browser/renderer_host/navigation_request.cc b/content/browser/renderer_host/navigation_request.cc
index 8a6186c..ab00c68 100644
--- a/content/browser/renderer_host/navigation_request.cc
+++ b/content/browser/renderer_host/navigation_request.cc
@@ -2717,6 +2717,9 @@
void NavigationRequest::
SelectFrameHostForCrossDocumentNavigationWithNoUrlLoader() {
DCHECK(!NeedsUrlLoader());
+ CHECK(!HasRenderFrameHost())
+ << "`render_frame_host_` should not be set before the "
+ "`NavigationRequest` starts to select the RFH.";
if (auto result =
frame_tree_node_->render_manager()->GetFrameHostForNavigation(
@@ -4283,6 +4286,9 @@
network::mojom::URLLoaderClientEndpointsPtr url_loader_client_endpoints,
bool is_download,
absl::optional<SubresourceLoaderParams> subresource_loader_params) {
+ CHECK(!HasRenderFrameHost())
+ << "`render_frame_host_` should not be set before the "
+ "`NavigationRequest` starts to select the RFH.";
ScopedCrashKeys crash_keys(*this);
std::string rfh_selected_reason;
@@ -4671,6 +4677,13 @@
// anymore from now while the error page is being loaded.
loader_.reset();
+ // Reset the RenderFrameHost R1 that had been computed for committing the
+ // failed navigation. This breaks the binding between the current
+ // NavigationRequest and R1, so that if we create another speculative
+ // RenderFrameHost R2 to commit an error page after this, deleting R1 won't
+ // try to delete this NavigationRequest along with it.
+ render_frame_host_ = absl::nullopt;
+
ssl_info_ = status.ssl_info;
devtools_instrumentation::OnNavigationRequestFailed(*this, status);
@@ -4722,6 +4735,10 @@
bool exists_in_cache,
bool skip_throttles,
const absl::optional<std::string>& error_page_content) {
+ CHECK(!HasRenderFrameHost())
+ << "`render_frame_host_` should not be set before the "
+ "`NavigationRequest` starts to select the RFH.";
+
switch (ComputeErrorPageProcess()) {
case ErrorPageProcess::kCurrentProcess:
// There's no way to get here with a same-document navigation, it would
@@ -4780,10 +4797,6 @@
}
}
- // Sanity check that we haven't changed the RenderFrameHost picked for the
- // error page in OnRequestFailedInternal when running the WillFailRequest
- // checks.
- CHECK(!HasRenderFrameHost() || GetRenderFrameHost() == render_frame_host);
render_frame_host_ = render_frame_host->GetSafeRef();
// Update the associated RenderFrameHost type.
@@ -5426,21 +5439,15 @@
!response_should_be_rendered_) {
MaybeAddResourceTimingEntryForCancelledNavigation();
- // Reset the RenderFrameHost that had been computed for the commit of the
- // navigation.
- // TODO(https://crbug.com/1416916): Reconsider if we really need to unset
- // the `render_frame_host_` here, as the NavigationRequest might stay alive
- // for a bit longer to commit an error page.
- render_frame_host_ = absl::nullopt;
-
// TODO(clamy): distinguish between CANCEL and CANCEL_AND_IGNORE.
if (!response_should_be_rendered_) {
- // DO NOT ADD CODE after this. The previous call to OnRequestFailed has
- // destroyed the NavigationRequest.
OnRequestFailedInternal(
network::URLLoaderCompletionStatus(net::ERR_ABORTED),
true /* skip_throttles */, absl::nullopt /* error_page_content */,
false /* collapse_frame */);
+
+ // DO NOT ADD CODE after this. The previous call to
+ // OnRequestFailedInternal has destroyed the NavigationRequest.
return;
}
@@ -5458,12 +5465,6 @@
if (result.action() == NavigationThrottle::BLOCK_RESPONSE) {
DCHECK_EQ(net::ERR_BLOCKED_BY_RESPONSE, result.net_error_code());
- // Reset the RenderFrameHost that had been computed for the commit of the
- // navigation.
- // TODO(https://crbug.com/1416916): Reconsider if we really need to unset
- // the `render_frame_host_` here, as the NavigationRequest might stay alive
- // for a bit longer to commit an error page.
- render_frame_host_ = absl::nullopt;
OnRequestFailedInternal(
network::URLLoaderCompletionStatus(result.net_error_code()),
true /* skip_throttles */, result.error_page_content(),
diff --git a/content/browser/renderer_host/navigation_request_browsertest.cc b/content/browser/renderer_host/navigation_request_browsertest.cc
index 77f190d..31b9039 100644
--- a/content/browser/renderer_host/navigation_request_browsertest.cc
+++ b/content/browser/renderer_host/navigation_request_browsertest.cc
@@ -4894,4 +4894,54 @@
~network::mojom::WebSandboxFlags::kAutomaticFeatures);
}
+// Tests the scenario when a navigation without URLLoader is cancelled and an
+// error page is committed using the same NavigationRequest.
+// See https://crbug.com/1487944.
+IN_PROC_BROWSER_TEST_F(
+ NavigationRequestBrowserTest,
+ ThrottleDeferAndCancelCommitWithoutUrlLoaderWithErrorPage) {
+ GURL url(embedded_test_server()->GetURL("/title1.html"));
+ GURL about_blank_url(url::kAboutBlankURL);
+
+ // Perform a new-document navigation (setup).
+ EXPECT_TRUE(NavigateToURL(shell(), url));
+
+ // Navigate to about:blank so the NavigationRequest is expected to commit
+ // without URL loader.
+ {
+ TestNavigationObserver navigation_observer(shell()->web_contents(), 1);
+ NavigationHandleObserver observer(shell()->web_contents(), about_blank_url);
+ TestNavigationThrottleInstaller installer(
+ shell()->web_contents(), NavigationThrottle::DEFER,
+ NavigationThrottle::DEFER, NavigationThrottle::DEFER,
+ NavigationThrottle::DEFER, NavigationThrottle::DEFER);
+
+ shell()->LoadURL(about_blank_url);
+
+ // Wait for WillCommitWithoutUrlLoader.
+ installer.WaitForThrottleWillCommitWithoutUrlLoader();
+ EXPECT_EQ(0, installer.will_start_called());
+ EXPECT_EQ(0, installer.will_redirect_called());
+ EXPECT_EQ(0, installer.will_fail_called());
+ EXPECT_EQ(0, installer.will_process_called());
+ EXPECT_EQ(1, installer.will_commit_without_url_loader_called());
+
+ // Cancel the deferred navigation with `net::ERR_BLOCKED_BY_RESPONSE`, so
+ // the NavigationRequest will be used for an error page commit.
+ installer.navigation_throttle()->CancelNavigation(
+ {NavigationThrottle::CANCEL_AND_IGNORE, net::ERR_BLOCKED_BY_RESPONSE});
+
+ // Wait for the end of the navigation.
+ navigation_observer.Wait();
+
+ EXPECT_FALSE(observer.is_same_document());
+ EXPECT_TRUE(observer.has_committed());
+ EXPECT_FALSE(observer.was_redirected());
+ EXPECT_TRUE(observer.is_error());
+
+ EXPECT_TRUE(
+ shell()->web_contents()->GetPrimaryMainFrame()->IsErrorDocument());
+ }
+}
+
} // namespace content
diff --git a/content/browser/renderer_host/render_frame_host_impl.cc b/content/browser/renderer_host/render_frame_host_impl.cc
index b7ad13fd..abdf7ba 100644
--- a/content/browser/renderer_host/render_frame_host_impl.cc
+++ b/content/browser/renderer_host/render_frame_host_impl.cc
@@ -1716,8 +1716,6 @@
} else if (navigation_request->state() >=
NavigationRequest::WILL_PROCESS_RESPONSE &&
navigation_request->GetRenderFrameHost() == this) {
- frame_tree_node_->ResetNavigationRequest(
- NavigationDiscardReason::kRenderFrameHostDestruction);
// As we are unable to come up with a case that will lead to this path,
// we instead record the dumps for debugging the scenario.
// TODO(crbug.com/1430653): if we verify that this path is impossible,
