Don't lower priority for scripts inserted by doc.write

Don't lower priority for scripts inserted by doc.write

HTML Chunks inserted by document.write() are run through a separate
instance of the preload scanner from the main document.  If the main
document scanner has already discovered an image then all scripts
discovered by any preload scanner regardless of where they came from
would be considered late-body.

This behavior causes a regression where a document.write() in the head
that includes multiple script tags will discover them but treat them as
late-body scripts and only load them one at a time.

BUG=713727, 712338
Review-Url: https://codereview.chromium.org/2826213003
Cr-Commit-Position: refs/heads/master@{#467029}
Committed: https://chromium.googlesource.com/chromium/src/+/1e0d3cbbf60c67ba9856eadcb9e829b3c4bec6c7

[Pat Meenan, 2017-04-20 18:29:29]

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

[commit-bot: I haz the power, 2017-04-20 18:30:15]

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

[Pat Meenan, 2017-04-20 20:21:24]

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

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

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

[Pat Meenan, 2017-04-20 20:25:17]

pmeenan@chromium.org changed reviewers:
+ kinuko@chromium.org, yoav@yoav.ws

[Pat Meenan, 2017-04-20 20:25:18]

yoav@ and kinuko@ could you PTAL?  This fixes an issue with preload scanning
document.write contents and giving scripts too low of a priority (see linked
bug)

[Charlie Harrison, 2017-04-20 20:44:12]

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

[Charlie Harrison, 2017-04-20 20:44:13]

I looked over the approach and it looks good to me.

[Charlie Harrison, 2017-04-20 20:45:16]

Thanks for making this change!

[commit-bot: I haz the power, 2017-04-20 22:35:14]

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

[commit-bot: I haz the power, 2017-04-20 22:35:15]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[kinuko, 2017-04-21 06:22:25]

I see... I read through the bug description and alternatives, looks like this
seems to be plausible yet least intrusive change.  Some nit comments

https://codereview.chromium.org/2826213003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.h (right):

https://codereview.chromium.org/2826213003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.h:85: enum
ScannerType { kMainDocumentScanner = 0, kInsertionScanner };
nit: enum class, and name each enum names shorter? (e.g. kMainDocument,
kInsertion)

Do we need explicit '= 0'?

https://codereview.chromium.org/2826213003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/loader/fetch/FetchParameters.h (right):

https://codereview.chromium.org/2826213003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/loader/fetch/FetchParameters.h:50: enum
SpeculativePreload {
SpeculativePreloadType ?

https://codereview.chromium.org/2826213003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/loader/fetch/FetchParameters.h:54: };
ditto for new enums (though it makes things less consistent)

and ditto, do we need '= 0' here?

https://codereview.chromium.org/2826213003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.h (right):

https://codereview.chromium.org/2826213003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.h:172: bool
is_link_preload = false);
I prefer using a single enum everywhere rather than booleans, could the
SpeculativePreload type enum put outside the FetchParameters and use it here
too?

[kinuko, 2017-04-21 08:27:21]

Also: can you update your priority doc once this lands? Thanks!

[Pat Meenan, 2017-04-21 16:09:37]

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

[commit-bot: I haz the power, 2017-04-21 16:10:04]

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

[Pat Meenan, 2017-04-21 16:11:02]

I'll add a not to the priority doc about how document.write() is handled.  I
don't think anything in the actual data table needs to change though.

Still trying to figure out why the mojo-loading version of the resource priority
test is failing.

https://codereview.chromium.org/2826213003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.h (right):

https://codereview.chromium.org/2826213003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.h:85: enum
ScannerType { kMainDocumentScanner = 0, kInsertionScanner };
On 2017/04/21 06:22:25, kinuko wrote:
> nit: enum class, and name each enum names shorter? (e.g. kMainDocument,
> kInsertion)
> 
> Do we need explicit '= 0'?

Done.

https://codereview.chromium.org/2826213003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/loader/fetch/FetchParameters.h (right):

https://codereview.chromium.org/2826213003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/loader/fetch/FetchParameters.h:50: enum
SpeculativePreload {
On 2017/04/21 06:22:25, kinuko wrote:
> SpeculativePreloadType ?

Done.

https://codereview.chromium.org/2826213003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/loader/fetch/FetchParameters.h:54: };
On 2017/04/21 06:22:25, kinuko wrote:
> ditto for new enums (though it makes things less consistent)
> 
> and ditto, do we need '= 0' here?

Changed it to a class with shorter enums (is that what you meant?).  Also change
"SetSpeculativePreload" to "SetSpeculativePreloadType" and the member from
speculative_preload_ to speculative_preload_type_

https://codereview.chromium.org/2826213003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.h (right):

https://codereview.chromium.org/2826213003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.h:172: bool
is_link_preload = false);
On 2017/04/21 06:22:25, kinuko wrote:
> I prefer using a single enum everywhere rather than booleans, could the
> SpeculativePreload type enum put outside the FetchParameters and use it here
> too?

Done.

[commit-bot: I haz the power, 2017-04-21 18:27:57]

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

[commit-bot: I haz the power, 2017-04-21 18:27:58]

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

[Pat Meenan, 2017-04-23 16:54:49]

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

[commit-bot: I haz the power, 2017-04-23 16:54:59]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-23 18:57:18]

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

