Introduce content-side Feature Policy object and maintain in parallel with renderer policy.

content/browser/frame_host/frame_tree_node.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/frame_host/frame_tree_node.h"
 
 #include <queue>
 #include <utility>
 
 #include "base/lazy_instance.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/profiler/scoped_tracker.h"
 #include "base/stl_util.h"
 #include "content/browser/frame_host/frame_tree.h"
 #include "content/browser/frame_host/navigation_request.h"
 #include "content/browser/frame_host/navigator.h"
 #include "content/browser/frame_host/render_frame_host_impl.h"
 #include "content/browser/renderer_host/render_view_host_impl.h"
+#include "content/common/feature_policy/feature_policy.h"
 #include "content/common/frame_messages.h"
 #include "content/common/site_isolation_policy.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/common/browser_side_navigation_policy.h"
 #include "third_party/WebKit/public/web/WebSandboxFlags.h"
 
 namespace content {
 
 namespace {
 
@@ skipping 245 matching lines @@
   }
 
   RecordUniqueNameLength(unique_name.size());
   render_manager_.OnDidUpdateName(name, unique_name);
   replication_state_.name = name;
   replication_state_.unique_name = unique_name;
 }
 
 void FrameTreeNode::SetFeaturePolicyHeader(
     const ParsedFeaturePolicyHeader& parsed_header) {
+  ResetFeaturePolicy();
   replication_state_.feature_policy_header = parsed_header;
+  current_frame_host()->GetFeaturePolicy()->SetHeaderPolicy(parsed_header);

[alexmos, 2017/02/07 00:21:35]

Could we move this call to RFHI::OnDidSetFeaturePolicyHeader, where we can
directly call SetHeaderPolicy on the RFHI's feature_policy_?

I'm realizing also that RFHI::OnDidSetFeaturePolicyHeader might be vulnerable to
the race from https://bugs.chromium.org/p/chromium/issues/detail?id=686188
(another cross-process navigation could commit just before we receive the
DidSetFeaturePolicyHeader IPC, making the receiving RFH pending deletion.  In
that case it probably doesn't make sense to update the FTN header, but I'll deal
with that separately, as it affects other IPCs as well.)

[iclelland, 2017/02/08 05:14:42]

Done -- you're absolutely right that it doesn't make sense to call back to the
RFHI here, since that's what called this method in the first place. 

 }
 
 void FrameTreeNode::ResetFeaturePolicy() {
   replication_state_.feature_policy_header.clear();
+  FeaturePolicy* parent_policy =
+      parent() ? parent()->current_frame_host()->GetFeaturePolicy() : nullptr;
+  current_frame_host()->SetFeaturePolicy(

[alexmos, 2017/02/07 00:21:35]

Similarly, it might be safer to call this on the right |render_frame_host|
directly from the ResetFeaturePolicy call site in NavigatorImpl::DidNavigate. 
Currently, it looks like that function might call DidNavigateFrame either before
or after the call to ResetFeaturePolicy, depending on |oopifs_possible|, and
DidNavigateFrame is what eventually calls CommitPending and updates
current_frame_host() (i.e., you might be grabbing the wrong one here).

[iclelland, 2017/02/07 15:47:37]

I can definitely do that -- and it brings up the issue that the current origin
isn't well defined at this point; the new origin has been set current in the
FTR, but not committed in the RFH. What do you think of moving this to near the
end of NavigatorImpl::DidNavigate, after the new origin is committed, or
actually into RFHI::set_last_committed_origin?

[alexmos, 2017/02/07 23:15:15]

Right, at the point where ResetFeaturePolicy is called you have to be careful in
that the FTN origin (from the replication state) has been updated, but the RFHI
origin hasn't.  All the work that updates the replication state
(FTN::SetCurrentOrigin, ResetFeaturePolicy, ResetContentSecurityPolicy) needs to
happen before DidNavigateFrame, which send that replication state to the new
proxy that replaces the old RF that we just navigated from.  So that can't be
moved after set_last_committed_origin, as that comes after DidNavigateFrame.

But I think calling SetFeaturePolicy() after the set_last_committed_origin() in
DidNavigate makes sense -- that will kind of go with the existing flow that
first we update the things stored on replication state, then commit the new RFH
with DidNavigateFrame, then update the things stored on RFHI.  Or you could just
pass the new origin (|params.origin|) to it directly if you wanted to keep all
feature policy calls together.

Having it inside RFHI::set_last_committed_origin seems a bit more weird, as
feature policy doesn't seem much related to updating the origin.

[iclelland, 2017/02/08 05:14:42]

That's what I've done in the latest patch.
I'd considered that, but I think this makes more sense; it's easier to see what
the ResetFeaturePolicy method does, and there's no question about what you need
to pass to it.

+      FeaturePolicy::CreateFromParentPolicy(parent_policy, current_origin()));
 }
 
 void FrameTreeNode::AddContentSecurityPolicy(
     const ContentSecurityPolicyHeader& header) {
   replication_state_.accumulated_csp_headers.push_back(header);
   render_manager_.OnDidAddContentSecurityPolicy(header);
 }
 
 void FrameTreeNode::ResetContentSecurityPolicy() {
   replication_state_.accumulated_csp_headers.clear();
@@ skipping 251 matching lines @@
       }
       return parent_->child_at(i + relative_offset);
     }
   }
 
   NOTREACHED() << "FrameTreeNode not found in its parent's children.";
   return nullptr;
 }
 
 }  // namespace content