Create PendingScript during ScriptLoader::PrepareScript() |
|||
Issue descriptionPreviously, 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.
,
Oct 23 2017
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
,
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.
,
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
,
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
,
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
,
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
,
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
,
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
,
Nov 6 2017
,
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 |
|||
Comment 1 by hirosh...@chromium.org
, Oct 23 2017