Add about:flags support for doc.write script blocking.

Add about:flags support for doc.write script blocking.

This patch introduces an additional blink setting which allows us to enable
blocking only on slow (2G) connections, or on all connections.

This is consistent with the 2G web font intervention, which has a feature
for enabling on 2G, and an additional feature to force the intervention on
in all connection types.

We then add support for enabling the setting that enables blocking on all
connection types from about:flags.

This change also removes about:flags support from the now discontinued low
priority iframe experiment, since it's nontrivial to add support for
multiple blink settings in about:flags.

BUG=602137
Committed: https://crrev.com/ed75dc7053dc072bdd0e72f64a869d59ec587f4b
Cr-Commit-Position: refs/heads/master@{#386769}

[Bryan McQuade, 2016-04-11 01:18:18]

Description was changed from

==========
Add blink Setting to force script blocking regardless of connection.

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

to

==========
Add about:flags support for doc.write script blocking.

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

[Bryan McQuade, 2016-04-11 01:23:12]

Description was changed from

==========
Add about:flags support for doc.write script blocking.

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

to

==========
Add about:flags support for doc.write script blocking.

This patch introduces an additional blink setting which allows us to enable
blocking only on slow (2G) connections, or on all connections.

We then add support for enabling the setting that enables blocking on all
connection types from about:flags.

This change also removes about:flags support from the now discontinued low
priority iframe experiment.

BUG=602137
==========

[Bryan McQuade, 2016-04-11 12:33:23]

Description was changed from

==========
Add about:flags support for doc.write script blocking.

This patch introduces an additional blink setting which allows us to enable
blocking only on slow (2G) connections, or on all connections.

We then add support for enabling the setting that enables blocking on all
connection types from about:flags.

This change also removes about:flags support from the now discontinued low
priority iframe experiment.

BUG=602137
==========

to

==========
Add about:flags support for doc.write script blocking.

This patch introduces an additional blink setting which allows us to enable
blocking only on slow (2G) connections, or on all connections.

This is consistent with the 2G web font intervention, which has a feature
for enabling on 2G, and an additional feature to force the intervention on
in all connection types.

We then add support for enabling the setting that enables blocking on all
connection types from about:flags.

This change also removes about:flags support from the now discontinued low
priority iframe experiment, since it's nontrivial to add support for
multiple blink settings in about:flags.

BUG=602137
==========

[Bryan McQuade, 2016-04-11 12:56:05]

bmcquade@chromium.org changed reviewers:
+ csharrison@chromium.org, japhet@chromium.org

[Bryan McQuade, 2016-04-11 12:56:49]

bmcquade@chromium.org changed reviewers:
+ shivanisha@chromium.org
- csharrison@chromium.org

[Bryan McQuade, 2016-04-11 12:57:27]

shivanisha, PTAL for general review
japhet, PTAL for Settings.in and FrameFetchContext changes
jkarlin and csharrison, please feel free to review as well

[kinuko, 2016-04-11 13:02:27]

(Drive-by)

https://codereview.chromium.org/1868413002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/1868413002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:80: bool
ShouldDisallowFetchForMainFrameScript(const ResourceRequest& request,
FetchRequest::DeferOption defer, Document& document)
Drive-by nit: please not indent in namespace

https://codereview.chromium.org/1868413002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:101: // only
synchronously loaded scripts should be blocked
nit: only -> Only, and end comment with period

[Bryan McQuade, 2016-04-11 13:15:24]

Thanks!

https://codereview.chromium.org/1868413002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/1868413002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:80: bool
ShouldDisallowFetchForMainFrameScript(const ResourceRequest& request,
FetchRequest::DeferOption defer, Document& document)
On 2016/04/11 at 13:02:27, kinuko wrote:
> Drive-by nit: please not indent in namespace

for some reason git cl format likes to add the spaces. removed.

https://codereview.chromium.org/1868413002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:101: // only
synchronously loaded scripts should be blocked
On 2016/04/11 at 13:02:27, kinuko wrote:
> nit: only -> Only, and end comment with period

Done.

[Bryan McQuade, 2016-04-11 13:42:01]

bmcquade@chromium.org changed reviewers:
+ kinuko@chromium.org
- japhet@chromium.org

[Bryan McQuade, 2016-04-11 13:42:32]

bmcquade@chromium.org changed reviewers:
+ japhet@chromium.org

[Bryan McQuade, 2016-04-11 13:43:58]

Adding Kinuko as additional reviewer for blink changes.

japhet or kinuko please feel free to review blink side.

Thanks,
Bryan

[shivanisha, 2016-04-11 14:26:56]

https://codereview.chromium.org/1868413002/diff/100001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1868413002/diff/100001/chrome/app/generated_r...
chrome/app/generated_resources.grd:6550: +        Disallows fetches for scripts
inserted into the main frame via document.write.
How about the description involving the conditions of blocked scripts as well,
such as:
"Disallows fetches for cross-origin, synchronous scripts inserted into the main
frame via document.write"

https://codereview.chromium.org/1868413002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-conn-type.html
(right):

https://codereview.chromium.org/1868413002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-conn-type.html:18:
}
May be add another test case in this file which sets the flag independent of
connection type to be true and then blocking should happen for a script in
'cellular3g' as well.

