New issue
Advanced search Search tips

Issue 777626 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Nov 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 686281



Sign in to add a comment

Decouple document.write() intervention from ScriptLoader

Project Member Reported by hirosh...@chromium.org, Oct 23 2017

Issue description

Currently, document.write() intervention is implemented by creating a new ScriptLoader for sending asynchronous request to blocked scripts.
However,
- This makes ScriptLoaders NOT correspond one by one to script elements,
- The asynchronous request should not cause script execution but
  ScriptLoader is the core class for script execution.

This issue decouples document.write() intervention from ScriptLoader, and
makes ScriptLoader and ScriptElementBase correspond one by one.

Main CL: https://chromium-review.googlesource.com/c/chromium/src/+/722162

 
Blocking: -652228 686281
Project Member

Comment 2 by bugdroid1@chromium.org, Oct 24 2017

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

commit 8d7feb3382e35ff57a1838c937345a59e366d07b
Author: Hiroshige Hayashizaki <hiroshige@chromium.org>
Date: Tue Oct 24 19:03:47 2017

Do not explicitly remove intervened ScriptResource from MemoryCache

Previously, we remove intervened ScriptResource from MemoryCache
in two cases:
1. The ScriptResource blocked due to intervention in
   HTMLParserScriptRunner::PossiblyFetchBlockedDocWriteScript(), and
2. The ScriptResource requested asynchronously to the blocked script
   in ScriptLoader::PendingScriptFinished().

This CL stops removing the ScriptResource from MemoryCache.

Correctness: Safe because:
- For Case 1, the ScriptResource should be already not on MemoryCache
  because it is errored due to blocking, and
- For Case 2, it is harmless to leave the ScriptResource on MemoryCache,
  because it doesn't affect the lifetime, and it doesn't break
  caching correctness.

Resource eviction from MemoryCache:
Slightly later but no persistent effects:
- Before this CL: the ScriptResource that is asynchronously-fetched by
  document.write() intervention is evicted immediately at load finish.
- After this CL: the ScriptResource is evicted in the next Oilpan GC
  after loading finishes.

Code: Simpler by removing PendingScript::RemoveFromMemoryCache().

