V8 bindings gen: Error if OriginTrialEnabled and SecureContext together.

V8 bindings gen: Error if OriginTrialEnabled and SecureContext together.

If these are found together, they currently generate completely broken
code (the origin trial is not checked). Therefore, throw an error at
compile time and point the unlucky developer to a workaround.

BUG=695123
Review-Url: https://codereview.chromium.org/2780093003
Cr-Commit-Position: refs/heads/master@{#461653}
Committed: https://chromium.googlesource.com/chromium/src/+/eb6e06daaefc9d173132724d905ce77a3ece95d3

[Matt Giuca, 2017-03-29 05:06:59]

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

[commit-bot: I haz the power, 2017-03-29 05:07:31]

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

[Matt Giuca, 2017-03-29 05:09:32]

mgiuca@chromium.org changed reviewers:
+ yukishiino@chromium.org

[Matt Giuca, 2017-03-29 05:09:33]

https://codereview.chromium.org/2780093003/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/scripts/v8_utilities.py (right):

https://codereview.chromium.org/2780093003/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/scripts/v8_utilities.py:407: %
definition_or_member.name)
This was crashing on idl_name... idl_name was removed in r427281 and I can't
find any way to access the name of the outer object. Oh well.

https://codereview.chromium.org/2780093003/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/scripts/v8_utilities.py:419:
is_feature_policy_enabled = 'FeaturePolicy' in extended_attributes
This looks like exactly the same logic as OriginTrialEnabled... does the same
restriction apply?

If so maybe we should factor this out to avoid duplicating both paths. (I can do
that in this CL.)

[commit-bot: I haz the power, 2017-03-29 05:16:31]

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

[commit-bot: I haz the power, 2017-03-29 05:16:32]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Matt Giuca, 2017-03-29 06:24:31]

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

[Matt Giuca, 2017-03-29 06:24:37]

https://codereview.chromium.org/2780093003/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/serviceworkers/NavigationPreloadManager.idl
(right):

https://codereview.chromium.org/2780093003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/NavigationPreloadManager.idl:11:
[SecureContext, CallWith=ScriptState] Promise<void> enable();
Oh no this is awful.

It turns out that NavigationPreloadManager is hitting the new error because it
uses OTE and SC at the same time. But this isn't actually a problem because the
SC is being automatically applied to all the members of NavigationPreloadManager
(not the class itself), and the class itself is being registered manually.

If I move the SecureContext down here to all the methods, it doesn't hit the
check and seems to generate the same code.

Is this acceptable?

[commit-bot: I haz the power, 2017-03-29 06:24:59]

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

[Matt Giuca, 2017-03-29 06:25:07]

mgiuca@chromium.org changed reviewers:
+ falken@chromium.org

[Matt Giuca, 2017-03-29 06:25:08]

+falken@ for NavigationPreloadManager.idl.

[Yuki, 2017-03-29 06:38:06]

https://codereview.chromium.org/2780093003/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/scripts/v8_utilities.py (right):

https://codereview.chromium.org/2780093003/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/scripts/v8_utilities.py:419:
is_feature_policy_enabled = 'FeaturePolicy' in extended_attributes
On 2017/03/29 05:09:33, Matt Giuca wrote:
> This looks like exactly the same logic as OriginTrialEnabled... does the same
> restriction apply?
> 
> If so maybe we should factor this out to avoid duplicating both paths. (I can
do
> that in this CL.)

Looks like [FeaturePolicy] is deprecated and actually not used.  I think we can
simply remove all related code about [FeaturePolicy].

https://codereview.chromium.org/2780093003/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/serviceworkers/NavigationPreloadManager.idl
(right):

https://codereview.chromium.org/2780093003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/NavigationPreloadManager.idl:11:
[SecureContext, CallWith=ScriptState] Promise<void> enable();
On 2017/03/29 06:24:37, Matt Giuca wrote:
> Oh no this is awful.
> 
> It turns out that NavigationPreloadManager is hitting the new error because it
> uses OTE and SC at the same time. But this isn't actually a problem because
the
> SC is being automatically applied to all the members of
NavigationPreloadManager
> (not the class itself), and the class itself is being registered manually.
> 
> If I move the SecureContext down here to all the methods, it doesn't hit the
> check and seems to generate the same code.
> 
> Is this acceptable?

