From 67e1aad4cc16ee14b6ed7d6a2057e36695ca8556 Mon Sep 17 00:00:00 2001
From: Anna Sato <annasato@chromium.org>
Date: Thu, 23 Jul 2026 23:12:41 -0700
Subject: [PATCH] [BFCache] Gate metrics reuse on main-frame SiteInstance

CreateOrReuseBackForwardCacheMetricsForNavigation reused the previous
NavigationEntry's BackForwardCacheMetrics whenever the renderer-supplied
document sequence number matched, regardless of SiteInstance. The
document sequence number alone is not a reliable signal that two
main-frame commits belong to the same document, so the previous entry's
NotRestoredReasons tree could end up attached to an unrelated cross-site
entry and later be reported via the NotRestoredReasons API for the wrong
document.

Same-document main-frame navigations never change SiteInstance, so
require the committing main-frame SiteInstance to match the previous
entry's before treating a document sequence number match as
same-document. A null previous-entry SiteInstance (e.g. after session
restore) continues to rely on the sequence number alone, matching the
existing behaviour in SetSkippableForSameDocumentEntries.

Also adds a unit test covering both the cross-SiteInstance and
same-SiteInstance cases.

TAG=agy
CONV=23a358e0-79f8-4fd1-8ddc-2be156279091

Bug: 520121111
Fixed: 520121111
Change-Id: I67b37359f65dbbf156bad242ec9f2ba10435ba66
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8137640
Commit-Queue: Anna Sato <annasato@chromium.org>
Reviewed-by: Rakina Zata Amni <rakina@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1667662}
---

diff --git a/content/browser/back_forward_cache/back_forward_cache_metrics.cc b/content/browser/back_forward_cache/back_forward_cache_metrics.cc
index 3283359..8c76e1bb 100644
--- a/content/browser/back_forward_cache/back_forward_cache_metrics.cc
+++ b/content/browser/back_forward_cache/back_forward_cache_metrics.cc
@@ -95,7 +95,8 @@
 BackForwardCacheMetrics::CreateOrReuseBackForwardCacheMetricsForNavigation(
     NavigationEntryImpl* previous_entry,
     bool is_main_frame_navigation,
-    int64_t committing_document_sequence_number) {
+    int64_t committing_document_sequence_number,
+    SiteInstanceImpl* committing_main_frame_site_instance) {
   // TODO(https://crbug.com/445585641): Make this enforceable on Android.
 #if !BUILDFLAG(IS_ANDROID)
   if (base::FeatureList::IsEnabled(kCheckDocumentSequenceNumber)) {
@@ -123,10 +124,18 @@
   }
 
   // Reuse `previous_entry_metrics` on subframe navigations and same-document
-  // navigations.
+  // main-frame navigations. The document sequence number used to detect the
+  // latter is supplied by the renderer; since same-document navigations never
+  // change SiteInstance, also require the committing SiteInstance to match
+  // `previous_entry`'s before treating the navigation as same-document.
+  // `previous_entry`'s SiteInstance may be null after session restore, in
+  // which case the document sequence number alone is trusted.
   if (!is_main_frame_navigation ||
-      committing_document_sequence_number ==
-          previous_entry_metrics->document_sequence_number_) {
+      (committing_document_sequence_number ==
+           previous_entry_metrics->document_sequence_number_ &&
+       (!previous_entry->site_instance() ||
+        previous_entry->site_instance() ==
+            committing_main_frame_site_instance))) {
     return previous_entry_metrics;
   }
 
diff --git a/content/browser/back_forward_cache/back_forward_cache_metrics.h b/content/browser/back_forward_cache/back_forward_cache_metrics.h
index 00f9887..6c5706a 100644
--- a/content/browser/back_forward_cache/back_forward_cache_metrics.h
+++ b/content/browser/back_forward_cache/back_forward_cache_metrics.h
@@ -42,13 +42,14 @@
 class NavigationEntryImpl;
 class NavigationRequest;
 class RenderFrameHostImpl;
+class SiteInstanceImpl;
 struct BackForwardCacheCanStoreDocumentResultWithTree;
 
 // Helper class for recording metrics around history navigations.
 // Associated with a main frame document and shared between all
 // NavigationEntries with the same document_sequence_number for the main
 // document.
-class BackForwardCacheMetrics
+class CONTENT_EXPORT BackForwardCacheMetrics
     : public base::RefCounted<BackForwardCacheMetrics> {
  public:
   using NotRestoredReason = BackForwardCache::NotRestoredReason;
@@ -99,13 +100,17 @@
   // navigation is a subframe navigation or if it's same-document with
   // `previous_entry`'s document.
   //
-  // |document_sequence_number| is the sequence number of the document
-  // associated with the navigating frame.
+  // `committing_document_sequence_number` is the sequence number of the
+  // document associated with the navigating frame.
+  // `committing_main_frame_site_instance` is the SiteInstance the main frame
+  // is committing in, and is only used when `is_main_frame_navigation` is
+  // true.
   static scoped_refptr<BackForwardCacheMetrics>
   CreateOrReuseBackForwardCacheMetricsForNavigation(
       NavigationEntryImpl* previous_entry,
       bool is_main_frame_navigation,
-      int64_t committing_document_sequence_number);
+      int64_t committing_document_sequence_number,
+      SiteInstanceImpl* committing_main_frame_site_instance);
 
   explicit BackForwardCacheMetrics(int64_t document_sequence_number);
 
@@ -188,11 +193,11 @@
 
   // Exported for testing.
   // The DisabledReason's source and id combined to give a unique uint64.
-  CONTENT_EXPORT static uint64_t MetricValue(BackForwardCache::DisabledReason);
+  static uint64_t MetricValue(BackForwardCache::DisabledReason);
 
   // Injects a clock for mocking time.
   // Should be called only from the UI thread.
-  CONTENT_EXPORT static void OverrideTimeForTesting(base::TickClock* clock);
+  static void OverrideTimeForTesting(base::TickClock* clock);
 
   class TestObserver {
    public:
diff --git a/content/browser/back_forward_cache/back_forward_cache_metrics_unittest.cc b/content/browser/back_forward_cache/back_forward_cache_metrics_unittest.cc
index 2d2cef5f..620d544 100644
--- a/content/browser/back_forward_cache/back_forward_cache_metrics_unittest.cc
+++ b/content/browser/back_forward_cache/back_forward_cache_metrics_unittest.cc
@@ -2,12 +2,17 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "content/browser/back_forward_cache/back_forward_cache_metrics.h"
+
 #include "base/sanitizer_buildflags.h"
 #include "base/test/metrics/histogram_tester.h"
 #include "base/test/scoped_feature_list.h"
 #include "base/test/simple_test_tick_clock.h"
 #include "components/ukm/test_ukm_recorder.h"
 #include "content/browser/back_forward_cache/back_forward_cache_impl.h"
+#include "content/browser/renderer_host/navigation_controller_impl.h"
+#include "content/browser/renderer_host/navigation_entry_impl.h"
+#include "content/browser/site_instance_impl.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/web_contents_delegate.h"
 #include "content/public/browser/web_contents_observer.h"
@@ -388,6 +393,49 @@
       BackForwardCacheMetrics::HistoryNavigationOutcome::kNotRestored, 1);
 }
 
