Blocking synchronous and third party doc.written scripts

Refers to doc.written scripts blocking (https://docs.google.com/document/d/1AtS83d9joGj-xmt_pZB4Uvvn_9d1aEReRg0rI_5runw/edit)
This patch enhances the existing code to restrict the blocking of only synchronous and third-party scripts
BUG=599875
Committed: https://crrev.com/f907a5fb93cf183c0a3f9529ede284af8f06874c
Cr-Commit-Position: refs/heads/master@{#386219}

[shivanisha, 2016-04-01 13:42:56]

Description was changed from

==========
Refers to doc.written scripts blocking
(https://docs.google.com/document/d/1AtS83d9joGj-xmt_pZB4Uvvn_9d1aEReRg0rI_5ru...)
This patch enhances the earlier patch to restrict the blocking of only
synchronous and third-party scripts
BUG=
==========

to

==========
Refers to doc.written scripts blocking
(https://docs.google.com/document/d/1AtS83d9joGj-xmt_pZB4Uvvn_9d1aEReRg0rI_5ru...)
This patch enhances the existing code to restrict the blocking of only
synchronous and third-party scripts
BUG=
==========

[shivanisha, 2016-04-01 13:45:04]

shivanisha@chromium.org changed reviewers:
+ bmcquade@chromium.org, japhet@chromium.org, jkarlin@chromium.org

[shivanisha, 2016-04-01 14:19:01]

Description was changed from

==========
Refers to doc.written scripts blocking
(https://docs.google.com/document/d/1AtS83d9joGj-xmt_pZB4Uvvn_9d1aEReRg0rI_5ru...)
This patch enhances the existing code to restrict the blocking of only
synchronous and third-party scripts
BUG=
==========

to

==========
Refers to doc.written scripts blocking
(https://docs.google.com/document/d/1AtS83d9joGj-xmt_pZB4Uvvn_9d1aEReRg0rI_5ru...)
This patch enhances the existing code to restrict the blocking of only
synchronous and third-party scripts
BUG=599862
==========

[shivanisha, 2016-04-01 14:51:09]

Description was changed from

==========
Refers to doc.written scripts blocking
(https://docs.google.com/document/d/1AtS83d9joGj-xmt_pZB4Uvvn_9d1aEReRg0rI_5ru...)
This patch enhances the existing code to restrict the blocking of only
synchronous and third-party scripts
BUG=599862
==========

to

==========
Refers to doc.written scripts blocking
(https://docs.google.com/document/d/1AtS83d9joGj-xmt_pZB4Uvvn_9d1aEReRg0rI_5ru...)
This patch enhances the existing code to restrict the blocking of only
synchronous and third-party scripts
BUG=599875
==========

[shivanisha, 2016-04-01 15:03:32]

On 2016/04/01 at 13:45:33, shivanisha wrote:
> 

Tested this change for the following example URLs:

synchronous and third-party doc.written scripts:
http://www.google-analytics.com/ga_exp.js blocked from http://m.cricbuzz.com/ 
synchronous and same domain doc.written scripts: support.js not blocked from
https://www.maharashtra.gov.in/site/common/governmentresolutions.aspx 
asynchronous doc.written scripts: http://cdn.getblueshift.com/blueshift.js (used
for analytics) and http://d21gpk1vhmjuf5.cloudfront.net/unbxdAnalytics.js not
blocked from http://m.shopclues.com/

[jkarlin, 2016-04-01 15:23:36]

This is looking good. A few comments. I'd really like to see layout tests in
this CL showing that when the option is enabled and the various conditions are
met, that the resource isn't loaded, otherwise it is.

https://codereview.chromium.org/1849223002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/fetch/ResourceFetcher.h (right):

https://codereview.chromium.org/1849223002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fetch/ResourceFetcher.h:167: void
initializeResourceRequest(ResourceRequest&, Resource::Type,
FetchRequest::DeferOption = FetchRequest::NoDefer);
I'd remove the default NoDefer here since there are only two callers.

https://codereview.chromium.org/1849223002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/1849223002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:59: #include
"core/page/NetworkStateNotifier.h"
This isn't used yet.

https://codereview.chromium.org/1849223002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:213: const bool
isInDocumentWrite = m_document && m_document->isInDocumentWrite();
Please also restrict to 2G connection type in this CL. E.g.,
networkStateNotifier().connectionType == WebConnectionTypeCellular2G.

We can post another CL later to let Finch determine what network types should be
restricted, but for now we should land the thing which is easiest to start an
experiment on.

https://codereview.chromium.org/1849223002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:215: const bool
isSync = (defer == FetchRequest::NoDefer);
no need for parens

https://codereview.chromium.org/1849223002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:216: const bool
isThirdParty = (request.url().host() !=
m_document->getSecurityOrigin()->domain());
no need for parens

[shivanisha, 2016-04-01 16:15:14]

https://codereview.chromium.org/1849223002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/1849223002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:213: const bool
isInDocumentWrite = m_document && m_document->isInDocumentWrite();
On 2016/04/01 at 15:23:36, jkarlin wrote:
> Please also restrict to 2G connection type in this CL. E.g.,
networkStateNotifier().connectionType == WebConnectionTypeCellular2G.
> 
> We can post another CL later to let Finch determine what network types should
be restricted, but for now we should land the thing which is easiest to start an
experiment on.

Ok. Will update

https://codereview.chromium.org/1849223002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:215: const bool
isSync = (defer == FetchRequest::NoDefer);
On 2016/04/01 at 15:23:36, jkarlin wrote:
> no need for parens

will remove parens

https://codereview.chromium.org/1849223002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:216: const bool
isThirdParty = (request.url().host() !=
m_document->getSecurityOrigin()->domain());
On 2016/04/01 at 15:23:36, jkarlin wrote:
> no need for parens

will remove parens

https://codereview.chromium.org/1849223002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/FetchContext.h (right):

https://codereview.chromium.org/1849223002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/FetchContext.h:75: virtual
ResourceRequestCachePolicy resourceRequestCachePolicy(const ResourceRequest&,
Resource::Type, FetchRequest::DeferOption = FetchRequest::NoDefer) const;
I will be reverting this and will upload a new patch. As per coding guidelines:
virtual functions should not have default arguments
(https://google.github.io/styleguide/cppguide.html#Default_Arguments)

[shivanisha, 2016-04-04 19:18:55]

Added tests and review comments incorporated

https://codereview.chromium.org/1849223002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/fetch/ResourceFetcher.h (right):

https://codereview.chromium.org/1849223002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fetch/ResourceFetcher.h:167: void
initializeResourceRequest(ResourceRequest&, Resource::Type,
FetchRequest::DeferOption = FetchRequest::NoDefer);
On 2016/04/01 at 15:23:36, jkarlin wrote:
> I'd remove the default NoDefer here since there are only two callers.

done.

https://codereview.chromium.org/1849223002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/1849223002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:551:
initializeResourceRequest(revalidatingRequest, resource->getType(),
FetchRequest::NoDefer);
I am using FetchRequest::NoDefer as the value passed to DeferOption instead of
the incoming fetch request's defer option as this revalidatingRequest seems not
to take the incoming FetchRequest's properties.

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

Looks good, and thanks for adding the layout tests! I have a few suggestions for
the layout tests. I'm not familiar with layout test conventions, though, so if
what I'm suggesting is not consistent with other tests, it may be better to
follow convention. Josh is more knowledgable here so he can give guidance on the
right approach.

https://codereview.chromium.org/1849223002/diff/40001/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/1849223002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-block.html:5:
var t = async_test('Makes sure synchronous third-party scripts in document.write
are blocked');
Since these scripts are loaded synchronously you may not need to use an async
test either (though i dont know layout tests well enough to say for sure).

Also, looks like this description does not match the test (same for some other
tests). I'm not familiar with layout tests. is convention to use the same
description for all related tests, or to make the description of each test match
the thing it is testing?

https://codereview.chromium.org/1849223002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-block.html:13:
document.write('<scr' + 'ipt src="' + src + '"></scr' + 'ipt>');
though it makes the test a bit longer, i tend to like asserting both the
positive and negative case in the same test. that way, since both script loads
are running in the same test environment, we are confirming that the difference
in behavior is due to the origin difference, rather than due to e.g. an
accidental test misconfiguration that happens to produce the intended result.

so for example, we can load 2 scripts one after the other in this test, one same
origin and one cross origin, and verify that the same origin one succeeds while
the cross origin one fails.

going a step further, we should also be able to verify that the cross origin
script loads correctly before you configure the test environment, to make sure
it's the enabling of the window.internals settings that causes the difference in
behavior there (rather than e.g. the script not being served correctly when
requested on port 8000 which would also result in a failed load):


var jsLoaded = false;
document.write(...crossOriginScript...);
assert_true(jsLoaded, ...);

if (window.internals){
  ... configure test environment, enable cross origin script blocking ...
}

jsLoaded = false;
document.write(...crossOriginScript...);
assert_false(jsLoaded, ...);

jsLoaded = false;
document.write(...sameOriginScript...);
assert_true(jsLoaded, ...);

Testing these 3 conditions in the same test flow lets us be more confident that
it's the enabling of internals settings that causes cross origin scripts to get
blocked.

I have encountered many tests that pass for the wrong reasons, so I think it's
really valuable to validate both positive and negative cases in the same test
flow. If layout tests tend to not like this approach, and want to test each case
in isolation, we can go that route.

See also the comment below about a common helper function to load the script and
assert the expected results, which should be useful here.

Note that if caching is enabled, then the second load of the cross origin script
may succeed because it's in cache. i'm not sure if there's a way to disable
caching or not. if there is a way to control caching, it'd be good to have a
test that verifies that once a script is cached, it successfully loads even if
it's cross origin.

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

https://codereview.chromium.org/1849223002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-block1.html:17:
assert_false(jsLoaded, "third party and sync doc.written scripts are blocked");
For this and other tests, let's also assert that the DOM APIs report the
expected load success/failure status. Something like:

var loadSuccess=undefined;
document.write('<script src=... onload="loadSuccess=true"
onerror="loadSuccess=false"></scr'+'ipt>');
assert_true(typeof loadSuccess === 'boolean', 'loadSuccess script load status
not set after load');
assert_false(loadSuccess, 'third party and sync doc.written scripts are
blocked');

You may want to roll this all up into a helper js function that takes a script
URL and an expected success / failure result, e.g.

function assertExpectedScriptLoad(scriptUrl, expectedResult) {
  var jsLoaded = false;
  var loadSuccess = undefined;
  document.write('<script src="' + scriptUrl + 
                 '" onload="loadSuccess=true"
onerror="loadSuccess=false"></scr'+'ipt>');
  assert_true(typeof loadSuccess === 'boolean', 'loadSuccess script load status
not set after load');
  assert_true(loadSuccess === expectedResult, ...);
  if (expectedResult) {
    assert_true(jsLoaded, ...);
  }
}

you should be able to use this to validate behavior for all except the async
case, which will require an async test.

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

https://codereview.chromium.org/1849223002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:216: const bool
isThirdParty = (request.url().host() !=
m_document->getSecurityOrigin()->domain());
let's add a comment here noting that we continue to allow same origin scripts to
run because they may be used to render main page content, whereas we block
cross-origin scripts because they are likely for third party content.

i just want to make sure readers of the code understand that we aren't doing an
origin check for security reasons (might be worth noting that in the comment
too) which is the more common reason to compare origins.

https://codereview.chromium.org/1849223002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:217: const bool
isSlowConnection = networkStateNotifier().connectionType() ==
WebConnectionTypeCellular2G;
Let's add a comment that for now we restrict slow connections to 2G, but that we
might try to expand coverage using the network quality estimator in the future.

[shivanisha, 2016-04-05 20:37:14]

https://codereview.chromium.org/1849223002/diff/40001/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/1849223002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-block.html:5:
var t = async_test('Makes sure synchronous third-party scripts in document.write
are blocked');
On 2016/04/05 at 12:46:11, Bryan McQuade wrote:
> Since these scripts are loaded synchronously you may not need to use an async
test either (though i dont know layout tests well enough to say for sure).
> 
> Also, looks like this description does not match the test (same for some other
tests). I'm not familiar with layout tests. is convention to use the same
description for all related tests, or to make the description of each test match
the thing it is testing?

Sounds good. changing to sync.

https://codereview.chromium.org/1849223002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-block.html:13:
document.write('<scr' + 'ipt src="' + src + '"></scr' + 'ipt>');
On 2016/04/05 at 12:46:11, Bryan McQuade wrote:
> though it makes the test a bit longer, i tend to like asserting both the
positive and negative case in the same test. that way, since both script loads
are running in the same test environment, we are confirming that the difference
in behavior is due to the origin difference, rather than due to e.g. an
accidental test misconfiguration that happens to produce the intended result.
> 
> so for example, we can load 2 scripts one after the other in this test, one
same origin and one cross origin, and verify that the same origin one succeeds
while the cross origin one fails.
> 
> going a step further, we should also be able to verify that the cross origin
script loads correctly before you configure the test environment, to make sure
it's the enabling of the window.internals settings that causes the difference in
behavior there (rather than e.g. the script not being served correctly when
requested on port 8000 which would also result in a failed load):
> 
> 
> var jsLoaded = false;
> document.write(...crossOriginScript...);
> assert_true(jsLoaded, ...);
> 
> if (window.internals){
>   ... configure test environment, enable cross origin script blocking ...
> }
> 
> jsLoaded = false;
> document.write(...crossOriginScript...);
> assert_false(jsLoaded, ...);
> 
> jsLoaded = false;
> document.write(...sameOriginScript...);
> assert_true(jsLoaded, ...);
> 
> Testing these 3 conditions in the same test flow lets us be more confident
that it's the enabling of internals settings that causes cross origin scripts to
get blocked.
> 
> I have encountered many tests that pass for the wrong reasons, so I think it's
really valuable to validate both positive and negative cases in the same test
flow. If layout tests tend to not like this approach, and want to test each case
in isolation, we can go that route.
> 
> See also the comment below about a common helper function to load the script
and assert the expected results, which should be useful here.
> 
> Note that if caching is enabled, then the second load of the cross origin
script may succeed because it's in cache. i'm not sure if there's a way to
disable caching or not. if there is a way to control caching, it'd be good to
have a test that verifies that once a script is cached, it successfully loads
even if it's cross origin.

sounds good. Added 4 tests in 1 html file:

cross origin script gets loaded before feature is enabled
feature enabled
same domain script not blocked
same cross origin script not blocked as it was cached
different cross origin script blocked

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

https://codereview.chromium.org/1849223002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-block1.html:17:
assert_false(jsLoaded, "third party and sync doc.written scripts are blocked");
It seems asserts provided by testharness.js only work well within the test
function so I am not able to use this modular function approach.

Also, I observed this works:
<script>
 var jsLoaded = false;
 document.write (script)
</script>
And then in another script tag, asserting the values of jsLoaded and loadSuccess
in the test function work as expected but if I put the test function in the
above script tag, they do not reflect the expected values of the variables after
script load. Not sure why this happens but for the time being keeping separate
script tags.

[Bryan McQuade, 2016-04-06 12:37:21]

Great, thanks for this! Change basically looks good to me - just a few small
things.

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

https://codereview.chromium.org/1849223002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-block1.html:6:
var loadsuccess = false;
for consistency let's name this loadSuccess

https://codereview.chromium.org/1849223002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-block1.html:57:
src = 'http://localhost:8000/loading/resources/js-loaded1.js';
great, thanks! this is fine, but you might be able to re-use js-loaded.js by
appending a query string, which gives it a different cache key but for most web
servers will still cause it to serve the same resource (though not sure if
that's the case for the layout test serving system). that'd let us avoid having
to add a second js resource as part of this change.

so for example:
src = 'http://localhost:8000/loading/resources/js-loaded.js?1';

give that a try, and if it lets you load js-loaded.js while bypassing cache,
that seems preferrable to adding an additional js resource. (if it doesn't work,
then let's keep js-loaded1.js as you have it).

https://codereview.chromium.org/1849223002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-block1.html:58:
document.write('<scr' + 'ipt src="' + src + '" onload="loadsuccess=true"></scr'
+ 'ipt>');
can we also add an onError="loadFailed=true" and assert that loadFailed gets set
to true?

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

https://codereview.chromium.org/1849223002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-block3.html:13:
document.write('<scr' + 'ipt src="' + src + '" onload="loadsuccess=true"></scr'
+ 'ipt>');
looks like we have loadsuccess here but we are setting loadSuccess above. let's
use loadSuccess everywhere.

[shivanisha, 2016-04-06 14:18:37]

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

https://codereview.chromium.org/1849223002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-block1.html:6:
var loadsuccess = false;
On 2016/04/06 at 12:37:21, Bryan McQuade wrote:
> for consistency let's name this loadSuccess

done

https://codereview.chromium.org/1849223002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-block1.html:57:
src = 'http://localhost:8000/loading/resources/js-loaded1.js';
Yes, this works. Removing js-loaded1.js

https://codereview.chromium.org/1849223002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-block1.html:58:
document.write('<scr' + 'ipt src="' + src + '" onload="loadsuccess=true"></scr'
+ 'ipt>');
On 2016/04/06 at 12:37:21, Bryan McQuade wrote:
> can we also add an onError="loadFailed=true" and assert that loadFailed gets
set to true?

Added

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

https://codereview.chromium.org/1849223002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-block3.html:13:
document.write('<scr' + 'ipt src="' + src + '" onload="loadsuccess=true"></scr'
+ 'ipt>');
On 2016/04/06 at 12:37:21, Bryan McQuade wrote:
> looks like we have loadsuccess here but we are setting loadSuccess above.
let's use loadSuccess everywhere.

Whoops. Made it loadSuccess in all places.

[Bryan McQuade, 2016-04-06 14:49:29]

Great, lgtm, thanks for this change! This still needs review from Nate and Josh.

[jkarlin, 2016-04-06 14:55:32]

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

https://codereview.chromium.org/1849223002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-block1.html:1:
<!DOCTYPE html>
instead of ...-block1.html, how about ...-block-third-party.html so it's clear
which tests should go in this file

https://codereview.chromium.org/1849223002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-block1.html:42:
src = 'http://localhost:8000/loading/resources/js-loaded.js';
It's subtle that this is a different origin. Can you either add a comment, or
better yet make two vars:

var sameOrigin = http://127.0.0.1:8000
var crossOrigin = http://localhost:8000 

and use those when composing the URLs?

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

https://codereview.chromium.org/1849223002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-block2.html:1:
<!DOCTYPE html>
instead of ...-block2.html, how about ...-block-sync.html so it's clear which
tests should go in this file

https://codereview.chromium.org/1849223002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-block3-expected.txt
(right):

https://codereview.chromium.org/1849223002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-block3-expected.txt:8:
Harness: the test ran to completion.
I *think* -expected files don't need to be uploaded if all of the tests pass.
They should only be required when a subset of tests fail.

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

https://codereview.chromium.org/1849223002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-block3.html:5:
var src = 'http://localhost:8000/loading/resources/js-loaded.js';
instead of ...-block3.html, how about ...-block-network.html so it's clear which
tests should go in this file

https://codereview.chromium.org/1849223002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/1849223002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:551:
initializeResourceRequest(revalidatingRequest, resource->getType(),
FetchRequest::NoDefer);
Why is this NoDefer? Shouldn't this be request.defer()?

https://codereview.chromium.org/1849223002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameFetchContext.h (right):

https://codereview.chromium.org/1849223002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.h:73:
ResourceRequestCachePolicy resourceRequestCachePolicy(const ResourceRequest&,
Resource::Type, FetchRequest::DeferOption = FetchRequest::NoDefer) const
override;
This is a virtual function so it shouldn't use default values.

https://codereview.chromium.org/1849223002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/testing/InternalSettings.idl (right):

https://codereview.chromium.org/1849223002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/testing/InternalSettings.idl:68: void
setDisallowFetchForDocWrittenScriptsInMainFrame(boolean enabled);
I don't think you need to add this, it looks like it's already been
auto-generated, see
https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/gen/blin...

[shivanisha, 2016-04-06 20:40:10]

Incorporated Josh's feedback.

I am looking into why blocking of sync third-party assert has stopped passing
after I have applied this patch on latest from master branch and added the
latest feedback. Gdb showed the cache policy getting set correctly but still the
script is getting loaded successfully in the layout test. 

Will probably upload another patch when I know the root cause for this. Also
will revert InternalSettings.idl as per Josh's comment that the setter function
is already created in InternalSettingsGenerated.idl.

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

https://codereview.chromium.org/1849223002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-block1.html:1:
<!DOCTYPE html>
sounds good. Will rename the three different test files as:
- doc-write-sync-third-party-script-block.html
- doc-write-async-third-party-script.html
- doc-write-sync-third-party-script-conn-type.html

https://codereview.chromium.org/1849223002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-block1.html:42:
src = 'http://localhost:8000/loading/resources/js-loaded.js';
sounds good

https://codereview.chromium.org/1849223002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-block3-expected.txt
(right):

https://codereview.chromium.org/1849223002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-block3-expected.txt:8:
Harness: the test ran to completion.
Because of the output messages due to it being in loading/ path we need the
expected file.
https://code.google.com/p/chromium/codesearch#chromium/src/components/test_ru...

https://codereview.chromium.org/1849223002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/1849223002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:551:
initializeResourceRequest(revalidatingRequest, resource->getType(),
FetchRequest::NoDefer);
Updating. I initially thought it is a fresh request but as we discussed its the
same request with some revalidating headers.

https://codereview.chromium.org/1849223002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameFetchContext.h (right):

https://codereview.chromium.org/1849223002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.h:73:
ResourceRequestCachePolicy resourceRequestCachePolicy(const ResourceRequest&,
Resource::Type, FetchRequest::DeferOption = FetchRequest::NoDefer) const
override;
Ah, I missed that even overrides of virtual function should not use default
arguments.

[Bryan McQuade, 2016-04-06 20:46:37]

On 2016/04/06 at 20:40:10, shivanisha wrote:
> Incorporated Josh's feedback.
> 
> I am looking into why blocking of sync third-party assert has stopped passing
after I have applied this patch on latest from master branch and added the
latest feedback. Gdb showed the cache policy getting set correctly but still the
script is getting loaded successfully in the layout test. 
> 
> Will probably upload another patch when I know the root cause for this. Also
will revert InternalSettings.idl as per Josh's comment that the setter function
is already created in InternalSettingsGenerated.idl.
> 
>
https://codereview.chromium.org/1849223002/diff/60001/third_party/WebKit/Layo...
> File
third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-block1.html
(right):
> 
>
https://codereview.chromium.org/1849223002/diff/60001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-block1.html:1:
<!DOCTYPE html>
> sounds good. Will rename the three different test files as:
> - doc-write-sync-third-party-script-block.html
> - doc-write-async-third-party-script.html
> - doc-write-sync-third-party-script-conn-type.html
> 
>
https://codereview.chromium.org/1849223002/diff/60001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-block1.html:42:
src = 'http://localhost:8000/loading/resources/js-loaded.js';
> sounds good
> 
>
https://codereview.chromium.org/1849223002/diff/60001/third_party/WebKit/Layo...
> File
third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-block3-expected.txt
(right):
> 
>
https://codereview.chromium.org/1849223002/diff/60001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-block3-expected.txt:8:
Harness: the test ran to completion.
> Because of the output messages due to it being in loading/ path we need the
expected file.
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/test_ru...
> 
>
https://codereview.chromium.org/1849223002/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):
> 
>
https://codereview.chromium.org/1849223002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:551:
initializeResourceRequest(revalidatingRequest, resource->getType(),
FetchRequest::NoDefer);
> Updating. I initially thought it is a fresh request but as we discussed its
the same request with some revalidating headers.
> 
>
https://codereview.chromium.org/1849223002/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/loader/FrameFetchContext.h (right):
> 
>
https://codereview.chromium.org/1849223002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/loader/FrameFetchContext.h:73:
ResourceRequestCachePolicy resourceRequestCachePolicy(const ResourceRequest&,
Resource::Type, FetchRequest::DeferOption = FetchRequest::NoDefer) const
override;
> Ah, I missed that even overrides of virtual function should not use default
arguments.

It may be that the change in test names causes them to execute in a different
order, and this causes a resource that should be blocked to be in the browser
cache. Not sure, but might be worth appending query string params to the URLs in
each test to see if that helps.

[shivanisha, 2016-04-06 20:52:52]

I am running the test doc-write-sync-third-party-script-block.html in isolation.

python Tools/Scripts/run-webkit-tests -t Default
LayoutTests/http/tests/loading/doc-write-sync-third-party-script-block.html

hopefully the cache should be clear when I start the run-webkit-tests script.

[Nate Chapin, 2016-04-07 16:25:48]

lgtm

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

https://codereview.chromium.org/1849223002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:209: const bool
disallowFetchForDocWriteScripts = frame()->settings() &&
frame()->settings()->disallowFetchForDocWrittenScriptsInMainFrame();
Why did this move?

[shivanisha, 2016-04-08 14:54:32]

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

https://codereview.chromium.org/1849223002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:209: const bool
disallowFetchForDocWriteScripts = frame()->settings() &&
frame()->settings()->disallowFetchForDocWrittenScriptsInMainFrame();
On 2016/04/07 at 16:25:48, Nate Chapin wrote:
> Why did this move?

Moving it back in order to have less diffs

[jkarlin, 2016-04-08 17:17:59]

lgtm with nits

https://codereview.chromium.org/1849223002/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/loading/doc-write-async-third-party-script.html
(right):

https://codereview.chromium.org/1849223002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/loading/doc-write-async-third-party-script.html:9:
if (window.internals){
space between ) and {

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

https://codereview.chromium.org/1849223002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-block.html:27:
internals.evictAllResources();
I don't think you meant to include this.

https://codereview.chromium.org/1849223002/diff/120001/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/1849223002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-conn-type.html:8:
if (window.internals){
space between ) and {

[shivanisha, 2016-04-08 18:41:11]

https://codereview.chromium.org/1849223002/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/loading/doc-write-async-third-party-script.html
(right):

https://codereview.chromium.org/1849223002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/loading/doc-write-async-third-party-script.html:9:
if (window.internals){
On 2016/04/08 at 17:17:59, jkarlin wrote:
> space between ) and {

Added

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

https://codereview.chromium.org/1849223002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-block.html:27:
internals.evictAllResources();
On 2016/04/08 at 17:17:59, jkarlin wrote:
> I don't think you meant to include this.

I am including this to make sure cached resources don't get served in any of the
subsequent requests. Also adding in other tests for the same reason.

https://codereview.chromium.org/1849223002/diff/120001/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/1849223002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-conn-type.html:8:
if (window.internals){
On 2016/04/08 at 17:17:59, jkarlin wrote:
> space between ) and {

added

[jkarlin, 2016-04-08 18:44:27]

slgtm

[shivanisha, 2016-04-08 19:44:31]

On 2016/04/08 at 18:44:27, jkarlin wrote:
> slgtm

Thanks!

Waiting for https://codereview.chromium.org/1863413005 to land before committing
this change. Since Charles's preload scanner was a REF with status set to test
it was enabled for layout tests. Thus my layout test blocking a script was
having a race with the preload scanner caching it. Charles's above CL will
change the REF from test to none and thus it will not be enabled by default for
layout tests.

[shivanisha, 2016-04-08 20:06:28]

The CQ bit was checked by shivanisha@chromium.org

[shivanisha, 2016-04-08 20:06:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmcquade@chromium.org, japhet@chromium.org, jkarlin@chromium.org
Link to the patchset: https://codereview.chromium.org/1849223002/#ps140001
(title: "formatting feedback on last patch, evictAllResources added in all
tests")

[commit-bot: I haz the power, 2016-04-08 20:06:43]

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

[commit-bot: I haz the power, 2016-04-08 20:08:42]

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

[commit-bot: I haz the power, 2016-04-08 20:08:43]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[shivanisha, 2016-04-08 21:05:15]

The CQ bit was checked by shivanisha@chromium.org

[shivanisha, 2016-04-08 21:05:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmcquade@chromium.org, japhet@chromium.org, jkarlin@chromium.org
Link to the patchset: https://codereview.chromium.org/1849223002/#ps160001
(title: "Resolved conflicts with latest")

[commit-bot: I haz the power, 2016-04-08 21:05:37]

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

[commit-bot: I haz the power, 2016-04-08 22:08:13]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-04-08 22:10:02]

Description was changed from

==========
Refers to doc.written scripts blocking
(https://docs.google.com/document/d/1AtS83d9joGj-xmt_pZB4Uvvn_9d1aEReRg0rI_5ru...)
This patch enhances the existing code to restrict the blocking of only
synchronous and third-party scripts
BUG=599875
==========

to

==========
Refers to doc.written scripts blocking
(https://docs.google.com/document/d/1AtS83d9joGj-xmt_pZB4Uvvn_9d1aEReRg0rI_5ru...)
This patch enhances the existing code to restrict the blocking of only
synchronous and third-party scripts
BUG=599875

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

[commit-bot: I haz the power, 2016-04-08 22:10:06]

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