Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(4)

Issue 2483703002: Replicate feature policy headers to remote frames (Closed)

Created:
4 years, 1 month ago by iclelland
Modified:
4 years ago
CC:
blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, dcheng, dglazkov+blink, gavinp+loader_chromium.org, jam, Nate Chapin, kinuko+watch, loading-reviews_chromium.org, mlamouri+watch-content_chromium.org, mlamouri+watch-blink_chromium.org, nasko+codewatch_chromium.org, site-isolation-reviews_chromium.org, tyoshino+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Replicate feature policy headers to remote frames BUG=661273 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/ab749ec9b38b37cfe2b173fda6f20f07ccd23564 Cr-Commit-Position: refs/heads/master@{#434081}

Patch Set 1 #

Total comments: 6

Patch Set 2 : Get replication actually working; add layout tests #

Total comments: 27

Patch Set 3 : Addressing review comments #

Total comments: 2

Patch Set 4 : Clear replicated FP header on navigation; add tests for same #

Patch Set 5 : Add missing test data files #

Patch Set 6 : Remove layout test; better coverage with unit tests #

Total comments: 15

Patch Set 7 : Addressing review comments #

Patch Set 8 : Add testing interface to override FP #

Patch Set 9 : Should just build 'all' all the time #

Patch Set 10 : Add browsertest for subframe navigation #

Total comments: 10

Patch Set 11 : Rebase #

Patch Set 12 : Addressing review comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+278 lines, -61 lines) Patch
M content/browser/frame_host/frame_tree_node.h View 1 2 3 1 chunk +7 lines, -0 lines 0 comments Download
M content/browser/frame_host/frame_tree_node.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +8 lines, -0 lines 0 comments Download
M content/browser/frame_host/navigator_impl.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +4 lines, -2 lines 0 comments Download
M content/browser/frame_host/render_frame_host_impl.h View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/frame_host/render_frame_host_impl.cc View 1 2 3 4 5 6 7 2 chunks +7 lines, -0 lines 0 comments Download
M content/browser/site_per_process_browsertest.cc View 1 2 3 4 5 6 7 8 9 10 2 chunks +106 lines, -0 lines 0 comments Download
M content/common/frame_messages.h View 1 2 3 4 5 6 7 8 9 10 2 chunks +5 lines, -0 lines 0 comments Download
M content/common/frame_replication_state.h View 1 1 chunk +4 lines, -0 lines 0 comments Download
M content/renderer/render_frame_impl.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -0 lines 0 comments Download
M content/renderer/render_frame_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +6 lines, -0 lines 0 comments Download
M content/renderer/render_frame_proxy.cc View 1 1 chunk +2 lines, -0 lines 0 comments Download
A content/test/data/feature-policy-main.html View 1 2 3 4 5 6 7 8 9 1 chunk +5 lines, -0 lines 0 comments Download
A content/test/data/feature-policy-main.html.mock-http-headers View 1 2 3 4 5 6 7 8 9 1 chunk +2 lines, -0 lines 0 comments Download
A content/test/data/feature-policy1.html View 1 2 3 4 1 chunk +4 lines, -0 lines 0 comments Download
A content/test/data/feature-policy1.html.mock-http-headers View 1 2 3 4 1 chunk +2 lines, -0 lines 0 comments Download
A content/test/data/feature-policy2.html View 1 2 3 4 1 chunk +4 lines, -0 lines 0 comments Download
A content/test/data/feature-policy2.html.mock-http-headers View 1 2 3 4 1 chunk +2 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/dom/SecurityContext.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +4 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/dom/SecurityContext.cpp View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +10 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/frame/FeaturePolicyInFrameTest.cpp View 1 2 3 4 5 6 7 4 chunks +6 lines, -6 lines 0 comments Download
M third_party/WebKit/Source/core/loader/FrameLoader.cpp View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +14 lines, -13 lines 0 comments Download
M third_party/WebKit/Source/core/loader/FrameLoaderClient.h View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.h View 1 2 3 4 5 6 7 1 chunk +3 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp View 1 2 3 5 chunks +12 lines, -8 lines 0 comments Download
M third_party/WebKit/Source/platform/feature_policy/FeaturePolicyFuzzer.cpp View 1 2 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/platform/feature_policy/FeaturePolicyTest.cpp View 1 2 3 4 5 6 7 8 22 chunks +28 lines, -28 lines 0 comments Download
M third_party/WebKit/Source/web/FrameLoaderClientImpl.h View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp View 1 2 3 1 chunk +6 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/web/WebRemoteFrameImpl.h View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +13 lines, -0 lines 0 comments Download
M third_party/WebKit/public/web/WebFrameClient.h View 1 2 3 1 chunk +4 lines, -0 lines 0 comments Download
M third_party/WebKit/public/web/WebRemoteFrame.h View 1 1 chunk +3 lines, -0 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 94 (60 generated)
raymes
https://codereview.chromium.org/2483703002/diff/1/content/common/frame_replication_state.h File content/common/frame_replication_state.h (right): https://codereview.chromium.org/2483703002/diff/1/content/common/frame_replication_state.h#newcode88 content/common/frame_replication_state.h:88: std::vector<std::string> feature_policy_headers; Does this really need to be a ...
4 years, 1 month ago (2016-11-07 02:16:01 UTC) #3
iclelland
https://codereview.chromium.org/2483703002/diff/1/content/common/frame_replication_state.h File content/common/frame_replication_state.h (right): https://codereview.chromium.org/2483703002/diff/1/content/common/frame_replication_state.h#newcode88 content/common/frame_replication_state.h:88: std::vector<std::string> feature_policy_headers; On 2016/11/07 02:16:01, raymes wrote: > Does ...
4 years, 1 month ago (2016-11-07 03:37:03 UTC) #4
iclelland
4 years, 1 month ago (2016-11-07 03:37:05 UTC) #5
iclelland
+r mkwst -- can you PTAL? (Especially as OWNER for frame_messages and WebKit, but also ...
4 years, 1 month ago (2016-11-07 21:42:25 UTC) #11
raymes
lg - this is cool :) https://codereview.chromium.org/2483703002/diff/20001/content/renderer/render_frame_impl.cc File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2483703002/diff/20001/content/renderer/render_frame_impl.cc#newcode3128 content/renderer/render_frame_impl.cc:3128: return; Hmm - ...
4 years, 1 month ago (2016-11-07 23:10:25 UTC) #12
iclelland
https://codereview.chromium.org/2483703002/diff/20001/content/renderer/render_frame_impl.cc File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2483703002/diff/20001/content/renderer/render_frame_impl.cc#newcode3128 content/renderer/render_frame_impl.cc:3128: return; On 2016/11/07 23:10:25, raymes wrote: > Hmm - ...
4 years, 1 month ago (2016-11-08 03:13:41 UTC) #15
Mike West
In general, this looks reasonable. I have some suggestions in comments, but I'll defer to ...
4 years, 1 month ago (2016-11-08 12:12:23 UTC) #17
iclelland
https://codereview.chromium.org/2483703002/diff/20001/third_party/WebKit/Source/core/loader/FrameLoader.cpp File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/2483703002/diff/20001/third_party/WebKit/Source/core/loader/FrameLoader.cpp#newcode603 third_party/WebKit/Source/core/loader/FrameLoader.cpp:603: } On 2016/11/08 12:12:23, Mike West wrote: > An ...
4 years, 1 month ago (2016-11-08 14:25:45 UTC) #18
alexmos
Looks reasonable to me as well. Just a few comments, and it seems you need ...
4 years, 1 month ago (2016-11-09 01:16:55 UTC) #19
alexmos
[also, +site-isolation-reviews]
4 years, 1 month ago (2016-11-09 01:27:49 UTC) #20
iclelland
Thanks, Alex! > Looks reasonable to me as well. Just a few comments, and it ...
4 years, 1 month ago (2016-11-09 18:07:59 UTC) #23
iclelland
On 2016/11/09 18:07:59, iclelland wrote: > Thanks, Alex! > > > Looks reasonable to me ...
4 years, 1 month ago (2016-11-09 18:13:52 UTC) #24
alexmos
https://codereview.chromium.org/2483703002/diff/20001/third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp File third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp (right): https://codereview.chromium.org/2483703002/diff/20001/third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp#newcode441 third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp:441: void WebRemoteFrameImpl::setReplicatedFeaturePolicyHeader( On 2016/11/09 18:07:59, iclelland wrote: > On ...
4 years, 1 month ago (2016-11-10 17:50:28 UTC) #27
iclelland
https://codereview.chromium.org/2483703002/diff/20001/third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp File third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp (right): https://codereview.chromium.org/2483703002/diff/20001/third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp#newcode441 third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp:441: void WebRemoteFrameImpl::setReplicatedFeaturePolicyHeader( On 2016/11/10 17:50:28, alexmos wrote: > On ...
4 years, 1 month ago (2016-11-16 20:21:32 UTC) #30
Mike West
This LGTM, but I'm marking alexmos@ as required. Once he's happy with the change, I'm ...
4 years, 1 month ago (2016-11-17 14:36:27 UTC) #42
iclelland
On 2016/11/17 14:36:27, Mike West wrote: > This LGTM, but I'm marking alexmos@ as required. ...
4 years, 1 month ago (2016-11-17 15:23:32 UTC) #43
iclelland
alexmos, can you take another look?
4 years, 1 month ago (2016-11-17 20:25:59 UTC) #44
alexmos
Thanks, looks close. One request about tests below and a few nits. https://codereview.chromium.org/2483703002/diff/90001/content/browser/frame_host/frame_tree_node.cc File content/browser/frame_host/frame_tree_node.cc ...
4 years, 1 month ago (2016-11-17 21:45:59 UTC) #45
iclelland
https://codereview.chromium.org/2483703002/diff/90001/content/browser/frame_host/frame_tree_node.cc File content/browser/frame_host/frame_tree_node.cc (right): https://codereview.chromium.org/2483703002/diff/90001/content/browser/frame_host/frame_tree_node.cc#newcode258 content/browser/frame_host/frame_tree_node.cc:258: void FrameTreeNode::ResetFeaturePolicy() { On 2016/11/17 21:45:58, alexmos wrote: > ...
4 years, 1 month ago (2016-11-18 16:12:38 UTC) #48
alexmos
Thanks, everything looks good, but looks like the trybots are red on a compile failure? ...
4 years, 1 month ago (2016-11-18 17:21:44 UTC) #51
iclelland
On 2016/11/18 17:21:44, alexmos wrote: > Thanks, everything looks good, but looks like the trybots ...
4 years, 1 month ago (2016-11-18 18:23:09 UTC) #52
alexmos
> No, the tests I removed will no longer pass, as they were testing the ...
4 years, 1 month ago (2016-11-18 21:47:15 UTC) #59
dcheng
https://codereview.chromium.org/2483703002/diff/170001/content/browser/frame_host/frame_tree_node.cc File content/browser/frame_host/frame_tree_node.cc (right): https://codereview.chromium.org/2483703002/diff/170001/content/browser/frame_host/frame_tree_node.cc#newcode263 content/browser/frame_host/frame_tree_node.cc:263: replication_state_.feature_policy_header = ""; Nit: std::string::clear() is slightly cheaper. Btw, ...
4 years ago (2016-11-20 21:04:12 UTC) #67
raymes
A few small thoughts after scanning through :) https://codereview.chromium.org/2483703002/diff/170001/third_party/WebKit/Source/core/dom/SecurityContext.cpp File third_party/WebKit/Source/core/dom/SecurityContext.cpp (right): https://codereview.chromium.org/2483703002/diff/170001/third_party/WebKit/Source/core/dom/SecurityContext.cpp#newcode105 third_party/WebKit/Source/core/dom/SecurityContext.cpp:105: Vector<String>* ...
4 years ago (2016-11-21 02:17:34 UTC) #68
iclelland
https://codereview.chromium.org/2483703002/diff/170001/content/browser/frame_host/frame_tree_node.cc File content/browser/frame_host/frame_tree_node.cc (right): https://codereview.chromium.org/2483703002/diff/170001/content/browser/frame_host/frame_tree_node.cc#newcode263 content/browser/frame_host/frame_tree_node.cc:263: replication_state_.feature_policy_header = ""; On 2016/11/20 21:04:12, dcheng wrote: > ...
4 years ago (2016-11-21 05:05:33 UTC) #73
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2483703002/210001
4 years ago (2016-11-22 18:45:56 UTC) #78
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/310687)
4 years ago (2016-11-22 18:54:30 UTC) #80
iclelland
+r pfeldman, Can you take a look as OWNER at content/browser/site_per_process_browsertest.cc content/common/* content/renderer/* Thanks!
4 years ago (2016-11-22 19:02:12 UTC) #82
pfeldman
lgtm
4 years ago (2016-11-22 19:47:51 UTC) #83
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2483703002/210001
4 years ago (2016-11-22 19:48:57 UTC) #85
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/342757)
4 years ago (2016-11-22 22:58:26 UTC) #87
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2483703002/210001
4 years ago (2016-11-22 23:37:11 UTC) #89
commit-bot: I haz the power
Committed patchset #12 (id:210001)
4 years ago (2016-11-23 02:01:18 UTC) #92
commit-bot: I haz the power
4 years ago (2016-11-23 02:02:58 UTC) #94
Message was sent while issue was closed.
Patchset 12 (id:??) landed as
https://crrev.com/ab749ec9b38b37cfe2b173fda6f20f07ccd23564
Cr-Commit-Position: refs/heads/master@{#434081}

Powered by Google App Engine
This is Rietveld 408576698