Sending an async GET request for doc.written blocked scripts. (issue 2260303002 by shivanisha@chromium.org)

[shiva...@chromium.org]

Reviewers: Charlie Harrison, Nate Chapin
CL: https://codereview.chromium.org/2260303002/

Description:
Sending an async GET request for doc.written blocked scripts.
BUG=639401

While fetching the original request, ScriptLoader sets
m_disallowedFetchForDocWrittenScript.
On receiving notifyFinished for the original request, HTMLScriptRunner creates a
new
ScriptLoader instance using the original Element with
m_blockedDocWriteScriptAsyncFetch set to true. It then calls
ScriptLoader::prepareScript which further calls fetchScript.
prepareScript sets the defer flag as LazyLoad and fetchScript sets the
intervention info and header in the resource request. The intervention info is
used to set the priority in ResourceFetcher::requestResource.

This CL also adds a link in the dev tools console warning.

The code was tested using netlog to verify the second request is sent and has
the correct priority (net::IDLE).
One of the example sites created by jkarlin@ is:
http://www.karlin.me/test/docwrite/

The rest of this intervention has been tested with layout tests. I did not find
a way to test this part, with layout tests. If you have thoughts on how to add
tests for this change, please let me know.

Base URL: https://chromium.googlesource.com/chromium/src.git@master

Affected files (+133, -31 lines):
M third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-block-all-conn-types-expected.txt
M third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-block-expected.txt
M third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-conn-type-expected.txt
M third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-reload-expected.txt
M third_party/WebKit/Source/core/dom/ScriptLoader.h
M third_party/WebKit/Source/core/dom/ScriptLoader.cpp
M third_party/WebKit/Source/core/dom/ScriptRunnerTest.cpp
M third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp
M third_party/WebKit/Source/core/html/parser/HTMLScriptRunner.cpp
M third_party/WebKit/Source/core/loader/FrameFetchContext.cpp
M third_party/WebKit/Source/platform/network/ResourceRequest.h
M third_party/WebKit/Source/platform/network/ResourceRequest.cpp

[shiva...@chromium.org]

On 2016/08/19 at 20:50:23, shivanisha wrote:
>

PTAL, Thanks!

https://codereview.chromium.org/2260303002/

[cshar...@chromium.org]

I'll let Nate sign off on the design portion. I left a few comments inline.

My main concern would be overloading the ScriptLoader instead of creating a new
class that initiates the fetch Are you sure you need all the logic in
ScriptLoader for this? I'm nervous that it will lead to unintended bugs.


https://codereview.chromium.org/2260303002/diff/60001/third_party/WebKit/Source/core/dom/ScriptLoader.h
File third_party/WebKit/Source/core/dom/ScriptLoader.h (right):

https://codereview.chromium.org/2260303002/diff/60001/third_party/WebKit/Source/core/dom/ScriptLoader.h#newcode125
third_party/WebKit/Source/core/dom/ScriptLoader.h:125: bool
m_disallowedFetchForDocWrittenScript : 1; // This script will be blocked
if not present in http cache.
Can these be combined into an enum flag?

https://codereview.chromium.org/2260303002/diff/60001/third_party/WebKit/Source/core/dom/ScriptLoader.h#newcode126
third_party/WebKit/Source/core/dom/ScriptLoader.h:126: const bool
m_blockedDocWriteScriptAsyncFetch : 1; // Non parser-blocking lowest
priority fetch for the blocked script.
The comment here doesn't really specify what the "true" or "false"
states correspond to.

https://codereview.chromium.org/2260303002/diff/60001/third_party/WebKit/Source/core/html/parser/HTMLScriptRunner.cpp
File third_party/WebKit/Source/core/html/parser/HTMLScriptRunner.cpp
(right):