[commit-bot: I haz the power, 2017-04-23 18:57:19]

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

[kinuko, 2017-04-24 03:59:55]

On 2017/04/21 16:11:02, Pat Meenan wrote:
> I'll add a not to the priority doc about how document.write() is handled.  I
> don't think anything in the actual data table needs to change though.

Makes sense, thanks!
 
> Still trying to figure out why the mojo-loading version of the resource
priority
> test is failing.

Looks like non-mojo version is equally failing, we've got docWritten log spilled
over sendXHRSync results, and things got mixed after that...  but only on mac??

sendXHRSync
Request: empty-script.js?docWritten-2 priority: Medium
Request: empty-script.js?docWritten-lastRequest priority: Medium
sendXHRAsync
Request: empty.html?xhr-sync&lastRequest priority: VeryHigh
sendImageRequest
Request: empty.html?xhr-async&lastRequest priority: High
sendStyleRequest
Request: abe.png?lastRequest priority: VeryLow
createIFrame
Request: style.css?lastRequest priority: VeryHigh

[Pat Meenan, 2017-04-24 12:55:50]

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

[commit-bot: I haz the power, 2017-04-24 12:56:10]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Pat Meenan, 2017-04-24 13:21:16]

On 2017/04/24 03:59:55, kinuko wrote:
> On 2017/04/21 16:11:02, Pat Meenan wrote:
> > I'll add a not to the priority doc about how document.write() is handled.  I
> > don't think anything in the actual data table needs to change though.
> 
> Makes sense, thanks!
>  
> > Still trying to figure out why the mojo-loading version of the resource
> priority
> > test is failing.
> 
> Looks like non-mojo version is equally failing, we've got docWritten log
spilled
> over sendXHRSync results, and things got mixed after that...  but only on
mac??
> 
> sendXHRSync
> Request: empty-script.js?docWritten-2 priority: Medium
> Request: empty-script.js?docWritten-lastRequest priority: Medium
> sendXHRAsync
> Request: empty.html?xhr-sync&lastRequest priority: VeryHigh
> sendImageRequest
> Request: empty.html?xhr-async&lastRequest priority: High
> sendStyleRequest
> Request: abe.png?lastRequest priority: VeryLow
> createIFrame
> Request: style.css?lastRequest priority: VeryHigh

