New issue
Advanced search Search tips

Issue 777612 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

Create PendingScript during ScriptLoader::PrepareScript()

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

Issue description

Previously, PendingScript can be created a little after PrepareScript().

This causes glue code in ScriptLoader to bridge the gap between fetch start (e.g. ScriptResource::Fetch()) and PendingScript creation.

Also, this gap decouples ScriptResource::Fetch() and its corresponding AddClient(), and thus makes it harder to assign TaskRunner to AddClient().
Context: [1] https://chromium-review.googlesource.com/c/chromium/src/+/723879

This issue aims to create PendingScript inside PrepareScript(), to resolve these issues.

This was also planned before ES6 module scripts are implemented
(as a part of doc:
https://docs.google.com/document/d/178GWQTPd-0HW4rH3hHnu08QLCb2sLxEhrtZ57V_Yolo/edit#heading=h.ojp3k4lrik91 )
but was postponed because it was hard at that time and it didn't block ES6 modules.
I restart the effort because now this blocks [1] and this is easier now after several refactoring in ScriptLoader.

 
Blocking: 686281
Cc: kinuko@chromium.org
This issue will also removes |ScriptLoader::resource_|, which is used for "delayed" PendingScript creation.
However, as a side effect, this causes the ScriptResource to be garbage-collected earlier, because after script execution |ScriptLoader::resource_| was the only reference from ScriptLoader to ScriptResource (in the case of classic scripts)...hmm

Comment 3 by kouhei@chromium.org, Oct 24 2017

c#3 I think this is the correct change to make.
We can always have a separate experiment if we really like to prolong ScriptResource lifetimes.
Project Member

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

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

commit 2e18a5d7159cad3f1db73ff69279504d2937b661
Author: Hiroshige Hayashizaki <hiroshige@chromium.org>
Date: Tue Oct 24 20:00:22 2017

Introduce ClassicPendingScript::is_external_ explicitly

To clarify whether it is external and decouple IsExternal()
(which must be constant during the ClassicPendingScript lifetime)
from GetResource() (which can manipulated for Resource lifetime
management or due to initialization/disposing order changes).

Particularly,
https://chromium-review.googlesource.com/c/chromium/src/+/730852
will create ClassicPendingScript before ScriptResource::Fetch() and
thus GetResource() is nullptr when an external script is created.

Migrated from https://codereview.chromium.org/2846363003

BUG: 686281,  777612 
Change-Id: I9373e731cf17b4b5b2545566c6b9c565447a7699
Reviewed-on: https://chromium-review.googlesource.com/602256
Commit-Queue: Hiroshige Hayashizaki <hiroshige@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Reviewed-by: Nate Chapin <japhet@chromium.org>
Cr-Commit-Position: refs/heads/master@{#511240}
[modify] https://crrev.com/2e18a5d7159cad3f1db73ff69279504d2937b661/third_party/WebKit/Source/core/dom/ClassicPendingScript.cpp
[modify] https://crrev.com/2e18a5d7159cad3f1db73ff69279504d2937b661/third_party/WebKit/Source/core/dom/ClassicPendingScript.h

Project Member

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

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

commit 9f8298b5ce1a7f67d3995606d50e63d56767dc67
Author: Hiroshige Hayashizaki <hiroshige@chromium.org>
Date: Tue Oct 24 22:20:23 2017

Remove ScriptLoader::GetResource()

HTMLParserScriptRunner obtains classic script's URL for trace arguments
via ScriptLoader::GetResource().
This CL replaces it with PendingScript::UrlForClassicScript().

This is to preparation for removing |ScriptLoader::resource_|
in https://chromium-review.googlesource.com/c/chromium/src/+/730852.

Bug: 686281,  777612 
Change-Id: I22bdb038a84cad2fb738ae4a762e97ed7df67786
Reviewed-on: https://chromium-review.googlesource.com/734137
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Reviewed-by: Nate Chapin <japhet@chromium.org>
Commit-Queue: Hiroshige Hayashizaki <hiroshige@chromium.org>
Cr-Commit-Position: refs/heads/master@{#511281}
[modify] https://crrev.com/9f8298b5ce1a7f67d3995606d50e63d56767dc67/third_party/WebKit/Source/core/dom/ScriptLoader.h
[modify] https://crrev.com/9f8298b5ce1a7f67d3995606d50e63d56767dc67/third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.cpp

Project Member

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

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

commit 9d0b896c09461e4c1fcee17856f768d3e66880bf
Author: Hiroshige Hayashizaki <hiroshige@chromium.org>
Date: Thu Oct 26 22:39:56 2017

Create PendingScript when script loading is started in PrepareScript()

Previously, PendingScript was created in
ScriptLoader::CreatePendingScript() which could be invoked by
non-ScriptLoader after ScriptLoader::PrepareScript().

This CL creates PendingScript during ScriptLoader::PrepareScript()
when the script fetch is initiated.

This is in order to:
- Couple ScriptResource::Fetch() and SetResource() (and thus
  AddClient()) to make it easier to assign TaskRunner to AddClient().
  (Context:
  https://chromium-review.googlesource.com/c/chromium/src/+/723879)
- Simplify the lifetime of PendingScript, and remove glue code for
  bridging PrepareScript() and CreatePendingScript().

This CL renames |ScriptLoader::resource_| into |resource_keep_alive_|,
because it is now used only to keep the ScriptResource alive even
after script execution.
This CL doesn't remove the reference from ScriptLoader to
ScriptResource, because removing it makes the ScriptResource's
lifetime shorter and thus evicted from MemoryCache (because it is
the only reference to the ScriptResource after script execution),
and causes devtools layout test failures.

Bug:  777612 , 686281, 778799
Change-Id: I5ea0a393ee567520c526ee69b82e08281caa7a7d
Reviewed-on: https://chromium-review.googlesource.com/730852
Commit-Queue: Hiroshige Hayashizaki <hiroshige@chromium.org>
Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#511991}
[modify] https://crrev.com/9d0b896c09461e4c1fcee17856f768d3e66880bf/third_party/WebKit/Source/bindings/core/v8/ScriptStreamerTest.cpp
[modify] https://crrev.com/9d0b896c09461e4c1fcee17856f768d3e66880bf/third_party/WebKit/Source/core/dom/ClassicPendingScript.cpp
[modify] https://crrev.com/9d0b896c09461e4c1fcee17856f768d3e66880bf/third_party/WebKit/Source/core/dom/ClassicPendingScript.h
[modify] https://crrev.com/9d0b896c09461e4c1fcee17856f768d3e66880bf/third_party/WebKit/Source/core/dom/PendingScript.h
[modify] https://crrev.com/9d0b896c09461e4c1fcee17856f768d3e66880bf/third_party/WebKit/Source/core/dom/ScriptLoader.cpp
[modify] https://crrev.com/9d0b896c09461e4c1fcee17856f768d3e66880bf/third_party/WebKit/Source/core/dom/ScriptLoader.h
[modify] https://crrev.com/9d0b896c09461e4c1fcee17856f768d3e66880bf/third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.cpp
[modify] https://crrev.com/9d0b896c09461e4c1fcee17856f768d3e66880bf/third_party/WebKit/Source/core/xml/parser/XMLParserScriptRunner.cpp

Project Member

Comment 7 by bugdroid1@chromium.org, Oct 27 2017

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

commit 859a3de4c564c8a8f6a77704df510d325fe4d57e
Author: Hiroshige Hayashizaki <hiroshige@chromium.org>
Date: Fri Oct 27 21:00:31 2017

Centralize ClassicPendingScript::CreateInline() call sites

This merges all ClassicPendingScript::CreateInline() call sites
into one in ScriptLoader::PrepareScript().
This
- reduces code duplication,
- makes PendingScript always created before the last step of
  ScriptLoader::PrepareScript() and makes TakePrepareScript()
  always called, and
- prepares for
  https://chromium-review.googlesource.com/c/chromium/src/+/736876.

Behavior changes: This CL resets line numbering for nested writes
for classic inline scripts controlled by {HTML,XML}ParserScriptRunner.
Previously, the line numbering is reset only when the classic inline
script is executed inside PrepareScript().

Bug:  771486 ,  777612 , 686281
Change-Id: Ibcaf9bcfa958bb302b1d7f5b7f17c501a7ac7027
Reviewed-on: https://chromium-review.googlesource.com/737055
Commit-Queue: Hiroshige Hayashizaki <hiroshige@chromium.org>
Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#512269}
[modify] https://crrev.com/859a3de4c564c8a8f6a77704df510d325fe4d57e/third_party/WebKit/Source/core/dom/ScriptLoader.cpp
[modify] https://crrev.com/859a3de4c564c8a8f6a77704df510d325fe4d57e/third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.cpp
[modify] https://crrev.com/859a3de4c564c8a8f6a77704df510d325fe4d57e/third_party/WebKit/Source/core/xml/parser/XMLParserScriptRunner.cpp

Project Member

Comment 8 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

Project Member

Comment 9 by bugdroid1@chromium.org, Nov 4 2017

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

commit 9a942faab2c8850c567c50fe73dc0d4a4976e4d8
Author: Hiroshige Hayashizaki <hiroshige@chromium.org>
Date: Sat Nov 04 14:03:05 2017

Fix crashes in ClassicPendingScript::UrlForClassicScript()

Follow-up / bug fix of [1].

Before [1], the URL for tracing is obtained only when
ScriptLoader::GetResource() is non-null (i.e. for classic
external scripts). But after [1]
ClassicPendingScript::UrlForClassicScript() tries to get the URL
even for classic inline scripts, and thus is crashing.

This CL adds checks to avoid null-dereferencing GetResource()
(i.e. returns NullURL() for classic inline scripts).

[1] https://chromium-review.googlesource.com/734137

Bug:  780598 ,  780412 , 686281,  777612 
Change-Id: I775faa270ecb7788a448e703db0689a17ea80aee
Reviewed-on: https://chromium-review.googlesource.com/754101
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Commit-Queue: Kouhei Ueno <kouhei@chromium.org>
Cr-Commit-Position: refs/heads/master@{#514049}
[modify] https://crrev.com/9a942faab2c8850c567c50fe73dc0d4a4976e4d8/third_party/WebKit/Source/core/dom/ClassicPendingScript.cpp

Status: Fixed (was: Started)
Project Member

Comment 11 by bugdroid1@chromium.org, Nov 15 2017

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

commit 79872df78047d79fbdab74da35fc31ecd3c89b33
Author: Hiroshige Hayashizaki <hiroshige@chromium.org>
Date: Wed Nov 15 13:42:12 2017

Clean up the code around PendingScript::UrlForClassicScript()

This CL
- Renames PendingScript::UrlForClassicScript() to UrlForTracing()
  and updates its comment, as it is now used only for tracing, and
- Allows calling UrlForTracing() for module scripts, and simplifies
  GetTraceArgsForScriptElement() conditions.

No behavior changes.

Bug: 686281,  777612 
Change-Id: Id2d6741071d03a697d49a8573967c6a4d2557224
Reviewed-on: https://chromium-review.googlesource.com/753531
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Commit-Queue: Hiroshige Hayashizaki <hiroshige@chromium.org>
Cr-Commit-Position: refs/heads/master@{#516685}
[modify] https://crrev.com/79872df78047d79fbdab74da35fc31ecd3c89b33/third_party/WebKit/Source/core/dom/ClassicPendingScript.cpp
[modify] https://crrev.com/79872df78047d79fbdab74da35fc31ecd3c89b33/third_party/WebKit/Source/core/dom/ClassicPendingScript.h
[modify] https://crrev.com/79872df78047d79fbdab74da35fc31ecd3c89b33/third_party/WebKit/Source/core/dom/ModulePendingScript.h
[modify] https://crrev.com/79872df78047d79fbdab74da35fc31ecd3c89b33/third_party/WebKit/Source/core/dom/PendingScript.h
[modify] https://crrev.com/79872df78047d79fbdab74da35fc31ecd3c89b33/third_party/WebKit/Source/core/dom/ScriptRunnerTest.cpp
[modify] https://crrev.com/79872df78047d79fbdab74da35fc31ecd3c89b33/third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.cpp

Sign in to add a comment