https://codereview.chromium.org/1868413002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/1868413002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:80: bool
ShouldDisallowFetchForMainFrameScript(const ResourceRequest& request,
FetchRequest::DeferOption defer, Document& document)
const Document&

https://codereview.chromium.org/1868413002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:99: const bool
disallowFetch =
document.settings()->disallowFetchForDocWrittenScriptsInMainFrame() ||
(document.settings()->disallowFetchForDocWrittenScriptsInMainFrameOnSlowConnections()
&& isSlowConnection);
do frame()->settings() and document.settings() always evaluate to same?

[jkarlin, 2016-04-11 14:52:26]

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

[jkarlin, 2016-04-11 14:52:27]

https://codereview.chromium.org/1868413002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-block.html
(right):

https://codereview.chromium.org/1868413002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-block.html:62:

why this newline?

https://codereview.chromium.org/1868413002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/1868413002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:80: bool
ShouldDisallowFetchForMainFrameScript(const ResourceRequest& request,
FetchRequest::DeferOption defer, Document& document)
const Document& ? 

It looks like isInDocumentWrite needs to be made const.

https://codereview.chromium.org/1868413002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:94: // be expanded
using the network quality estimator.
Not a sentence, perhaps s/Only Blocking in/Block in/

I'd remove the NQE speculation from the comment.

https://codereview.chromium.org/1868413002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:106: // Not blocking
same origin scripts as they may be used to render main page
Not a sentence, perhaps s/Not blocking/Don't block/

[Bryan McQuade, 2016-04-11 16:28:30]

Thanks! Addressed comments. PTAL.

https://codereview.chromium.org/1868413002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-block.html
(right):

https://codereview.chromium.org/1868413002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-block.html:62:

On 2016/04/11 at 14:52:27, jkarlin wrote:
> why this newline?

Oh, I didn't add a newline - just removed four whitespaces from the existing
line. But I agree it isn't needed - I've now removed the newline as part of the
change.

https://codereview.chromium.org/1868413002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/1868413002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:80: bool
ShouldDisallowFetchForMainFrameScript(const ResourceRequest& request,
FetchRequest::DeferOption defer, Document& document)
On 2016/04/11 at 14:52:27, jkarlin wrote:
> const Document& ? 
> 
> It looks like isInDocumentWrite needs to be made const.

Done

https://codereview.chromium.org/1868413002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:94: // be expanded
using the network quality estimator.
On 2016/04/11 at 14:52:27, jkarlin wrote:
> Not a sentence, perhaps s/Only Blocking in/Block in/
> 
> I'd remove the NQE speculation from the comment.

Good call. Once I removed this part of the comment, I decided it was appropriate
to remove the entire comment, since it doesn't convey anything beyond what's
captured in the code below. Japhet encouraged me to try to remove obvious
comments in a prior review. Let me know if you think the comment should be
restored.

https://codereview.chromium.org/1868413002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:106: // Not blocking
same origin scripts as they may be used to render main page
On 2016/04/11 at 14:52:27, jkarlin wrote:
> Not a sentence, perhaps s/Not blocking/Don't block/

Done