Just a question, why NavigationPreloadManager is OriginTrialEnabled?
If it's because it's not yet stable and we'd like to expose the APIs to limited
users and we're planning to remove [OriginTrialEnabled], it makes a sense.

However, with this change, we're going to expose NavigationPreloadManager
interface itself as long as the origin trial is specified even when it's not a
secure context.

Should we expose NavigationPreloadManager only when the origin trial is
specified *and* it's a secure context?

https://w3c.github.io/ServiceWorker/#navigation-preload-manager
The spec seems saying we shouldn't expose the interface if it's not a secure
context.

[haraken, 2017-03-29 06:39:45]

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

[haraken, 2017-03-29 06:39:45]

Mu gut feeling is that the implementation of [SecureContext] is not sufficient
in a couple of ways and you're hitting one of the issues.

Would it be an option to try to improve the implementation of [SecureContext]
instead of introducing some hack?

+mkwst

[falken, 2017-03-29 06:41:17]

falken@chromium.org changed reviewers:
- haraken@chromium.org, mkwst@chromium.org

[falken, 2017-03-29 06:41:18]

Sorry I'm having trouble understanding the current state of things vs the state
after the patch.

In the current state, is navigation preload generating "broken code (the origin
trial is not checked)"?

I'm wondering why we weren't able to detect this in testing.

[haraken, 2017-03-29 06:43:10]

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

[haraken, 2017-03-29 06:43:11]

https://codereview.chromium.org/2780093003/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/scripts/v8_utilities.py (right):

https://codereview.chromium.org/2780093003/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/scripts/v8_utilities.py:419:
is_feature_policy_enabled = 'FeaturePolicy' in extended_attributes
On 2017/03/29 06:38:06, Yuki wrote:
> On 2017/03/29 05:09:33, Matt Giuca wrote:
> > This looks like exactly the same logic as OriginTrialEnabled... does the
same
> > restriction apply?
> > 
> > If so maybe we should factor this out to avoid duplicating both paths. (I
can
> do
> > that in this CL.)
> 
> Looks like [FeaturePolicy] is deprecated and actually not used.  I think we
can
> simply remove all related code about [FeaturePolicy].

This looks interesting.

+iclelland What's your plan about the [FeaturePolicy] IDL extended attribute?

[falken, 2017-03-29 06:43:45]

https://codereview.chromium.org/2780093003/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/serviceworkers/NavigationPreloadManager.idl
(right):

https://codereview.chromium.org/2780093003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/NavigationPreloadManager.idl:11:
[SecureContext, CallWith=ScriptState] Promise<void> enable();
On 2017/03/29 06:38:06, Yuki wrote:
> On 2017/03/29 06:24:37, Matt Giuca wrote:
> > Oh no this is awful.
> > 
> > It turns out that NavigationPreloadManager is hitting the new error because
it
> > uses OTE and SC at the same time. But this isn't actually a problem because
> the
> > SC is being automatically applied to all the members of
> NavigationPreloadManager
> > (not the class itself), and the class itself is being registered manually.
> > 
> > If I move the SecureContext down here to all the methods, it doesn't hit the
> > check and seems to generate the same code.
> > 
> > Is this acceptable?
> 
> Just a question, why NavigationPreloadManager is OriginTrialEnabled?
> If it's because it's not yet stable and we'd like to expose the APIs to
limited
> users and we're planning to remove [OriginTrialEnabled], it makes a sense.
> 
> However, with this change, we're going to expose NavigationPreloadManager
> interface itself as long as the origin trial is specified even when it's not a
> secure context.
> 
> Should we expose NavigationPreloadManager only when the origin trial is
> specified *and* it's a secure context?
> 
> https://w3c.github.io/ServiceWorker/#navigation-preload-manager
> The spec seems saying we shouldn't expose the interface if it's not a secure
> context.

Yes, NavigationPreloadManager is an experimental feature that is in an origin
trial. The feature should only be exposed in secure contexts.

