New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 713727 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Last visit > 30 days ago
Closed: May 2017
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 712338



Sign in to add a comment

doc.write() script tags incorrectly treated as late-body scripts by preload scanner

Project Member Reported by pmeenan@chromium.org, Apr 20 2017

Issue description

document.write() chunks of HTML are fed to the preload scanner so all of the resources in it can be discovered and started loading without waiting for the parser to parse each one of them.

If the preload scanner has already triggered an image load from something it discovered in the body then the any scripts discovered in chunks of document.write() html will be treated as late-body scripts.

A trivial case is attached where an external script (docwrite.js) writes 100 script tags in a loop:

for (var i = 1; i <= 100; i++) {
  document.write('<scr' + 'ipt src="written.js?i=' + i + '"></scr' + 'ipt>\n')
}

Using a trivial HTML example:

<html>
<head>
<script src="docwrite.js"></script>
</head>
<body>
<img src="image.png">
Loaded
</body>
</html>

Both pages are hosted here for testing: http://test.patrickmeenan.com/docwrite/

If the img tag is present, the scripts written into the head of the page will be requested with Medium priority but if the image isn't in the markup the scripts will be fetched with High priority (correct).

To make matters worse, when loaded as Medium priority, the scripts will only load one-at-a-time because Chrome is still in the layout-blocking phase of loading.

This is because the preload scanner running on the HTML already discovered the image which is used as the separator for "early" and "late" scripts.  When the HTML snippet that includes the 100 tags is appended to the document and run through a new preload scanner they are treated as late-body scripts.

WebPageTest test with image in the page (loading one at a time):
http://www.webpagetest.org/result/170420_7N_5ec652efc3cbb82e7bbcf42e4c135e8c/1/details/#waterfall_view_step1

WebPageTest test with no image in the page (loading six at a time):
http://www.webpagetest.org/result/170420_YW_cdfd5cfbc15d1a5b1215d8149b4175b9/1/details/#waterfall_view_step1
 
docwrite.zip
1.3 KB Download
There are a few ways to fix it but none of them are particularly clean:

1 - Eliminate the concept of "late-body" scripts.  This would give treat all non-async scripts as equal and would load them in parser-discovered order with no limits on the concurrency of loading scripts.

This would be good for this kind of case but really bad for sites that put a bunch of script at the end and will significantly delay "Time to First Meaningful Paint" for a lot of the web.


2 - Allow loading more than one "low" priority request at a time while in render-blocking mode, maybe only when other non-low-priority requests are not also in-flight.

This gets a bit racy because when a new high-priority request is discovered there may already be a large number of low priority requests in-flight.


3 - Have each preload scanner instance track it's own concept of early or late-body and attach it to any requests as an extra attribute.  The preload scanners run against the document.write() snippets won't inherit any state from the main document and will always be "early".


4 - Make the preload scanner aware that it is scanning a document.written snippet instead of the main HTML stream and have it append that information to the requests generated.  Any non-async scripts preloaded from a snippet will bypass the "late-in-body" check and will get the correct (medium) priority.


#3 or #4 feel like the best solutions (with #3 possibly being the cleaner of the two but it carries more state with each request).
Potential fix is mostly done: https://codereview.chromium.org/2826213003 (just have to write some tests to make sure the issue doesn't happen again)
Project Member

Comment 3 by bugdroid1@chromium.org, Apr 25 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1e0d3cbbf60c67ba9856eadcb9e829b3c4bec6c7

commit 1e0d3cbbf60c67ba9856eadcb9e829b3c4bec6c7
Author: pmeenan <pmeenan@chromium.org>
Date: Tue Apr 25 17:14:26 2017

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}