https://codereview.chromium.org/1868413002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:106: // Not blocking
same origin scripts as they may be used to render main page
On 2016/04/11 at 14:52:27, jkarlin wrote:
> Not a sentence, perhaps s/Not blocking/Don't block/

Done

https://codereview.chromium.org/1868413002/diff/100001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1868413002/diff/100001/chrome/app/generated_r...
chrome/app/generated_resources.grd:6550: +        Disallows fetches for scripts
inserted into the main frame via document.write.
On 2016/04/11 at 14:26:56, shivanisha wrote:
> How about the description involving the conditions of blocked scripts as well,
such as:
> "Disallows fetches for cross-origin, synchronous scripts inserted into the
main frame via document.write"

Good idea. Done. I used 'third-party' rather than 'cross-origin' since more
users will understand that. I also used 'parser-blocking' rather than
synchronous.

https://codereview.chromium.org/1868413002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-conn-type.html
(right):

https://codereview.chromium.org/1868413002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-conn-type.html:18:
}
On 2016/04/11 at 14:26:56, shivanisha wrote:
> May be add another test case in this file which sets the flag independent of
connection type to be true and then blocking should happen for a script in
'cellular3g' as well.

Sure. I decided to add a new test file for this one since I felt it was a little
hairy to be changing internals settings multiple times through a single test
file. Hope that is ok. Let me know what you think.

https://codereview.chromium.org/1868413002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/1868413002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:80: bool
ShouldDisallowFetchForMainFrameScript(const ResourceRequest& request,
FetchRequest::DeferOption defer, Document& document)
On 2016/04/11 at 14:26:56, shivanisha wrote:
> const Document&

Done. I had to make Document::isInDocumentWrite a const method to enable this
(seems like a good change).

https://codereview.chromium.org/1868413002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:99: const bool
disallowFetch =
document.settings()->disallowFetchForDocWrittenScriptsInMainFrame() ||
(document.settings()->disallowFetchForDocWrittenScriptsInMainFrameOnSlowConnections()
&& isSlowConnection);
On 2016/04/11 at 14:26:56, shivanisha wrote:
> do frame()->settings() and document.settings() always evaluate to same?

Yeah - Document::settings is implemented as:
Settings* Document::settings() const
{
    return m_frame ? m_frame->settings() : 0;
}

So assuming the frame we have is the same frame that the document has (would be
a bug if not) they should return the same value.

[shivanisha, 2016-04-11 16:57:56]

https://codereview.chromium.org/1868413002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-conn-type.html
(right):

https://codereview.chromium.org/1868413002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-conn-type.html:18:
}
I am good either ways. 
Since the test is synchronous, changing internal settings between two tests in
the same file should be fine, though.

[Bryan McQuade, 2016-04-11 18:15:41]

https://codereview.chromium.org/1868413002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-conn-type.html
(right):

https://codereview.chromium.org/1868413002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-conn-type.html:18:
}
On 2016/04/11 at 16:57:56, shivanisha wrote:
> I am good either ways. 
> Since the test is synchronous, changing internal settings between two tests in
the same file should be fine, though.

I think I like this slightly better since it's easier to reason about test set
up and tear down as happening once at beginning and end of the test life cycle.
I can switch it if you prefer. Let me know. Thanks!

[Bryan McQuade, 2016-04-11 18:44:04]

On 2016/04/11 at 18:15:41, Bryan McQuade wrote:
>
https://codereview.chromium.org/1868413002/diff/100001/third_party/WebKit/Lay...
> File
third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-conn-type.html
(right):
> 
>
https://codereview.chromium.org/1868413002/diff/100001/third_party/WebKit/Lay...
>
third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-conn-type.html:18:
}
> On 2016/04/11 at 16:57:56, shivanisha wrote:
> > I am good either ways. 
> > Since the test is synchronous, changing internal settings between two tests
in the same file should be fine, though.
> 
> I think I like this slightly better since it's easier to reason about test set
up and tear down as happening once at beginning and end of the test life cycle.
I can switch it if you prefer. Let me know. Thanks!

Made a slight improvement to the new test - we first verify that the script
loads successfully, enable the internals setting, then verify that the script
load is blocked. PTAL.

[shivanisha, 2016-04-11 19:01:07]