+TEST_F(BackForwardCacheMetricsTest,
+       MetricsNotReusedForCrossSiteInstanceMainFrameNavigation) {
+  const GURL url1("http://a.com/");
+  const GURL url2("http://b.com/");
+
+  NavigationControllerImpl& controller =
+      static_cast<NavigationControllerImpl&>(contents()->GetController());
+
+  // Navigate to `url1` and capture the entry's metrics, SiteInstance and
+  // document sequence number.
+  NavigationSimulator::NavigateAndCommitFromDocument(url1, main_test_rfh());
+  NavigationEntryImpl* entry1 = controller.GetEntryAtIndex(0);
+  BackForwardCacheMetrics* metrics1 = entry1->back_forward_cache_metrics();
+  SiteInstanceImpl* site_instance1 = entry1->site_instance();
+  int64_t dsn1 = entry1->root_node()->frame_entry->document_sequence_number();
+  ASSERT_TRUE(metrics1);
+  ASSERT_TRUE(site_instance1);
+
+  // Navigate cross-site to `url2`, which commits in a different SiteInstance.
+  NavigationSimulator::NavigateAndCommitFromDocument(url2, main_test_rfh());
+  NavigationEntryImpl* entry2 = controller.GetEntryAtIndex(1);
+  SiteInstanceImpl* site_instance2 = entry2->site_instance();
+  ASSERT_TRUE(site_instance2);
+  ASSERT_NE(site_instance1, site_instance2);
+  ASSERT_NE(entry2->back_forward_cache_metrics(), metrics1);
+
+  // A main-frame navigation that reports `entry1`'s document sequence number
+  // but commits in `entry2`'s SiteInstance must not reuse `entry1`'s metrics,
+  // since the document sequence number is renderer-supplied and a
+  // same-document navigation never changes SiteInstance.
+  scoped_refptr<BackForwardCacheMetrics> result = BackForwardCacheMetrics::
+      CreateOrReuseBackForwardCacheMetricsForNavigation(
+          entry1, /*is_main_frame_navigation=*/true, dsn1, site_instance2);
+  EXPECT_NE(result.get(), metrics1);
+
+  // A main-frame navigation reporting `entry1`'s document sequence number and
+  // committing in `entry1`'s SiteInstance should still reuse the metrics.
+  result = BackForwardCacheMetrics::
+      CreateOrReuseBackForwardCacheMetricsForNavigation(
+          entry1, /*is_main_frame_navigation=*/true, dsn1, site_instance1);
+  EXPECT_EQ(result.get(), metrics1);
+}
+
 TEST_F(BackForwardCacheMetricsTest, PageWithFormsMetricsNotStore) {
   DisableBackForwardCacheForTesting(contents(),
                                     BackForwardCache::TEST_REQUIRES_NO_CACHING);
diff --git a/content/browser/renderer_host/navigation_controller_impl.cc b/content/browser/renderer_host/navigation_controller_impl.cc

diff --git a/content/browser/back_forward_cache/back_forward_cache_metrics_unittest.cc b/content/browser/back_forward_cache/back_forward_cache_metrics_unittest.cc
index 2d2cef5f..620d544 100644
--- a/content/browser/back_forward_cache/back_forward_cache_metrics_unittest.cc
+++ b/content/browser/back_forward_cache/back_forward_cache_metrics_unittest.cc
@@ -2,12 +2,17 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "content/browser/back_forward_cache/back_forward_cache_metrics.h"
+
 #include "base/sanitizer_buildflags.h"
 #include "base/test/metrics/histogram_tester.h"
 #include "base/test/scoped_feature_list.h"
 #include "base/test/simple_test_tick_clock.h"
 #include "components/ukm/test_ukm_recorder.h"
 #include "content/browser/back_forward_cache/back_forward_cache_impl.h"
+#include "content/browser/renderer_host/navigation_controller_impl.h"
+#include "content/browser/renderer_host/navigation_entry_impl.h"
+#include "content/browser/site_instance_impl.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/web_contents_delegate.h"
 #include "content/public/browser/web_contents_observer.h"
@@ -388,6 +393,49 @@
       BackForwardCacheMetrics::HistoryNavigationOutcome::kNotRestored, 1);
 }
 