[modify] https://crrev.com/1e0d3cbbf60c67ba9856eadcb9e829b3c4bec6c7/third_party/WebKit/LayoutTests/http/tests/inspector/network/resource-priority-expected.txt
[modify] https://crrev.com/1e0d3cbbf60c67ba9856eadcb9e829b3c4bec6c7/third_party/WebKit/LayoutTests/http/tests/inspector/network/resource-priority.html
[add] https://crrev.com/1e0d3cbbf60c67ba9856eadcb9e829b3c4bec6c7/third_party/WebKit/LayoutTests/http/tests/inspector/network/resources/docwrite.js
[modify] https://crrev.com/1e0d3cbbf60c67ba9856eadcb9e829b3c4bec6c7/third_party/WebKit/Source/core/html/parser/BackgroundHTMLParser.cpp
[modify] https://crrev.com/1e0d3cbbf60c67ba9856eadcb9e829b3c4bec6c7/third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp
[modify] https://crrev.com/1e0d3cbbf60c67ba9856eadcb9e829b3c4bec6c7/third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.h
[modify] https://crrev.com/1e0d3cbbf60c67ba9856eadcb9e829b3c4bec6c7/third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.cpp
[modify] https://crrev.com/1e0d3cbbf60c67ba9856eadcb9e829b3c4bec6c7/third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.h
[modify] https://crrev.com/1e0d3cbbf60c67ba9856eadcb9e829b3c4bec6c7/third_party/WebKit/Source/core/html/parser/HTMLPreloadScannerFuzzer.cpp
[modify] https://crrev.com/1e0d3cbbf60c67ba9856eadcb9e829b3c4bec6c7/third_party/WebKit/Source/core/html/parser/HTMLPreloadScannerTest.cpp
[modify] https://crrev.com/1e0d3cbbf60c67ba9856eadcb9e829b3c4bec6c7/third_party/WebKit/Source/core/html/parser/PreloadRequest.cpp
[modify] https://crrev.com/1e0d3cbbf60c67ba9856eadcb9e829b3c4bec6c7/third_party/WebKit/Source/core/html/parser/PreloadRequest.h
[modify] https://crrev.com/1e0d3cbbf60c67ba9856eadcb9e829b3c4bec6c7/third_party/WebKit/Source/platform/loader/fetch/FetchParameters.cpp
[modify] https://crrev.com/1e0d3cbbf60c67ba9856eadcb9e829b3c4bec6c7/third_party/WebKit/Source/platform/loader/fetch/FetchParameters.h
[modify] https://crrev.com/1e0d3cbbf60c67ba9856eadcb9e829b3c4bec6c7/third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp
[modify] https://crrev.com/1e0d3cbbf60c67ba9856eadcb9e829b3c4bec6c7/third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.h

Project Member

Comment 4 by bugdroid1@chromium.org, Apr 26 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7cf26442aea7fbcf8883717309287e2fc4d64639

commit 7cf26442aea7fbcf8883717309287e2fc4d64639
Author: pmeenan <pmeenan@chromium.org>
Date: Wed Apr 26 13:23:30 2017

Revert of Don't lower priority for scripts inserted by doc.write (patchset #5 id:70001 of https://codereview.chromium.org/2826213003/ )

Reason for revert:
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.

Original issue's description:
> 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

TBR=yoav@yoav.ws,kinuko@chromium.org,csharrison@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 713727 ,712338