https://codereview.chromium.org/1868413002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-conn-type.html
(right):

https://codereview.chromium.org/1868413002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-conn-type.html:18:
}
On 2016/04/11 at 18:15:40, Bryan McQuade wrote:
> On 2016/04/11 at 16:57:56, shivanisha wrote:
> > I am good either ways. 
> > Since the test is synchronous, changing internal settings between two tests
in the same file should be fine, though.
> 
> I think I like this slightly better since it's easier to reason about test set
up and tear down as happening once at beginning and end of the test life cycle.
I can switch it if you prefer. Let me know. Thanks!

Sounds good to me to have separate test files. Thanks!

[jkarlin, 2016-04-12 13:36:36]

lgtm!

[shivanisha, 2016-04-12 13:47:36]

On 2016/04/12 at 13:36:36, jkarlin wrote:
> lgtm!

lgtm

[Nate Chapin, 2016-04-12 16:53:15]

LGTM w/nits

https://codereview.chromium.org/1868413002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/1868413002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:78: namespace {
Why this namespace?

https://codereview.chromium.org/1868413002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:80: bool
ShouldDisallowFetchForMainFrameScript(const ResourceRequest& request,
FetchRequest::DeferOption defer, const Document& document)
Nit: shouldDisallowFetchForMainFrameScript

https://codereview.chromium.org/1868413002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:84: if
(!document.isInDocumentWrite()) {
Here and below, no {} for ifs with single-line bodies.

[Bryan McQuade, 2016-04-12 17:28:38]

Thanks! Comments addressed.

https://codereview.chromium.org/1868413002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/1868413002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:78: namespace {
On 2016/04/12 at 16:53:15, Nate Chapin wrote:
> Why this namespace?

Oh, I'm accustomed to using an anonymous namespace for functions in the C++
file, per the C++ style guide
https://google.github.io/styleguide/cppguide.html#Namespaces

I do see some code in blink that does this as well.

I can remove the anonymous namespace if you prefer - let me know.

https://codereview.chromium.org/1868413002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:80: bool
ShouldDisallowFetchForMainFrameScript(const ResourceRequest& request,
FetchRequest::DeferOption defer, const Document& document)
On 2016/04/12 at 16:53:15, Nate Chapin wrote:
> Nit: shouldDisallowFetchForMainFrameScript

Done

https://codereview.chromium.org/1868413002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:84: if
(!document.isInDocumentWrite()) {
On 2016/04/12 at 16:53:15, Nate Chapin wrote:
> Here and below, no {} for ifs with single-line bodies.

Done

[Bryan McQuade, 2016-04-12 19:21:12]

The CQ bit was checked by bmcquade@chromium.org

[Bryan McQuade, 2016-04-12 19:21:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
shivanisha@chromium.org, jkarlin@chromium.org, japhet@chromium.org
Link to the patchset: https://codereview.chromium.org/1868413002/#ps220001
(title: "address comments")

[commit-bot: I haz the power, 2016-04-12 19:21:43]

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

[commit-bot: I haz the power, 2016-04-12 19:41:34]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2016-04-12 19:42:51]

Description was changed from

==========
Add about:flags support for doc.write script blocking.

This patch introduces an additional blink setting which allows us to enable
blocking only on slow (2G) connections, or on all connections.

This is consistent with the 2G web font intervention, which has a feature
for enabling on 2G, and an additional feature to force the intervention on
in all connection types.

We then add support for enabling the setting that enables blocking on all
connection types from about:flags.

This change also removes about:flags support from the now discontinued low
priority iframe experiment, since it's nontrivial to add support for
multiple blink settings in about:flags.

BUG=602137
==========

to

==========
Add about:flags support for doc.write script blocking.

This patch introduces an additional blink setting which allows us to enable
blocking only on slow (2G) connections, or on all connections.

This is consistent with the 2G web font intervention, which has a feature
for enabling on 2G, and an additional feature to force the intervention on
in all connection types.

We then add support for enabling the setting that enables blocking on all
connection types from about:flags.

This change also removes about:flags support from the now discontinued low
priority iframe experiment, since it's nontrivial to add support for
multiple blink settings in about:flags.

BUG=602137

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

[commit-bot: I haz the power, 2016-04-12 19:42:53]

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