Browse Source

chore: cherry-pick 8731bd8a30f6 from chromium (#37658)

Co-authored-by: electron-patch-conflict-fixer[bot] <83340002+electron-patch-conflict-fixer[bot]@users.noreply.github.com>
Pedro Pontes 2 years ago
parent
commit
2809de80f6
2 changed files with 602 additions and 0 deletions
  1. 1 0
      patches/chromium/.patches
  2. 601 0
      patches/chromium/cherry-pick-8731bd8a30f6.patch

+ 1 - 0
patches/chromium/.patches

@@ -155,4 +155,5 @@ cherry-pick-0407102d19b9.patch
 cherry-pick-38de42d2bbc3.patch
 cherry-pick-bfd926be8178.patch
 cherry-pick-d202ad3c6aeb.patch
+cherry-pick-8731bd8a30f6.patch
 cherry-pick-1235110fce18.patch

+ 601 - 0
patches/chromium/cherry-pick-8731bd8a30f6.patch

@@ -0,0 +1,601 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: Alex Moshchuk <[email protected]>
+Date: Fri, 3 Mar 2023 22:17:24 +0000
+Subject: Use optional SafeRef to save RenderFrameHost in NavigationRequest
+
+This prevents use-after-free if NavigationRequests somehow still
+points to an already-deleted RFH, which is currently possible (see bug).
+
+Also converts usages of `render_frame_host_` to use the
+GetRenderFrameHost() function to ensure that they are all called after
+the final RenderFrameHost is picked for the navigation.
+
+(cherry picked from commit 7b75ae34df6d15acf4e5a45f12c9dca4ce7f2586)
+
+Bug: 1416916
+Change-Id: I45569e7bb1f160158dc3139fc9e49d7d6bb56738
+Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4278923
+Reviewed-by: Hiroki Nakagawa <[email protected]>
+Commit-Queue: Rakina Zata Amni <[email protected]>
+Reviewed-by: Alex Moshchuk <[email protected]>
+Cr-Original-Commit-Position: refs/heads/main@{#1112656}
+Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4308070
+Reviewed-by: Charlie Reis <[email protected]>
+Commit-Queue: Alex Moshchuk <[email protected]>
+Cr-Commit-Position: refs/branch-heads/5481@{#1322}
+Cr-Branched-From: 130f3e4d850f4bc7387cfb8d08aa993d288a67a9-refs/heads/main@{#1084008}
+
+diff --git a/content/browser/renderer_host/navigation_request.cc b/content/browser/renderer_host/navigation_request.cc
+index 060b310d38db85944e37b8a202493212106d8946..9d4627b2014f3f924bdaf03a68347790a24d3498 100644
+--- a/content/browser/renderer_host/navigation_request.cc
++++ b/content/browser/renderer_host/navigation_request.cc
+@@ -1404,7 +1404,7 @@ NavigationRequest::CreateForSynchronousRendererCommit(
+             subresource_web_bundle_navigation_info->token(),
+             subresource_web_bundle_navigation_info->render_process_id()));
+   }
+-  navigation_request->render_frame_host_ = render_frame_host;
++  navigation_request->render_frame_host_ = render_frame_host->GetSafeRef();
+   navigation_request->coep_reporter_ = std::move(coep_reporter);
+   navigation_request->isolation_info_for_subresources_ =
+       isolation_info_for_subresources;
+@@ -1897,7 +1897,7 @@ NavigationRequest::GetCommitDeferringConditionForTesting() {
+ void NavigationRequest::BeginNavigation() {
+   EnterChildTraceEvent("BeginNavigation", this);
+   DCHECK(!loader_);
+-  DCHECK(!render_frame_host_);
++  DCHECK(!HasRenderFrameHost());
+   ScopedCrashKeys crash_keys(*this);
+ 
+   if (begin_navigation_callback_for_testing_) {
+@@ -2243,7 +2243,7 @@ void NavigationRequest::BeginNavigationImpl() {
+     // expect us to use the current RenderFrameHost for this NavigationRequest,
+     // and https://crbug.com/1125106.
+     if (IsSameDocument()) {
+-      render_frame_host_ = frame_tree_node_->current_frame_host();
++      render_frame_host_ = frame_tree_node_->current_frame_host()->GetSafeRef();
+     } else {
+       // [spec]: https://html.spec.whatwg.org/C/#process-a-navigate-response
+       // 4. if [...] the result of checking a navigation response's adherence to
+@@ -2268,11 +2268,12 @@ void NavigationRequest::BeginNavigationImpl() {
+           origin, net::NetworkIsolationKey(origin, origin));
+ 
+       // Select an appropriate RenderFrameHost.
+-      render_frame_host_ =
+-          frame_tree_node_->render_manager()->GetFrameHostForNavigation(this);
++      render_frame_host_ = frame_tree_node_->render_manager()
++                               ->GetFrameHostForNavigation(this)
++                               ->GetSafeRef();
+ 
+       CHECK(Navigator::CheckWebUIRendererDoesNotDisplayNormalURL(
+-          render_frame_host_, GetUrlInfo(),
++          &*render_frame_host_.value(), GetUrlInfo(),
+           /*is_renderer_initiated_check=*/false));
+     }
+ 
+@@ -2495,8 +2496,8 @@ void NavigationRequest::ResetForCrossDocumentRestart() {
+ 
+   // Reset the previously selected RenderFrameHost. This is expected to be null
+   // at the beginning of a new navigation. See https://crbug.com/936962.
+-  DCHECK(render_frame_host_);
+-  render_frame_host_ = nullptr;
++  DCHECK(HasRenderFrameHost());
++  render_frame_host_ = absl::nullopt;
+ 
+   // Convert the navigation type to the appropriate cross-document one.
+   common_params_->navigation_type =
+@@ -2554,7 +2555,7 @@ mojom::NavigationClient* NavigationRequest::GetCommitNavigationClient() {
+ 
+   // Instantiate a new NavigationClient interface.
+   commit_navigation_client_ =
+-      render_frame_host_->GetNavigationClientFromInterfaceProvider();
++      GetRenderFrameHost()->GetNavigationClientFromInterfaceProvider();
+   HandleInterfaceDisconnection(
+       &commit_navigation_client_,
+       base::BindOnce(
+@@ -2618,7 +2619,7 @@ void NavigationRequest::CreateCoepReporter(
+       common_params_->url,
+       policies.cross_origin_embedder_policy.reporting_endpoint,
+       policies.cross_origin_embedder_policy.report_only_reporting_endpoint,
+-      render_frame_host_->GetFrameToken().value(),
++      GetRenderFrameHost()->GetFrameToken().value(),
+       isolation_info_for_subresources_.network_isolation_key());
+ }
+ 
+@@ -3019,7 +3020,7 @@ void NavigationRequest::DetermineOriginAgentClusterEndResult() {
+           ? url::Origin::Create(common_params_->base_url_for_data_url)
+           : url::Origin::Create(common_params_->url);
+   const IsolationContext& isolation_context =
+-      render_frame_host_->GetSiteInstance()->GetIsolationContext();
++      GetRenderFrameHost()->GetSiteInstance()->GetIsolationContext();
+ 
+   bool is_requested = IsOriginAgentClusterOptInRequested();
+   bool expects_origin_agent_cluster = is_requested || IsIsolationImplied();
+@@ -3127,7 +3128,7 @@ void NavigationRequest::ProcessOriginAgentClusterEndResult() {
+       origin_agent_cluster_end_result_ ==
+           OriginAgentClusterEndResult::kExplicitlyRequestedButNotOriginKeyed) {
+     GetContentClient()->browser()->LogWebFeatureForCurrentPage(
+-        render_frame_host_,
++        GetRenderFrameHost(),
+         blink::mojom::WebFeature::kOriginAgentClusterHeader);
+   }
+ 
+@@ -3137,7 +3138,7 @@ void NavigationRequest::ProcessOriginAgentClusterEndResult() {
+           OriginAgentClusterEndResult::kRequestedButNotOriginKeyed ||
+       origin_agent_cluster_end_result_ ==
+           OriginAgentClusterEndResult::kExplicitlyRequestedButNotOriginKeyed) {
+-    render_frame_host_->AddMessageToConsole(
++    GetRenderFrameHost()->AddMessageToConsole(
+         blink::mojom::ConsoleMessageLevel::kWarning,
+         base::StringPrintf(
+             "The page requested an origin-keyed agent cluster using the "
+@@ -3152,7 +3153,7 @@ void NavigationRequest::ProcessOriginAgentClusterEndResult() {
+           OriginAgentClusterEndResult::kNotRequestedButOriginKeyed ||
+       origin_agent_cluster_end_result_ ==
+           OriginAgentClusterEndResult::kExplicitlyNotRequestedButOriginKeyed) {
+-    render_frame_host_->AddMessageToConsole(
++    GetRenderFrameHost()->AddMessageToConsole(
+         blink::mojom::ConsoleMessageLevel::kWarning,
+         base::StringPrintf(
+             "The page did not request an origin-keyed agent cluster, but was "
+@@ -3675,45 +3676,47 @@ void NavigationRequest::OnResponseStarted(
+     BackForwardCacheImpl::Entry* entry =
+         controller->GetBackForwardCache().GetEntry(nav_entry_id_);
+     CHECK(entry);
+-    render_frame_host_ = entry->render_frame_host();
+-    CHECK(render_frame_host_);
++    CHECK(entry->render_frame_host());
++    render_frame_host_ = entry->render_frame_host()->GetSafeRef();
+   } else if (IsPrerenderedPageActivation()) {
+     // Prerendering requires changing pages starting at the root node.
+     DCHECK(IsInMainFrame());
+ 
+-    render_frame_host_ =
+-        GetPrerenderHostRegistry().GetRenderFrameHostForReservedHost(
+-            prerender_frame_tree_node_id_.value());
+-    // TODO(https://crbug.com/1181712): Handle the cases when the prerender is
+-    // cancelled and RFH is destroyed while NavigationRequest is alive.
++    render_frame_host_ = GetPrerenderHostRegistry()
++                             .GetRenderFrameHostForReservedHost(
++                                 prerender_frame_tree_node_id_.value())
++                             ->GetSafeRef();
+   } else if (response_should_be_rendered_) {
+-    render_frame_host_ =
+-        frame_tree_node_->render_manager()->GetFrameHostForNavigation(this);
++    render_frame_host_ = frame_tree_node_->render_manager()
++                             ->GetFrameHostForNavigation(this)
++                             ->GetSafeRef();
+ 
+     // Update the associated RenderFrameHost type, which could have changed
+     // due to redirects during navigation.
+     set_associated_rfh_type(
+-        render_frame_host_ ==
++        GetRenderFrameHost() ==
+                 frame_tree_node_->render_manager()->current_frame_host()
+             ? AssociatedRenderFrameHostType::CURRENT
+             : AssociatedRenderFrameHostType::SPECULATIVE);
+ 
+     if (!Navigator::CheckWebUIRendererDoesNotDisplayNormalURL(
+-            render_frame_host_, GetUrlInfo(),
++            GetRenderFrameHost(), GetUrlInfo(),
+             /* is_renderer_initiated_check */ false)) {
+       CHECK(false);
+     }
+   } else {
+-    render_frame_host_ = nullptr;
++    render_frame_host_ = absl::nullopt;
+   }
+-  if (!render_frame_host_)
++  if (!HasRenderFrameHost()) {
+     DCHECK(!response_should_be_rendered_);
++  }
+ 
+-  if (render_frame_host_)
++  if (HasRenderFrameHost()) {
+     DetermineOriginAgentClusterEndResult();
++  }
+ 
+-  if (!commit_params_->is_browser_initiated && render_frame_host_ &&
+-      render_frame_host_->GetProcess() !=
++  if (!commit_params_->is_browser_initiated && HasRenderFrameHost() &&
++      GetRenderFrameHost()->GetProcess() !=
+           frame_tree_node_->current_frame_host()->GetProcess()) {
+     // Allow the embedder to cancel the cross-process commit if needed.
+     if (!frame_tree_node_->navigator()
+@@ -3733,12 +3736,12 @@ void NavigationRequest::OnResponseStarted(
+ 
+   subresource_loader_params_ = std::move(subresource_loader_params);
+ 
+-  if (render_frame_host_) {
++  if (HasRenderFrameHost()) {
+     // Set the site URL now if it hasn't been set already. If the site requires
+     // a dedicated process, this will lock the process to that site, which will
+     // prevent other sites from incorrectly reusing this process. See
+     // https://crbug.com/738634.
+-    SiteInstanceImpl* instance = render_frame_host_->GetSiteInstance();
++    SiteInstanceImpl* instance = GetRenderFrameHost()->GetSiteInstance();
+     if (!instance->HasSite() &&
+         SiteInstanceImpl::ShouldAssignSiteForURL(common_params_->url)) {
+       instance->ConvertToDefaultOrSetSite(GetUrlInfo());
+@@ -3760,7 +3763,7 @@ void NavigationRequest::OnResponseStarted(
+     // navigation, and in the meantime another navigation reads the incorrect
+     // IsUnused() value from the same process when making a process reuse
+     // decision.
+-    render_frame_host_->GetProcess()->SetIsUsed();
++    GetRenderFrameHost()->GetProcess()->SetIsUsed();
+ 
+     // Now that we know the IsolationContext for the assigned SiteInstance, we
+     // opt the origin into OAC here if needed. Note that this doesn't need to
+@@ -3874,7 +3877,7 @@ void NavigationRequest::OnResponseStarted(
+     return;
+   }
+ 
+-  if (render_frame_host_ &&
++  if (HasRenderFrameHost() &&
+       !CheckPermissionsPoliciesForFencedFrames(GetOriginToCommit())) {
+     OnRequestFailedInternal(
+         network::URLLoaderCompletionStatus(net::ERR_ABORTED),
+@@ -3904,7 +3907,7 @@ NavigationRequest::CreateNavigationEarlyHintsManagerParams(
+     const network::mojom::EarlyHints& early_hints) {
+   // Early Hints preloads should happen only before the final response is
+   // received, and limited only in the main frame for now.
+-  CHECK(!render_frame_host_);
++  CHECK(!HasRenderFrameHost());
+   CHECK(loader_);
+   CHECK_LT(state_, WILL_PROCESS_RESPONSE);
+   CHECK(!IsSameDocument());
+@@ -4047,12 +4050,12 @@ void NavigationRequest::OnRequestFailedInternal(
+   // Sanity check that we haven't changed the RenderFrameHost picked for the
+   // error page in OnRequestFailedInternal when running the WillFailRequest
+   // checks.
+-  CHECK(!render_frame_host_ || render_frame_host_ == render_frame_host);
+-  render_frame_host_ = render_frame_host;
++  CHECK(!HasRenderFrameHost() || GetRenderFrameHost() == render_frame_host);
++  render_frame_host_ = render_frame_host->GetSafeRef();
+ 
+   // Update the associated RenderFrameHost type.
+   set_associated_rfh_type(
+-      render_frame_host_ ==
++      GetRenderFrameHost() ==
+               frame_tree_node_->render_manager()->current_frame_host()
+           ? AssociatedRenderFrameHostType::CURRENT
+           : AssociatedRenderFrameHostType::SPECULATIVE);
+@@ -4060,17 +4063,17 @@ void NavigationRequest::OnRequestFailedInternal(
+   // Set the site URL now if it hasn't been set already.  It's possible to get
+   // here if we navigate to an error out of an initial "blank" SiteInstance.
+   // Also mark the process as used, since it will be hosting an error page.
+-  SiteInstanceImpl* instance = render_frame_host_->GetSiteInstance();
++  SiteInstanceImpl* instance = GetRenderFrameHost()->GetSiteInstance();
+   if (!instance->HasSite())
+     instance->ConvertToDefaultOrSetSite(GetUrlInfo());
+-  render_frame_host_->GetProcess()->SetIsUsed();
++  GetRenderFrameHost()->GetProcess()->SetIsUsed();
+ 
+   // The check for WebUI should be performed only if error page isolation is
+   // enabled for this failed navigation. It is possible for subframe error page
+   // to be committed in a WebUI process as shown in https://crbug.com/944086.
+   if (frame_tree_node_->IsErrorPageIsolationEnabled()) {
+     if (!Navigator::CheckWebUIRendererDoesNotDisplayNormalURL(
+-            render_frame_host_, GetUrlInfo(),
++            GetRenderFrameHost(), GetUrlInfo(),
+             /* is_renderer_initiated_check */ false)) {
+       CHECK(false);
+     }
+@@ -4378,7 +4381,7 @@ void NavigationRequest::OnStartChecksComplete(
+           ->CreateURLLoaderNetworkObserverForNavigationRequest(*this),
+       NetworkServiceDevToolsObserver::MakeSelfOwned(frame_tree_node_),
+       std::move(cached_response_head), std::move(interceptor));
+-  DCHECK(!render_frame_host_);
++  DCHECK(!HasRenderFrameHost());
+ 
+   base::UmaHistogramTimes(
+       base::StrCat({"Navigation.WillStartRequestToLoaderStart.",
+@@ -4650,7 +4653,10 @@ void NavigationRequest::OnWillProcessResponseChecksComplete(
+       !response_should_be_rendered_) {
+     // Reset the RenderFrameHost that had been computed for the commit of the
+     // navigation.
+-    render_frame_host_ = nullptr;
++    // 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_) {
+@@ -4679,7 +4685,10 @@ void NavigationRequest::OnWillProcessResponseChecksComplete(
+     DCHECK_EQ(net::ERR_BLOCKED_BY_RESPONSE, result.net_error_code());
+     // Reset the RenderFrameHost that had been computed for the commit of the
+     // navigation.
+-    render_frame_host_ = nullptr;
++    // 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(),
+@@ -4752,7 +4761,7 @@ void NavigationRequest::CommitErrorPage(
+   commit_params_->origin_to_commit =
+       url::Origin::Create(common_params_->url).DeriveNewOpaqueOrigin();
+   if (request_navigation_client_.is_bound()) {
+-    if (render_frame_host_ == frame_tree_node()->current_frame_host()) {
++    if (GetRenderFrameHost() == frame_tree_node()->current_frame_host()) {
+       // Reuse the request NavigationClient for commit.
+       commit_navigation_client_ = std::move(request_navigation_client_);
+     } else {
+@@ -4766,7 +4775,7 @@ void NavigationRequest::CommitErrorPage(
+ 
+   // Use a separate cache shard, and no cookies, for error pages.
+   isolation_info_for_subresources_ = net::IsolationInfo::CreateTransient();
+-  render_frame_host_->FailedNavigation(
++  GetRenderFrameHost()->FailedNavigation(
+       this, *common_params_, *commit_params_, has_stale_copy_in_cache_,
+       net_error_, extended_error_code_, error_page_content);
+ 
+@@ -4833,7 +4842,7 @@ void NavigationRequest::CommitNavigation() {
+   // TODO(crbug.com/979296): Consider changing this code to copy an origin
+   // instead of creating one from a URL which lacks opacity information.
+   isolation_info_for_subresources_ =
+-      render_frame_host_->ComputeIsolationInfoForSubresourcesForPendingCommit(
++      GetRenderFrameHost()->ComputeIsolationInfoForSubresourcesForPendingCommit(
+           origin, is_anonymous());
+   DCHECK(!isolation_info_for_subresources_.IsEmpty());
+ 
+@@ -4841,8 +4850,8 @@ void NavigationRequest::CommitNavigation() {
+   // moment. We will be able to use it once the browser can compute the origin
+   // to commit.
+   absl::optional<base::UnguessableToken> nonce =
+-      render_frame_host_->ComputeNonce(is_anonymous());
+-  commit_params_->storage_key = render_frame_host_->CalculateStorageKey(
++      GetRenderFrameHost()->ComputeNonce(is_anonymous());
++  commit_params_->storage_key = GetRenderFrameHost()->CalculateStorageKey(
+       GetOriginToCommit(), base::OptionalOrNullptr(nonce));
+ 
+   if (IsServedFromBackForwardCache() || IsPrerenderedPageActivation()) {
+@@ -4863,13 +4872,13 @@ void NavigationRequest::CommitNavigation() {
+   if (!weak_self)
+     return;
+ 
+-  DCHECK(render_frame_host_ ==
++  DCHECK(GetRenderFrameHost() ==
+              frame_tree_node_->render_manager()->current_frame_host() ||
+-         render_frame_host_ ==
++         GetRenderFrameHost() ==
+              frame_tree_node_->render_manager()->speculative_frame_host());
+ 
+   if (request_navigation_client_.is_bound()) {
+-    if (render_frame_host_ == frame_tree_node()->current_frame_host()) {
++    if (GetRenderFrameHost() == frame_tree_node()->current_frame_host()) {
+       // Reuse the request NavigationClient for commit.
+       commit_navigation_client_ = std::move(request_navigation_client_);
+     } else {
+@@ -4879,9 +4888,9 @@ void NavigationRequest::CommitNavigation() {
+     }
+   }
+ 
+-  CreateCoepReporter(render_frame_host_->GetProcess()->GetStoragePartition());
++  CreateCoepReporter(GetRenderFrameHost()->GetProcess()->GetStoragePartition());
+   coop_status_.UpdateReporterStoragePartition(
+-      render_frame_host_->GetProcess()->GetStoragePartition());
++      GetRenderFrameHost()->GetProcess()->GetStoragePartition());
+ 
+   BrowserContext* browser_context =
+       frame_tree_node_->navigator().controller().GetBrowserContext();
+@@ -4924,7 +4933,7 @@ void NavigationRequest::CommitNavigation() {
+     // Notify the service worker navigation handle that navigation commit is
+     // about to go.
+     service_worker_handle_->OnBeginNavigationCommit(
+-        render_frame_host_->GetGlobalId(),
++        GetRenderFrameHost()->GetGlobalId(),
+         policy_container_builder_->FinalPolicies().cross_origin_embedder_policy,
+         std::move(reporter_remote), &service_worker_container_info,
+         commit_params_->document_ukm_source_id);
+@@ -4973,7 +4982,7 @@ void NavigationRequest::CommitNavigation() {
+         std::move(subresource_loader_params_->prefetched_signed_exchanges);
+   }
+ 
+-  render_frame_host_->CommitNavigation(
++  GetRenderFrameHost()->CommitNavigation(
+       this, std::move(common_params), std::move(commit_params),
+       std::move(response_head), std::move(response_body_),
+       std::move(url_loader_client_endpoints_),
+@@ -4986,7 +4995,7 @@ void NavigationRequest::CommitNavigation() {
+   // BrowserContext.  This is mostly needed to make sure the spare is warmed-up
+   // if it wasn't done in RenderProcessHostImpl::GetProcessHostForSiteInstance.
+   RenderProcessHostImpl::NotifySpareManagerAboutRecentlyUsedBrowserContext(
+-      render_frame_host_->GetSiteInstance()->GetBrowserContext());
++      GetRenderFrameHost()->GetSiteInstance()->GetBrowserContext());
+ 
+   SendDeferredConsoleMessages();
+ }
+@@ -5666,14 +5675,14 @@ void NavigationRequest::OnRendererRequestedNavigationCancellation() {
+   if (!IsWaitingToCommit()) {
+     // The cancellation happens before READY_TO_COMMIT.
+     frame_tree_node_->navigator().CancelNavigation(frame_tree_node_);
+-  } else if (render_frame_host_ ==
++  } else if (GetRenderFrameHost() ==
+                  frame_tree_node_->render_manager()->current_frame_host() ||
+-             !render_frame_host_->IsRenderFrameLive()) {
++             !GetRenderFrameHost()->IsRenderFrameLive()) {
+     // If the NavigationRequest has already reached READY_TO_COMMIT,
+     // `render_frame_host_` owns `this`. Cache any needed state in stack
+     // variables to avoid a use-after-free.
+     FrameTreeNode* frame_tree_node = frame_tree_node_;
+-    render_frame_host_->NavigationRequestCancelled(this);
++    GetRenderFrameHost()->NavigationRequestCancelled(this);
+     // Ensure that the speculative RFH, if any, is also cleaned up. In theory,
+     // `ResetNavigationRequest()` should handle this; however, it early-returns
+     // if there is no navigation request associated with the FrameTreeNode.
+@@ -6101,7 +6110,7 @@ void NavigationRequest::DidCommitNavigation(
+   // Navigations in non-primary frame trees or portals don't appear in history.
+   if ((should_update_history_ && IsSameDocument() && !HasUserGesture() &&
+        params.url == previous_main_frame_url) ||
+-      !render_frame_host_->GetPage().IsPrimary() ||
++      !GetRenderFrameHost()->GetPage().IsPrimary() ||
+       frame_tree_node()->frame_tree()->IsPortal()) {
+     should_update_history_ = false;
+   }
+@@ -6135,13 +6144,13 @@ void NavigationRequest::DidCommitNavigation(
+ 
+       if (!initiator_render_frame_host ||
+           initiator_render_frame_host->frame_tree_node() !=
+-              render_frame_host_->frame_tree_node()) {
++              GetRenderFrameHost()->frame_tree_node()) {
+         // The policy of disallowing a frame inside a fenced frame to navigate
+         // ancestors should've already been enforced in BeginNavigation().
+         CHECK(!initiator_render_frame_host ||
+               !initiator_render_frame_host->IsNestedWithinFencedFrame());
+ 
+-        render_frame_host_->frame_tree_node()
++        GetRenderFrameHost()->frame_tree_node()
+             ->set_shared_storage_budget_metadata(nullptr);
+       }
+     }
+@@ -6198,7 +6207,7 @@ void NavigationRequest::DidCommitNavigation(
+     ui::PageTransition transition =
+         ui::PageTransitionFromInt(common_params_->transition);
+     absl::optional<bool> is_background =
+-        render_frame_host_->GetProcess()->IsProcessBackgrounded();
++        GetRenderFrameHost()->GetProcess()->IsProcessBackgrounded();
+ 
+     RecordStartToCommitMetrics(
+         common_params_->navigation_start, transition, ready_to_commit_time_,
+@@ -6359,7 +6368,7 @@ void NavigationRequest::ReadyToCommitNavigation(bool is_error) {
+   // disconnection from the renderer NavigationClient; but browser-initiated
+   // navigations do not, so we must look explicitly. We should not proceed and
+   // claim "ReadyToCommitNavigation" to the delegate if the renderer is gone.
+-  if (!render_frame_host_->IsRenderFrameLive()) {
++  if (!GetRenderFrameHost()->IsRenderFrameLive()) {
+     OnRendererRequestedNavigationCancellation();
+     // DO NOT ADD CODE AFTER THIS, as the NavigationHandle has been deleted
+     // by the previous call.
+@@ -6372,20 +6381,20 @@ void NavigationRequest::ReadyToCommitNavigation(bool is_error) {
+   // where the FrameTreeNode has no NavigationRequest, yet the
+   // RenderFrameHostImpl is not marked as loading yet, causing
+   // FrameTreeNode::IsLoading() to incorrectly return false.
+-  frame_tree_node_->TransferNavigationRequestOwnership(render_frame_host_);
++  frame_tree_node_->TransferNavigationRequestOwnership(GetRenderFrameHost());
+ 
+   // When a speculative RenderFrameHost reaches ReadyToCommitNavigation, the
+   // browser process has asked the renderer to commit the navigation and is
+   // waiting for confirmation of the commit. Update the LifecycleStateImpl to
+   // kPendingCommit as RenderFrameHost isn't considered speculative anymore and
+   // was chosen to commit as this navigation's final RenderFrameHost.
+-  if (render_frame_host_->lifecycle_state() ==
++  if (GetRenderFrameHost()->lifecycle_state() ==
+       RenderFrameHostImpl::LifecycleStateImpl::kSpeculative) {
+     // Only cross-RenderFrameHost navigations create speculative
+     // RenderFrameHosts whereas SameDocument, BackForwardCache and
+     // PrerenderedActivation navigations don't.
+     DCHECK(!IsSameDocument() && !IsPageActivation());
+-    render_frame_host_->SetLifecycleState(
++    GetRenderFrameHost()->SetLifecycleState(
+         RenderFrameHostImpl::LifecycleStateImpl::kPendingCommit);
+   }
+ 
+@@ -6407,11 +6416,11 @@ void NavigationRequest::ReadyToCommitNavigation(bool is_error) {
+   // Record metrics for the time it takes to get to this state from the
+   // beginning of the navigation.
+   if (!IsSameDocument() && !is_error) {
+-    is_same_process_ = render_frame_host_->GetProcess()->GetID() ==
++    is_same_process_ = GetRenderFrameHost()->GetProcess()->GetID() ==
+                        previous_render_frame_host->GetProcess()->GetID();
+ 
+     RecordReadyToCommitMetrics(
+-        previous_render_frame_host, render_frame_host_, *common_params_.get(),
++        previous_render_frame_host, GetRenderFrameHost(), *common_params_.get(),
+         ready_to_commit_time_, origin_agent_cluster_end_result_,
+         did_receive_early_hints_before_cross_origin_redirect_);
+   }
+@@ -6420,7 +6429,7 @@ void NavigationRequest::ReadyToCommitNavigation(bool is_error) {
+   same_origin_ = (previous_render_frame_host->GetLastCommittedOrigin() ==
+                   GetOriginToCommit());
+ 
+-  SetExpectedProcess(render_frame_host_->GetProcess());
++  SetExpectedProcess(GetRenderFrameHost()->GetProcess());
+ 
+   commit_params_->is_load_data_with_base_url = IsLoadDataWithBaseURL();
+ 
+@@ -6438,7 +6447,7 @@ void NavigationRequest::ReadyToCommitNavigation(bool is_error) {
+     NavigationEntry* entry = GetNavigationEntry();
+     if (entry && entry->IsViewSourceMode()) {
+       // Put the renderer in view source mode.
+-      render_frame_host_->GetAssociatedLocalFrame()->EnableViewSourceMode();
++      GetRenderFrameHost()->GetAssociatedLocalFrame()->EnableViewSourceMode();
+     }
+   }
+ 
+@@ -6735,7 +6744,7 @@ NavigationRequest::MakeDidCommitProvisionalLoadParamsForActivation() {
+   // Use the DidCommitProvisionalLoadParams last used to commit the frame being
+   // restored as a starting point.
+   mojom::DidCommitProvisionalLoadParamsPtr params =
+-      render_frame_host_->TakeLastCommitParams();
++      GetRenderFrameHost()->TakeLastCommitParams();
+ 
+   // Params must have been set when the RFH being restored from the cache last
+   // navigated.
+@@ -6994,7 +7003,10 @@ RenderFrameHostImpl* NavigationRequest::GetRenderFrameHost() const {
+   }
+   static_assert(WILL_FAIL_REQUEST > WILL_PROCESS_RESPONSE,
+                 "WillFailRequest state should come after WillProcessResponse");
+-  return render_frame_host_;
++  if (HasRenderFrameHost()) {
++    return &*render_frame_host_.value();
++  }
++  return nullptr;
+ }
+ 
+ const net::HttpRequestHeaders& NavigationRequest::GetRequestHeaders() {
+@@ -7453,7 +7465,8 @@ bool NavigationRequest::CoopCoepSanityCheck() {
+   // TODO(https://crbug.com/1278207) add other embedded cases if needed.
+   network::mojom::CrossOriginOpenerPolicyValue coop_value =
+       GetParentFrameOrOuterDocument()
+-          ? render_frame_host_->GetOutermostMainFrame()
++          ? GetRenderFrameHost()
++                ->GetOutermostMainFrame()
+                 ->cross_origin_opener_policy()
+                 .value
+           : policies.cross_origin_opener_policy.value;
+@@ -8003,8 +8016,8 @@ void NavigationRequest::SendDeferredConsoleMessages() {
+   for (auto& message : console_messages_) {
+     // TODO(https://crbug.com/721329): We should have a way of sending console
+     // messaged to devtools without going through the renderer.
+-    render_frame_host_->AddMessageToConsole(message.level,
+-                                            std::move(message.message));
++    GetRenderFrameHost()->AddMessageToConsole(message.level,
++                                              std::move(message.message));
+   }
+   console_messages_.clear();
+ }
+diff --git a/content/browser/renderer_host/navigation_request.h b/content/browser/renderer_host/navigation_request.h
+index f6b43e83734fef815d8cfa89cf34e3ef34cb370b..0b48eb3a5f1f2c037474d394d1f214c94a357ef8 100644
+--- a/content/browser/renderer_host/navigation_request.h
++++ b/content/browser/renderer_host/navigation_request.h
+@@ -458,6 +458,8 @@ class CONTENT_EXPORT NavigationRequest
+     associated_rfh_type_ = type;
+   }
+ 
++  bool HasRenderFrameHost() const { return render_frame_host_.has_value(); }
++
+   void set_was_discarded() { commit_params_->was_discarded = true; }
+ 
+   void set_net_error(net::Error net_error) { net_error_ = net_error; }
+@@ -1595,8 +1597,17 @@ class CONTENT_EXPORT NavigationRequest
+   //  - the synchronous about:blank navigation.
+   const bool is_synchronous_renderer_commit_;
+ 
+-  // Invariant: At least one of |loader_| or |render_frame_host_| is null.
+-  raw_ptr<RenderFrameHostImpl> render_frame_host_ = nullptr;
++  // The RenderFrameHost that this navigation intends to commit in. The value
++  // will be set when we know the final RenderFrameHost that the navigation will
++  // commit in (i.e. when we receive the final network response for most
++  // navigations). Note that currently this can be reset to absl::nullopt for
++  // cross-document restarts and some failed navigations.
++  // TODO(https://crbug.com/1416916): Don't reset this on failed navigations,
++  // and ensure the NavigationRequest doesn't outlive the `render_frame_host_`
++  // picked for failed Back/Forward Cache restores.
++  // Invariant: At least one of |loader_| or |render_frame_host_| is
++  // null/absl::nullopt.
++  absl::optional<base::SafeRef<RenderFrameHostImpl>> render_frame_host_;
+ 
+   // Initialized on creation of the NavigationRequest. Sent to the renderer when
+   // the navigation is ready to commit.