https://codereview.chromium.org/2260303002/diff/60001/third_party/WebKit/Source/core/html/parser/HTMLScriptRunner.cpp#newcode277
third_party/WebKit/Source/core/html/parser/HTMLScriptRunner.cpp:277: if
(isExecutingScript() && cachedResource->wasCanceled()) {
Will we ever hit this condition? Judging from the comment it seems like
we might?

https://codereview.chromium.org/2260303002/

[shiva...@chromium.org]

On 2016/08/19 at 21:24:27, csharrison wrote:
> I'll let Nate sign off on the design portion. I left a few comments inline.
>
> My main concern would be overloading the ScriptLoader instead of creating a
new class that initiates the fetch Are you sure you need all the logic in
ScriptLoader for this? I'm nervous that it will lead to unintended bugs.

Took a second look at the logic in ScriptLoader. I think if we make it separate
from ScriptLoader and do not reuse the prepareScript and fetchScript
functionalities then we might run a risk of unintended bugs since we want this
script fetch to be like any other script fetch (except for the priority and
custom header). The main difference lies in what we do when we receive this
script(we do not execute it) vs a normal script.
Since all the logic related to this fetch in ScriptLoader is completely
encapsulated with the use of the flag, this looks like a reasonably safe change.
Nate, Charles: Thoughts?

(We could also refactor prepare script and fetchscript and reuse parts of it,
but that seems to be a bigger change than what we intend for this change.)

[shiva...@chromium.org]

https://codereview.chromium.org/2260303002/diff/60001/third_party/WebKit/Source/core/dom/ScriptLoader.h
File third_party/WebKit/Source/core/dom/ScriptLoader.h (right):

https://codereview.chromium.org/2260303002/diff/60001/third_party/WebKit/Source/core/dom/ScriptLoader.h#newcode125
third_party/WebKit/Source/core/dom/ScriptLoader.h:125: bool
m_disallowedFetchForDocWrittenScript : 1; // This script will be blocked
if not present in http cache.

On 2016/08/19 at 21:24:27, Charlie Harrison wrote:
> Can these be combined into an enum flag?

Done for readability and the fact that both of these flags are mutually
exclusive. (even though it increases the size a bit from 2 booleans to
enum)

https://codereview.chromium.org/2260303002/diff/60001/third_party/WebKit/Source/core/dom/ScriptLoader.h#newcode126
third_party/WebKit/Source/core/dom/ScriptLoader.h:126: const bool
m_blockedDocWriteScriptAsyncFetch : 1; // Non parser-blocking lowest
priority fetch for the blocked script.

On 2016/08/19 at 21:24:27, Charlie Harrison wrote:
> The comment here doesn't really specify what the "true" or "false"
states correspond to.

done.

https://codereview.chromium.org/2260303002/diff/60001/third_party/WebKit/Source/core/html/parser/HTMLScriptRunner.cpp
File third_party/WebKit/Source/core/html/parser/HTMLScriptRunner.cpp
(right):

https://codereview.chromium.org/2260303002/diff/60001/third_party/WebKit/Source/core/html/parser/HTMLScriptRunner.cpp#newcode277
third_party/WebKit/Source/core/html/parser/HTMLScriptRunner.cpp:277: if
(isExecutingScript() && cachedResource->wasCanceled()) {

On 2016/08/19 at 21:24:27, Charlie Harrison wrote:
> Will we ever hit this condition? Judging from the comment it seems
like we might?

Since its a canceled scenario, its fine to return from here and not send
the async GET request.

https://codereview.chromium.org/2260303002/

[shiva...@chromium.org]

Haraken@, PTAL, Thanks!

https://codereview.chromium.org/2260303002/

[jap...@chromium.org]

https://codereview.chromium.org/2260303002/diff/140001/third_party/WebKit/Source/core/dom/ScriptLoader.h
File third_party/WebKit/Source/core/dom/ScriptLoader.h (left):

https://codereview.chromium.org/2260303002/diff/140001/third_party/WebKit/Source/core/dom/ScriptLoader.h#oldcode126
third_party/WebKit/Source/core/dom/ScriptLoader.h:126:
Why these deletes?

https://codereview.chromium.org/2260303002/diff/140001/third_party/WebKit/Source/core/dom/ScriptLoader.h
File third_party/WebKit/Source/core/dom/ScriptLoader.h (right):

https://codereview.chromium.org/2260303002/diff/140001/third_party/WebKit/Source/core/dom/ScriptLoader.h#newcode42
third_party/WebKit/Source/core/dom/ScriptLoader.h:42: static
ScriptLoader* create(Element* element, bool createdByParser, bool
isEvaluated, bool createdDuringDocumentWrite = false, bool
blockedDocWriteScriptAsyncFetch = false)
Rather than adding another boolean parameter to a function that already
has 3, can we have a separate setter? This is a sufficiently unique case
that I don't think it belongs in the constructor.

https://codereview.chromium.org/2260303002/diff/140001/third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/2260303002/diff/140001/third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp#newcode166
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:166: priority =
request.resourceRequest().computePriorityForInterventions(priority);
This seems like overkill at first glance. Is there a reason marking the
request as LazyLoad is insufficient?

https://codereview.chromium.org/2260303002/diff/140001/third_party/WebKit/Source/core/html/parser/HTMLScriptRunner.cpp
File third_party/WebKit/Source/core/html/parser/HTMLScriptRunner.cpp
(right):

https://codereview.chromium.org/2260303002/diff/140001/third_party/WebKit/Source/core/html/parser/HTMLScriptRunner.cpp#newcode297
third_party/WebKit/Source/core/html/parser/HTMLScriptRunner.cpp:297: if
(blockedDocWriteScriptAsyncFetch) {
Does this logic absolutely need to be after notifyScriptLoaded()? It
adds a bunch of complexity to have to spread it over.

https://codereview.chromium.org/2260303002/

[shiva...@chromium.org]

https://codereview.chromium.org/2260303002/diff/140001/third_party/WebKit/Source/core/dom/ScriptLoader.h
File third_party/WebKit/Source/core/dom/ScriptLoader.h (left):

https://codereview.chromium.org/2260303002/diff/140001/third_party/WebKit/Source/core/dom/ScriptLoader.h#oldcode126
third_party/WebKit/Source/core/dom/ScriptLoader.h:126:

On 2016/08/22 at 19:47:03, Nate Chapin wrote:
> Why these deletes?

reverted.

https://codereview.chromium.org/2260303002/diff/140001/third_party/WebKit/Source/core/dom/ScriptLoader.h
File third_party/WebKit/Source/core/dom/ScriptLoader.h (right):

https://codereview.chromium.org/2260303002/diff/140001/third_party/WebKit/Source/core/dom/ScriptLoader.h#newcode42
third_party/WebKit/Source/core/dom/ScriptLoader.h:42: static
ScriptLoader* create(Element* element, bool createdByParser, bool
isEvaluated, bool createdDuringDocumentWrite = false, bool
blockedDocWriteScriptAsyncFetch = false)

On 2016/08/22 at 19:47:03, Nate Chapin wrote:
> Rather than adding another boolean parameter to a function that
already has 3, can we have a separate setter? This is a sufficiently
unique case that I don't think it belongs in the constructor.

Done.

https://codereview.chromium.org/2260303002/diff/140001/third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/2260303002/diff/140001/third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp#newcode166
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:166: priority =
request.resourceRequest().computePriorityForInterventions(priority);

On 2016/08/22 at 19:47:03, Nate Chapin wrote:
> This seems like overkill at first glance. Is there a reason marking
the request as LazyLoad is insufficient?

LazyLoad results in priority ResourceLoadPriorityLow which maps to
net::LOWEST but we want a priority net::IDLE for this case.

https://codereview.chromium.org/2260303002/diff/140001/third_party/WebKit/Source/core/html/parser/HTMLScriptRunner.cpp
File third_party/WebKit/Source/core/html/parser/HTMLScriptRunner.cpp
(right):

https://codereview.chromium.org/2260303002/diff/140001/third_party/WebKit/Source/core/html/parser/HTMLScriptRunner.cpp#newcode297
third_party/WebKit/Source/core/html/parser/HTMLScriptRunner.cpp:297: if
(blockedDocWriteScriptAsyncFetch) {

On 2016/08/22 at 19:47:03, Nate Chapin wrote:
> Does this logic absolutely need to be after notifyScriptLoaded()? It
adds a bunch of complexity to have to spread it over.

I am not completely aware of the other threads that are going on
simultaneously but notifyScriptLoaded() does a bunch of things including
resumeParsingAfterScriptExecution() that calls
HTMLDocumentParser::pumpTokenizer() and
HTMLDocumentParser::pumpPendingSpeculations(). If it does something that
can lead to posting tasks to other threads then it would be good to not
delay its execution. While debugging fetchBlockedDocWriteScript() using
gdb I was seeing it took a noticeable time before returning from
ScriptResource::fetch() call.

Thoughts?

https://codereview.chromium.org/2260303002/

[jap...@chromium.org]

https://codereview.chromium.org/2260303002/diff/140001/third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/2260303002/diff/140001/third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp#newcode166
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:166: priority =
request.resourceRequest().computePriorityForInterventions(priority);

On 2016/08/22 20:44:00, shivanisha wrote:
> On 2016/08/22 at 19:47:03, Nate Chapin wrote:
> > This seems like overkill at first glance. Is there a reason marking
the
> request as LazyLoad is insufficient?
>
> LazyLoad results in priority ResourceLoadPriorityLow which maps to
net::LOWEST
> but we want a priority net::IDLE for this case.

Ah, I forgot about the script-specific LazyLoad priority.

Adding an Idle value to the ResourceFetcher::DeferOption enum seemed
like it'd be cleaner than plumbing this through ResourceRequest. Can we
do that instead?

https://codereview.chromium.org/2260303002/diff/140001/third_party/WebKit/Source/core/html/parser/HTMLScriptRunner.cpp
File third_party/WebKit/Source/core/html/parser/HTMLScriptRunner.cpp
(right):

https://codereview.chromium.org/2260303002/diff/140001/third_party/WebKit/Source/core/html/parser/HTMLScriptRunner.cpp#newcode297
third_party/WebKit/Source/core/html/parser/HTMLScriptRunner.cpp:297: if
(blockedDocWriteScriptAsyncFetch) {

On 2016/08/22 20:44:00, shivanisha wrote:
> On 2016/08/22 at 19:47:03, Nate Chapin wrote:
> > Does this logic absolutely need to be after notifyScriptLoaded()? It
adds a
> bunch of complexity to have to spread it over.
>
> I am not completely aware of the other threads that are going on
simultaneously
> but notifyScriptLoaded() does a bunch of things including
> resumeParsingAfterScriptExecution() that calls
> HTMLDocumentParser::pumpTokenizer() and
> HTMLDocumentParser::pumpPendingSpeculations(). If it does something
that can
> lead to posting tasks to other threads then it would be good to not
delay its
> execution. While debugging fetchBlockedDocWriteScript() using gdb I
was seeing
> it took a noticeable time before returning from
ScriptResource::fetch() call.
>
> Thoughts?

Hrm, ok. One option is to move all the new code here into a helper
(maybeFetchBlockedDocWriteScript() or something like that), have it
return false if it didn't fetch a blocked script, and let it have a
m_host->notifyScriptLoaded() call in the appropriate place in its own
flow. If it doesn't fetch anything, then it just falls through to the
notifyScriptLoaded() call here. That we at least contain this
specialized logic. WDYT?

https://codereview.chromium.org/2260303002/

[shiva...@chromium.org]

Removing haraken@ as reviewer since ResourceRequest.* will no longer have any
code changes after incorporating Nate's comments.

https://codereview.chromium.org/2260303002/

[har...@chromium.org]

+hiroshige for MemoryCache handling.



https://codereview.chromium.org/2260303002/diff/160001/third_party/WebKit/Source/core/dom/ScriptLoader.h
File third_party/WebKit/Source/core/dom/ScriptLoader.h (right):

https://codereview.chromium.org/2260303002/diff/160001/third_party/WebKit/Source/core/dom/ScriptLoader.h#newcode91
third_party/WebKit/Source/core/dom/ScriptLoader.h:91: bool
disallowedFetchForDocWrittenScript() { return
m_documentWriteIntervention ==
DocumentWriteIntervention::DisallowedFetchForDocWrittenScript; }

disallowedFetchForDocWrittenScript => shouldFetchDocWrittenScript ?

https://codereview.chromium.org/2260303002/diff/160001/third_party/WebKit/Source/core/dom/ScriptLoader.h#newcode92
third_party/WebKit/Source/core/dom/ScriptLoader.h:92: void
setBlockedDocWriteScriptAsyncFetch();

"BlockedDocWrite" sounds a bit strange to me. Is there any non-blocking
document.write?

https://codereview.chromium.org/2260303002/diff/160001/third_party/WebKit/Source/core/dom/ScriptLoader.h#newcode131
third_party/WebKit/Source/core/dom/ScriptLoader.h:131:
DisallowedFetchForDocWrittenScript,

DoNotFetchDocWrittenScript ?

https://codereview.chromium.org/2260303002/diff/160001/third_party/WebKit/Source/core/dom/ScriptLoader.h#newcode133
third_party/WebKit/Source/core/dom/ScriptLoader.h:133:
AsyncLowPriorityFetchForBlockedScript,

FetchDocWrittenScriptAsynchrously ?

https://codereview.chromium.org/2260303002/

[shiva...@chromium.org]

https://codereview.chromium.org/2260303002/diff/140001/third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/2260303002/diff/140001/third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp#newcode166
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:166: priority =
request.resourceRequest().computePriorityForInterventions(priority);

On 2016/08/22 at 20:55:12, Nate Chapin wrote:
> On 2016/08/22 20:44:00, shivanisha wrote:
> > On 2016/08/22 at 19:47:03, Nate Chapin wrote:
> > > This seems like overkill at first glance. Is there a reason
marking the
> > request as LazyLoad is insufficient?
> >
> > LazyLoad results in priority ResourceLoadPriorityLow which maps to
net::LOWEST
> > but we want a priority net::IDLE for this case.
>
> Ah, I forgot about the script-specific LazyLoad priority.
>
> Adding an Idle value to the ResourceFetcher::DeferOption enum seemed
like it'd be cleaner than plumbing this through ResourceRequest. Can we
do that instead?

That's a good suggestion. Done.

https://codereview.chromium.org/2260303002/diff/140001/third_party/WebKit/Source/core/html/parser/HTMLScriptRunner.cpp
File third_party/WebKit/Source/core/html/parser/HTMLScriptRunner.cpp
(right):

https://codereview.chromium.org/2260303002/diff/140001/third_party/WebKit/Source/core/html/parser/HTMLScriptRunner.cpp#newcode297
third_party/WebKit/Source/core/html/parser/HTMLScriptRunner.cpp:297: if
(blockedDocWriteScriptAsyncFetch) {

Sounds good. done

https://codereview.chromium.org/2260303002/

[shiva...@chromium.org]

https://codereview.chromium.org/2260303002/diff/160001/third_party/WebKit/Source/core/dom/ScriptLoader.h
File third_party/WebKit/Source/core/dom/ScriptLoader.h (right):

https://codereview.chromium.org/2260303002/diff/160001/third_party/WebKit/Source/core/dom/ScriptLoader.h#newcode91
third_party/WebKit/Source/core/dom/ScriptLoader.h:91: bool
disallowedFetchForDocWrittenScript() { return
m_documentWriteIntervention ==
DocumentWriteIntervention::DisallowedFetchForDocWrittenScript; }

On 2016/08/23 at 00:30:39, haraken wrote:
> disallowedFetchForDocWrittenScript => shouldFetchDocWrittenScript ?

Named it as disallowedFetchForDocWrittenScript so that it is consistent
with the function shouldDisallowFetchForMainFrameScript() which is the
decision making function. This one just returns true or false based on
what decision shouldDisallowFetchForMainFrameScript() took.

https://codereview.chromium.org/2260303002/diff/160001/third_party/WebKit/Source/core/dom/ScriptLoader.h#newcode92
third_party/WebKit/Source/core/dom/ScriptLoader.h:92: void
setBlockedDocWriteScriptAsyncFetch();

On 2016/08/23 at 00:30:39, haraken wrote:
> "BlockedDocWrite" sounds a bit strange to me. Is there any
non-blocking document.write?

Here blocking is used not in the sense of parser-blocking but whether
the intervention actually blocked the fetch of this script. There are
conditions like a reload, cache-hit where the intervention does not
block the script.

https://codereview.chromium.org/2260303002/diff/160001/third_party/WebKit/Source/core/dom/ScriptLoader.h#newcode131
third_party/WebKit/Source/core/dom/ScriptLoader.h:131:
DisallowedFetchForDocWrittenScript,

On 2016/08/23 at 00:30:39, haraken wrote:
> DoNotFetchDocWrittenScript ?

Named such so that it is consistent with
shouldDisallowFetchForMainFrameScript()

https://codereview.chromium.org/2260303002/diff/160001/third_party/WebKit/Source/core/dom/ScriptLoader.h#newcode133
third_party/WebKit/Source/core/dom/ScriptLoader.h:133:
AsyncLowPriorityFetchForBlockedScript,

On 2016/08/23 at 00:30:39, haraken wrote:
> FetchDocWrittenScriptAsynchrously ?

Wanted to say that it is a fetch for a script whose original fetch was
blocked by the intervention, thus "block" is part of the name.

https://codereview.chromium.org/2260303002/

[shiva...@chromium.org]

Thanks all, for the feedback. I have incorporated/responded to all the feedback.
PTAL, thanks!


https://codereview.chromium.org/2260303002/

[har...@chromium.org]

I now understand your point, but the enum name is really confusing. At least
please add a very good comment about what it means.

(Nate would be a good reviewer for the overall change.)


https://codereview.chromium.org/2260303002/

[shiva...@chromium.org]

Thanks! Took another look at the names and I agree they are a little verbose and
thus may be confusing to the reader. Renamed the enum values and added a
comment, based on feedback from haraken@.

https://codereview.chromium.org/2260303002/

[shiva...@chromium.org]

The layout tests are failing after rebasing. The test function that they used
testRunner.dumpResourceRequestPriorities has been removed and I am looking into
how to re-write the test using WebFrameTest as recommended in the removing CL:
https://chromium.googlesource.com/chromium/src/+/8489f1bf78c2c6f296e0df6c529751d29e8a0188

https://codereview.chromium.org/2260303002/

[shiva...@chromium.org]

On 2016/09/01 at 18:58:04, shivanisha wrote:
> The layout tests are failing after rebasing. The test function that they used
testRunner.dumpResourceRequestPriorities has been removed and I am looking into
how to re-write the test using WebFrameTest as recommended in the removing CL:
https://chromium.googlesource.com/chromium/src/+/8489f1bf78c2c6f296e0df6c529751d29e8a0188

Nate,
Given that this test is slightly more complex (requires a blink setting and a
cross origin script for the first fetch to be blocked and requires testing the
async low priority fetch when the first fetch fails), it is probably best tested
using a layout test. Also since this CL is feature blocking and high priority,
what are your thoughts on getting the dumpResourceRequestPriorities back to
support this case?

- Shivani

https://codereview.chromium.org/2260303002/

[jap...@chromium.org]

LGTM, just a couple nitpicks.


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

https://codereview.chromium.org/2260303002/diff/240001/third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-block.html#newcode84
third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-block.html:84:
if (window.testRunner) {
This shouldn't be needed twice? and the dumpAsText() appears
unnecessary, given that the expectations are already text.

https://codereview.chromium.org/2260303002/diff/240001/third_party/WebKit/Source/core/html/parser/HTMLScriptRunner.cpp
File third_party/WebKit/Source/core/html/parser/HTMLScriptRunner.cpp
(right):

https://codereview.chromium.org/2260303002/diff/240001/third_party/WebKit/Source/core/html/parser/HTMLScriptRunner.cpp#newcode263
third_party/WebKit/Source/core/html/parser/HTMLScriptRunner.cpp:263:
bool HTMLScriptRunner::possiblyFetchBlockedDocWriteScript(Resource*
cachedResource)
Nit: |cachedResource| is anachronistic. Just |resource| is preferable.

https://codereview.chromium.org/2260303002/

[shiva...@chromium.org]

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

https://codereview.chromium.org/2260303002/diff/240001/third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-block.html#newcode84
third_party/WebKit/LayoutTests/http/tests/loading/doc-write-sync-third-party-script-block.html:84:
if (window.testRunner) {

On 2016/09/06 at 20:28:12, Nate Chapin wrote:
> This shouldn't be needed twice? and the dumpAsText() appears
unnecessary, given that the expectations are already text.

Passing this test will require putting dumpResourceRequestPriorities
back , which was removed by you in CL
https://chromium.googlesource.com/chromium/src/+/8489f1bf78c2c6f296e0df6c529751d29e8a0188
Just wanted to confirm if that sounds good as well.

https://codereview.chromium.org/2260303002/

[jap...@chromium.org]

Whoops, missed that. dumpResourceRequestPriorities() wasn't just removed from
that test, I ripped out the plumbing. You'd need a unit test to verify the
request priority. You can probably add a case to WebFrameTest.ScriptPriority

https://codereview.chromium.org/2260303002/

[har...@chromium.org]

https://codereview.chromium.org/2260303002/diff/240001/third_party/WebKit/Source/core/dom/ScriptLoader.h
File third_party/WebKit/Source/core/dom/ScriptLoader.h (right):

https://codereview.chromium.org/2260303002/diff/240001/third_party/WebKit/Source/core/dom/ScriptLoader.h#newcode137
third_party/WebKit/Source/core/dom/ScriptLoader.h:137:
FetchDocWrittenScriptDeferIdle,

FetchDocWrittenScriptInIdleTask ?

https://codereview.chromium.org/2260303002/diff/240001/third_party/WebKit/Source/core/html/parser/HTMLScriptRunner.cpp
File third_party/WebKit/Source/core/html/parser/HTMLScriptRunner.cpp
(right):

https://codereview.chromium.org/2260303002/diff/240001/third_party/WebKit/Source/core/html/parser/HTMLScriptRunner.cpp#newcode279
third_party/WebKit/Source/core/html/parser/HTMLScriptRunner.cpp:279:
shouldFetchBlockedDocWriteScript = true;

It's confusing to set "shouldFetch"BlockedDocWriteScript true when
"disallowedFetch"ForDocWrittenScript returns true...

Does "DoNotFetchDocWrittenScript" mean that we should not fetch the
script during loading but should fetch the script after the loading has
finished? Then how is it different from
"FetchDocWrittenScriptDeferIdle"?

https://codereview.chromium.org/2260303002/

[shiva...@chromium.org]

I spent a lot of time implementing the test but I am not completely sure if
that's the best approach. This is a special case where the fetch we are trying
to test is a result of a previously blocked fetch. That was possible to test
using the layout tests but not with unit tests. With unit tests, I was hoping to
test just the second fetch request by creating a ScriptLoader and invoking
setFetchDocWrittenScriptDeferIdle() on it followed by prepareScript(). The
closest framework I found to test this is ScriptRunnerTest but there is an issue
coming in that approach:
- prepareScript requires the script document to either have a context document
or a frame. I tried using DummyPageHolder to provide a frame but that's not
working as expected. Its hitting an assertion during DummyPageHolder::create and
I am debugging why that's the case.
- When this is fixed, I am not completely sure if I can test the fetch request's
priority. I can probably just test that prepareScript returns a success.

Any thoughts on a better way to test this?

https://codereview.chromium.org/2260303002/

[jap...@chromium.org]

I think my terminology was clumsy and misled you :(

We have a way in the unit testing harness to load a webpage from
Source/web/tests/data/ and have it more or less load and run the page normally,
but you can reach into blink's internal state directly or listen to public/web/
callbacks. I was thinking you should be able to more or less copy
doc-write-sync-third-party-script-block.html to Source/web/tests/data/, create a
unit test in WebFrameTest that loads that html file, and use
TestResourcePriorityWebFrameClient or make a variant of it, which can listen to
willSendRequest() callbacks and inspect the resource priority.

Might something like that work?


https://codereview.chromium.org/2260303002/

[shiva...@chromium.org]

The script blocking requires the script to be cross origin and a blink-settings
flag to be enabled to achieve blocking. Are these two conditions achievable?
The script actually gets blocked in the browser process but I am assuming the
WebFrameTest framework does not invoke the browser, does it?


https://codereview.chromium.org/2260303002/

[jap...@chromium.org]

You should be able to emulate a cross origin load by provided a cross-origin url
to URLTestHelpers::reigsterMockedURLLoad, and a blink settings flag can be
modified directly via WebView::settings().

As for where the script gets blocked, that's trickier. Can you point me to where
that blocking happens?

https://codereview.chromium.org/2260303002/

[shiva...@chromium.org]

When shouldDisallowFetchForMainFrameScript() in FrameFetchContext.cpp returns
true, the cache policy is set to WebCachePolicy::ReturnCacheDataDontLoad and
that leads to the resource being blocked if its not in the cache. This is
checked in the browser process http cache code.

https://codereview.chromium.org/2260303002/

[jap...@chromium.org]

Hmm, then we don't really care about the script being blocked from blink's
perspective, right? It's sufficient to make sure that the request has the proper
WebCachePolicy. That should be possible to inspect in a willSendRequest() hook,
similar to the request's priority.

https://codereview.chromium.org/2260303002/

[shiva...@chromium.org]

But that's just the first leg of the scenario for which we already have layout
tests. The second leg of the scenario (this CL) is the one that needs tests
which happens when the first fetch gets blocked and then the second fetch is
done for the same script with a very low priority.

https://codereview.chromium.org/2260303002/

[shiva...@chromium.org]

I and Nate discussed this over VC, and I am looking into two of the possible
testing strategies:
- Adding a test function in internals API to test this in layout tests.
(Preferred since rest of the tests for this feature already exist in layout
tests)
- Adding a unit test that mocks the failure of the first fetch using
registerMockedErrorURLLoad.

https://codereview.chromium.org/2260303002/

[shiva...@chromium.org]

The new patch adds a new internal.GetResourcePriority for testing and addresses
feedback.

https://codereview.chromium.org/2260303002/diff/240001/third_party/WebKit/Source/core/dom/ScriptLoader.h
File third_party/WebKit/Source/core/dom/ScriptLoader.h (right):

https://codereview.chromium.org/2260303002/diff/240001/third_party/WebKit/Source/core/dom/ScriptLoader.h#newcode137
third_party/WebKit/Source/core/dom/ScriptLoader.h:137:
FetchDocWrittenScriptDeferIdle,

On 2016/09/07 at 01:47:36, haraken wrote:
> FetchDocWrittenScriptInIdleTask ?

used DeferIdle in the name to convey the usage of DeferOption::IdleLoad.

https://codereview.chromium.org/2260303002/diff/240001/third_party/WebKit/Source/core/html/parser/HTMLScriptRunner.cpp
File third_party/WebKit/Source/core/html/parser/HTMLScriptRunner.cpp
(right):

https://codereview.chromium.org/2260303002/diff/240001/third_party/WebKit/Source/core/html/parser/HTMLScriptRunner.cpp#newcode263
third_party/WebKit/Source/core/html/parser/HTMLScriptRunner.cpp:263:
bool HTMLScriptRunner::possiblyFetchBlockedDocWriteScript(Resource*
cachedResource)

On 2016/09/06 at 20:28:12, Nate Chapin wrote:

> Nit: |cachedResource| is anachronistic. Just |resource| is preferable.

done.

https://codereview.chromium.org/2260303002/diff/240001/third_party/WebKit/Source/core/html/parser/HTMLScriptRunner.cpp#newcode279
third_party/WebKit/Source/core/html/parser/HTMLScriptRunner.cpp:279:
shouldFetchBlockedDocWriteScript = true;

disallowedFetch"ForDocWrittenScript was set when this resource was
requested and this code is executed on receiving the response/error for
the resource.
shouldFetchBlockedDocWriteScript implies if another fetch should be done
for this resource.

DoNotFetchDocWrittenScript simply means that this request should not be
fetched from the network.(set for 1st fetch request)
FetchDocWrittenScriptDeferIdle means that a script that was blocked
should be fetched again using idle priority. (set for 2nd fetch request)

https://codereview.chromium.org/2260303002/

[jap...@chromium.org]

LGTM with a couple of nits.


https://codereview.chromium.org/2260303002/diff/300001/third_party/WebKit/Source/core/testing/Internals.cpp
File third_party/WebKit/Source/core/testing/Internals.cpp (right):

https://codereview.chromium.org/2260303002/diff/300001/third_party/WebKit/Source/core/testing/Internals.cpp#newcode419
third_party/WebKit/Source/core/testing/Internals.cpp:419: Resource*
resource =
document->fetcher()->allResources().get(URLTestHelpers::toKURL(url.utf8().data()));
KURL(ParsedURLString, url) is the canonical way to do a String->KURL
conversion if the url is known to be absolute.

https://codereview.chromium.org/2260303002/diff/300001/third_party/WebKit/Source/core/testing/Internals.idl
File third_party/WebKit/Source/core/testing/Internals.idl (right):

https://codereview.chromium.org/2260303002/diff/300001/third_party/WebKit/Source/core/testing/Internals.idl#newcode39
third_party/WebKit/Source/core/testing/Internals.idl:39: unsigned long
getResourcePriority (DOMString url, Document document);
Nit: suprious whitespace after getResourcePriority

https://codereview.chromium.org/2260303002/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch. Follow status at

https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2260303002/300001


https://codereview.chromium.org/2260303002/

[commit-bot@chromium.org via codereview.chromium.org]

Committed patchset #16 (id:300001)

https://codereview.chromium.org/2260303002/

[commit-bot@chromium.org via codereview.chromium.org]

Patchset 16 (id:??) landed as
https://crrev.com/57536eaf61f21cb9f6e351663ef75c3d6e09d705
Cr-Commit-Position: refs/heads/master@{#419476}

https://codereview.chromium.org/2260303002/