[commit-bot: I haz the power, 2017-03-29 07:45:47]

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

[commit-bot: I haz the power, 2017-03-29 07:45:48]

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_...)

[Matt Giuca, 2017-03-30 02:41:34]

On 2017/03/29 06:39:45, haraken wrote:
> Mu gut feeling is that the implementation of [SecureContext] is not sufficient
> in a couple of ways and you're hitting one of the issues.
> 
> Would it be an option to try to improve the implementation of [SecureContext]
> instead of introducing some hack?

https://crbug.com/695123 is about fixing the implementation of SecureContext. I
don't have the resources or expertise to fix that now. I am not introducing a
hack, the hack is already being used by WebUSB. I am simply documenting it and
adding a check to make sure you *don't* try to do the obvious thing that
currently does an incredibly wrong thing. (I spent 2 hours debugging and finally
found out there was a known issue about it. With my new compile error, Chrome
developers will be instantly alerted to the problem and told how to work around
it.)

On 2017/03/29 06:41:18, falken wrote:
> Sorry I'm having trouble understanding the current state of things vs the
state
> after the patch.
> 
> In the current state, is navigation preload generating "broken code (the
origin
> trial is not checked)"?
> 
> I'm wondering why we weren't able to detect this in testing.

No, navigation preload is not generating broken code. But only because it is
"lucky".

The state after the patch is the same for navigation preload. I just had to
alter the IDL to work around the new check (which makes sure you don't combine
OTE and SC).

Let me explain a bit more (this is making my head spin...).

1. The bug in https://crbug.com/695123 is that if you combine OriginTrialEnabled
and SecureContext together, you end up generating V8 bindings code that is
conditional on secure context, but NOT origin trial. That means as long as your
page is https, the feature is visible to all sites (but it likely crashes upon
use due to the origin trial flag being enforced elsewhere in the system). This
is BAD.
2. It seems that NavigationPreloadManager somehow managed to be exempt from this
bug... it uses both OTE and SC, and yet nowhere is there auto-generated code to
register the interface. (It's registered by hand, and correctly checked.) I
believe this is because it's a top-level interface (not a method or a partial
interface). So this is *not currently broken*.
3. All the other APIs that want to be OriginTrialEnabled and SecureContext (in
other words, ALL origin trials) have to drop the [SecureContext] from their IDL
and manually enforce security. WebUSB does this in
ConditionalFeaturesForModules.cpp as you can see in the comment I touched in
this CL.

Basically, nothing user-facing is currently broken. But the bindings generator
is extremely fragile: if you use OTE and SC together (which every single origin
trial going forwards is going to want to do), you are probably going to get
broken behaviour (exposing a crash on the open web). So I am making it a compile
error instead.

NavigationPreloadManager was an unexpected collateral: because it isn't actually
broken, but it still combines OTE and SC, it hits my new compile-time check. The
simplest way around this is to simply move SC to the individual methods, rather
than the interface. (Which generates the same code... note that even now,
NavigationPreloadManager itself is still available on a non-secure context, it
just has no methods.)

[Matt Giuca, 2017-03-30 02:57:28]

https://codereview.chromium.org/2780093003/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/serviceworkers/NavigationPreloadManager.idl
(right):

https://codereview.chromium.org/2780093003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/NavigationPreloadManager.idl:11:
[SecureContext, CallWith=ScriptState] Promise<void> enable();
On 2017/03/29 06:43:45, falken wrote:
> On 2017/03/29 06:38:06, Yuki wrote:
> > On 2017/03/29 06:24:37, Matt Giuca wrote:
> > > Oh no this is awful.
> > > 
> > > It turns out that NavigationPreloadManager is hitting the new error
because
> it
> > > uses OTE and SC at the same time. But this isn't actually a problem
because
> > the
> > > SC is being automatically applied to all the members of
> > NavigationPreloadManager
> > > (not the class itself), and the class itself is being registered manually.
> > > 
> > > If I move the SecureContext down here to all the methods, it doesn't hit
the
> > > check and seems to generate the same code.
> > > 
> > > Is this acceptable?
> > 
> > Just a question, why NavigationPreloadManager is OriginTrialEnabled?
> > If it's because it's not yet stable and we'd like to expose the APIs to
> limited
> > users and we're planning to remove [OriginTrialEnabled], it makes a sense.
> > 
> > However, with this change, we're going to expose NavigationPreloadManager
> > interface itself as long as the origin trial is specified even when it's not
a
> > secure context.
> > 
> > Should we expose NavigationPreloadManager only when the origin trial is
> > specified *and* it's a secure context?
> > 
> > https://w3c.github.io/ServiceWorker/#navigation-preload-manager
> > The spec seems saying we shouldn't expose the interface if it's not a secure
> > context.
> 
> Yes, NavigationPreloadManager is an experimental feature that is in an origin
> trial. The feature should only be exposed in secure contexts.

