Make content::FeaturePolicy implement WebFeaturePolicy, and use it in blink code

Make content::FeaturePolicy implement of WebFeaturePolicy, and use it in blink code

This is part 4 of 5 in the effort to move the FeaturePolicy implementation
into the content layer, which will facilitate use of the framework for browser-
based policy decisions.

See the other CLs in this series:
[1/5] https://codereview.chromium.org/2648423002/ (Rename classes)
[2/5] https://codereview.chromium.org/2654873004/ (Centralize content-side code)
[3/5] https://codereview.chromium.org/2655663004/ (Maintain parallel FP in browser)
[4/5] (This CL)                                   (Use content/ FP in blink)
[5/5] https://codereview.chromium.org/2656533004/ (Remove unused blink code.)

BUG=685195
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2651883008
Cr-Commit-Position: refs/heads/master@{#454949}
Committed: https://chromium.googlesource.com/chromium/src/+/4a6080201a29001f398861f94f8a473d646facd2

[iclelland, 2017-01-25 15:28:00]

The CQ bit was checked by iclelland@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-25 15:28:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[iclelland, 2017-01-25 15:29:03]

Description was changed from

==========
Make content::FeaturePolicy an instance of WebFeaturePolicy, and use it in blink
code

BUG=
==========

to

==========
Make content::FeaturePolicy an instance of WebFeaturePolicy, and use it in blink
code

This is part 4 of 5 in the effort to move the FeaturePolicy implementation
into the content layer, which will facilitate use of the framework for browser-
based policy decisions.

See the other CLs in this series:
[1/5] https://codereview.chromium.org/2648423002/ (Rename classes)
[2/5] https://codereview.chromium.org/2654873004/ (Centralize content-side code)
[3/5] https://codereview.chromium.org/2655663004/ (Maintain parallel FP in
browser)
[4/5] (This CL)                                   (Use content/ FP in blink)
[5/5] https://codereview.chromium.org/2656533004/ (Remove unused blink code.)

BUG=685195
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[iclelland, 2017-01-25 15:29:03]

iclelland@chromium.org changed reviewers:
+ raymes@chromium.org

[commit-bot: I haz the power, 2017-01-25 16:30:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-25 16:30:34]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[iclelland, 2017-01-25 18:51:29]

The CQ bit was checked by iclelland@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-25 18:52:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-25 19:36:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-25 19:36:31]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[iclelland, 2017-01-27 20:52:45]

The CQ bit was checked by iclelland@chromium.org to run a CQ dry run

[iclelland, 2017-01-27 20:53:39]

Description was changed from

==========
Make content::FeaturePolicy an instance of WebFeaturePolicy, and use it in blink
code

This is part 4 of 5 in the effort to move the FeaturePolicy implementation
into the content layer, which will facilitate use of the framework for browser-
based policy decisions.

See the other CLs in this series:
[1/5] https://codereview.chromium.org/2648423002/ (Rename classes)
[2/5] https://codereview.chromium.org/2654873004/ (Centralize content-side code)
[3/5] https://codereview.chromium.org/2655663004/ (Maintain parallel FP in
browser)
[4/5] (This CL)                                   (Use content/ FP in blink)
[5/5] https://codereview.chromium.org/2656533004/ (Remove unused blink code.)

BUG=685195
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Make content::FeaturePolicy implement of WebFeaturePolicy, and use it in blink
code

This is part 4 of 5 in the effort to move the FeaturePolicy implementation
into the content layer, which will facilitate use of the framework for browser-
based policy decisions.

See the other CLs in this series:
[1/5] https://codereview.chromium.org/2648423002/ (Rename classes)
[2/5] https://codereview.chromium.org/2654873004/ (Centralize content-side code)
[3/5] https://codereview.chromium.org/2655663004/ (Maintain parallel FP in
browser)
[4/5] (This CL)                                   (Use content/ FP in blink)
[5/5] https://codereview.chromium.org/2656533004/ (Remove unused blink code.)

BUG=685195
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2017-01-27 20:53:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-27 21:26:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-27 21:26:07]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[iclelland, 2017-01-31 21:42:31]

The CQ bit was checked by iclelland@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-31 21:43:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-31 22:34:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-31 22:35:00]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[iclelland, 2017-02-01 14:01:59]

The CQ bit was checked by iclelland@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-01 14:02:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-01 14:05:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-01 14:05:14]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[iclelland, 2017-02-01 14:41:03]

The CQ bit was checked by iclelland@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-01 14:41:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-01 15:26:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-01 15:26:19]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[iclelland, 2017-02-01 17:39:51]

The CQ bit was checked by iclelland@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-01 17:40:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-01 18:21:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-01 18:21:39]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[iclelland, 2017-02-08 20:45:40]

The CQ bit was checked by iclelland@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-08 20:46:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-08 21:28:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-08 21:28:33]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[iclelland, 2017-02-10 03:24:14]

The CQ bit was checked by iclelland@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-10 03:24:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[iclelland, 2017-02-10 05:13:20]

+r raymes -- I forgot that this one wasn't actually sent, oops :(

Can you PTAL? I expect there are a bunch of nits; I'm going to try to clean it
up tomorrow, but wanted to get this out before your weekend.

This actually makes use of the content::FeaturePolicy object (as the
implementation of a WebFeaturePolicy) in blink, so that we have only one
implementation for both browser and renderer.

Thanks!

[commit-bot: I haz the power, 2017-02-10 05:57:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-10 05:57:06]

