Refactor PolicyContainerHost inheritance in NavigationRequest.

This change is a functional no-op.

Along the way, I took the opportunity to improve documentation on
PolicyContainerHost constructors that are called in this change.

Bug: chromium:1126856, chromium:1154729
Change-Id: I28a600c00fbcb107d9c2ed72dd6070bc05fb62ca
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2600744
Commit-Queue: Titouan Rigoudy <titouan@chromium.org>
Auto-Submit: Titouan Rigoudy <titouan@chromium.org>
Reviewed-by: Arthur Sonzogni <arthursonzogni@chromium.org>
Reviewed-by: Antonio Sartori <antoniosartori@chromium.org>
Cr-Commit-Position: refs/heads/master@{#845193}

content/browser/renderer_host/navigation_request.cc

@@ -1109,41 +1109,7 @@ NavigationRequest::NavigationRequest(
   TRACE_EVENT_NESTABLE_ASYNC_BEGIN0("navigation", "Initializing",
                                     navigation_id_);
 
-  policy_container_host_ = base::MakeRefCounted<PolicyContainerHost>();
-
-  if (frame_entry && frame_entry->document_policies()) {
-    // If there is a history entry with some document policies, initialize the
-    // PolicyContainerHost with them, so that they will get applied to the
-    // document created by the navigation.
-    policy_container_host_ = base::MakeRefCounted<PolicyContainerHost>(
-        *frame_entry->document_policies());
-  } else if (common_params_->url.IsAboutSrcdoc()) {
-    // Srcdoc iframes inherit their policies from their parent.
-    // If there is no parent, the navigation will be blocked in BeginNavigation.
-    if (frame_tree_node_->parent()) {
-      policy_container_host_ =
-          frame_tree_node_->parent()->policy_container_host()->Clone();
-    }
-  } else if (common_params_->url.SchemeIs(url::kAboutScheme) ||
-             common_params_->url.SchemeIs(url::kDataScheme) ||
-             common_params_->url.SchemeIs(url::kBlobScheme) ||
-             common_params_->url.SchemeIs(url::kFileSystemScheme)) {
-    // Local schemes inherit the policy container  from the initiator.
-    //
-    // TODO(antoniosartori): Fill up the PolicyContainerHost and/or replace it
-    // with a new one whenever needed (e.g. blob: or filesystem: URLs should get
-    // the policy container from the document which created them and not from
-    // the initiator of the navigation).
-    if (initiator_frame_token_) {
-      // We use PolicyContainerHost::FromFrameToken directly since this will
-      // retrieve the PolicyContainerHost of the initiator RenderFrameHost even
-      // if the RenderFrameHost has already been deleted.
-      PolicyContainerHost* initiator_policy_container_host =
-          PolicyContainerHost::FromFrameToken(initiator_frame_token_.value());
-      DCHECK(initiator_policy_container_host);
-      policy_container_host_ = initiator_policy_container_host->Clone();
-    }
-  }
+  InitializePolicyContainerHost(frame_entry);
 
   // Initialize the ClientSecurityState's COEP to that of the current document.
   // It will be updated when a network response is received. For navigations
@@ -1310,6 +1276,67 @@ NavigationRequest::NavigationRequest(
   commit_params_->is_browser_initiated = browser_initiated_;
 }
 
+scoped_refptr<PolicyContainerHost>
+NavigationRequest::MaybeInheritPolicyContainerHost(
+    const FrameNavigationEntry* frame_navigation_entry) {
+  if (frame_navigation_entry && frame_navigation_entry->document_policies()) {
+    // If there is a history entry with some document policies, initialize the
+    // PolicyContainerHost with them, so that they will get applied to the
+    // document created by the navigation.
+    return base::MakeRefCounted<PolicyContainerHost>(
+        *frame_navigation_entry->document_policies());
+  }
+
+  // Srcdoc iframes inherit their policies from their parent.
+  if (common_params_->url.IsAboutSrcdoc()) {
+    RenderFrameHostImpl* parent = GetParentFrame();
+    if (!parent) {
+      // The navigation will be blocked in BeginNavigation.
+      return nullptr;
+    }
+
+    return parent->policy_container_host()->Clone();
+  }
+
+  // Local schemes inherit the policy container from the initiator.
+  //
+  // TODO(antoniosartori): Fill up the PolicyContainerHost and/or replace it
+  // with a new one whenever needed (e.g. blob: or filesystem: URLs should get
+  // the policy container from the document which created them and not from the
+  // initiator of the navigation).
+  if (common_params_->url.SchemeIs(url::kAboutScheme) ||
+      common_params_->url.SchemeIs(url::kDataScheme) ||
+      common_params_->url.SchemeIs(url::kBlobScheme) ||
+      common_params_->url.SchemeIs(url::kFileSystemScheme)) {
+    if (!initiator_frame_token_) {
+      return nullptr;
+    }
+
+    // We use PolicyContainerHost::FromFrameToken directly since this will
+    // retrieve the PolicyContainerHost of the initiator RenderFrameHost even if
+    // the RenderFrameHost has already been deleted.
+    PolicyContainerHost* initiator_policy_container_host =
+        PolicyContainerHost::FromFrameToken(initiator_frame_token_.value());
+    DCHECK(initiator_policy_container_host);
+
+    return initiator_policy_container_host->Clone();
+  }
+
+  return nullptr;
+}
+
+void NavigationRequest::InitializePolicyContainerHost(
+    const FrameNavigationEntry* frame_navigation_entry) {
+  policy_container_host_ =
+      MaybeInheritPolicyContainerHost(frame_navigation_entry);
+
+  // Use a default value if none was inherited. It will be filled up with data
+  // from this navigation before it commits.
+  if (!policy_container_host_) {
+    policy_container_host_ = base::MakeRefCounted<PolicyContainerHost>();
+  }
+}
+
 NavigationRequest::~NavigationRequest() {
 #if DCHECK_IS_ON()
   // If |is_safe_to_delete_| is false, it means |this| is being deleted at an

content/browser/renderer_host/navigation_request.h

@@ -823,6 +823,19 @@ class CONTENT_EXPORT NavigationRequest
       int initiator_process_id,
       bool was_opener_suppressed);
 
+  // Helper for InitializePolicyContainerHost().
+  //
+  // Logically const, as it does not mutate this class' state. It does however
+  // call into NavigationHandle interface methods which are non-const, which
+  // prevents us from marking this method `const`.
+  scoped_refptr<PolicyContainerHost> MaybeInheritPolicyContainerHost(
+      const FrameNavigationEntry* frame_navigation_entry);
+
+  // Initializes |policy_container_host_| to a non-nullptr value.
+  // Constructor helper.
+  void InitializePolicyContainerHost(
+      const FrameNavigationEntry* frame_navigation_entry);
+
   // Checks if the response requests an isolated origin via the
   // Origin-Agent-Cluster header, and if so opts in the origin to be isolated.
   void CheckForIsolationOptIn(const GURL& url);

content/browser/renderer_host/policy_container_host.h

@@ -57,8 +57,15 @@ class CONTENT_EXPORT PolicyContainerHost
         network::mojom::IPAddressSpace::kUnknown;
   };
 
+  // Constructs a PolicyContainerHost containing default document policies and
+  // an unbound mojo receiver.
   PolicyContainerHost();
+
+  // Constructs a PolicyContainerHost containing the given |document_policies|
+  // and an unbound mojo receiver.
   explicit PolicyContainerHost(const DocumentPolicies& document_policies);
+
+  // PolicyContainerHost instances are neither copyable nor movable.
   PolicyContainerHost(const PolicyContainerHost&) = delete;
   PolicyContainerHost& operator=(const PolicyContainerHost&) = delete;
 
@@ -73,6 +80,10 @@ class CONTENT_EXPORT PolicyContainerHost
   // PolicyContainerHost::FromFrameToken. This function can be called only once.
   void AssociateWithFrameToken(const base::UnguessableToken& token);
 
+  const DocumentPolicies& document_policies() const {
+    return document_policies_;
+  }
+
   network::mojom::ReferrerPolicy referrer_policy() const {
     return document_policies_.referrer_policy;
   }
@@ -84,10 +95,6 @@ class CONTENT_EXPORT PolicyContainerHost
     document_policies_.ip_address_space = ip_address_space;
   }
 
-  const DocumentPolicies& document_policies() const {
-    return document_policies_;
-  }
-
   // Return a PolicyContainer containing copies of the policies and a pending
   // mojo remote that can be used to update policies in this object. If called a
   // second time, it resets the receiver and creates a new PolicyContainer,