[bfcache] Use NotRestoredReasonBuilder to add eviction reasons

This CL uses the new class to iterate thru the frame tree to check
bfcache eligibility, in order to correctly capture eviction reasons.
Now NotRestoredReason tree is only created through the builder.

This also adds test cases to make sure eviction reasons are recorded.

Bug: 1297914
Change-Id: I727da918ff4c71152554d0af598fa25fd2457b42
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3467300
Reviewed-by: Fergal Daly <fergal@chromium.org>
Reviewed-by: Rakina Zata Amni <rakina@chromium.org>
Commit-Queue: Yuzu Saijo <yuzus@chromium.org>
Cr-Commit-Position: refs/heads/main@{#984701}

content/browser/back_forward_cache_browsertest.cc

@@ -141,6 +141,11 @@ BackForwardCacheBrowserTest::~BackForwardCacheBrowserTest() {
   }
 }
 
+void BackForwardCacheBrowserTest::NotifyNotRestoredReasons(
+    std::unique_ptr<BackForwardCacheCanStoreTreeResult> tree_result) {
+  tree_result_ = std::move(tree_result);
+}
+
   // Disables checking metrics that are recorded recardless of the domains. By
   // default, this class' Expect* function checks the metrics both for the
   // specific domain and for all domains at the same time. In the case when the
@@ -2507,7 +2512,8 @@ RenderFrameHostImpl* ChildFrame(RenderFrameHostImpl* rfh, int child_index) {
 }
 
 // Verifies that the reasons match those given and no others.