I made a few changes to the test to clean up a few possible issues that came to
mind (re-using the same URL may have had something loading from memory cache
which should cause a timeout and triggering moving to the next test by magic
strings in the URL could cause the sequence issues if the load order is
different).  Neither should fix the issue outright though because an order
change would still fail (and shouldn't be possible).

How did you get the "actual" version of the output from the bots?  Clicking on
the result and going to the buildbot page lets me see the stdio but that only
shows that the test failed.  One of the linux bots on an earlier run had a json
output that had the actual and diff which was useful but if there's a way to get
the output from the mac bots that would help debug the issue.

[Pat Meenan, 2017-04-24 13:28:06]

On 2017/04/24 13:21:16, Pat Meenan wrote:
> On 2017/04/24 03:59:55, kinuko wrote:
> > On 2017/04/21 16:11:02, Pat Meenan wrote:
> > > I'll add a not to the priority doc about how document.write() is handled. 
I
> > > don't think anything in the actual data table needs to change though.
> > 
> > Makes sense, thanks!
> >  
> > > Still trying to figure out why the mojo-loading version of the resource
> > priority
> > > test is failing.
> > 
> > Looks like non-mojo version is equally failing, we've got docWritten log
> spilled
> > over sendXHRSync results, and things got mixed after that...  but only on
> mac??
> > 
> > sendXHRSync
> > Request: empty-script.js?docWritten-2 priority: Medium
> > Request: empty-script.js?docWritten-lastRequest priority: Medium
> > sendXHRAsync
> > Request: empty.html?xhr-sync&lastRequest priority: VeryHigh
> > sendImageRequest
> > Request: empty.html?xhr-async&lastRequest priority: High
> > sendStyleRequest
> > Request: abe.png?lastRequest priority: VeryLow
> > createIFrame
> > Request: style.css?lastRequest priority: VeryHigh
> 
> I made a few changes to the test to clean up a few possible issues that came
to
> mind (re-using the same URL may have had something loading from memory cache
> which should cause a timeout and triggering moving to the next test by magic
> strings in the URL could cause the sequence issues if the load order is
> different).  Neither should fix the issue outright though because an order
> change would still fail (and shouldn't be possible).
> 
> How did you get the "actual" version of the output from the bots?  Clicking on
> the result and going to the buildbot page lets me see the stdio but that only
> shows that the test failed.  One of the linux bots on an earlier run had a
json
> output that had the actual and diff which was useful but if there's a way to
get
> the output from the mac bots that would help debug the issue.

nm, found the docs explaining where to download the zip of the layout test
results.  There will probably be a sequence of patches and try bot runs while I
sort through why it is failing on the mac.  I'll send out an update when it is
sorted out and ready for review.

[commit-bot: I haz the power, 2017-04-24 15:52:47]

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

[commit-bot: I haz the power, 2017-04-24 15:52:48]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Pat Meenan, 2017-04-24 17:34:14]

On 2017/04/24 13:28:06, Pat Meenan wrote:
> On 2017/04/24 13:21:16, Pat Meenan wrote:
> > On 2017/04/24 03:59:55, kinuko wrote:
> > > On 2017/04/21 16:11:02, Pat Meenan wrote:
> > > > I'll add a not to the priority doc about how document.write() is
handled. 
> I
> > > > don't think anything in the actual data table needs to change though.
> > > 
> > > Makes sense, thanks!
> > >  
> > > > Still trying to figure out why the mojo-loading version of the resource
> > > priority
> > > > test is failing.
> > > 
> > > Looks like non-mojo version is equally failing, we've got docWritten log
> > spilled
> > > over sendXHRSync results, and things got mixed after that...  but only on
> > mac??
> > > 
> > > sendXHRSync
> > > Request: empty-script.js?docWritten-2 priority: Medium
> > > Request: empty-script.js?docWritten-lastRequest priority: Medium
> > > sendXHRAsync
> > > Request: empty.html?xhr-sync&lastRequest priority: VeryHigh
> > > sendImageRequest
> > > Request: empty.html?xhr-async&lastRequest priority: High
> > > sendStyleRequest
> > > Request: abe.png?lastRequest priority: VeryLow
> > > createIFrame
> > > Request: style.css?lastRequest priority: VeryHigh
> > 
> > I made a few changes to the test to clean up a few possible issues that came
> to
> > mind (re-using the same URL may have had something loading from memory cache
> > which should cause a timeout and triggering moving to the next test by magic
> > strings in the URL could cause the sequence issues if the load order is
> > different).  Neither should fix the issue outright though because an order
> > change would still fail (and shouldn't be possible).
> > 
> > How did you get the "actual" version of the output from the bots?  Clicking
on
> > the result and going to the buildbot page lets me see the stdio but that
only
> > shows that the test failed.  One of the linux bots on an earlier run had a
> json
> > output that had the actual and diff which was useful but if there's a way to
> get
> > the output from the mac bots that would help debug the issue.
> 
> nm, found the docs explaining where to download the zip of the layout test
> results.  There will probably be a sequence of patches and try bot runs while
I
> sort through why it is failing on the mac.  I'll send out an update when it is
> sorted out and ready for review.

And apparently the cleanup fixed the issues on Mac so it's green across the
board now ¯\_(ツ)_/¯.

Any other issues you see or is it ready to go?

[kinuko, 2017-04-25 05:44:23]

lgtm

> And apparently the cleanup fixed the issues on Mac so it's green across the
> board now ¯\_(ツ)_/¯.

¯\_(ツ)_/¯

[Pat Meenan, 2017-04-25 13:08:21]

The CQ bit was checked by pmeenan@chromium.org

[commit-bot: I haz the power, 2017-04-25 13:08:29]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-25 17:14:31]

CQ is committing da patch.

Bot data: {"patchset_id": 70001, "attempt_start_ts": 1493125701271410,
"parent_rev": "73b2161bb2976f9d8976bec92d9a988674cdeb45", "commit_rev":
"1e0d3cbbf60c67ba9856eadcb9e829b3c4bec6c7"}

[commit-bot: I haz the power, 2017-04-25 17:14:40]

Description was changed from

==========
Don't lower priority for scripts inserted by doc.write

HTML Chunks inserted by document.write() are run through a separate
instance of the preload scanner from the main document.  If the main
document scanner has already discovered an image then all scripts
discovered by any preload scanner regardless of where they came from
would be considered late-body.

This behavior causes a regression where a document.write() in the head
that includes multiple script tags will discover them but treat them as
late-body scripts and only load them one at a time.

BUG=713727,712338
==========

to

==========
Don't lower priority for scripts inserted by doc.write

HTML Chunks inserted by document.write() are run through a separate
instance of the preload scanner from the main document.  If the main
document scanner has already discovered an image then all scripts
discovered by any preload scanner regardless of where they came from
would be considered late-body.

This behavior causes a regression where a document.write() in the head
that includes multiple script tags will discover them but treat them as
late-body scripts and only load them one at a time.

BUG=713727,712338

Review-Url: https://codereview.chromium.org/2826213003
Cr-Commit-Position: refs/heads/master@{#467029}
Committed:
https://chromium.googlesource.com/chromium/src/+/1e0d3cbbf60c67ba9856eadcb9e8...
==========

[commit-bot: I haz the power, 2017-04-25 17:14:41]

Committed patchset #5 (id:70001) as
https://chromium.googlesource.com/chromium/src/+/1e0d3cbbf60c67ba9856eadcb9e8...

[Pat Meenan, 2017-04-26 13:22:35]

A revert of this CL (patchset #5 id:70001) has been created in
https://codereview.chromium.org/2843013002/ by pmeenan@chromium.org.

The reason for reverting is: The layout test was broken and racy
(http://crbug.com/715528).  SHould be trivial to fix (use the same query param
for the preloaded and real image) but best to revert, fix and re-land
separately..