+TEST_F(BackForwardCacheMetricsTest,
+       MetricsNotReusedForCrossSiteInstanceMainFrameNavigation) {
+  const GURL url1("http://a.com/");
+  const GURL url2("http://b.com/");
+
+  NavigationControllerImpl& controller =
+      static_cast<NavigationControllerImpl&>(contents()->GetController());
+
+  // Navigate to `url1` and capture the entry's metrics, SiteInstance and
+  // document sequence number.
+  NavigationSimulator::NavigateAndCommitFromDocument(url1, main_test_rfh());
+  NavigationEntryImpl* entry1 = controller.GetEntryAtIndex(0);
+  BackForwardCacheMetrics* metrics1 = entry1->back_forward_cache_metrics();
+  SiteInstanceImpl* site_instance1 = entry1->site_instance();
+  int64_t dsn1 = entry1->root_node()->frame_entry->document_sequence_number();
+  ASSERT_TRUE(metrics1);
+  ASSERT_TRUE(site_instance1);
+
+  // Navigate cross-site to `url2`, which commits in a different SiteInstance.
+  NavigationSimulator::NavigateAndCommitFromDocument(url2, main_test_rfh());
+  NavigationEntryImpl* entry2 = controller.GetEntryAtIndex(1);
+  SiteInstanceImpl* site_instance2 = entry2->site_instance();
+  ASSERT_TRUE(site_instance2);
+  ASSERT_NE(site_instance1, site_instance2);
+  ASSERT_NE(entry2->back_forward_cache_metrics(), metrics1);
+
+  // A main-frame navigation that reports `entry1`'s document sequence number
+  // but commits in `entry2`'s SiteInstance must not reuse `entry1`'s metrics,
+  // since the document sequence number is renderer-supplied and a
+  // same-document navigation never changes SiteInstance.
+  scoped_refptr<BackForwardCacheMetrics> result = BackForwardCacheMetrics::
+      CreateOrReuseBackForwardCacheMetricsForNavigation(
+          entry1, /*is_main_frame_navigation=*/true, dsn1, site_instance2);
+  EXPECT_NE(result.get(), metrics1);
+
+  // A main-frame navigation reporting `entry1`'s document sequence number and
+  // committing in `entry1`'s SiteInstance should still reuse the metrics.
+  result = BackForwardCacheMetrics::
+      CreateOrReuseBackForwardCacheMetricsForNavigation(
+          entry1, /*is_main_frame_navigation=*/true, dsn1, site_instance1);
+  EXPECT_EQ(result.get(), metrics1);
+}
+
 TEST_F(BackForwardCacheMetricsTest, PageWithFormsMetricsNotStore) {
   DisableBackForwardCacheForTesting(contents(),
                                     BackForwardCache::TEST_REQUIRES_NO_CACHING);