Bug:  777626 , 686281
Change-Id: Ic6386103b73a76c8a8440d47c2ae5e0c6de62166
Reviewed-on: https://chromium-review.googlesource.com/721250
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Commit-Queue: Hiroshige Hayashizaki <hiroshige@chromium.org>
Cr-Commit-Position: refs/heads/master@{#511224}
[modify] https://crrev.com/8d7feb3382e35ff57a1838c937345a59e366d07b/third_party/WebKit/Source/core/dom/ClassicPendingScript.cpp
[modify] https://crrev.com/8d7feb3382e35ff57a1838c937345a59e366d07b/third_party/WebKit/Source/core/dom/ClassicPendingScript.h
[modify] https://crrev.com/8d7feb3382e35ff57a1838c937345a59e366d07b/third_party/WebKit/Source/core/dom/ModulePendingScript.h
[modify] https://crrev.com/8d7feb3382e35ff57a1838c937345a59e366d07b/third_party/WebKit/Source/core/dom/PendingScript.h
[modify] https://crrev.com/8d7feb3382e35ff57a1838c937345a59e366d07b/third_party/WebKit/Source/core/dom/ScriptLoader.cpp
[modify] https://crrev.com/8d7feb3382e35ff57a1838c937345a59e366d07b/third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.cpp

Project Member

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

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

commit f2cc4535785ac845c2729470d1b71f4b9b28bb46
Author: Hiroshige Hayashizaki <hiroshige@chromium.org>
Date: Wed Oct 25 19:10:40 2017

Move document.write() intervention layout tests out of loading/

The tests were placed under loading/ and thus messages for frame
loading were output.
After https://chromium-review.googlesource.com/c/chromium/src/+/722162,
error/warning messages for document.write() intervention might appear
before or after frame loading messages, and thus the tests become flaky.

This CL deflakes the tests by suppressing the frame loading messages
by moving the tests out of loading/.

Bug:  777626 
Change-Id: Iffd7f3fccebc5c652ccdb7cd19f8d21a69f77cfa
Reviewed-on: https://chromium-review.googlesource.com/736549
Reviewed-by: Shivani Sharma <shivanisha@chromium.org>
Commit-Queue: Hiroshige Hayashizaki <hiroshige@chromium.org>
Cr-Commit-Position: refs/heads/master@{#511545}
[modify] https://crrev.com/f2cc4535785ac845c2729470d1b71f4b9b28bb46/third_party/WebKit/LayoutTests/FlagExpectations/enable-features=NetworkService
[modify] https://crrev.com/f2cc4535785ac845c2729470d1b71f4b9b28bb46/third_party/WebKit/LayoutTests/TestExpectations
[rename] https://crrev.com/f2cc4535785ac845c2729470d1b71f4b9b28bb46/third_party/WebKit/LayoutTests/http/tests/doc-write-intervention/doc-write-async-third-party-script.html
[rename] https://crrev.com/f2cc4535785ac845c2729470d1b71f4b9b28bb46/third_party/WebKit/LayoutTests/http/tests/doc-write-intervention/doc-write-sync-third-party-script-block-all-conn-types-expected.txt
[rename] https://crrev.com/f2cc4535785ac845c2729470d1b71f4b9b28bb46/third_party/WebKit/LayoutTests/http/tests/doc-write-intervention/doc-write-sync-third-party-script-block-all-conn-types.html
[rename] https://crrev.com/f2cc4535785ac845c2729470d1b71f4b9b28bb46/third_party/WebKit/LayoutTests/http/tests/doc-write-intervention/doc-write-sync-third-party-script-block-effectively-2g-expected.txt
[rename] https://crrev.com/f2cc4535785ac845c2729470d1b71f4b9b28bb46/third_party/WebKit/LayoutTests/http/tests/doc-write-intervention/doc-write-sync-third-party-script-block-effectively-2g.html
[rename] https://crrev.com/f2cc4535785ac845c2729470d1b71f4b9b28bb46/third_party/WebKit/LayoutTests/http/tests/doc-write-intervention/doc-write-sync-third-party-script-block-expected.txt
[rename] https://crrev.com/f2cc4535785ac845c2729470d1b71f4b9b28bb46/third_party/WebKit/LayoutTests/http/tests/doc-write-intervention/doc-write-sync-third-party-script-block.html
[rename] https://crrev.com/f2cc4535785ac845c2729470d1b71f4b9b28bb46/third_party/WebKit/LayoutTests/http/tests/doc-write-intervention/doc-write-sync-third-party-script-conn-type-expected.txt
[rename] https://crrev.com/f2cc4535785ac845c2729470d1b71f4b9b28bb46/third_party/WebKit/LayoutTests/http/tests/doc-write-intervention/doc-write-sync-third-party-script-conn-type.html
[rename] https://crrev.com/f2cc4535785ac845c2729470d1b71f4b9b28bb46/third_party/WebKit/LayoutTests/http/tests/doc-write-intervention/doc-write-sync-third-party-script-reload-expected.txt
[rename] https://crrev.com/f2cc4535785ac845c2729470d1b71f4b9b28bb46/third_party/WebKit/LayoutTests/http/tests/doc-write-intervention/doc-write-sync-third-party-script-reload.html

Project Member

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

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

commit 0f36150300cbfd42043e81b58104d1e05393ed94
Author: Hiroshige Hayashizaki <hiroshige@chromium.org>
Date: Thu Oct 26 01:10:31 2017

Decouple logic for sending requests to blocked scripts from ScriptLoader

Previously, document.write() intervention sends an asynchronous request
to a blocked script by creating a new ScriptLoader that is not referenced
from the corresponding HTMLScriptElement and by running its
ScriptLoader::PrepareScript().
This is not good in terms of code health because:
- This makes ScriptLoaders NOT correspond one by one to script elements,
- The asynchronous request should not cause script execution but
  ScriptLoader is the core class for script execution.

To resolve these issues, this CL moves the intervention logic from
ScriptLoader and HTMLParserScriptRunner to DocumentWriteIntervention.
Instead of using ScriptLoader, the asynchronous request is send via
FetchBlockedDocWriteScriptClient, separately from the code paths for
script execution such as ClassicPendingScript, ScriptLoader, and
HTMLParserScriptRunner.

Bug:  777626 , 686281
Change-Id: I174bb85a184b741b41ef98c2d0d7501cab00cbb9
Reviewed-on: https://chromium-review.googlesource.com/722162
Commit-Queue: Hiroshige Hayashizaki <hiroshige@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Shivani Sharma <shivanisha@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Cr-Commit-Position: refs/heads/master@{#511678}
[modify] https://crrev.com/0f36150300cbfd42043e81b58104d1e05393ed94/third_party/WebKit/LayoutTests/http/tests/doc-write-intervention/doc-write-sync-third-party-script-block-expected.txt
[modify] https://crrev.com/0f36150300cbfd42043e81b58104d1e05393ed94/third_party/WebKit/LayoutTests/http/tests/doc-write-intervention/doc-write-sync-third-party-script-block.html
[modify] https://crrev.com/0f36150300cbfd42043e81b58104d1e05393ed94/third_party/WebKit/Source/core/dom/DocumentWriteIntervention.cpp
[modify] https://crrev.com/0f36150300cbfd42043e81b58104d1e05393ed94/third_party/WebKit/Source/core/dom/DocumentWriteIntervention.h
[modify] https://crrev.com/0f36150300cbfd42043e81b58104d1e05393ed94/third_party/WebKit/Source/core/dom/ScriptLoader.cpp
[modify] https://crrev.com/0f36150300cbfd42043e81b58104d1e05393ed94/third_party/WebKit/Source/core/dom/ScriptLoader.h
[modify] https://crrev.com/0f36150300cbfd42043e81b58104d1e05393ed94/third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.cpp
[modify] https://crrev.com/0f36150300cbfd42043e81b58104d1e05393ed94/third_party/WebKit/Source/core/html/parser/PreloadRequest.cpp
[modify] https://crrev.com/0f36150300cbfd42043e81b58104d1e05393ed94/third_party/WebKit/Source/platform/loader/fetch/FetchParameters.h

Project Member

Comment 5 by bugdroid1@chromium.org, Nov 1 2017

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

commit 43fd0bdec8f2d7141bf61624fe7a4c69184b2517
Author: Hiroshige Hayashizaki <hiroshige@chromium.org>
Date: Wed Nov 01 05:27:16 2017

Remove ScriptElementBase::InitiatorName()

ScriptElementBase::InitiatorName() is "script" for both of HTMLScriptElement and
SVGScriptElement.
This CL replaces InitiatorName() with hard-coded "script" to reduce dependencies to
ScriptElementBase.

Bug:  777626 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: I39bf17f0e95e169acf2cf4956c84d35b0511b618
Reviewed-on: https://chromium-review.googlesource.com/740873
Commit-Queue: Hiroshige Hayashizaki <hiroshige@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Cr-Commit-Position: refs/heads/master@{#513079}
[modify] https://crrev.com/43fd0bdec8f2d7141bf61624fe7a4c69184b2517/third_party/WebKit/Source/core/dom/ClassicPendingScript.cpp
[modify] https://crrev.com/43fd0bdec8f2d7141bf61624fe7a4c69184b2517/third_party/WebKit/Source/core/dom/ScriptElementBase.h
[modify] https://crrev.com/43fd0bdec8f2d7141bf61624fe7a4c69184b2517/third_party/WebKit/Source/core/html/HTMLScriptElement.cpp
[modify] https://crrev.com/43fd0bdec8f2d7141bf61624fe7a4c69184b2517/third_party/WebKit/Source/core/html/HTMLScriptElement.h
[modify] https://crrev.com/43fd0bdec8f2d7141bf61624fe7a4c69184b2517/third_party/WebKit/Source/core/svg/SVGScriptElement.cpp
[modify] https://crrev.com/43fd0bdec8f2d7141bf61624fe7a4c69184b2517/third_party/WebKit/Source/core/svg/SVGScriptElement.h

Project Member

Comment 6 by bugdroid1@chromium.org, Nov 3 2017

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

commit 570093f29e09ec6902ce80a4b0051373151e00c0
Author: Hiroshige Hayashizaki <hiroshige@chromium.org>
Date: Fri Nov 03 20:47:28 2017

Use ScriptFetchOptions for document.write() intervention

Instead of saving FetchParameters in FetchBlockedDocWriteScriptClient,
this CL uses ScriptFetchOptions stored in ClassicPendingScript, because
ScriptFetchOptions is cleaner and smaller than FetchParameters.

FetchBlockedDocWriteScriptClient is removed and the logic of
FetchBlockedDocWriteScriptClient::NotifyFinished() is moved to
PossiblyFetchBlockedDocWriteScript() that is called from
ClassicPendingScript::NotifyFinished().
This saves memory (by removing the stored FetchParameters) and
makes ScriptResource::Fetch() and AddClient() in ClassicPendingScript
correspond one by one.

The FetchParameters construction code in ClassicPendingScript::Fetch()
is moved to ScriptFetchOptions::CreateFetchParameters() in order to
use it from DocumentWriteIntervention.cpp.

Bug:  777626 ,  777612 
Change-Id: Ia61673e20d6e6c6553af04b0393e236599c9eb9d
Reviewed-on: https://chromium-review.googlesource.com/737269
Commit-Queue: Hiroshige Hayashizaki <hiroshige@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Cr-Commit-Position: refs/heads/master@{#513900}
[modify] https://crrev.com/570093f29e09ec6902ce80a4b0051373151e00c0/third_party/WebKit/Source/core/dom/ClassicPendingScript.cpp
[modify] https://crrev.com/570093f29e09ec6902ce80a4b0051373151e00c0/third_party/WebKit/Source/core/dom/ClassicPendingScript.h
[modify] https://crrev.com/570093f29e09ec6902ce80a4b0051373151e00c0/third_party/WebKit/Source/core/dom/DocumentWriteIntervention.cpp
[modify] https://crrev.com/570093f29e09ec6902ce80a4b0051373151e00c0/third_party/WebKit/Source/core/dom/DocumentWriteIntervention.h
[modify] https://crrev.com/570093f29e09ec6902ce80a4b0051373151e00c0/third_party/WebKit/Source/platform/loader/BUILD.gn
[add] https://crrev.com/570093f29e09ec6902ce80a4b0051373151e00c0/third_party/WebKit/Source/platform/loader/fetch/ScriptFetchOptions.cpp
[modify] https://crrev.com/570093f29e09ec6902ce80a4b0051373151e00c0/third_party/WebKit/Source/platform/loader/fetch/ScriptFetchOptions.h

Status: Fixed (was: Started)

Sign in to add a comment