-testing::Matcher<BackForwardCacheCanStoreDocumentResult> MatchesDocumentResult(
+testing::Matcher<BackForwardCacheCanStoreDocumentResult>
+BackForwardCacheBrowserTest::MatchesDocumentResult(
     testing::Matcher<NotStoredReasons> not_stored,
     BlockListedFeatures block_listed) {
   return testing::AllOf(
@@ -2530,7 +2536,7 @@ testing::Matcher<BackForwardCacheCanStoreDocumentResult> MatchesDocumentResult(
 }
 
 // Check the contents of the BackForwardCacheCanStoreTreeResult of a page.
-IN_PROC_BROWSER_TEST_F(BackForwardCacheBrowserTest, TreeResult1) {
+IN_PROC_BROWSER_TEST_F(BackForwardCacheBrowserTest, TreeResultFeatureUsage) {
   ASSERT_TRUE(embedded_test_server()->Start());
   GURL url_a(embedded_test_server()->GetURL(
       "a.com", "/cross_site_iframe_factory.html?a(a, b, c)"));
@@ -2604,6 +2610,115 @@ IN_PROC_BROWSER_TEST_F(BackForwardCacheBrowserTest, TreeResult1) {
                             BlockListedFeatures(BlockListedFeatures())));
 }
 
+// Check the contents of the BackForwardCacheCanStoreTreeResult of a page when
+// it is evicted.
+IN_PROC_BROWSER_TEST_F(BackForwardCacheBrowserTest,
+                       TreeResultEvictionMainFrame) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+  GURL url_a(embedded_test_server()->GetURL("a.com", "/title1.html"));
+  GURL url_b(embedded_test_server()->GetURL("b.com", "/title1.html"));
+
+  // 1) Navigate to a.
+  ASSERT_TRUE(NavigateToURL(shell(), url_a));
+  RenderFrameHostImplWrapper rfh_a(current_frame_host());
+  rfh_a->GetBackForwardCacheMetrics()->SetObserverForTesting(this);
+
+  // 2) Navigate to B and evict A by JavaScript execution.
+  ASSERT_TRUE(NavigateToURL(shell(), url_b));
+  EvictByJavaScript(rfh_a.get());
+  ASSERT_TRUE(rfh_a.WaitUntilRenderFrameDeleted());
+
+  // 3) Go back to A.
+  ASSERT_TRUE(HistoryGoBack(web_contents()));
+  ExpectNotRestored({NotRestoredReason::kJavaScriptExecution}, {}, {}, {}, {},
+                    FROM_HERE);
+  EXPECT_THAT(GetTreeResult()->GetDocumentResult(),
+              MatchesDocumentResult(
+                  NotStoredReasons(NotRestoredReason::kJavaScriptExecution),
+                  BlockListedFeatures()));
+}
+
+// Check the contents of the BackForwardCacheCanStoreTreeResult of a page when
+// its subframe is evicted.
+IN_PROC_BROWSER_TEST_F(BackForwardCacheBrowserTest,
+                       TreeResultEvictionSubFrame) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+  GURL url_a(embedded_test_server()->GetURL(
+      "a.com", "/cross_site_iframe_factory.html?a(b)"));
+  GURL url_c(embedded_test_server()->GetURL("c.com", "/title1.html"));
+
+  // 1) Navigate to A.
+  ASSERT_TRUE(NavigateToURL(shell(), url_a));
+  RenderFrameHostImplWrapper rfh_a(current_frame_host());
+  RenderFrameHostImplWrapper rfh_b(
+      current_frame_host()->child_at(0)->current_frame_host());
+  rfh_a->GetBackForwardCacheMetrics()->SetObserverForTesting(this);
+
+  // 2) Navigate to C and evict A's subframe B by JavaScript execution.
+  ASSERT_TRUE(NavigateToURL(shell(), url_c));
+  EvictByJavaScript(rfh_b.get());
+  ASSERT_TRUE(rfh_a.WaitUntilRenderFrameDeleted());
+
+  // 3) Go back to A.
+  ASSERT_TRUE(HistoryGoBack(web_contents()));
+  ExpectNotRestored({NotRestoredReason::kJavaScriptExecution}, {}, {}, {}, {},
+                    FROM_HERE);
+  // Main frame result in the tree is empty.
+  EXPECT_THAT(GetTreeResult()->GetDocumentResult(),
+              MatchesDocumentResult(NotStoredReasons(), BlockListedFeatures()));
+  // Subframe result in the tree contains the reason.
+  EXPECT_THAT(GetTreeResult()->GetChildren().at(0)->GetDocumentResult(),
+              MatchesDocumentResult(
+                  NotStoredReasons(NotRestoredReason::kJavaScriptExecution),
+                  BlockListedFeatures()));
+}
+
+// Check the contents of the BackForwardCacheCanStoreTreeResult of a page when
+// its subframe's subframe is evicted.
+IN_PROC_BROWSER_TEST_F(BackForwardCacheBrowserTest,
+                       TreeResultEvictionSubFramesSubframe) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+  GURL url_a(embedded_test_server()->GetURL(
+      "a.com", "/cross_site_iframe_factory.html?a(b(c))"));
+  GURL url_d(embedded_test_server()->GetURL("d.com", "/title1.html"));
+
+  // 1) Navigate to A.
+  ASSERT_TRUE(NavigateToURL(shell(), url_a));
+  RenderFrameHostImplWrapper rfh_a(current_frame_host());
+  RenderFrameHostImplWrapper rfh_c(current_frame_host()
+                                       ->child_at(0)
+                                       ->current_frame_host()
+                                       ->child_at(0)
+                                       ->current_frame_host());
+  rfh_a->GetBackForwardCacheMetrics()->SetObserverForTesting(this);
+
+  // 2) Navigate to D and evict C by JavaScript execution.
+  ASSERT_TRUE(NavigateToURL(shell(), url_d));
+  EvictByJavaScript(rfh_c.get());
+  ASSERT_TRUE(rfh_a.WaitUntilRenderFrameDeleted());
+
+  // 3) Go back to A.
+  ASSERT_TRUE(HistoryGoBack(web_contents()));
+  ExpectNotRestored({NotRestoredReason::kJavaScriptExecution}, {}, {}, {}, {},
+                    FROM_HERE);
+  // Main frame result in the tree is empty.
+  EXPECT_THAT(GetTreeResult()->GetDocumentResult(),
+              MatchesDocumentResult(NotStoredReasons(), BlockListedFeatures()));
+  // The first level subframe result in the tree is empty.
+  EXPECT_THAT(GetTreeResult()->GetChildren().at(0)->GetDocumentResult(),
+              MatchesDocumentResult(NotStoredReasons(), BlockListedFeatures()));
+  // The second level subframe result in the tree contains the reason.
+  EXPECT_THAT(GetTreeResult()
+                  ->GetChildren()
+                  .at(0)
+                  ->GetChildren()
+                  .at(0)
+                  ->GetDocumentResult(),
+              MatchesDocumentResult(
+                  NotStoredReasons(NotRestoredReason::kJavaScriptExecution),
+                  BlockListedFeatures()));
+}
+
 class BackForwardCacheOptInBrowserTest : public BackForwardCacheBrowserTest {
  protected:
   void SetUpCommandLine(base::CommandLine* command_line) override {

content/browser/back_forward_cache_browsertest.h

@@ -24,6 +24,10 @@
 
 namespace content {
 
+using NotStoredReasons =
+    BackForwardCacheCanStoreDocumentResult::NotStoredReasons;
+using NotRestoredReason = BackForwardCacheMetrics::NotRestoredReason;
+
 // Match RenderFrameHostImpl* that are in the BackForwardCache.
 MATCHER(InBackForwardCache, "") {
   return arg->IsInBackForwardCache();
@@ -58,12 +62,18 @@ struct FeatureEqualOperator {
 }  // namespace
 
 // Test about the BackForwardCache.
-class BackForwardCacheBrowserTest : public ContentBrowserTest,
-                                    public WebContentsObserver {
+class BackForwardCacheBrowserTest
+    : public ContentBrowserTest,
+      public WebContentsObserver,
+      public BackForwardCacheMetrics::TestObserver {
  public:
   BackForwardCacheBrowserTest();
   ~BackForwardCacheBrowserTest() override;
 
+  // TestObserver:
+  void NotifyNotRestoredReasons(
+      std::unique_ptr<BackForwardCacheCanStoreTreeResult> tree_result) override;
+
  protected:
   using UkmMetrics = ukm::TestUkmRecorder::HumanReadableUkmMetrics;
 
@@ -172,6 +182,16 @@ class BackForwardCacheBrowserTest : public ContentBrowserTest,
   // assert that the URL after navigation is |url|.
   void NavigateAndBlock(GURL url, int history_offset);
 
+  static testing::Matcher<BackForwardCacheCanStoreDocumentResult>
+  MatchesDocumentResult(testing::Matcher<NotStoredReasons> not_stored,
+                        BlockListedFeatures block_listed);
+
+  // Access the tree result of NotRestoredReason for the last main frame
+  // navigation.
+  BackForwardCacheCanStoreTreeResult* GetTreeResult() {
+    return tree_result_.get();
+  }
+
   base::HistogramTester histogram_tester_;
 
   bool same_site_back_forward_cache_enabled_ = true;
@@ -242,6 +262,10 @@ class BackForwardCacheBrowserTest : public ContentBrowserTest,
   std::vector<UkmMetrics> expected_ukm_not_restored_reasons_;
   std::unique_ptr<ukm::TestAutoSetUkmRecorder> ukm_recorder_;
 
+  // Store the tree result of NotRestoredReasons for the last main frame
+  // navigation.
+  std::unique_ptr<BackForwardCacheCanStoreTreeResult> tree_result_;
+
   // Indicates whether metrics for all sites regardless of the domains are
   // checked or not.
   bool check_all_sites_ = true;

content/browser/back_forward_cache_internal_browsertest.cc

@@ -757,35 +757,39 @@ IN_PROC_BROWSER_TEST_F(BackForwardCacheBrowserTest, EvictPageWithInfiniteLoop) {
 
   // 1) Navigate to A.
   EXPECT_TRUE(NavigateToURL(shell(), url_a));
-  RenderFrameHostImpl* rfh_a = current_frame_host();
+  RenderFrameHostImplWrapper rfh_a(current_frame_host());
+  rfh_a->GetBackForwardCacheMetrics()->SetObserverForTesting(this);
 
-  ExecuteScriptAsync(rfh_a, R"(
+  ExecuteScriptAsync(rfh_a.get(), R"(
     let i = 0;
     while (true) { i++; }
   )");
 
-  RenderFrameDeletedObserver delete_observer_rfh_a(rfh_a);
-  RenderProcessHost* process = rfh_a->GetProcess();
+  RenderProcessHost* process = rfh_a.get()->GetProcess();
   RenderProcessHostWatcher destruction_observer(
       process, RenderProcessHostWatcher::WATCH_FOR_HOST_DESTRUCTION);
 
   // 2) Navigate to B.
   EXPECT_TRUE(NavigateToURL(shell(), url_b));
-  RenderFrameHostImpl* rfh_b = current_frame_host();
-  RenderFrameDeletedObserver delete_observer_rfh_b(rfh_b);
+  RenderFrameHostImplWrapper rfh_b(current_frame_host());
 
   // rfh_a should be destroyed (not kept in the cache).
   destruction_observer.Wait();
-  delete_observer_rfh_a.WaitUntilDeleted();
+  EXPECT_TRUE(rfh_a.WaitUntilRenderFrameDeleted());
 
   // rfh_b should still be the current frame.
-  EXPECT_EQ(current_frame_host(), rfh_b);
-  EXPECT_FALSE(delete_observer_rfh_b.deleted());
+  EXPECT_EQ(current_frame_host(), rfh_b.get());
 
   // 3) Go back to A.
   ASSERT_TRUE(HistoryGoBack(web_contents()));
   ExpectNotRestored({NotRestoredReason::kTimeoutPuttingInCache}, {}, {}, {}, {},
                     FROM_HERE);
+
+  // Make sure that the tree reasons match the flattened reasons.
+  EXPECT_THAT(GetTreeResult()->GetDocumentResult(),
+              MatchesDocumentResult(
+                  NotStoredReasons(NotRestoredReason::kTimeoutPuttingInCache),
+                  BlockListedFeatures()));
 }
 
 // Test the race condition where a document is evicted from the BackForwardCache
@@ -1166,12 +1170,13 @@ IN_PROC_BROWSER_TEST_F(BackForwardCacheBrowserTest, TimedEviction) {
 
   // 1) Navigate to A.
   EXPECT_TRUE(NavigateToURL(shell(), url_a));
-  RenderFrameHostImpl* rfh_a = current_frame_host();
-  RenderFrameDeletedObserver delete_observer_rfh_a(rfh_a);
+  RenderFrameHostImplWrapper rfh_a(current_frame_host());
+  RenderFrameDeletedObserver delete_observer_rfh_a(rfh_a.get());
+  rfh_a->GetBackForwardCacheMetrics()->SetObserverForTesting(this);
 
   // 2) Navigate to B.
   EXPECT_TRUE(NavigateToURL(shell(), url_b));
-  RenderFrameHostImpl* rfh_b = current_frame_host();
+  RenderFrameHostImplWrapper rfh_b(current_frame_host());
 
   // 3) Fast forward to just before eviction is due.
   task_runner->FastForwardBy(time_to_live_in_back_forward_cache - delta);
@@ -1185,11 +1190,16 @@ IN_PROC_BROWSER_TEST_F(BackForwardCacheBrowserTest, TimedEviction) {
 
   // 6) Confirm A is evicted.
   delete_observer_rfh_a.WaitUntilDeleted();
-  EXPECT_EQ(current_frame_host(), rfh_b);
+  EXPECT_EQ(current_frame_host(), rfh_b.get());
 
   // 7) Go back to A.
   ASSERT_TRUE(HistoryGoBack(web_contents()));
   ExpectNotRestored({NotRestoredReason::kTimeout}, {}, {}, {}, {}, FROM_HERE);
+  // Make sure that the tree reasons match the flattened reasons.
+  EXPECT_THAT(
+      GetTreeResult()->GetDocumentResult(),
+      MatchesDocumentResult(NotStoredReasons(NotRestoredReason::kTimeout),
+                            BlockListedFeatures()));
 }
 
 IN_PROC_BROWSER_TEST_F(

content/browser/renderer_host/back_forward_cache_impl.cc

@@ -983,17 +983,41 @@ void BackForwardCacheImpl::PopulateReasonsForDocument(
   }
 }
 
+std::unique_ptr<BackForwardCacheCanStoreTreeResult>
+BackForwardCacheImpl::CreateEvictionBackForwardCacheCanStoreTreeResult(
+    RenderFrameHostImpl& rfh,
+    BackForwardCacheCanStoreDocumentResult& eviction_reason) {
+  BackForwardCacheImpl::NotRestoredReasonBuilder builder(
+      rfh.GetMainFrame(),
+      /* include_non_sticky = */ false,
+      /* create_tree = */ true,
+      BackForwardCacheImpl::NotRestoredReasonBuilder::EvictionInfo(
+          rfh, &eviction_reason));
+  return builder.GetTreeResult();
+}
+
 BackForwardCacheImpl::NotRestoredReasonBuilder::NotRestoredReasonBuilder(
     RenderFrameHostImpl* root_rfh,
     bool include_non_sticky,
     bool create_tree)
+    : NotRestoredReasonBuilder(root_rfh,
+                               include_non_sticky,
+                               create_tree,
+                               /* eviction_info = */ absl::nullopt) {}
+
+BackForwardCacheImpl::NotRestoredReasonBuilder::NotRestoredReasonBuilder(
+    RenderFrameHostImpl* root_rfh,
+    bool include_non_sticky,
+    bool create_tree,
+    absl::optional<EvictionInfo> eviction_info)
     : root_rfh_(root_rfh),
       bfcache_(root_rfh_->frame_tree_node()
                    ->navigator()
                    .controller()
                    .GetBackForwardCache()),
       include_non_sticky_(include_non_sticky),
-      create_tree_(create_tree) {
+      create_tree_(create_tree),
+      eviction_info_(eviction_info) {
   // |root_rfh_| should be either primary main frame or back/forward cached
   // page's main frame.
   DCHECK(root_rfh_->IsInPrimaryMainFrame() ||
@@ -1008,9 +1032,23 @@ BackForwardCacheImpl::NotRestoredReasonBuilder::~NotRestoredReasonBuilder() =
 std::unique_ptr<BackForwardCacheCanStoreTreeResult> BackForwardCacheImpl::
     NotRestoredReasonBuilder::PopulateReasonsAndReturnSubtreeIfNeededFor(
         RenderFrameHostImpl* rfh) {
+  // TODO(https://crbug.com/1280150): Add cache-control:no-store reasons to the
+  // tree.
+
   BackForwardCacheCanStoreDocumentResult result_for_rfh;
-  // Populate |result_for_rfh| by checking the bfcache eligibility of |rfh|.
-  bfcache_.PopulateReasonsForDocument(result_for_rfh, rfh, include_non_sticky_);
+  if (eviction_info_.has_value()) {
+    // When |eviction_info_| is set, that means that we are populating the
+    // reasons for eviction. In that case, we do not need to check each frame's
+    // eligibility, but only mark |rfh_to_be_evicted| with |reasons|, as it is
+    // the cause of eviction.
+    if (rfh == eviction_info_->rfh_to_be_evicted) {
+      result_for_rfh.AddReasonsFrom(*(eviction_info_->reasons));
+    }
+  } else {
+    // Populate |result_for_rfh| by checking the bfcache eligibility of |rfh|.
+    bfcache_.PopulateReasonsForDocument(result_for_rfh, rfh,
+                                        include_non_sticky_);
+  }
   flattened_result_.AddReasonsFrom(result_for_rfh);
 
   // Finds the reasons recursively and create the reason subtree for the
@@ -1281,8 +1319,7 @@ BackForwardCacheImpl::Entry* BackForwardCacheImpl::GetEntry(
     if (!can_store) {
       (*matching_entry)
           ->render_frame_host()
-          ->EvictFromBackForwardCacheWithReasons(
-              can_store.flattened_reasons, std::move(can_store.tree_reasons));
+          ->EvictFromBackForwardCacheWithFlattenedAndTreeReasons(can_store);
     }
   }