Store the list of trial tokens in OriginTrialContext

Store the list of trial tokens in OriginTrialContext

Refactor the way tokens are found by storing them separately from the
document tree in OriginTrialContext.

This simplifies the architecture a bit, making it easier to:
- add support for tokens specified in http headers.
- add support for trials in workers.
- somehow limit which tokens are considered "valid".

BUG=601108
Committed: https://crrev.com/b03579678fa2678748d5cbcec2e18a8a08e40701
Cr-Commit-Position: refs/heads/master@{#385504}

[Marijn Kruisselbrink, 2016-03-25 18:22:36]

Patchset #2 (id:20001) has been deleted

[Marijn Kruisselbrink, 2016-03-25 19:08:38]

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

[Marijn Kruisselbrink, 2016-03-25 19:08:39]

I hope I'm not getting in the way of any work you were (planning on) doing, but
the header & worker support become a lot more straight forward with this change
done first...

https://codereview.chromium.org/1833063002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/HttpEquiv.cpp (right):

https://codereview.chromium.org/1833063002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/HttpEquiv.cpp:48: bool isScriptGenerated =
document.hasFinishedParsing() || (document.scriptableDocumentParser() &&
document.scriptableDocumentParser()->isExecutingScript());
I'm not sure if this is entirely the right check to detect any script-made
modifications to these meta tags (nor am I entirely sure what we exactly want to
prevent), but this seems to work for the cases I've tested.

[iclelland, 2016-03-31 15:00:00]

I've uploaded my WIP as https://codereview.chromium.org/1840193006/ -- you can
take a look at that to see what I'd done.

I like making OriginTrialContext(Base) a base class for OriginTrials; I had done
something similar, but just using OriginTrialsImpl to implement methods declared
on OriginTrials, rather than using inheritance.

Your way lets us keep the method declarations out of the template files, though,
which I like.

I'm going to give this one more close look today, but this is looking pretty
good so far.

https://codereview.chromium.org/1833063002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/origin_trials/javascript-insert-token.html
(right):

https://codereview.chromium.org/1833063002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/origin_trials/javascript-insert-token.html:3:
<!-- TODO(iclelland): Generate this sample token during the build. The token
The comment is still valid; should probably be in the script, though.

https://codereview.chromium.org/1833063002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/origin_trials/javascript-insert-token.html:10:
document.head.appendChild(meta);
Can we define these four lines as a function here and call it, and call again in
the onload handler?

https://codereview.chromium.org/1833063002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/origin_trials/javascript-insert-token.html:29:
assert_idl_attribute(window.internals, 'frobulate');
Can this be replaced with a call to expect_failure?

https://codereview.chromium.org/1833063002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/HttpEquiv.cpp (right):

https://codereview.chromium.org/1833063002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/HttpEquiv.cpp:47: } else if
(equalIgnoringCase(equiv, HTTPNames::Origin_Trial)) {
I like using HTTPNames here -- do you have any idea why the other cases don't do
that?

https://codereview.chromium.org/1833063002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/HttpEquiv.cpp:48: bool isScriptGenerated =
document.hasFinishedParsing() || (document.scriptableDocumentParser() &&
document.scriptableDocumentParser()->isExecutingScript());
On 2016/03/25 19:08:39, Marijn Kruisselbrink wrote:
> I'm not sure if this is entirely the right check to detect any script-made
> modifications to these meta tags (nor am I entirely sure what we exactly want
to
> prevent), but this seems to work for the cases I've tested.

You can see what I'd done in https://codereview.chromium.org/1840193006/ (WIP,
uploaded for comparison here). Rather than specifically checking whether a
script is running, I figured we'd probably want to have a concept of
'finalizing' the set of tokens, either when the <head> closes out, or the first
<script> tag is encountered. Then we can't get any XSS-added tags, and we also
never get a situation where a trial is disabled in one script (before the meta
tag) and enabled in the next (after it).

https://codereview.chromium.org/1833063002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/origin_trials/OriginTrialsBase.h (right):

https://codereview.chromium.org/1833063002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/origin_trials/OriginTrialsBase.h:1: // Copyright
2015 The Chromium Authors. All rights reserved.
2016?

https://codereview.chromium.org/1833063002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/origin_trials/OriginTrialsBase.h:19: class
CORE_EXPORT OriginTrialsBase : public
NoBaseWillBeGarbageCollectedFinalized<OriginTrialsBase> {
Are we still using WillBe types? I think they're all being removed right now.

https://codereview.chromium.org/1833063002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/origin_trials/OriginTrialsBaseTest.cpp
(right):

https://codereview.chromium.org/1833063002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/origin_trials/OriginTrialsBaseTest.cpp:79: using
OriginTrialsBase::isFeatureEnabled;
I had no idea you could do that -- that's amazingly useful, thanks!

[iclelland, 2016-03-31 18:05:10]

On 2016/03/31 15:00:00, iclelland wrote:
> I've uploaded my WIP as https://codereview.chromium.org/1840193006/ -- you can
> take a look at that to see what I'd done.
> 
> I like making OriginTrialContext(Base) a base class for OriginTrials; I had
done
> something similar, but just using OriginTrialsImpl to implement methods
declared
> on OriginTrials, rather than using inheritance.
> 
> Your way lets us keep the method declarations out of the template files,
though,
> which I like.
> 

I guess the philosophical objection to doing it this way is that it's kind of an
abuse of inheritance -- there's no way in which OriginTrials is a substitute for
OriginTrialsBase; there's no 'is-a' relationship here. It's a base class here
just in order to get access to some common implementation, much like the
OriginTrialsContext class did, through composition. The change in
crrev.com/1840193006 achieves the same thing, but by putting the implementation
right into OriginTrials, and only defining the methods in a different cpp file
so that it's not in the template.

They all have different disadvantages; I'm not overly committed to any of these
strategies, just laying out my thoughts on the differences.

[iclelland, 2016-03-31 18:05:20]

https://codereview.chromium.org/1833063002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/origin_trials/OriginTrialsBaseTest.cpp
(right):

https://codereview.chromium.org/1833063002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/origin_trials/OriginTrialsBaseTest.cpp:79: using
OriginTrialsBase::isFeatureEnabled;
On 2016/03/31 15:00:00, iclelland wrote:
> I had no idea you could do that -- that's amazingly useful, thanks!

Although, as chasej@ points out to me, isn't it more common in Chromium to
declare OriginTrialsBaseTest a friend of OriginTrialsBase, and then have access
to its internals that way, rather than subclassing to expose methods?

[Marijn Kruisselbrink, 2016-03-31 19:01:48]

On 2016/03/31 at 18:05:10, iclelland wrote:
> On 2016/03/31 15:00:00, iclelland wrote:
> > I've uploaded my WIP as https://codereview.chromium.org/1840193006/ -- you
can
> > take a look at that to see what I'd done.
Oops, sorry. I didn't realize you had started work on this already. The bug was
marked as "available" without an owner...

> > I like making OriginTrialContext(Base) a base class for OriginTrials; I had
done
> > something similar, but just using OriginTrialsImpl to implement methods
declared
> > on OriginTrials, rather than using inheritance.
> > 
> > Your way lets us keep the method declarations out of the template files,
though,
> > which I like.
> > 
> 
> I guess the philosophical objection to doing it this way is that it's kind of
an abuse of inheritance -- there's no way in which OriginTrials is a substitute
for OriginTrialsBase; there's no 'is-a' relationship here. It's a base class
here just in order to get access to some common implementation, much like the
OriginTrialsContext class did, through composition. The change in
crrev.com/1840193006 achieves the same thing, but by putting the implementation
right into OriginTrials, and only defining the methods in a different cpp file
so that it's not in the template.
> 
> They all have different disadvantages; I'm not overly committed to any of
these strategies, just laying out my thoughts on the differences.

I'm not overly committed to any of these either. But keeping as much code as
possible out of the .tmpl files seems like a good goal (since those don't get
processed by presubmits checking things like coding style). Maybe private
inheritance could be an alternative, but not sure if/how that will mesh with
oilpan. And it would be a somewhat esoteric c++ thing that isn't really used
much. 

Do you know if the "API defined in .tmpl.h but implementation in a separate file
elsewhere in the code" pattern is something that's used in other places in the
code? Having a separate FooBase and Foo class where part of the Foo
implementation lives in FooBase does seem like a pattern that is used a fair
amount (mostly in c++ templated code to keeps templatized code and
non-templatized code separate, or to hide extra template parameters from the
"actual" interface; but the concept seems to apply here as well).

https://codereview.chromium.org/1833063002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/origin_trials/javascript-insert-token.html
(right):

https://codereview.chromium.org/1833063002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/origin_trials/javascript-insert-token.html:29:
assert_idl_attribute(window.internals, 'frobulate');
On 2016/03/31 at 15:00:00, iclelland wrote:
> Can this be replaced with a call to expect_failure?

Not really. expect_failure() creates new tests rather than just implementing
assertions, so it would end up associating assertions with the wrong test. But I
agree that this test still needs some work/cleanup put in it, and there is
probably a way to make it cleaner.

https://codereview.chromium.org/1833063002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/HttpEquiv.cpp (right):

https://codereview.chromium.org/1833063002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/HttpEquiv.cpp:47: } else if
(equalIgnoringCase(equiv, HTTPNames::Origin_Trial)) {
On 2016/03/31 at 15:00:00, iclelland wrote:
> I like using HTTPNames here -- do you have any idea why the other cases don't
do that?

No idea, so maybe core/ owners will object to using HTTPNames here...

https://codereview.chromium.org/1833063002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/HttpEquiv.cpp:48: bool isScriptGenerated =
document.hasFinishedParsing() || (document.scriptableDocumentParser() &&
document.scriptableDocumentParser()->isExecutingScript());
On 2016/03/31 at 15:00:00, iclelland wrote:
> On 2016/03/25 19:08:39, Marijn Kruisselbrink wrote:
> > I'm not sure if this is entirely the right check to detect any script-made
> > modifications to these meta tags (nor am I entirely sure what we exactly
want to
> > prevent), but this seems to work for the cases I've tested.
> 
> You can see what I'd done in https://codereview.chromium.org/1840193006/ (WIP,
uploaded for comparison here). Rather than specifically checking whether a
script is running, I figured we'd probably want to have a concept of
'finalizing' the set of tokens, either when the <head> closes out, or the first
<script> tag is encountered. Then we can't get any XSS-added tags, and we also
never get a situation where a trial is disabled in one script (before the meta
tag) and enabled in the next (after it).

Yeah, I wasn't sure what situations we actually want to allow/prevent. If in the
case of <head><script><meta><script></head> we indeed want to ignore the tokens
in the meta tag then doing something like what you did is probably a better
approach. Probably for things like this it might actually make sense to do try
to come up with a more formal spec to specify how/when tokens should get
applied. Even if we don't expect other browser to follow the same meta tag
format, clearly documenting these things still seems like a good idea (and gives
us something to point to for the meta tags and http header registries).

https://codereview.chromium.org/1833063002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/origin_trials/OriginTrialsBase.h (right):

https://codereview.chromium.org/1833063002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/origin_trials/OriginTrialsBase.h:19: class
CORE_EXPORT OriginTrialsBase : public
NoBaseWillBeGarbageCollectedFinalized<OriginTrialsBase> {
On 2016/03/31 at 15:00:00, iclelland wrote:
> Are we still using WillBe types? I think they're all being removed right now.

We definitely were still using them last week, and I don't think haraken has
managed to land any of the CLs that remove them yet even now. Of course if those
have landed before this is finished, there won't be a need for WillBe types here
anymore.

https://codereview.chromium.org/1833063002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/origin_trials/OriginTrialsBaseTest.cpp
(right):

https://codereview.chromium.org/1833063002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/origin_trials/OriginTrialsBaseTest.cpp:79: using
OriginTrialsBase::isFeatureEnabled;
On 2016/03/31 at 18:05:20, iclelland wrote:
> On 2016/03/31 15:00:00, iclelland wrote:
> > I had no idea you could do that -- that's amazingly useful, thanks!
> 
> Although, as chasej@ points out to me, isn't it more common in Chromium to
declare OriginTrialsBaseTest a friend of OriginTrialsBase, and then have access
to its internals that way, rather than subclassing to expose methods?

I generally try to avoid friending whenever I can. Most of the time an API that
can only be tested by having access to implementation details/internals of the
class your testing is a pretty bad API. Also depending on internals rather than
on parts that the class being tested has explicitly made available makes
refactoring much harder (and makes it more likely that you're tests aren't
actually testing what you want to test but just test one particular
implementation).

(more specifically about using "using" this way, I'm not actually sure it's
allowed. It's a c++11 thing that wasn't supported by MSVC2013. The c++11 allowed
features page doesn't mention it as either allowed or denied (it only mentions
using for constructors). So I'd probably replace it with an explicit
isFeatureEnabled method that just calls OriginTrialsBase::isFeatureEnabled. Or
just add a isFeatureEnabledForTesting to OriginTrialsBase; which would still be
way better than adding a friend definition as it would be enforced by presubmit
hooks that only test code calls it).

[iclelland, 2016-04-01 15:19:09]

https://codereview.chromium.org/1833063002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/origin_trials/javascript-insert-token.html
(right):

https://codereview.chromium.org/1833063002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/origin_trials/javascript-insert-token.html:29:
assert_idl_attribute(window.internals, 'frobulate');
On 2016/03/31 19:01:48, Marijn Kruisselbrink wrote:
> On 2016/03/31 at 15:00:00, iclelland wrote:
> > Can this be replaced with a call to expect_failure?
> 
> Not really. expect_failure() creates new tests rather than just implementing
> assertions, so it would end up associating assertions with the wrong test. But
I
> agree that this test still needs some work/cleanup put in it, and there is
> probably a way to make it cleaner.

Maybe factoring the common code out into a named function in origin_trials.js
would be enough?

https://codereview.chromium.org/1833063002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/HttpEquiv.cpp (right):

https://codereview.chromium.org/1833063002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/HttpEquiv.cpp:47: } else if
(equalIgnoringCase(equiv, HTTPNames::Origin_Trial)) {
On 2016/03/31 19:01:48, Marijn Kruisselbrink wrote:
> On 2016/03/31 at 15:00:00, iclelland wrote:
> > I like using HTTPNames here -- do you have any idea why the other cases
don't
> do that?
> 
> No idea, so maybe core/ owners will object to using HTTPNames here...

Okay; we should be explicit and #include "platform/HTTPNames.h" then (and see if
it sticks -- I don't see a problem, core/ is allowed to include platform/, but
I'll defer to owners.)

https://codereview.chromium.org/1833063002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/HttpEquiv.cpp:48: bool isScriptGenerated =
document.hasFinishedParsing() || (document.scriptableDocumentParser() &&
document.scriptableDocumentParser()->isExecutingScript());
On 2016/03/31 19:01:48, Marijn Kruisselbrink wrote:
> On 2016/03/31 at 15:00:00, iclelland wrote:
> > On 2016/03/25 19:08:39, Marijn Kruisselbrink wrote:
> > > I'm not sure if this is entirely the right check to detect any script-made
> > > modifications to these meta tags (nor am I entirely sure what we exactly
> want to
> > > prevent), but this seems to work for the cases I've tested.
> > 
> > You can see what I'd done in https://codereview.chromium.org/1840193006/
(WIP,
> uploaded for comparison here). Rather than specifically checking whether a
> script is running, I figured we'd probably want to have a concept of
> 'finalizing' the set of tokens, either when the <head> closes out, or the
first
> <script> tag is encountered. Then we can't get any XSS-added tags, and we also
> never get a situation where a trial is disabled in one script (before the meta
> tag) and enabled in the next (after it).
> 
> Yeah, I wasn't sure what situations we actually want to allow/prevent. If in
the
> case of <head><script><meta><script></head> we indeed want to ignore the
tokens
> in the meta tag then doing something like what you did is probably a better
> approach. Probably for things like this it might actually make sense to do try
> to come up with a more formal spec to specify how/when tokens should get
> applied. Even if we don't expect other browser to follow the same meta tag
> format, clearly documenting these things still seems like a good idea (and
gives
> us something to point to for the meta tags and http header registries).

Let's keep this here, then -- I haven't been able to completely rule out JS
execution in the HEAD yet (maybe something like an onload="" attribute in a
<link> tag)
I'll merge my code in afterwards to restrict the placement of the tags to
pre-script only.

https://codereview.chromium.org/1833063002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/origin_trials/OriginTrialsBase.h (right):

https://codereview.chromium.org/1833063002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/origin_trials/OriginTrialsBase.h:19: class
CORE_EXPORT OriginTrialsBase : public
NoBaseWillBeGarbageCollectedFinalized<OriginTrialsBase> {
On 2016/03/31 19:01:48, Marijn Kruisselbrink wrote:
> On 2016/03/31 at 15:00:00, iclelland wrote:
> > Are we still using WillBe types? I think they're all being removed right
now.
> 
> We definitely were still using them last week, and I don't think haraken has
> managed to land any of the CLs that remove them yet even now. Of course if
those
> have landed before this is finished, there won't be a need for WillBe types
here
> anymore.

Got it. I thought those had landed; hadn't finished reading the entire threads.

https://codereview.chromium.org/1833063002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/origin_trials/OriginTrialsBaseTest.cpp
(right):

https://codereview.chromium.org/1833063002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/origin_trials/OriginTrialsBaseTest.cpp:79: using
OriginTrialsBase::isFeatureEnabled;
On 2016/03/31 19:01:48, Marijn Kruisselbrink wrote:
> On 2016/03/31 at 18:05:20, iclelland wrote:
> > On 2016/03/31 15:00:00, iclelland wrote:
> > > I had no idea you could do that -- that's amazingly useful, thanks!
> > 
> > Although, as chasej@ points out to me, isn't it more common in Chromium to
> declare OriginTrialsBaseTest a friend of OriginTrialsBase, and then have
access
> to its internals that way, rather than subclassing to expose methods?
> 
> I generally try to avoid friending whenever I can. Most of the time an API
that
> can only be tested by having access to implementation details/internals of the
> class your testing is a pretty bad API. Also depending on internals rather
than
> on parts that the class being tested has explicitly made available makes
> refactoring much harder (and makes it more likely that you're tests aren't
> actually testing what you want to test but just test one particular
> implementation).
> 
> (more specifically about using "using" this way, I'm not actually sure it's
> allowed. It's a c++11 thing that wasn't supported by MSVC2013. The c++11
allowed
> features page doesn't mention it as either allowed or denied (it only mentions
> using for constructors).

Reading some more, it looks like this use should be allowed. MSDN says it's
available at least as far back as MSVC2010
https://msdn.microsoft.com/en-us/library/was37tzw(v=vs.100).aspx -- see B::g in
the last example on that page.

AFAICT, this goes back at least to C++03, which would explain its absence from
the C++11 allowed features list.
Inherited constructors, *are* a more recent (C++11) addition, and may still be
buggy in MSVC2013, although they're listed as supported. (And I think we
switched to 2015 recently anyway)


> So I'd probably replace it with an explicit
> isFeatureEnabled method that just calls OriginTrialsBase::isFeatureEnabled. Or
> just add a isFeatureEnabledForTesting to OriginTrialsBase; which would still
be
> way better than adding a friend definition as it would be enforced by
presubmit
> hooks that only test code calls it).

[Marijn Kruisselbrink, 2016-04-01 23:48:29]

Patchset #3 (id:60001) has been deleted

[Marijn Kruisselbrink, 2016-04-01 23:54:48]

Also changed OriginTrials to be a namespace with just the auto-generated
statics, and have OriginTrialContext be the Supplement<ExecutionContext> that
contains state and other code.

https://codereview.chromium.org/1833063002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/origin_trials/javascript-insert-token.html
(right):

https://codereview.chromium.org/1833063002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/origin_trials/javascript-insert-token.html:10:
document.head.appendChild(meta);
On 2016/03/31 at 15:00:00, iclelland wrote:
> Can we define these four lines as a function here and call it, and call again
in the onload handler?

Not done yet. I'm not sure if it really gains us much though... Especially with
the addition of a <link onerror> test, where calling a function won't be
possible anyway. But if you feel there is still value in abstracting it out for
the two places where we could share code I'd be happy to do so.

https://codereview.chromium.org/1833063002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/origin_trials/javascript-insert-token.html:29:
assert_idl_attribute(window.internals, 'frobulate');
On 2016/04/01 at 15:19:09, iclelland wrote:
> On 2016/03/31 19:01:48, Marijn Kruisselbrink wrote:
> > On 2016/03/31 at 15:00:00, iclelland wrote:
> > > Can this be replaced with a call to expect_failure?
> > 
> > Not really. expect_failure() creates new tests rather than just implementing
> > assertions, so it would end up associating assertions with the wrong test.
But I
> > agree that this test still needs some work/cleanup put in it, and there is
> > probably a way to make it cleaner.
> 
> Maybe factoring the common code out into a named function in origin_trials.js
would be enough?

I changed expect_failure to optionally accept a test object (when provided it
will associate assertions with that test rather than creating its own new
tests).

https://codereview.chromium.org/1833063002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/HttpEquiv.cpp (right):

https://codereview.chromium.org/1833063002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/HttpEquiv.cpp:48: bool isScriptGenerated =
document.hasFinishedParsing() || (document.scriptableDocumentParser() &&
document.scriptableDocumentParser()->isExecutingScript());
On 2016/04/01 at 15:19:09, iclelland wrote:
> On 2016/03/31 19:01:48, Marijn Kruisselbrink wrote:
> > On 2016/03/31 at 15:00:00, iclelland wrote:
> > > On 2016/03/25 19:08:39, Marijn Kruisselbrink wrote:
> > > > I'm not sure if this is entirely the right check to detect any
script-made
> > > > modifications to these meta tags (nor am I entirely sure what we exactly
> > want to
> > > > prevent), but this seems to work for the cases I've tested.
> > > 
> > > You can see what I'd done in https://codereview.chromium.org/1840193006/
(WIP,
> > uploaded for comparison here). Rather than specifically checking whether a
> > script is running, I figured we'd probably want to have a concept of
> > 'finalizing' the set of tokens, either when the <head> closes out, or the
first
> > <script> tag is encountered. Then we can't get any XSS-added tags, and we
also
> > never get a situation where a trial is disabled in one script (before the
meta
> > tag) and enabled in the next (after it).
> > 
> > Yeah, I wasn't sure what situations we actually want to allow/prevent. If in
the
> > case of <head><script><meta><script></head> we indeed want to ignore the
tokens
> > in the meta tag then doing something like what you did is probably a better
> > approach. Probably for things like this it might actually make sense to do
try
> > to come up with a more formal spec to specify how/when tokens should get
> > applied. Even if we don't expect other browser to follow the same meta tag
> > format, clearly documenting these things still seems like a good idea (and
gives
> > us something to point to for the meta tags and http header registries).
> 
> Let's keep this here, then -- I haven't been able to completely rule out JS
execution in the HEAD yet (maybe something like an onload="" attribute in a
<link> tag)
> I'll merge my code in afterwards to restrict the placement of the tags to
pre-script only.

I started trying to specify what behavior we're aiming for at
https://mkruisselbrink.github.io/ExperimentalFramework/ (and as a pull request).
As you speculate onload/onerror on a link attribute is indeed another way to
execute script in the HEAD without a script tag. That one isn't actually caught
by my current implementation (so I added it to the layout test, but commented
out the link tag).

But I do wonder how important it is to block all this anyway. Where did this
requirement come from?

[iclelland, 2016-04-05 17:26:28]

https://codereview.chromium.org/1833063002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/origin_trials/javascript-insert-token.html
(right):

https://codereview.chromium.org/1833063002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/origin_trials/javascript-insert-token.html:10:
document.head.appendChild(meta);
On 2016/04/01 23:54:47, Marijn Kruisselbrink wrote:
> On 2016/03/31 at 15:00:00, iclelland wrote:
> > Can we define these four lines as a function here and call it, and call
again
> in the onload handler?
> 
> Not done yet. I'm not sure if it really gains us much though... Especially
with
> the addition of a <link onerror> test, where calling a function won't be
> possible anyway. But if you feel there is still value in abstracting it out
for
> the two places where we could share code I'd be happy to do so.

I think we should share code -- definitely the opaque token string, if not the
snippet of code that attaches it to the document.

https://codereview.chromium.org/1833063002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/HttpEquiv.cpp (right):

https://codereview.chromium.org/1833063002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/HttpEquiv.cpp:48: bool isScriptGenerated =
document.hasFinishedParsing() || (document.scriptableDocumentParser() &&
document.scriptableDocumentParser()->isExecutingScript());
On 2016/04/01 23:54:47, Marijn Kruisselbrink wrote:
> On 2016/04/01 at 15:19:09, iclelland wrote:
> > On 2016/03/31 19:01:48, Marijn Kruisselbrink wrote:
> > > On 2016/03/31 at 15:00:00, iclelland wrote:
> > > > On 2016/03/25 19:08:39, Marijn Kruisselbrink wrote:
> > > > > I'm not sure if this is entirely the right check to detect any
> script-made
> > > > > modifications to these meta tags (nor am I entirely sure what we
exactly
> > > want to
> > > > > prevent), but this seems to work for the cases I've tested.
> > > > 
> > > > You can see what I'd done in https://codereview.chromium.org/1840193006/
> (WIP,
> > > uploaded for comparison here). Rather than specifically checking whether a
> > > script is running, I figured we'd probably want to have a concept of
> > > 'finalizing' the set of tokens, either when the <head> closes out, or the
> first
> > > <script> tag is encountered. Then we can't get any XSS-added tags, and we
> also
> > > never get a situation where a trial is disabled in one script (before the
> meta
> > > tag) and enabled in the next (after it).
> > > 
> > > Yeah, I wasn't sure what situations we actually want to allow/prevent. If
in
> the
> > > case of <head><script><meta><script></head> we indeed want to ignore the
> tokens
> > > in the meta tag then doing something like what you did is probably a
better
> > > approach. Probably for things like this it might actually make sense to do
> try
> > > to come up with a more formal spec to specify how/when tokens should get
> > > applied. Even if we don't expect other browser to follow the same meta tag
> > > format, clearly documenting these things still seems like a good idea (and
> gives
> > > us something to point to for the meta tags and http header registries).
> > 
> > Let's keep this here, then -- I haven't been able to completely rule out JS
> execution in the HEAD yet (maybe something like an onload="" attribute in a
> <link> tag)
> > I'll merge my code in afterwards to restrict the placement of the tags to
> pre-script only.
> 
> I started trying to specify what behavior we're aiming for at
> https://mkruisselbrink.github.io/ExperimentalFramework/ (and as a pull
request).
> As you speculate onload/onerror on a link attribute is indeed another way to
> execute script in the HEAD without a script tag. That one isn't actually
caught
> by my current implementation (so I added it to the layout test, but commented
> out the link tag).
> 
> But I do wonder how important it is to block all this anyway. Where did this
> requirement come from?

Out of a bunch of discussions, so a spec is probably in order. I think part of
what we're aiming for is the principle of least surprise --
 - Can we avoid a situation where one script can't see the experimental
attributes, but the next script can? That could mess up feature detection, if
libraries do it early enough.
Also, we want to avoid XSS attacks - especially stored XSS, where an attacker
could enable a trial on a page that the page's owner doesn't want it used on.

If we can say that the tokens must be present before any JS runs, then it
doesn't matter when we initialize the objects in V8, and we could eventually get
to a place where the JS world is just constructed with the right experimental
features enabled, with no possibility of initialization-after-the-fact. If we
allow tokens after script tags, then we can't do that.

https://codereview.chromium.org/1833063002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/build/scripts/templates/OriginTrials.h.tmpl
(right):

https://codereview.chromium.org/1833063002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/templates/OriginTrials.h.tmpl:15: //
experimental framework via API keys.
Ugh. Old comment. Maybe change it to "via origin trial tokens"?

[iclelland, 2016-04-05 17:29:30]

Sorry for the delay -- other than those couple of issues (which are really
nits), this lgtm. (Assuming the remaining merge conflicts don't involve anything
nasty)

[Marijn Kruisselbrink, 2016-04-05 20:56:07]

mek@chromium.org changed reviewers:
+ rbyers@chromium.org

[Marijn Kruisselbrink, 2016-04-05 20:56:08]

+rbyers for core/ OWNERS

https://codereview.chromium.org/1833063002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/origin_trials/javascript-insert-token.html
(right):

https://codereview.chromium.org/1833063002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/origin_trials/javascript-insert-token.html:10:
document.head.appendChild(meta);
On 2016/04/05 at 17:26:28, iclelland wrote:
> On 2016/04/01 23:54:47, Marijn Kruisselbrink wrote:
> > On 2016/03/31 at 15:00:00, iclelland wrote:
> > > Can we define these four lines as a function here and call it, and call
again
> > in the onload handler?
> > 
> > Not done yet. I'm not sure if it really gains us much though... Especially
with
> > the addition of a <link onerror> test, where calling a function won't be
> > possible anyway. But if you feel there is still value in abstracting it out
for
> > the two places where we could share code I'd be happy to do so.
> 
> I think we should share code -- definitely the opaque token string, if not the
snippet of code that attaches it to the document.

There are still multiple copies of the opaque token string, since not all can be
done by javascript the same way, but shared as much code as possible now.

https://codereview.chromium.org/1833063002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/HttpEquiv.cpp (right):

https://codereview.chromium.org/1833063002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/HttpEquiv.cpp:48: bool isScriptGenerated =
document.hasFinishedParsing() || (document.scriptableDocumentParser() &&
document.scriptableDocumentParser()->isExecutingScript());
On 2016/04/05 at 17:26:28, iclelland wrote:
> Out of a bunch of discussions, so a spec is probably in order. I think part of
what we're aiming for is the principle of least surprise --
>  - Can we avoid a situation where one script can't see the experimental
attributes, but the next script can? That could mess up feature detection, if
libraries do it early enough.
> Also, we want to avoid XSS attacks - especially stored XSS, where an attacker
could enable a trial on a page that the page's owner doesn't want it used on.
> 
> If we can say that the tokens must be present before any JS runs, then it
doesn't matter when we initialize the objects in V8, and we could eventually get
to a place where the JS world is just constructed with the right experimental
features enabled, with no possibility of initialization-after-the-fact. If we
allow tokens after script tags, then we can't do that.

Hmmm, okay. At least it seems the code in this CL is a step in the right
direction, even if it doesn't fully capture whatever behavior we want. I'm not
sure I entirely get what kind of XSS attacks we want to avoid, but that's
probably best to have as a separate discussion on the spec repo. And the answer
to that will probably also depend on what we decide to do about trial tokens for
about:blank, blob urls, data urls etc. I don't think "before any JS runs" is
very well defined though (and rather hard to define precisely: what about one
page including another page in an iframe. The iframed page never "runs and JS",
but the outside page can run JS and do whatever it wants with the iframe).

And what if in the future we launch some experimental css feature using the
experimental framework. Do we then all of a sudden also ignore all tokens after
the first <link> and/or <style> elements? Or even the first element with an
inline style= attribute? I'm not convinced that ignoring tokens because they
appear in the "wrong" location is really the least surprising. Ignoring tokens
outside of <head>, and tokens only being applied after they were encountered at
least is consistent with things like csp policies specified by meta tags.

https://codereview.chromium.org/1833063002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/build/scripts/templates/OriginTrials.h.tmpl
(right):

https://codereview.chromium.org/1833063002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/templates/OriginTrials.h.tmpl:15: //
experimental framework via API keys.
On 2016/04/05 at 17:26:28, iclelland wrote:
> Ugh. Old comment. Maybe change it to "via origin trial tokens"?

Done

[Rick Byers, 2016-04-05 21:09:01]

I'm confused about the purpose of this change, let's discuss in the bug (and
leave the CR for the implementation details).

[Marijn Kruisselbrink, 2016-04-05 21:13:20]

On 2016/04/05 at 21:09:01, rbyers wrote:
> I'm confused about the purpose of this change, let's discuss in the bug (and
leave the CR for the implementation details).

From my point of view most of this change is actually not about disallowing
javascript created meta tags (which really is only one line in the patch). I
care more about the refactoring that makes the code both architecturally simpler
and makes it much easier to specify tokens via http headers and enable
experiments in workers (follow up patches I've written).
So maybe it makes sense for me to remove the "is script generated" part from
this CL, so I can land this CL and not block the other changes, while we figure
out what exactly the behavior is we want to achieve (and I totally agree that
"block script generated meta tags" is rather ill defined and doesn't necessarily
make sense).

[Rick Byers, 2016-04-05 21:19:35]

On 2016/04/05 21:13:20, Marijn Kruisselbrink wrote:
> On 2016/04/05 at 21:09:01, rbyers wrote:
> > I'm confused about the purpose of this change, let's discuss in the bug (and
> leave the CR for the implementation details).
> 
> From my point of view most of this change is actually not about disallowing
> javascript created meta tags (which really is only one line in the patch). I
> care more about the refactoring that makes the code both architecturally
simpler
> and makes it much easier to specify tokens via http headers and enable
> experiments in workers (follow up patches I've written).
> So maybe it makes sense for me to remove the "is script generated" part from
> this CL, so I can land this CL and not block the other changes, while we
figure
> out what exactly the behavior is we want to achieve (and I totally agree that
> "block script generated meta tags" is rather ill defined and doesn't
necessarily
> make sense).

Yeah sounds like we should treat those separately.  I need to run now but I can
take a look at this patch tomorrow morning (sorry I was trying to get the big
picture before digging into the code changes).

[Marijn Kruisselbrink, 2016-04-05 21:22:57]

Description was changed from

==========
Don't allow javascript created meta tags to enable origin trials.

Refactor the way tokens are found by storing them separately from the
document tree, and only adding tokens that weren't created by script.

This also simplifies the architecture a bit, and makes it easier to
support trials in workers.

BUG=588151
==========

to

==========
Store the list of trial tokens in OriginTrialContext

Refactor the way tokens are found by storing them separately from the
document tree in OriginTrialContext.

This simplifies the architecture a bit, making it easier to:
- add support for tokens specified in http headers.
- add support for trials in workers.
- somehow limit which tokens are considered "valid".

BUG=588151
==========

[Marijn Kruisselbrink, 2016-04-05 21:23:31]

On 2016/04/05 at 21:19:35, rbyers wrote:
> On 2016/04/05 21:13:20, Marijn Kruisselbrink wrote:
> > On 2016/04/05 at 21:09:01, rbyers wrote:
> > > I'm confused about the purpose of this change, let's discuss in the bug
(and
> > leave the CR for the implementation details).
> > 
> > From my point of view most of this change is actually not about disallowing
> > javascript created meta tags (which really is only one line in the patch). I
> > care more about the refactoring that makes the code both architecturally
simpler
> > and makes it much easier to specify tokens via http headers and enable
> > experiments in workers (follow up patches I've written).
> > So maybe it makes sense for me to remove the "is script generated" part from
> > this CL, so I can land this CL and not block the other changes, while we
figure
> > out what exactly the behavior is we want to achieve (and I totally agree
that
> > "block script generated meta tags" is rather ill defined and doesn't
necessarily
> > make sense).
> 
> Yeah sounds like we should treat those separately.  I need to run now but I
can take a look at this patch tomorrow morning (sorry I was trying to get the
big picture before digging into the code changes).

No problem. I removed the script generated related changes of this CL and
updated the description.

[Rick Byers, 2016-04-06 16:26:04]

LGTM, thanks!

Is the BUG= line still the best one to refer to? Doesn't matter to me, just
wasn't sure if you forgot to update it.

[Marijn Kruisselbrink, 2016-04-06 17:53:11]

Description was changed from

==========
Store the list of trial tokens in OriginTrialContext

Refactor the way tokens are found by storing them separately from the
document tree in OriginTrialContext.

This simplifies the architecture a bit, making it easier to:
- add support for tokens specified in http headers.
- add support for trials in workers.
- somehow limit which tokens are considered "valid".

BUG=588151
==========

to

==========
Store the list of trial tokens in OriginTrialContext

Refactor the way tokens are found by storing them separately from the
document tree in OriginTrialContext.

This simplifies the architecture a bit, making it easier to:
- add support for tokens specified in http headers.
- add support for trials in workers.
- somehow limit which tokens are considered "valid".

BUG=601108
==========

[Marijn Kruisselbrink, 2016-04-06 17:55:14]

On 2016/04/06 at 16:26:04, rbyers wrote:
> LGTM, thanks!
> 
> Is the BUG= line still the best one to refer to? Doesn't matter to me, just
wasn't sure if you forgot to update it.

Of the existing bugs it probably was the best one, but not a very good one. I
filed a new bug for this and marked it as blocking the other 5 bugs that become
easier because of this change.

[Marijn Kruisselbrink, 2016-04-06 17:56:04]

The CQ bit was checked by mek@chromium.org

[Marijn Kruisselbrink, 2016-04-06 17:56:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
iclelland@chromium.org
Link to the patchset: https://codereview.chromium.org/1833063002/#ps140001
(title: "remove script generated related changes from this CL")

[commit-bot: I haz the power, 2016-04-06 17:56:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1833063002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1833063002/140001

[commit-bot: I haz the power, 2016-04-06 18:03:07]

Committed patchset #6 (id:140001)

[commit-bot: I haz the power, 2016-04-06 18:04:04]

Description was changed from

==========
Store the list of trial tokens in OriginTrialContext

Refactor the way tokens are found by storing them separately from the
document tree in OriginTrialContext.

This simplifies the architecture a bit, making it easier to:
- add support for tokens specified in http headers.
- add support for trials in workers.
- somehow limit which tokens are considered "valid".

BUG=601108
==========

to

==========
Store the list of trial tokens in OriginTrialContext

Refactor the way tokens are found by storing them separately from the
document tree in OriginTrialContext.

This simplifies the architecture a bit, making it easier to:
- add support for tokens specified in http headers.
- add support for trials in workers.
- somehow limit which tokens are considered "valid".

BUG=601108

Committed: https://crrev.com/b03579678fa2678748d5cbcec2e18a8a08e40701
Cr-Commit-Position: refs/heads/master@{#385504}
==========

[commit-bot: I haz the power, 2016-04-06 18:04:05]

Patchset 6 (id:??) landed as
https://crrev.com/b03579678fa2678748d5cbcec2e18a8a08e40701
Cr-Commit-Position: refs/heads/master@{#385504}

[Nico, 2016-04-08 01:49:27]

A revert of this CL (patchset #6 id:140001) has been created in
https://codereview.chromium.org/1875593003/ by thakis@chromium.org.

The reason for reverting is: Broke release component builds on Windows.