Dry run: This issue passed the CQ dry run.

[iclelland, 2017-02-10 18:23:04]

The CQ bit was checked by iclelland@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-10 18:23:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-10 22:03:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-10 22:03:27]

Dry run: This issue passed the CQ dry run.

[raymes, 2017-02-13 04:45:22]

Thanks Ian!

https://codereview.chromium.org/2651883008/diff/180001/content/common/feature...
File content/common/feature_policy/feature_policy.cc (right):

https://codereview.chromium.org/2651883008/diff/180001/content/common/feature...
content/common/feature_policy/feature_policy.cc:141: // https://crbug.com/690520
I put a question on the bug about this. I'm wondering if we can avoid needing to
handle unique origins?

https://codereview.chromium.org/2651883008/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp (right):

https://codereview.chromium.org/2651883008/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp:155: if
(!RuntimeEnabledFeatures::featurePolicyEnabled() || !frame) {
Currently the only callsites I see for this check that FP is enabled prior to
calling this function. Maybe some of these changes could be put into an
orthogonal CL?

https://codereview.chromium.org/2651883008/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp:161: return
(isSameOriginSubframe ||
This might be a bit simpler to read as:
if (isSameOriginSubframe) {
  return feature != WebFeaturePolicyFeature::x && 
         feature != WebFeaturePolicyFeature::y;
}

https://codereview.chromium.org/2651883008/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp:162:
!(feature == WebFeaturePolicyFeature::Fullscreen ||
Does this mean fullscreen won't work at all in a cross-origin iframe? Shouldn't
it work if the attribute is provided? Same with payments?

https://codereview.chromium.org/2651883008/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp:163: feature
== WebFeaturePolicyFeature::Geolocation ||
Should geolocation be here?

https://codereview.chromium.org/2651883008/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/SecurityContext.cpp (right):

https://codereview.chromium.org/2651883008/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/SecurityContext.cpp:114: void
SecurityContext::updateFeaturePolicyOrigin() {
Should we just call this from setSecurityOrigin instead? Would that provide more
of a guarantee that things would be in-sync?

https://codereview.chromium.org/2651883008/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/SecurityContext.cpp:118:
m_featurePolicy.get(), WebSecurityOrigin(m_securityOrigin));
Do you know under what circumstances we can change the security origin at
runtime? I haven't thought hard about this case, I hope it doesn't violate our
assumptions :)

https://codereview.chromium.org/2651883008/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FeaturePolicyInFrameTest.cpp (left):

https://codereview.chromium.org/2651883008/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FeaturePolicyInFrameTest.cpp:3: // found in
the LICENSE file.
I may have asked this before, but are we losing significant coverage with this
being dropped? (I don't necessarily think we are but just want to make sure
we've considered it :)

https://codereview.chromium.org/2651883008/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/payments/PaymentTestHelper.cpp (right):

https://codereview.chromium.org/2651883008/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentTestHelper.cpp:172:
document.updateSecurityOrigin(
Are there other places that call setSecurityOrigin that should call
updateSecurityOrigin? Should setSecurityOrigin be private now? (These are
genuine questions since I don't know this code at all really :)

[iclelland, 2017-02-16 21:48:56]

The CQ bit was checked by iclelland@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-16 21:50:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-16 21:57:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-16 21:57:09]

Dry run: Try jobs failed on following builders:
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[iclelland, 2017-02-23 20:01:40]

The CQ bit was checked by iclelland@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-23 20:02:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[iclelland, 2017-02-23 20:04:12]

https://codereview.chromium.org/2651883008/diff/180001/content/common/feature...
File content/common/feature_policy/feature_policy.cc (right):

https://codereview.chromium.org/2651883008/diff/180001/content/common/feature...
content/common/feature_policy/feature_policy.cc:141: // https://crbug.com/690520
On 2017/02/13 04:45:22, raymes wrote:
> I put a question on the bug about this. I'm wondering if we can avoid needing
to
> handle unique origins?

I don't know if we can -- they are going to need *some* kind of a policy, even
if it's the default one.

If we can, then I'll have to update the test framework to ensure that a dummy
page always has a valid origin; the fullscreen tests were failing for a while
because of this. Sandboxed frames are going to be the real issue, here, I think:
Not so much data or file urls. (Although I'm sure there are uses for each of
those as well, that's a policy rather than a technical issue, once we allow it
in sandbox.)

https://codereview.chromium.org/2651883008/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp (right):

https://codereview.chromium.org/2651883008/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp:155: if
(!RuntimeEnabledFeatures::featurePolicyEnabled() || !frame) {
On 2017/02/13 04:45:22, raymes wrote:
> Currently the only callsites I see for this check that FP is enabled prior to
> calling this function. Maybe some of these changes could be put into an
> orthogonal CL?

Agreed; most of this should be removed; when feature policy is disabled, this
shouldn't be called at all. That will simplify a lot of the code, including the
awkward edge-case handling below.

https://codereview.chromium.org/2651883008/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp:161: return
(isSameOriginSubframe ||
On 2017/02/13 04:45:22, raymes wrote:
> This might be a bit simpler to read as:
> if (isSameOriginSubframe) {
>   return feature != WebFeaturePolicyFeature::x && 
>          feature != WebFeaturePolicyFeature::y;
> }
>   

Done.

https://codereview.chromium.org/2651883008/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp:162:
!(feature == WebFeaturePolicyFeature::Fullscreen ||
On 2017/02/13 04:45:22, raymes wrote:
> Does this mean fullscreen won't work at all in a cross-origin iframe?
Shouldn't
> it work if the attribute is provided? Same with payments?

That edge case actually isn't ever hit -- it was a rough "is this page cross
origin" check for the features which are disabled by default in cross-origin
frames (like vibrate), to handle the case when FeaturePolicy was not enabled.
Fullscreen won't run this code, though, in that case; if FP is disabled, it just
walks up the frame tree looking for 'allowfullscreen' attributes on frame
owners. Same with payments. This is really just for vibrate. (And then vibrate
went ahead and used the runtime feature check, so this whole block is unused
now.)

https://codereview.chromium.org/2651883008/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp:163: feature
== WebFeaturePolicyFeature::Geolocation ||
On 2017/02/13 04:45:22, raymes wrote:
> Should geolocation be here?

It was just all of the features that had a default defined in the spec of
["self"].

https://codereview.chromium.org/2651883008/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/SecurityContext.cpp (right):

https://codereview.chromium.org/2651883008/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/SecurityContext.cpp:114: void
SecurityContext::updateFeaturePolicyOrigin() {
On 2017/02/13 04:45:22, raymes wrote:
> Should we just call this from setSecurityOrigin instead? Would that provide
more
> of a guarantee that things would be in-sync?

Yes. Done.

https://codereview.chromium.org/2651883008/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/SecurityContext.cpp:118:
m_featurePolicy.get(), WebSecurityOrigin(m_securityOrigin));
On 2017/02/13 04:45:22, raymes wrote:
> Do you know under what circumstances we can change the security origin at
> runtime? I haven't thought hard about this case, I hope it doesn't violate our
> assumptions :)

There are a few odd scenarios around document.open (and write, by extension, I
think). I'm not sure when else, but I'll do some spelunking in the code and see
what I can find out.

https://codereview.chromium.org/2651883008/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FeaturePolicyInFrameTest.cpp (left):

https://codereview.chromium.org/2651883008/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FeaturePolicyInFrameTest.cpp:3: // found in
the LICENSE file.
On 2017/02/13 04:45:22, raymes wrote:
> I may have asked this before, but are we losing significant coverage with this
> being dropped? (I don't necessarily think we are but just want to make sure
> we've considered it :)

I don't think we're losing significant coverage -- this test was for the
composition of the policies created from blink code (which are now tested in
content, because that's where they are created).

It also tests the operation of the isFeatureEnabledInFrame code. If we reinstate
this, and we could, just to make sure that isFeatureEnabledInFrame is covered,
then we'd probably need to use real features, like fullscreen, rather than the
DefaultOn/DefaultOff/DefaultSelf test features. (This test was removed because
the set of features is now under the control of content, and it was no longer
trivial to add new features from blink.)

https://codereview.chromium.org/2651883008/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/payments/PaymentTestHelper.cpp (right):

https://codereview.chromium.org/2651883008/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentTestHelper.cpp:172:
document.updateSecurityOrigin(
On 2017/02/13 04:45:22, raymes wrote:
> Are there other places that call setSecurityOrigin that should call
> updateSecurityOrigin? Should setSecurityOrigin be private now? (These are
> genuine questions since I don't know this code at all really :)

That's actually a really good point -- Most of the calls to setSecurityOrigin
seem to be in initialization methods, and those, I presume, shouldn't really be
touched. Maybe the best solution here is to have setSecurityOrigin update the
policy if necessary.

(Done)

[commit-bot: I haz the power, 2017-02-23 20:05:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-23 20:05:53]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[iclelland, 2017-02-23 20:44:34]

The CQ bit was checked by iclelland@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-23 20:45:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-23 23:09:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-23 23:09:36]

Dry run: This issue passed the CQ dry run.

[iclelland, 2017-02-27 16:19:03]

iclelland@chromium.org changed reviewers:
+ pfeldman@chromium.org

[iclelland, 2017-02-27 16:19:04]

+r pfeldman, can you PTAL? Thanks!

[pfeldman, 2017-02-27 17:07:49]

I can perform an owners review, but i'd prefer to see some l-g-t-m-s from
reviewers familiar with the matter before i do so!

[iclelland, 2017-02-27 18:43:23]

On 2017/02/27 17:07:49, pfeldman wrote:
> I can perform an owners review, but i'd prefer to see some l-g-t-m-s from
> reviewers familiar with the matter before i do so!

Absolutely :) Hit the button on that too soon.

Pinging Raymes, can you take a look again?

[raymes, 2017-02-28 00:39:51]

lgtm but it would be nice to have more clarity on the below comment before
shipping.

I think supporting unique origins is probably ok (we just need the proper code
support which I know you're looking at :).

https://codereview.chromium.org/2651883008/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/SecurityContext.cpp (right):

https://codereview.chromium.org/2651883008/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/SecurityContext.cpp:120:
m_featurePolicy.get(), WebSecurityOrigin(m_securityOrigin));
This still worries me a bit just because we've always thought about the policy
as being tied to a single origin through its lifetime and I don't know what
assumptions we're breaking when this happens. 

For example, a feature may be enabled in a frame, and the origin changes but we
don't recheck the policy so it's still enabled. I guess it boils down to me not
understanding the circumstances under which origins can change in a
SecurityContext - it seems like it should be a rare thing.

Either way, I defer on this point since I don't know the answer but I do think
it would be good to feel more confident about it. Perhaps dcheng or mkwst can
give better advice here?

[raymes, 2017-02-28 00:44:05]

On 2017/02/28 00:39:51, raymes wrote:
> lgtm but it would be nice to have more clarity on the below comment before
> shipping.
> 
> I think supporting unique origins is probably ok (we just need the proper code
> support which I know you're looking at :).
> 
>
https://codereview.chromium.org/2651883008/diff/240001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/dom/SecurityContext.cpp (right):
> 
>
https://codereview.chromium.org/2651883008/diff/240001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/dom/SecurityContext.cpp:120:
> m_featurePolicy.get(), WebSecurityOrigin(m_securityOrigin));
> This still worries me a bit just because we've always thought about the policy
> as being tied to a single origin through its lifetime and I don't know what
> assumptions we're breaking when this happens. 
> 
> For example, a feature may be enabled in a frame, and the origin changes but
we
> don't recheck the policy so it's still enabled. I guess it boils down to me
not
> understanding the circumstances under which origins can change in a
> SecurityContext - it seems like it should be a rare thing.
> 
> Either way, I defer on this point since I don't know the answer but I do think
> it would be good to feel more confident about it. Perhaps dcheng or mkwst can
> give better advice here?

I filed https://bugs.chromium.org/p/chromium/issues/detail?id=696819 to track
that. Thanks Ian!

[iclelland, 2017-02-28 05:25:19]

iclelland@chromium.org changed reviewers:
+ dcheng@chromium.org, mkwst@chromium.org

[iclelland, 2017-02-28 05:25:56]

https://codereview.chromium.org/2651883008/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/SecurityContext.cpp (right):

https://codereview.chromium.org/2651883008/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/SecurityContext.cpp:120:
m_featurePolicy.get(), WebSecurityOrigin(m_securityOrigin));
On 2017/02/28 00:39:51, raymes wrote:
> This still worries me a bit just because we've always thought about the policy
> as being tied to a single origin through its lifetime and I don't know what
> assumptions we're breaking when this happens. 
> 
> For example, a feature may be enabled in a frame, and the origin changes but
we
> don't recheck the policy so it's still enabled.

With this CL, we would recalculate the feature policy in this case, so there
would have to be code in the renderer which is caching the result of
isFeatureEnabled, rather than calling it each time. If it's possible for the
policy to change in a way that would make that caching invalid, then we'll need
to document that callers shoudn't cache the results of that method.
.
> This would have to be  I guess it boils down to me not
> understanding the circumstances under which origins can change in a
> SecurityContext - it seems like it should be a rare thing.
> 
> Either way, I defer on this point since I don't know the answer but I do think
> it would be good to feel more confident about it. Perhaps dcheng or mkwst can
> give better advice here?

I believe, from reading specs and code, that post-document-creation, there are
only a couple of ways that the document's origin could change:

 - Setting document.domain can update the domain properly
 - Using document.write or .open from *another* document sets the first
document's origin to be the same as the second one's.

That second technique is also restricted in code: the two documents need to
already be same-scheme-host-port-and-suborigin, so I believe that the only
properties of the origin that can reasonably change are the domain and some
flags like m_universalAccess, m_canLoadLocalResources and
m_blockLocalAccessFromLocalOrigin, which we don't use for FP decisions, and are
not normally set on the open web.

+r mkwst and dcheng for any more useful insight here -- are there other
situations where the origin can change post-creation?

[raymes, 2017-02-28 06:13:12]

On 2017/02/28 05:25:56, iclelland wrote:
>
https://codereview.chromium.org/2651883008/diff/240001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/dom/SecurityContext.cpp (right):
> 
>
https://codereview.chromium.org/2651883008/diff/240001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/dom/SecurityContext.cpp:120:
> m_featurePolicy.get(), WebSecurityOrigin(m_securityOrigin));
> On 2017/02/28 00:39:51, raymes wrote:
> > This still worries me a bit just because we've always thought about the
policy
> > as being tied to a single origin through its lifetime and I don't know what
> > assumptions we're breaking when this happens. 
> > 
> > For example, a feature may be enabled in a frame, and the origin changes but
> we
> > don't recheck the policy so it's still enabled.
> 
> With this CL, we would recalculate the feature policy in this case, so there
> would have to be code in the renderer which is caching the result of
> isFeatureEnabled, rather than calling it each time. If it's possible for the
> policy to change in a way that would make that caching invalid, then we'll
need
> to document that callers shoudn't cache the results of that method.
> .
> > This would have to be  I guess it boils down to me not
> > understanding the circumstances under which origins can change in a
> > SecurityContext - it seems like it should be a rare thing.
> > 
> > Either way, I defer on this point since I don't know the answer but I do
think
> > it would be good to feel more confident about it. Perhaps dcheng or mkwst
can
> > give better advice here?
> 
> I believe, from reading specs and code, that post-document-creation, there are
> only a couple of ways that the document's origin could change:
> 
>  - Setting document.domain can update the domain properly
>  - Using document.write or .open from *another* document sets the first
> document's origin to be the same as the second one's.
> 
> That second technique is also restricted in code: the two documents need to
> already be same-scheme-host-port-and-suborigin, so I believe that the only
> properties of the origin that can reasonably change are the domain and some
> flags like m_universalAccess, m_canLoadLocalResources and
> m_blockLocalAccessFromLocalOrigin, which we don't use for FP decisions, and
are
> not normally set on the open web.
> 
> +r mkwst and dcheng for any more useful insight here -- are there other
> situations where the origin can change post-creation?

Thanks Ian - I think that makes sense.

[dcheng, 2017-02-28 06:19:05]

On 2017/02/28 05:25:56, iclelland wrote:
>
https://codereview.chromium.org/2651883008/diff/240001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/dom/SecurityContext.cpp (right):
> 
>
https://codereview.chromium.org/2651883008/diff/240001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/dom/SecurityContext.cpp:120:
> m_featurePolicy.get(), WebSecurityOrigin(m_securityOrigin));
> On 2017/02/28 00:39:51, raymes wrote:
> > This still worries me a bit just because we've always thought about the
policy
> > as being tied to a single origin through its lifetime and I don't know what
> > assumptions we're breaking when this happens. 
> > 
> > For example, a feature may be enabled in a frame, and the origin changes but
> we
> > don't recheck the policy so it's still enabled.
> 
> With this CL, we would recalculate the feature policy in this case, so there
> would have to be code in the renderer which is caching the result of
> isFeatureEnabled, rather than calling it each time. If it's possible for the
> policy to change in a way that would make that caching invalid, then we'll
need
> to document that callers shoudn't cache the results of that method.
> .
> > This would have to be  I guess it boils down to me not
> > understanding the circumstances under which origins can change in a
> > SecurityContext - it seems like it should be a rare thing.
> > 
> > Either way, I defer on this point since I don't know the answer but I do
think
> > it would be good to feel more confident about it. Perhaps dcheng or mkwst
can
> > give better advice here?
> 
> I believe, from reading specs and code, that post-document-creation, there are
> only a couple of ways that the document's origin could change:
> 
>  - Setting document.domain can update the domain properly
>  - Using document.write or .open from *another* document sets the first
> document's origin to be the same as the second one's.
> 
> That second technique is also restricted in code: the two documents need to
> already be same-scheme-host-port-and-suborigin, so I believe that the only
> properties of the origin that can reasonably change are the domain and some
> flags like m_universalAccess, m_canLoadLocalResources and
> m_blockLocalAccessFromLocalOrigin, which we don't use for FP decisions, and
are
> not normally set on the open web.
> 
> +r mkwst and dcheng for any more useful insight here -- are there other
> situations where the origin can change post-creation?

I think this pretty much covers it, with one small correction: the three flags
mentioned here should be set at security context initialization, and shouldn't
dynamically change after that.

[dcheng, 2017-02-28 06:19:21]

On 2017/02/28 06:19:05, dcheng wrote:
> On 2017/02/28 05:25:56, iclelland wrote:
> >
>
https://codereview.chromium.org/2651883008/diff/240001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/core/dom/SecurityContext.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2651883008/diff/240001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/dom/SecurityContext.cpp:120:
> > m_featurePolicy.get(), WebSecurityOrigin(m_securityOrigin));
> > On 2017/02/28 00:39:51, raymes wrote:
> > > This still worries me a bit just because we've always thought about the
> policy
> > > as being tied to a single origin through its lifetime and I don't know
what
> > > assumptions we're breaking when this happens. 
> > > 
> > > For example, a feature may be enabled in a frame, and the origin changes
but
> > we
> > > don't recheck the policy so it's still enabled.
> > 
> > With this CL, we would recalculate the feature policy in this case, so there
> > would have to be code in the renderer which is caching the result of
> > isFeatureEnabled, rather than calling it each time. If it's possible for the
> > policy to change in a way that would make that caching invalid, then we'll
> need
> > to document that callers shoudn't cache the results of that method.
> > .
> > > This would have to be  I guess it boils down to me not
> > > understanding the circumstances under which origins can change in a
> > > SecurityContext - it seems like it should be a rare thing.
> > > 
> > > Either way, I defer on this point since I don't know the answer but I do
> think
> > > it would be good to feel more confident about it. Perhaps dcheng or mkwst
> can
> > > give better advice here?
> > 
> > I believe, from reading specs and code, that post-document-creation, there
are
> > only a couple of ways that the document's origin could change:
> > 
> >  - Setting document.domain can update the domain properly
> >  - Using document.write or .open from *another* document sets the first
> > document's origin to be the same as the second one's.
> > 
> > That second technique is also restricted in code: the two documents need to
> > already be same-scheme-host-port-and-suborigin, so I believe that the only
> > properties of the origin that can reasonably change are the domain and some
> > flags like m_universalAccess, m_canLoadLocalResources and
> > m_blockLocalAccessFromLocalOrigin, which we don't use for FP decisions, and
> are
> > not normally set on the open web.
> > 
> > +r mkwst and dcheng for any more useful insight here -- are there other
> > situations where the origin can change post-creation?
> 
> I think this pretty much covers it, with one small correction: the three flags
> mentioned here should be set at security context initialization, and shouldn't
> dynamically change after that.

(This meaning the situations covered in
https://bugs.chromium.org/p/chromium/issues/detail?id=696819)

[haraken, 2017-02-28 06:41:41]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2017-02-28 06:41:43]

https://codereview.chromium.org/2651883008/diff/240001/content/child/blink_pl...
File content/child/blink_platform_impl.cc (right):

https://codereview.chromium.org/2651883008/diff/240001/content/child/blink_pl...
content/child/blink_platform_impl.cc:869: return static_cast<const
FeaturePolicy*>(policy)->IsFeatureEnabled(feature);

Hmm, it looks a bit weird that we're redirecting a Platform's method to
WebFeaturePolicy's method using a static cast.

Can we move createFeaturePolicy, isFeatureEnabledByPolicy and
updateFeaturePolicyOrigin to WebFeaturePolicy?

[iclelland, 2017-02-28 15:23:14]

On 2017/02/28 06:19:05, dcheng wrote:
> On 2017/02/28 05:25:56, iclelland wrote:
> >
>
https://codereview.chromium.org/2651883008/diff/240001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/core/dom/SecurityContext.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2651883008/diff/240001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/dom/SecurityContext.cpp:120:
> > m_featurePolicy.get(), WebSecurityOrigin(m_securityOrigin));
> > On 2017/02/28 00:39:51, raymes wrote:
> > > This still worries me a bit just because we've always thought about the
> policy
> > > as being tied to a single origin through its lifetime and I don't know
what
> > > assumptions we're breaking when this happens. 
> > > 
> > > For example, a feature may be enabled in a frame, and the origin changes
but
> > we
> > > don't recheck the policy so it's still enabled.
> > 
> > With this CL, we would recalculate the feature policy in this case, so there
> > would have to be code in the renderer which is caching the result of
> > isFeatureEnabled, rather than calling it each time. If it's possible for the
> > policy to change in a way that would make that caching invalid, then we'll
> need
> > to document that callers shoudn't cache the results of that method.
> > .
> > > This would have to be  I guess it boils down to me not
> > > understanding the circumstances under which origins can change in a
> > > SecurityContext - it seems like it should be a rare thing.
> > > 
> > > Either way, I defer on this point since I don't know the answer but I do
> think
> > > it would be good to feel more confident about it. Perhaps dcheng or mkwst
> can
> > > give better advice here?
> > 
> > I believe, from reading specs and code, that post-document-creation, there
are
> > only a couple of ways that the document's origin could change:
> > 
> >  - Setting document.domain can update the domain properly
> >  - Using document.write or .open from *another* document sets the first
> > document's origin to be the same as the second one's.
> > 
> > That second technique is also restricted in code: the two documents need to
> > already be same-scheme-host-port-and-suborigin, so I believe that the only
> > properties of the origin that can reasonably change are the domain and some
> > flags like m_universalAccess, m_canLoadLocalResources and
> > m_blockLocalAccessFromLocalOrigin, which we don't use for FP decisions, and
> are
> > not normally set on the open web.
> > 
> > +r mkwst and dcheng for any more useful insight here -- are there other
> > situations where the origin can change post-creation?
> 
> I think this pretty much covers it, with one small correction: the three flags
> mentioned here should be set at security context initialization, and shouldn't
> dynamically change after that.

The specific situation I was thinking of is here:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Docum...
The flags are set on the origins, not the security context, and it looks like
it's possible for the new origin to have different flags than the old one; maybe
that's not the case, and you could never reach this code if they were different,
I haven't tried to actually trigger it.

[iclelland, 2017-02-28 18:47:32]

The CQ bit was checked by iclelland@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-28 18:48:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-28 18:50:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-28 18:50:59]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[iclelland, 2017-02-28 18:59:55]

Patchset #14 (id:260001) has been deleted

[iclelland, 2017-02-28 20:55:54]

The CQ bit was checked by iclelland@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-28 20:56:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[iclelland, 2017-02-28 21:02:39]

https://codereview.chromium.org/2651883008/diff/240001/content/child/blink_pl...
File content/child/blink_platform_impl.cc (right):

https://codereview.chromium.org/2651883008/diff/240001/content/child/blink_pl...
content/child/blink_platform_impl.cc:869: return static_cast<const
FeaturePolicy*>(policy)->IsFeatureEnabled(feature);
On 2017/02/28 06:41:43, haraken wrote:
> 
> Hmm, it looks a bit weird that we're redirecting a Platform's method to
> WebFeaturePolicy's method using a static cast.
> 
> Can we move createFeaturePolicy, isFeatureEnabledByPolicy and
> updateFeaturePolicyOrigin to WebFeaturePolicy?

I can move isFeatureEnabledByPolicy and updateFeaturePolicyOrigin to virtual
methods on WebFeaturePolicy; I think that makes sense.

createFeaturePolicy is necessarily a static method, so it's not something that
would be easily implemented by subclasses. It makes sense to make it a platform
responsibility to create an appropriate FeaturePolicy object on request, and
then that object implements the other methods (isFeatureEnabled, updateOrigin).
WDYT?

[commit-bot: I haz the power, 2017-02-28 22:10:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-28 22:10:06]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[haraken, 2017-03-01 01:23:39]

On 2017/02/28 21:02:39, iclelland wrote:
>
https://codereview.chromium.org/2651883008/diff/240001/content/child/blink_pl...
> File content/child/blink_platform_impl.cc (right):
> 
>
https://codereview.chromium.org/2651883008/diff/240001/content/child/blink_pl...
> content/child/blink_platform_impl.cc:869: return static_cast<const
> FeaturePolicy*>(policy)->IsFeatureEnabled(feature);
> On 2017/02/28 06:41:43, haraken wrote:
> > 
> > Hmm, it looks a bit weird that we're redirecting a Platform's method to
> > WebFeaturePolicy's method using a static cast.
> > 
> > Can we move createFeaturePolicy, isFeatureEnabledByPolicy and
> > updateFeaturePolicyOrigin to WebFeaturePolicy?
> 
> I can move isFeatureEnabledByPolicy and updateFeaturePolicyOrigin to virtual
> methods on WebFeaturePolicy; I think that makes sense.
> 
> createFeaturePolicy is necessarily a static method, so it's not something that
> would be easily implemented by subclasses. It makes sense to make it a
platform
> responsibility to create an appropriate FeaturePolicy object on request, and
> then that object implements the other methods (isFeatureEnabled,
updateOrigin).
> WDYT?

Sounds good.

[iclelland, 2017-03-01 04:13:51]

The CQ bit was checked by iclelland@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-01 04:14:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-01 05:33:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-01 05:33:26]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[iclelland, 2017-03-01 19:22:38]

On 2017/03/01 01:23:39, haraken wrote:
> On 2017/02/28 21:02:39, iclelland wrote:
> >
>
https://codereview.chromium.org/2651883008/diff/240001/content/child/blink_pl...
> > File content/child/blink_platform_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/2651883008/diff/240001/content/child/blink_pl...
> > content/child/blink_platform_impl.cc:869: return static_cast<const
> > FeaturePolicy*>(policy)->IsFeatureEnabled(feature);
> > On 2017/02/28 06:41:43, haraken wrote:
> > > 
> > > Hmm, it looks a bit weird that we're redirecting a Platform's method to
> > > WebFeaturePolicy's method using a static cast.
> > > 
> > > Can we move createFeaturePolicy, isFeatureEnabledByPolicy and
> > > updateFeaturePolicyOrigin to WebFeaturePolicy?
> > 
> > I can move isFeatureEnabledByPolicy and updateFeaturePolicyOrigin to virtual
> > methods on WebFeaturePolicy; I think that makes sense.
> > 
> > createFeaturePolicy is necessarily a static method, so it's not something
that
> > would be easily implemented by subclasses. It makes sense to make it a
> platform
> > responsibility to create an appropriate FeaturePolicy object on request, and
> > then that object implements the other methods (isFeatureEnabled,
> updateOrigin).
> > WDYT?
> 
> Sounds good.

It's far more difficult than it should be to add updateOrigin to
WebFeaturePolicy -- in public/platform, it needs to take a WebSecurityOrigin,
but in content/common, there's no way to convert that to a url::Origin. The
conversion methods are only available in content/child or content/renderer, to
avoid linking the browser against blink.

After thinking on it, though, a better idiom when the origin changes might be to
create a new policy, a duplicate of the old one, with the same set of inherited
features, but anchored to the new origin. The old policy can be destroyed at
that point (it's a unique_ptr on SecurityContext), and there's never a question
that the policies themselves are immutable. I'm going to give that a try and see
if I can get it to work.

[iclelland, 2017-03-02 05:03:59]

The CQ bit was checked by iclelland@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-02 05:05:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-02 07:12:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-02 07:12:55]

Dry run: This issue passed the CQ dry run.

[iclelland, 2017-03-02 13:54:47]

On 2017/03/01 19:22:38, iclelland wrote:
> On 2017/03/01 01:23:39, haraken wrote:
> > On 2017/02/28 21:02:39, iclelland wrote:
> > >
> >
>
https://codereview.chromium.org/2651883008/diff/240001/content/child/blink_pl...
> > > File content/child/blink_platform_impl.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2651883008/diff/240001/content/child/blink_pl...
> > > content/child/blink_platform_impl.cc:869: return static_cast<const
> > > FeaturePolicy*>(policy)->IsFeatureEnabled(feature);
> > > On 2017/02/28 06:41:43, haraken wrote:
> > > > 
> > > > Hmm, it looks a bit weird that we're redirecting a Platform's method to
> > > > WebFeaturePolicy's method using a static cast.
> > > > 
> > > > Can we move createFeaturePolicy, isFeatureEnabledByPolicy and
> > > > updateFeaturePolicyOrigin to WebFeaturePolicy?
> > > 
> > > I can move isFeatureEnabledByPolicy and updateFeaturePolicyOrigin to
virtual
> > > methods on WebFeaturePolicy; I think that makes sense.
> > > 
> > > createFeaturePolicy is necessarily a static method, so it's not something
> that
> > > would be easily implemented by subclasses. It makes sense to make it a
> > platform
> > > responsibility to create an appropriate FeaturePolicy object on request,
and
> > > then that object implements the other methods (isFeatureEnabled,
> > updateOrigin).
> > > WDYT?
> > 
> > Sounds good.
> 
> It's far more difficult than it should be to add updateOrigin to
> WebFeaturePolicy -- in public/platform, it needs to take a WebSecurityOrigin,
> but in content/common, there's no way to convert that to a url::Origin. The
> conversion methods are only available in content/child or content/renderer, to
> avoid linking the browser against blink.
> 
> After thinking on it, though, a better idiom when the origin changes might be
to
> create a new policy, a duplicate of the old one, with the same set of
inherited
> features, but anchored to the new origin. The old policy can be destroyed at
> that point (it's a unique_ptr on SecurityContext), and there's never a
question
> that the policies themselves are immutable. I'm going to give that a try and
see
> if I can get it to work.

haraken, that change has been implemented in the most recent patchset (#17). can
you PTAL? (It's the same delta as in
https://codereview.chromium.org/2729623003/)

[haraken, 2017-03-04 07:36:01]

Sorry about the review delay. LGTM.

https://codereview.chromium.org/2651883008/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ConditionalFeaturesForCore.h
(right):

https://codereview.chromium.org/2651883008/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ConditionalFeaturesForCore.h:25:
CORE_EXPORT bool isFeatureEnabledInFrame(WebFeaturePolicyFeature, const Frame*);

Nit: Another option is to make isFeatureEnabledInFrame a method of Frame.

https://codereview.chromium.org/2651883008/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/SecurityContext.cpp (right):

https://codereview.chromium.org/2651883008/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/SecurityContext.cpp:120: *m_featurePolicy,
WebSecurityOrigin(m_securityOrigin)));

Oh, if your intention is just to *update* (not duplicate) the feature policy,
I'm also fine with an API like updateFeaturePolicyWithOrigin(m_featurePolicy),
which automatically updates m_featurePolicy.

https://codereview.chromium.org/2651883008/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp (right):

https://codereview.chromium.org/2651883008/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp:440:
frame()->securityContext()->initializeFeaturePolicy(parsedHeader,

Also we can make initializeFeaturePolicy a method of Frame.

[iclelland, 2017-03-06 04:17:40]

pfeldman, can you take a look at this point?

Thanks!

https://codereview.chromium.org/2651883008/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ConditionalFeaturesForCore.h
(right):

https://codereview.chromium.org/2651883008/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ConditionalFeaturesForCore.h:25:
CORE_EXPORT bool isFeatureEnabledInFrame(WebFeaturePolicyFeature, const Frame*);
On 2017/03/04 07:36:00, haraken wrote:
> 
> Nit: Another option is to make isFeatureEnabledInFrame a method of Frame.

Thanks, I've been thinking about moving this out of bindings/ for a bit. I'll
line up another CL for that.

https://codereview.chromium.org/2651883008/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/SecurityContext.cpp (right):

https://codereview.chromium.org/2651883008/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/SecurityContext.cpp:120: *m_featurePolicy,
WebSecurityOrigin(m_securityOrigin)));
On 2017/03/04 07:36:01, haraken wrote:
> 
> Oh, if your intention is just to *update* (not duplicate) the feature policy,
> I'm also fine with an API like updateFeaturePolicyWithOrigin(m_featurePolicy),
> which automatically updates m_featurePolicy.

That is really the only modification being considered here, but after writing
it, I'm slightly more comfortable with the idea that the policy objects are
effectively immutable after they've been constructed, and the FP headers have
been read. The origin is used for interpreting the policy, and we might end up
breaking some assumptions inadvertently if we allow it to change after the
policy has been set.

[iclelland, 2017-03-06 04:19:32]

On 2017/03/06 04:17:40, iclelland wrote:
> pfeldman, can you take a look at this point?
> 

(At content/ at least: as OWNER, I think haraken's l g t m should suffice for
3p/WebKit/)

[pfeldman, 2017-03-06 18:05:37]

content lgtm

[iclelland, 2017-03-06 18:26:10]

The CQ bit was checked by iclelland@chromium.org

[iclelland, 2017-03-06 18:26:11]

The patchset sent to the CQ was uploaded after l-g-t-m from raymes@chromium.org
Link to the patchset: https://codereview.chromium.org/2651883008/#ps340001
(title: "Duplicate FP object rather than modifying in-place")

[commit-bot: I haz the power, 2017-03-06 18:26:43]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-06 20:47:30]