I would be changing the IDL away from the spec, but it has the same effect in
Chrome (the NavigationPreloadManager object itself is still available in a
non-secure context, just not the methods). I can add a comment saying this is a
work-around this bug.

[falken, 2017-03-30 05:33:24]

> (Which generates the same code... note that even now,
NavigationPreloadManager itself is still available on a non-secure context, it
just has no methods.)

I see, I didn't realize SecureContext is not really working as expected in the
current implementation. If this is not changing webexposed behavior then, the
changes to NavigationPreloadManager.idl lgtm.

[Reilly Grant (use Gerrit), 2017-03-30 06:47:05]

Note that in theory [OriginTrialEnabled] would imply [SecureContext] except that
when the --experimental-web-platform-features flag is passed that short-circuits
the OriginTrials::fooEnabled() checks to true before the [SecureContext] check
it normally performs when validating that a token is present.

[Yuki, 2017-03-30 07:43:00]

Thanks for the explanation.  LGTM.

[haraken, 2017-03-30 14:25:26]

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

[haraken, 2017-03-30 14:25:26]

LGTM

However, it's unfortunate that the current implementation of [SecureContext] is
wrong and causing a lot of confusions.

mkwst@: Would you take a look at crbug.com/695123 or find someone to take a
look?

[Matt Giuca, 2017-03-31 05:27:37]

Leaving this open to decide what to do about the FeaturePolicy part (I sent an
email to Ian).

[Matt Giuca, 2017-04-04 05:36:51]

I didn't hear back from Ian. Under the "better than nothing" policy, I'm going
to land this, then figure out what to do about FeaturePolicy mixed with
SecureContext.

[Matt Giuca, 2017-04-04 05:41:55]

The CQ bit was checked by mgiuca@chromium.org

[Matt Giuca, 2017-04-04 05:41:55]

The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org,
haraken@chromium.org, yukishiino@chromium.org
Link to the patchset: https://codereview.chromium.org/2780093003/#ps60001
(title: "Rebase.")

[commit-bot: I haz the power, 2017-04-04 05:42:04]

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-04-04 07:22:41]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1491284515313910,
"parent_rev": "565e54c04c1b33ae517caa33a0ba64448fd5c56a", "commit_rev":
"eb6e06daaefc9d173132724d905ce77a3ece95d3"}

[commit-bot: I haz the power, 2017-04-04 07:23:28]

Description was changed from

==========
V8 bindings gen: Error if OriginTrialEnabled and SecureContext together.

If these are found together, they currently generate completely broken
code (the origin trial is not checked). Therefore, throw an error at
compile time and point the unlucky developer to a workaround.

BUG=695123
==========

to

==========
V8 bindings gen: Error if OriginTrialEnabled and SecureContext together.

If these are found together, they currently generate completely broken
code (the origin trial is not checked). Therefore, throw an error at
compile time and point the unlucky developer to a workaround.

BUG=695123

Review-Url: https://codereview.chromium.org/2780093003
Cr-Commit-Position: refs/heads/master@{#461653}
Committed:
https://chromium.googlesource.com/chromium/src/+/eb6e06daaefc9d173132724d905c...
==========

[commit-bot: I haz the power, 2017-04-04 07:23:29]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/eb6e06daaefc9d173132724d905c...

[Matt Giuca, 2017-04-06 03:40:11]

Patchset #5 (id:80001) has been deleted