Replicate feature policy headers to remote frames

content/browser/site_per_process_browsertest.cc

Index: content/browser/site_per_process_browsertest.cc
diff --git a/content/browser/site_per_process_browsertest.cc b/content/browser/site_per_process_browsertest.cc
index c1d62ea883c48a8b77fad5ad336fd3fc5b782cc5..128d19d3f74c9397b0b9ea0bf450dcf22c8907af 100644
--- a/content/browser/site_per_process_browsertest.cc
+++ b/content/browser/site_per_process_browsertest.cc
@@ -558,6 +558,8 @@ void SitePerProcessBrowserTest::SetUpCommandLine(
   // FrameOwnerPropertiesPropagationScrolling. crbug.com/662196.
   command_line->AppendSwitch(switches::kDisableOverlayScrollbar);
 #endif
+  command_line->AppendSwitchASCII(switches::kEnableBlinkFeatures,

[alexmos, 2016/11/17 21:45:59]

Do you mind splitting this out into a new subclass of SitePerProcessBrowserTest?
 Similar to the way SitePerProcessHighDPIBrowserTest or
SitePerProcessEmbedderCSPEnforcementBrowserTest is set up?

[iclelland, 2016/11/18 16:12:37]

That's a good idea, I hadn't seen that -- thanks. Done.

+                                  "FeaturePolicy");
 };
 
 void SitePerProcessBrowserTest::SetUpOnMainThread() {
@@ -8562,4 +8564,56 @@ IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest,
   EXPECT_EQ(expected_title, title_watcher.WaitAndGetTitle());
 }
 
+IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest,

[alexmos, 2016/11/17 21:45:59]

These tests provide good coverage that the browser process has correct knowledge
about the feature policy, but they don't test whether we also replicate it
correctly to remote frames, which I think we also should have, if possible.  The
layout tests you used to have provided most of that coverage, so you could bring
those back, or you could add another test here with a cross-site iframe, similar
to what's done in CrossSiteIframeBlockedByParentCSPFromHeaders,
FrameOwnerPropertiesPropagationCSP,
PassiveMixedContentInIframeWithStrictBlocking, etc.  With a cross-site iframe,
we'd be checking that we propagate the feature policy correctly to the OOPIF's
remote parent in order for the frame itself to get the right policy.

[iclelland, 2016/11/18 16:12:37]

Definitely should have that. I'm bringing the layout tests back for specific
features, so those should test it, on the site-isolation bots. 

For the generic framework, I like your idea of adding a couple more browser
tests. Unfortunately, I don't currently have a generic way to get the renderer
to tell the browser about its feature policy details. (We haven't specced a JS
api for that). I could create an internals interface for it, or we could leave
the testing to layout tests for specific features (See
https://codereview.chromium.org/2492623002/, for instance)
 

[alexmos, 2016/11/18 17:21:44]

In that case I think it's fine to go with layout tests.  But it doesn't seem
like they made it to the latest patchset?

+                       TestFeaturePolicyReplicationOnSameOriginNavigation) {
+  GURL a_url(embedded_test_server()->GetURL("a.com", "/feature-policy1.html"));
+  GURL b_url(embedded_test_server()->GetURL("a.com", "/feature-policy2.html"));

[alexmos, 2016/11/17 21:45:59]

nit: b_url implies that it's at b.com (this is the convention we've been using
in this file).  Can you rename it to something else?  Same for c_url in this
test.

[iclelland, 2016/11/18 16:12:37]

Done.

+  GURL c_url(embedded_test_server()->GetURL("a.com", "/title2.html"));
+
+  EXPECT_TRUE(NavigateToURL(shell(), a_url));
+
+  FrameTreeNode* root = web_contents()->GetFrameTree()->root();
+  EXPECT_EQ("{\"vibrate\":[\"self\"]}",
+            root->current_replication_state().feature_policy_header);
+
+  // When the main frame navigates to a page with a new policy, it should
+  // overwrite the old one.
+  EXPECT_TRUE(NavigateToURL(shell(), b_url));
+  EXPECT_EQ("{\"vibrate\":[\"*\"]}",
+            root->current_replication_state().feature_policy_header);
+
+  // When the main frame navigates to a page without a policy, the replicated
+  // policy header should be cleared.
+  EXPECT_TRUE(NavigateToURL(shell(), c_url));
+  EXPECT_EQ("", root->current_replication_state().feature_policy_header);
+}
+
+IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest,
+                       TestFeaturePolicyReplicationOnCrossOriginNavigation) {
+  GURL a_url(embedded_test_server()->GetURL("a.com", "/feature-policy1.html"));
+  GURL b_url(embedded_test_server()->GetURL("b.com", "/feature-policy2.html"));
+  GURL c_url(embedded_test_server()->GetURL("c.com", "/title2.html"));
+
+  EXPECT_TRUE(NavigateToURL(shell(), a_url));
+
+  FrameTreeNode* root = web_contents()->GetFrameTree()->root();
+  EXPECT_EQ("{\"vibrate\":[\"self\"]}",
+            root->current_replication_state().feature_policy_header);
+
+  TestFrameNavigationObserver frame_observer(root);

[alexmos, 2016/11/17 21:45:58]

This doesn't seem necessary.  NavigateToURL will wait for for the navigation to
fully finish: it will return after DidStopLoading, which is after commit.  You
can use one or the other; I think just NavigateToURL should be sufficient.

[iclelland, 2016/11/18 16:12:37]

Removed... somehow during testing it looked like I was seeing a situation where
the second navigation wouldn't start because the first one was still pending,
but I must have been mistaken (or it's been resolved somehow) because it doesn't
look like I need it now. Thanks.

+
+  // When the main frame navigates to a page with a new policy, it should
+  // overwrite the old one.
+  EXPECT_TRUE(NavigateToURL(shell(), b_url));
+  frame_observer.WaitForCommit();
+  EXPECT_EQ("{\"vibrate\":[\"*\"]}",
+            root->current_replication_state().feature_policy_header);
+
+  // When the main frame navigates to a page without a policy, the replicated
+  // policy header should be cleared.
+  EXPECT_TRUE(NavigateToURL(shell(), c_url));
+  frame_observer.WaitForCommit();
+  EXPECT_EQ("", root->current_replication_state().feature_policy_header);
+}
+
 }  // namespace content