CQ is committing da patch.

Bot data: {"patchset_id": 340001, "attempt_start_ts": 1488824770399770,
"parent_rev": "65dff8b6da7b33d422b5e02356e58254fded9006", "commit_rev":
"4a6080201a29001f398861f94f8a473d646facd2"}

[commit-bot: I haz the power, 2017-03-06 20:48:18]

Description was changed from

==========
Make content::FeaturePolicy implement of WebFeaturePolicy, and use it in blink
code

This is part 4 of 5 in the effort to move the FeaturePolicy implementation
into the content layer, which will facilitate use of the framework for browser-
based policy decisions.

See the other CLs in this series:
[1/5] https://codereview.chromium.org/2648423002/ (Rename classes)
[2/5] https://codereview.chromium.org/2654873004/ (Centralize content-side code)
[3/5] https://codereview.chromium.org/2655663004/ (Maintain parallel FP in
browser)
[4/5] (This CL)                                   (Use content/ FP in blink)
[5/5] https://codereview.chromium.org/2656533004/ (Remove unused blink code.)

BUG=685195
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Make content::FeaturePolicy implement of WebFeaturePolicy, and use it in blink
code

This is part 4 of 5 in the effort to move the FeaturePolicy implementation
into the content layer, which will facilitate use of the framework for browser-
based policy decisions.

See the other CLs in this series:
[1/5] https://codereview.chromium.org/2648423002/ (Rename classes)
[2/5] https://codereview.chromium.org/2654873004/ (Centralize content-side code)
[3/5] https://codereview.chromium.org/2655663004/ (Maintain parallel FP in
browser)
[4/5] (This CL)                                   (Use content/ FP in blink)
[5/5] https://codereview.chromium.org/2656533004/ (Remove unused blink code.)

BUG=685195
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2651883008
Cr-Commit-Position: refs/heads/master@{#454949}
Committed:
https://chromium.googlesource.com/chromium/src/+/4a6080201a29001f398861f94f8a...
==========

[commit-bot: I haz the power, 2017-03-06 20:48:20]

Committed patchset #17 (id:340001) as
https://chromium.googlesource.com/chromium/src/+/4a6080201a29001f398861f94f8a...