[bfcache] Do not lose CSP policy headers after bfcache restore

At the moment CSP headers are stored in FrameTreeNode and reset when
the document in the FTN changes.

This patch:
- Disables resetting CSP policies stored on RFH when it is restored from bfcache.
- Copies the policies stored on RFH to FTN's replication state during bfcache restore.
- Evicts the page from bfcache if it has frame policy headers.

R=alexmos@chromium.org,arthursonzogni@chromium.org
BUG=1146025

Change-Id: I486c9bf30670a21e631ec70f7efc2ed305c96e62
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2519460
Commit-Queue: Alexander Timin <altimin@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Auto-Submit: Alexander Timin <altimin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#824694}

content/browser/back_forward_cache_browsertest.cc

@@ -8375,4 +8375,90 @@ IN_PROC_BROWSER_TEST_F(BackForwardCacheBrowserTest,
   delete_observer_rfh_a2.WaitUntilDeleted();
 }
 
+IN_PROC_BROWSER_TEST_F(BackForwardCacheBrowserTest,
+                       MainDocumentCSPHeadersAreRestored) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+
+  GURL url_a(embedded_test_server()->GetURL(
+      "a.com",
+      "/set-header?"
+      "Content-Security-Policy: frame-src 'none'"));
+  GURL url_b(embedded_test_server()->GetURL("b.com", "/title1.html"));
+
+  // 1) Navigate to A, which should set CSP.
+  EXPECT_TRUE(NavigateToURL(shell(), url_a));
+  RenderFrameHostImpl* rfh_a = current_frame_host();
+
+  // Check that CSP was set.
+  {
+    const std::vector<network::mojom::ContentSecurityPolicyHeader>& root_csp =
+        current_frame_host()
+            ->frame_tree_node()
+            ->current_replication_state()
+            .accumulated_csp_headers;
+    EXPECT_EQ(1u, root_csp.size());
+    EXPECT_EQ("frame-src 'none'", root_csp[0].header_value);
+  }
+
+  // 2) Navigate to B.
+  EXPECT_TRUE(NavigateToURL(shell(), url_b));
+
+  // 3) Navigate back and expect that the CSP headers are present on the main
+  // frame.
+  web_contents()->GetController().GoBack();
+  EXPECT_TRUE(WaitForLoadStop(shell()->web_contents()));
+  EXPECT_EQ(rfh_a, current_frame_host());
+  ExpectOutcome(BackForwardCacheMetrics::HistoryNavigationOutcome::kRestored,
+                FROM_HERE);
+
+  // Check that CSP was restored.
+  {
+    const std::vector<network::mojom::ContentSecurityPolicyHeader>& root_csp =
+        current_frame_host()
+            ->frame_tree_node()
+            ->current_replication_state()
+            .accumulated_csp_headers;
+    EXPECT_EQ(1u, root_csp.size());
+    EXPECT_EQ("frame-src 'none'", root_csp[0].header_value);
+  }
+}
+
+// Sandboxed documents are not cached because we don't properly restore sandbox
+// flags at the moment.
+// TODO(altimin, carlscab): Remove this after sandbox flags will move to RFH /
+// BrowsingInstanceFrameState.
+IN_PROC_BROWSER_TEST_F(BackForwardCacheBrowserTest, SandboxedFramesNotCached) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+
+  GURL url_a(
+      embedded_test_server()->GetURL("a.com",
+                                     "/set-header?"
+                                     "Content-Security-Policy: sandbox"));
+  GURL url_b(embedded_test_server()->GetURL("b.com", "/title1.html"));
+
+  // 1) Navigate to A, which should set CSP.
+  EXPECT_TRUE(NavigateToURL(shell(), url_a));
+
+  // Check that CSP was set.
+  {
+    const std::vector<network::mojom::ContentSecurityPolicyHeader>& root_csp =
+        current_frame_host()
+            ->frame_tree_node()
+            ->current_replication_state()
+            .accumulated_csp_headers;
+    EXPECT_EQ(1u, root_csp.size());
+    EXPECT_EQ("sandbox", root_csp[0].header_value);
+  }
+
+  // 2) Navigate to B.
+  EXPECT_TRUE(NavigateToURL(shell(), url_b));
+
+  // 3) Navigate back and expect that the page wasn't restored from bfcache.
+  web_contents()->GetController().GoBack();
+  EXPECT_TRUE(WaitForLoadStop(shell()->web_contents()));
+  ExpectNotRestored(
+      {BackForwardCacheMetrics::NotRestoredReason::kFrameTreeNodeStateReset},
+      FROM_HERE);
+}
+
 }  // namespace content