Review-Url: https://codereview.chromium.org/2843013002
Cr-Commit-Position: refs/heads/master@{#467310}

[modify] https://crrev.com/7cf26442aea7fbcf8883717309287e2fc4d64639/third_party/WebKit/LayoutTests/http/tests/inspector/network/resource-priority-expected.txt
[modify] https://crrev.com/7cf26442aea7fbcf8883717309287e2fc4d64639/third_party/WebKit/LayoutTests/http/tests/inspector/network/resource-priority.html
[delete] https://crrev.com/79a8c1f3a373b60846fd2a163966b7f8e30991a6/third_party/WebKit/LayoutTests/http/tests/inspector/network/resources/docwrite.js
[modify] https://crrev.com/7cf26442aea7fbcf8883717309287e2fc4d64639/third_party/WebKit/Source/core/html/parser/BackgroundHTMLParser.cpp
[modify] https://crrev.com/7cf26442aea7fbcf8883717309287e2fc4d64639/third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp
[modify] https://crrev.com/7cf26442aea7fbcf8883717309287e2fc4d64639/third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.h
[modify] https://crrev.com/7cf26442aea7fbcf8883717309287e2fc4d64639/third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.cpp
[modify] https://crrev.com/7cf26442aea7fbcf8883717309287e2fc4d64639/third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.h
[modify] https://crrev.com/7cf26442aea7fbcf8883717309287e2fc4d64639/third_party/WebKit/Source/core/html/parser/HTMLPreloadScannerFuzzer.cpp
[modify] https://crrev.com/7cf26442aea7fbcf8883717309287e2fc4d64639/third_party/WebKit/Source/core/html/parser/HTMLPreloadScannerTest.cpp
[modify] https://crrev.com/7cf26442aea7fbcf8883717309287e2fc4d64639/third_party/WebKit/Source/core/html/parser/PreloadRequest.cpp
[modify] https://crrev.com/7cf26442aea7fbcf8883717309287e2fc4d64639/third_party/WebKit/Source/core/html/parser/PreloadRequest.h
[modify] https://crrev.com/7cf26442aea7fbcf8883717309287e2fc4d64639/third_party/WebKit/Source/platform/loader/fetch/FetchParameters.cpp
[modify] https://crrev.com/7cf26442aea7fbcf8883717309287e2fc4d64639/third_party/WebKit/Source/platform/loader/fetch/FetchParameters.h
[modify] https://crrev.com/7cf26442aea7fbcf8883717309287e2fc4d64639/third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp
[modify] https://crrev.com/7cf26442aea7fbcf8883717309287e2fc4d64639/third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.h

Project Member

Comment 5 by bugdroid1@chromium.org, Apr 27 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/591cbec71ef07912c8c7d2b0acc1efb25196d7f1

commit 591cbec71ef07912c8c7d2b0acc1efb25196d7f1
Author: pmeenan <pmeenan@chromium.org>
Date: Thu Apr 27 14:41:28 2017

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.

reland of http://crbug.com/2826213003

Only change from the original was to fix the layout test.  Specifically,
the URL for the preload image was not re-used for the actual image
which caused the test to issue a console warning about the preload
not being used but only if the overall test took long enough for the
warning to trigger.

Patch set #1 is the original CL untouched

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

patch from issue 2826213003 at patchset 70001 (http://crrev.com/2826213003#ps70001)

Review-Url: https://codereview.chromium.org/2841363002
Cr-Commit-Position: refs/heads/master@{#467672}

[modify] https://crrev.com/591cbec71ef07912c8c7d2b0acc1efb25196d7f1/third_party/WebKit/LayoutTests/http/tests/inspector/network/resource-priority-expected.txt
[modify] https://crrev.com/591cbec71ef07912c8c7d2b0acc1efb25196d7f1/third_party/WebKit/LayoutTests/http/tests/inspector/network/resource-priority.html
[add] https://crrev.com/591cbec71ef07912c8c7d2b0acc1efb25196d7f1/third_party/WebKit/LayoutTests/http/tests/inspector/network/resources/docwrite.js
[modify] https://crrev.com/591cbec71ef07912c8c7d2b0acc1efb25196d7f1/third_party/WebKit/Source/core/html/parser/BackgroundHTMLParser.cpp
[modify] https://crrev.com/591cbec71ef07912c8c7d2b0acc1efb25196d7f1/third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp
[modify] https://crrev.com/591cbec71ef07912c8c7d2b0acc1efb25196d7f1/third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.h
[modify] https://crrev.com/591cbec71ef07912c8c7d2b0acc1efb25196d7f1/third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.cpp
[modify] https://crrev.com/591cbec71ef07912c8c7d2b0acc1efb25196d7f1/third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.h
[modify] https://crrev.com/591cbec71ef07912c8c7d2b0acc1efb25196d7f1/third_party/WebKit/Source/core/html/parser/HTMLPreloadScannerFuzzer.cpp
[modify] https://crrev.com/591cbec71ef07912c8c7d2b0acc1efb25196d7f1/third_party/WebKit/Source/core/html/parser/HTMLPreloadScannerTest.cpp
[modify] https://crrev.com/591cbec71ef07912c8c7d2b0acc1efb25196d7f1/third_party/WebKit/Source/core/html/parser/PreloadRequest.cpp
[modify] https://crrev.com/591cbec71ef07912c8c7d2b0acc1efb25196d7f1/third_party/WebKit/Source/core/html/parser/PreloadRequest.h
[modify] https://crrev.com/591cbec71ef07912c8c7d2b0acc1efb25196d7f1/third_party/WebKit/Source/platform/loader/fetch/FetchParameters.cpp
[modify] https://crrev.com/591cbec71ef07912c8c7d2b0acc1efb25196d7f1/third_party/WebKit/Source/platform/loader/fetch/FetchParameters.h
[modify] https://crrev.com/591cbec71ef07912c8c7d2b0acc1efb25196d7f1/third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp
[modify] https://crrev.com/591cbec71ef07912c8c7d2b0acc1efb25196d7f1/third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.h

Status: Verified (was: Started)
Fix is in the current Canary and will roll out with 60.
Project Member

Comment 7 by bugdroid1@chromium.org, May 5 2017

Labels: merge-merged-3071
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d255072f8bf07ba7b1058fa6d9a5943d43cbeaef

commit d255072f8bf07ba7b1058fa6d9a5943d43cbeaef
Author: Patrick Meenan <pmeenan@chromium.org>
Date: Fri May 05 17:42:00 2017

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/2841363002
Cr-Commit-Position: refs/heads/master@{#467672}
(cherry picked from commit 591cbec71ef07912c8c7d2b0acc1efb25196d7f1)

Review-Url: https://codereview.chromium.org/2862303002 .
Cr-Commit-Position: refs/branch-heads/3071@{#420}
Cr-Branched-From: a106f0abbf69dad349d4aaf4bcc4f5d376dd2377-refs/heads/master@{#464641}

[modify] https://crrev.com/d255072f8bf07ba7b1058fa6d9a5943d43cbeaef/third_party/WebKit/LayoutTests/http/tests/inspector/network/resource-priority-expected.txt
[modify] https://crrev.com/d255072f8bf07ba7b1058fa6d9a5943d43cbeaef/third_party/WebKit/LayoutTests/http/tests/inspector/network/resource-priority.html
[add] https://crrev.com/d255072f8bf07ba7b1058fa6d9a5943d43cbeaef/third_party/WebKit/LayoutTests/http/tests/inspector/network/resources/docwrite.js
[modify] https://crrev.com/d255072f8bf07ba7b1058fa6d9a5943d43cbeaef/third_party/WebKit/Source/core/html/parser/BackgroundHTMLParser.cpp
[modify] https://crrev.com/d255072f8bf07ba7b1058fa6d9a5943d43cbeaef/third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp
[modify] https://crrev.com/d255072f8bf07ba7b1058fa6d9a5943d43cbeaef/third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.h
[modify] https://crrev.com/d255072f8bf07ba7b1058fa6d9a5943d43cbeaef/third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.cpp
[modify] https://crrev.com/d255072f8bf07ba7b1058fa6d9a5943d43cbeaef/third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.h
[modify] https://crrev.com/d255072f8bf07ba7b1058fa6d9a5943d43cbeaef/third_party/WebKit/Source/core/html/parser/HTMLPreloadScannerFuzzer.cpp
[modify] https://crrev.com/d255072f8bf07ba7b1058fa6d9a5943d43cbeaef/third_party/WebKit/Source/core/html/parser/HTMLPreloadScannerTest.cpp
[modify] https://crrev.com/d255072f8bf07ba7b1058fa6d9a5943d43cbeaef/third_party/WebKit/Source/core/html/parser/PreloadRequest.cpp
[modify] https://crrev.com/d255072f8bf07ba7b1058fa6d9a5943d43cbeaef/third_party/WebKit/Source/core/html/parser/PreloadRequest.h
[modify] https://crrev.com/d255072f8bf07ba7b1058fa6d9a5943d43cbeaef/third_party/WebKit/Source/platform/loader/fetch/FetchParameters.cpp
[modify] https://crrev.com/d255072f8bf07ba7b1058fa6d9a5943d43cbeaef/third_party/WebKit/Source/platform/loader/fetch/FetchParameters.h
[modify] https://crrev.com/d255072f8bf07ba7b1058fa6d9a5943d43cbeaef/third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp
[modify] https://crrev.com/d255072f8bf07ba7b1058fa6d9a5943d43cbeaef/third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.h

Sign in to add a comment