From cdb798347ad78bcb1cffdbdc0e06eb1ec1867a57 Mon Sep 17 00:00:00 2001
From: Charlie Reis <creis@chromium.org>
Date: Fri, 14 Feb 2025 14:04:46 -0800
Subject: [PATCH] Do not reuse document sequence number on cross-origin navigations.

Cross-document navigations only reuse document sequence numbers for
"logically related" navigations. This CL ensures that they are not
reused for cross-origin navigations as well (e.g., about:blank with
different owners), by computing this in the browser process and sending
it in the CommitNavigationParams.

Note that an exception is needed for error pages, which should preserve
the document sequence number and other history item state, though they
transition to and from an opaque origin with the same precursor.

Bug: 40051596
Change-Id: I502251ce12ec8b3e613596b914fbe8a63330b4fc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6185347
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Charlie Reis <creis@chromium.org>
Reviewed-by: Rakina Zata Amni <rakina@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1420736}
---

diff --git a/content/browser/renderer_host/navigation_controller_impl.cc b/content/browser/renderer_host/navigation_controller_impl.cc
index 597384a..715a22a 100644
--- a/content/browser/renderer_host/navigation_controller_impl.cc
+++ b/content/browser/renderer_host/navigation_controller_impl.cc
@@ -4125,7 +4125,8 @@
           /*visited_link_salt=*/std::nullopt,
           /*local_surface_id=*/std::nullopt,
           node->current_frame_host()->GetCachedPermissionStatuses(),
-          /*should_skip_screentshot=*/false);
+          /*should_skip_screentshot=*/false,
+          /*force_new_document_sequence_number=*/false);
 #if BUILDFLAG(IS_ANDROID)
   if (ValidateDataURLAsString(params.data_url_as_string)) {
     commit_params->data_url_as_string = params.data_url_as_string->as_string();
diff --git a/content/browser/renderer_host/navigation_controller_impl_browsertest.cc b/content/browser/renderer_host/navigation_controller_impl_browsertest.cc
index 7923c93..9a7f5fe60 100644
--- a/content/browser/renderer_host/navigation_controller_impl_browsertest.cc
+++ b/content/browser/renderer_host/navigation_controller_impl_browsertest.cc
@@ -5304,9 +5304,7 @@
   scoped_refptr<FrameNavigationEntry> frame_entry_blank_data =
       controller.GetLastCommittedEntry()->GetFrameEntry(inner_frame);
   int64_t dsn_blank = frame_entry_blank_data->document_sequence_number();
-  //  TODO(crbug.com/40051596): Fix Blink to use a different document sequence
-  //  number for this navigation.
-  EXPECT_EQ(dsn_a1, dsn_blank);
+  EXPECT_NE(dsn_a1, dsn_blank);
 
   // Go back. This should not be treated as same-document, because the origin
   // changed in the previous navigation.
@@ -19169,6 +19167,10 @@
     EXPECT_EQ(previous_frame_entry,
               controller.GetLastCommittedEntry()->GetFrameEntry(child));
     EXPECT_TRUE(capturer.did_replace_entry());
+
+    // We keep the same history.state value, even in the error page, so that it
+    // can be used when the load later succeeds in step 4.
+    EXPECT_EQ("foo", EvalJs(child, "history.state"));
   }
 
   // 4) Test successfully navigating the subframe to the same URL after a failed
diff --git a/content/browser/renderer_host/navigation_entry_impl.cc b/content/browser/renderer_host/navigation_entry_impl.cc
index c1d1d97d..5578f71 100644
--- a/content/browser/renderer_host/navigation_entry_impl.cc
+++ b/content/browser/renderer_host/navigation_entry_impl.cc
@@ -1005,7 +1005,8 @@
           /*visited_link_salt=*/std::nullopt,
           /*local_surface_id=*/std::nullopt,
           /*initial_permission_statuses=*/std::nullopt,
-          /*should_skip_screenshot*/ false);
+          /*should_skip_screenshot*/ false,
+          /*force_new_document_sequence_number=*/false);
 #if BUILDFLAG(IS_ANDROID)
   // `data_url_as_string` is saved in NavigationEntry but should only be used by
   // main frames, because loadData* navigations can only happen on the main