content/browser/renderer_host/back_forward_cache_can_store_document_result.cc

@@ -131,6 +131,9 @@ std::string BackForwardCacheCanStoreDocumentResult::NotRestoredReasonToString(
       return "BackForwardCache is disabled through command line (may include "
              "cases where the embedder disabled it due to, e.g., enterprise "
              "policy)";
+    case Reason::kFrameTreeNodeStateReset:
+      return "document-associated state stored in FrameTreeNode was lost after "
+             "navigating away";
   }
 }
 

content/browser/renderer_host/back_forward_cache_metrics.h

@@ -85,7 +85,8 @@ class BackForwardCacheMetrics
     // BackForwardCache is disabled due to command-line switch (may include
     // cases where the embedder disabled it due to, e.g., enterprise policy).
     kBackForwardCacheDisabledByCommandLine = 35,
-    kMaxValue = kBackForwardCacheDisabledByCommandLine,
+    kFrameTreeNodeStateReset = 36,
+    kMaxValue = kFrameTreeNodeStateReset,
   };
 
   using NotRestoredReasons =

content/browser/renderer_host/frame_tree_node.cc

@@ -235,21 +235,42 @@ bool FrameTreeNode::IsMainFrame() const {
   return frame_tree_->root() == this;
 }
 
-void FrameTreeNode::ResetForNavigation() {
-  // Discard any CSP headers from the previous document.
+FrameTreeNode::ResetForNavigationResult FrameTreeNode::ResetForNavigation(
+    bool was_served_from_back_forward_cache) {
+  // TODO(altimin,carlscab): Remove this logic after the relevant states are
+  // moved to RenderFrameHost or BrowsingInstanceFrameState.
+  ResetForNavigationResult result;
+
   replication_state_.accumulated_csp_headers.clear();
-  render_manager_.OnDidResetContentSecurityPolicy();
+  if (!was_served_from_back_forward_cache) {
+    render_manager_.OnDidResetContentSecurityPolicy();
+  } else {
+    for (auto& policy : current_frame_host()->ContentSecurityPolicies()) {
+      replication_state_.accumulated_csp_headers.push_back(*policy->header);
+    }
+    // Note: there is no need to call OnDidResetContentSecurityPolicy or any
+    // other update as the proxies are being restored from bfcache as well and
+    // they already have the correct value.
+  }
 
   // Clear any CSP-set sandbox flags, and the declared feature policy for the
   // frame.
-  UpdateFramePolicyHeaders(network::mojom::WebSandboxFlags::kNone, {});
+  // TODO(https://crbug.com/1145886): Remove this.
+  result.changed_frame_policy =
+      UpdateFramePolicyHeaders(network::mojom::WebSandboxFlags::kNone, {});
 
   // This frame has had its user activation bits cleared in the renderer
   // before arriving here. We just need to clear them here and in the other
   // renderer processes that may have a reference to this frame.
+  //
+  // We do not take user activation into account when calculating
+  // |ResetForNavigationResult|, as we are using it to determine bfcache
+  // eligibility and the page can get another user gesture after restore.
   UpdateUserActivationState(
       blink::mojom::UserActivationUpdateType::kClearActivation,
       blink::mojom::UserActivationNotificationType::kNone);
+
+  return result;
 }
 
 size_t FrameTreeNode::GetFrameTreeSize() const {
@@ -732,7 +753,7 @@ FrameTreeNode* FrameTreeNode::GetSibling(int relative_offset) const {
   return nullptr;
 }
 
-void FrameTreeNode::UpdateFramePolicyHeaders(
+bool FrameTreeNode::UpdateFramePolicyHeaders(
     network::mojom::WebSandboxFlags sandbox_flags,
     const blink::ParsedFeaturePolicy& parsed_header) {
   bool changed = false;
@@ -751,6 +772,7 @@ void FrameTreeNode::UpdateFramePolicyHeaders(
   // Notify any proxies if the policies have been changed.
   if (changed)
     render_manager()->OnDidSetFramePolicyHeaders();
+  return changed;
 }
 
 void FrameTreeNode::PruneChildFrameNavigationEntries(

content/browser/renderer_host/frame_tree_node.h

@@ -93,12 +93,19 @@ class CONTENT_EXPORT FrameTreeNode {
 
   bool IsMainFrame() const;
 
+  struct ResetForNavigationResult {
+    bool changed_frame_policy = false;
+  };
+
   // Clears any state in this node which was set by the document itself (CSP
   // Headers, Feature Policy Headers, and CSP-set sandbox flags), and notifies
   // proxies as appropriate. Invoked after committing navigation to a new
   // document (since the new document comes with a fresh set of CSP and
   // Feature-Policy HTTP headers).
-  void ResetForNavigation();
+  // Returns the details of the reset result — whether any important state (e.g.
+  // frame policy headers) was lost during the update.
+  ResetForNavigationResult ResetForNavigation(
+      bool was_served_from_back_forward_cache);
 
   FrameTree* frame_tree() const { return frame_tree_; }
   Navigator& navigator() { return frame_tree()->navigator(); }
@@ -383,7 +390,9 @@ class CONTENT_EXPORT FrameTreeNode {
   // Feature-Policy header being set. Note that on navigation, these updates
   // will be cleared, and the flags in the pending frame policy will be applied
   // to the frame.
-  void UpdateFramePolicyHeaders(
+  // Returns true iff this operation has changed state of either sandbox flags
+  // or feature policy.
+  bool UpdateFramePolicyHeaders(
       network::mojom::WebSandboxFlags sandbox_flags,
       const blink::ParsedFeaturePolicy& parsed_header);
 

content/browser/renderer_host/navigator.cc

@@ -231,14 +231,15 @@ void Navigator::DidNavigate(
   DCHECK(navigation_request);
   FrameTreeNode* frame_tree_node = render_frame_host->frame_tree_node();
   FrameTree* frame_tree = frame_tree_node->frame_tree();
-  RenderFrameHostImpl* old_frame_host =
-      frame_tree_node->render_manager()->current_frame_host();
+  base::WeakPtr<RenderFrameHostImpl> old_frame_host =
+      frame_tree_node->render_manager()->current_frame_host()->GetWeakPtr();
 
   bool is_same_document_navigation = controller_->IsURLSameDocumentNavigation(
       params.url, params.origin, was_within_same_document, render_frame_host);
   // If a frame claims the navigation was same-document, it must be the current
   // frame, not a pending one.
-  if (is_same_document_navigation && render_frame_host != old_frame_host) {
+  if (is_same_document_navigation &&
+      render_frame_host != old_frame_host.get()) {
     bad_message::ReceivedBadMessage(render_frame_host->GetProcess(),
                                     bad_message::NI_IN_PAGE_NAVIGATION);
     is_same_document_navigation = false;
@@ -265,13 +266,15 @@ void Navigator::DidNavigate(
     // cache), because on normal same-site navigations the unloading of the old
     // RenderFrameHost happens before commit. We're measuring how often this
     // case happens to determine the risk of this change.
-    DCHECK_NE(old_frame_host, render_frame_host);
+    DCHECK(old_frame_host.get());
+    DCHECK_NE(old_frame_host.get(), render_frame_host);
     DCHECK(frame_tree_node->IsMainFrame());
     DCHECK(!old_frame_host->GetSiteInstance()->IsRelatedSiteInstance(
         render_frame_host->GetSiteInstance()));
     DCHECK(is_cross_document_same_site_navigation);
     bool can_store_in_back_forward_cache =
-        controller_->GetBackForwardCache().CanStorePageNow(old_frame_host);
+        controller_->GetBackForwardCache().CanStorePageNow(
+            old_frame_host.get());
     UMA_HISTOGRAM_BOOLEAN(
         "BackForwardCache.ProactiveSameSiteBISwap.EligibilityDuringCommit",
         can_store_in_back_forward_cache);
@@ -327,8 +330,24 @@ void Navigator::DidNavigate(
   if (!is_same_document_navigation) {
     // Navigating to a new location means a new, fresh set of http headers
     // and/or <meta> elements - we need to reset CSP and Feature Policy.
-    render_frame_host->ResetContentSecurityPolicies();
-    frame_tree_node->ResetForNavigation();
+    // However, if the navigation is restoring the given |render_frame_host|
+    // from back-forward cache, it does not change the document in the given
+    // RenderFrameHost and the existing Content Security Policy should be kept.
+    if (!navigation_request->IsServedFromBackForwardCache())
+      render_frame_host->ResetContentSecurityPolicies();
+
+    auto reset_result = frame_tree_node->ResetForNavigation(
+        navigation_request->IsServedFromBackForwardCache());
+
+    // |old_frame_host| might get immediately deleted after the DidNavigateFrame
+    // call above, so use weak pointer here.
+    if (old_frame_host && old_frame_host->IsInBackForwardCache()) {
+      if (reset_result.changed_frame_policy) {
+        old_frame_host->EvictFromBackForwardCacheWithReason(
+            BackForwardCacheMetrics::NotRestoredReason::
+                kFrameTreeNodeStateReset);
+      }
+    }
   }
 
   // Update the site of the SiteInstance if it doesn't have one yet, unless