diff --git a/content/browser/renderer_host/navigation_request.cc b/content/browser/renderer_host/navigation_request.cc
index 4921db5..07309925 100644
--- a/content/browser/renderer_host/navigation_request.cc
+++ b/content/browser/renderer_host/navigation_request.cc
@@ -1445,7 +1445,8 @@
           /*visited_link_salt=*/std::nullopt,
           /*local_surface_id=*/std::nullopt,
           frame_tree_node->current_frame_host()->GetCachedPermissionStatuses(),
-          /*should_skip_screenshot=*/false);
+          /*should_skip_screenshot=*/false,
+          /*force_new_document_sequence_number=*/false);
 
   commit_params->navigation_timing->system_entropy_at_navigation_start =
       SystemEntropyUtils::ComputeSystemEntropyForFrameTreeNode(
@@ -1597,7 +1598,8 @@
           /*visited_link_salt=*/std::nullopt,
           /*local_surface_id=*/std::nullopt,
           render_frame_host->GetCachedPermissionStatuses(),
-          /*should_skip_screenshot=*/false);
+          /*should_skip_screenshot=*/false,
+          /*force_new_document_sequence_number=*/false);
   blink::mojom::BeginNavigationParamsPtr begin_params =
       blink::mojom::BeginNavigationParams::New();
   std::unique_ptr<NavigationRequest> navigation_request(new NavigationRequest(
@@ -6061,6 +6063,30 @@
     return;
   }
 
+  // Ensure the renderer does not reuse the document sequence number for
+  // cross-origin navigations (which can lead to later bugs with same-document
+  // navigations appearing to be cross-origin). All error pages have unique
+  // opaque origins and are considered cross-origin.
+  //
+  // The one exception is for transitioning to or from a compatible error page,
+  // which preserves state in case a temporary failure later succeeds. Here, we
+  // must check for any cases where the committing error page has a valid
+  // precursor that agrees with the current document, whether the current
+  // document is already an error page or not. (The renderer process will narrow
+  // DSN reuse further, to cases the URL is a closer match, ignoring fragments.)
+  // See also CommitNavigation for the other direction.
+  // TODO(crbug.com/396645697): Use a different technique for preserving history
+  // item state on error pages, separate from the error page's own state.
+  RenderFrameHostImpl* previous_rfh = frame_tree_node()->current_frame_host();
+  const url::Origin& previous_origin = previous_rfh->GetLastCommittedOrigin();
+  bool is_error_page_with_same_precursor =
+      previous_origin.GetTupleOrPrecursorTupleIfOpaque().IsValid() &&
+      commit_params_->origin_to_commit->GetTupleOrPrecursorTupleIfOpaque() ==
+          previous_origin.GetTupleOrPrecursorTupleIfOpaque();
+  if (!is_error_page_with_same_precursor) {
+    commit_params_->force_new_document_sequence_number = true;
+  }
+
   PopulateDocumentTokenForCrossDocumentNavigation();
   // Use a separate cache shard, and no cookies, for error pages.
   isolation_info_for_subresources_ =
@@ -6375,6 +6401,35 @@
         GetNavigationController()->GetNavigationApiHistoryEntryVectors(
             frame_tree_node_, this);
     PopulateDocumentTokenForCrossDocumentNavigation();
+
+    // Ensure the renderer does not reuse the document sequence number for
+    // cross-origin navigations (which can lead to later bugs with same-document
+    // navigations appearing to be cross-origin).
+    //
+    // The one exception is for transitioning to or from a compatible error
+    // page, which preserves state in case a temporary failure later succeeds.
+    // Here, we must check for the case that the user navigates from an error
+    // page to a non-error page that matches the (valid) precursor origin. See
+    // also CommitErrorPage for the other direction.
+    // TODO(crbug.com/396645697): Use a different technique for preserving
+    // history item state on error pages, separate from the error page's own
+    // state.
+    RenderFrameHostImpl* previous_rfh = frame_tree_node()->current_frame_host();
+    const url::Origin& previous_origin = previous_rfh->GetLastCommittedOrigin();
+    // Skip this check if kUseBrowserCalculatedOrigin is disabled.
+    if (base::FeatureList::IsEnabled(features::kUseBrowserCalculatedOrigin)) {
+      bool is_cross_origin_navigation =
+          !commit_params_->origin_to_commit->IsSameOriginWith(previous_origin);
+      bool compatible_with_error_page =
+          previous_rfh->IsErrorDocument() &&
+          previous_origin.GetTupleOrPrecursorTupleIfOpaque().IsValid() &&
+          commit_params_->origin_to_commit
+                  ->GetTupleOrPrecursorTupleIfOpaque() ==
+              previous_origin.GetTupleOrPrecursorTupleIfOpaque();
+      if (is_cross_origin_navigation && !compatible_with_error_page) {
+        commit_params_->force_new_document_sequence_number = true;
+      }
+    }
   }
 
   if (early_hints_manager_) {
diff --git a/content/browser/renderer_host/render_frame_host_impl.cc b/content/browser/renderer_host/render_frame_host_impl.cc
index 19601b38..69e121b0 100644
--- a/content/browser/renderer_host/render_frame_host_impl.cc
+++ b/content/browser/renderer_host/render_frame_host_impl.cc
@@ -12227,9 +12227,10 @@
   commit_params->should_skip_screenshot =
       NavigationTransitionUtils::ShouldSkipScreenshot(*navigation_request);
 
+  RenderFrameHostImpl* previous_rfh =
+      navigation_request->frame_tree_node()->current_frame_host();
   if (is_same_document) {
-    DCHECK_EQ(navigation_request->frame_tree_node()->current_frame_host(),
-              this);
+    DCHECK_EQ(previous_rfh, this);
     const base::UnguessableToken& navigation_token =
         commit_params->navigation_token;
     commit_params->has_ua_visual_transition =
@@ -12358,8 +12359,7 @@
     // point just before the navigation commits.
     // TODO(altimin, crbug.com/933147): Remove this logic after we are done with
     // implementing back-forward cache.
-    if (!GetParent() &&
-        navigation_request->frame_tree_node()->current_frame_host() == this) {
+    if (!GetParent() && previous_rfh == this) {
       if (NavigationEntryImpl* last_committed_entry =
               NavigationEntryImpl::FromNavigationEntry(
                   navigation_request->frame_tree_node()
@@ -12390,10 +12390,7 @@
     // processes.
     const bool maybe_new_process_is_used =
         GetProcess()->GetRenderFrameHostCount() == 1 &&

diff --git a/content/browser/renderer_host/navigation_controller_impl_browsertest.cc b/content/browser/renderer_host/navigation_controller_impl_browsertest.cc
index 7923c93..9a7f5fe60 100644
--- a/content/browser/renderer_host/navigation_controller_impl_browsertest.cc
+++ b/content/browser/renderer_host/navigation_controller_impl_browsertest.cc
@@ -5304,9 +5304,7 @@
   scoped_refptr<FrameNavigationEntry> frame_entry_blank_data =
       controller.GetLastCommittedEntry()->GetFrameEntry(inner_frame);
   int64_t dsn_blank = frame_entry_blank_data->document_sequence_number();
-  //  TODO(crbug.com/40051596): Fix Blink to use a different document sequence
-  //  number for this navigation.
-  EXPECT_EQ(dsn_a1, dsn_blank);
+  EXPECT_NE(dsn_a1, dsn_blank);
 
   // Go back. This should not be treated as same-document, because the origin
   // changed in the previous navigation.
@@ -19169,6 +19167,10 @@
     EXPECT_EQ(previous_frame_entry,
               controller.GetLastCommittedEntry()->GetFrameEntry(child));
     EXPECT_TRUE(capturer.did_replace_entry());
+
+    // We keep the same history.state value, even in the error page, so that it
+    // can be used when the load later succeeds in step 4.
+    EXPECT_EQ("foo", EvalJs(child, "history.state"));
   }
 
   // 4) Test successfully navigating the subframe to the same URL after a failed