New issue
Advanced search Search tips

Issue 725816 link

Starred by 2 users

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug



Sign in to add a comment

<script type=module> leaks v8::Context and blink::Document

Project Member Reported by kouhei@chromium.org, May 24 2017

Issue description

There are multiple failures. I'll use this crbug entry as an ☂ to track fix CLs.

Please feel free to file leak bugs be blocked on this crbug.
 
Project Member

Comment 1 by bugdroid1@chromium.org, May 25 2017

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

commit 7fa31ddf3b196d9873d615e7aabe4285fed4cef8
Author: kouhei <kouhei@chromium.org>
Date: Thu May 25 03:33:19 2017

[ES6 modules] Fix context leak. ModuleScript should use TraceWrapperV8Reference to hold onto v8::Module.

Before this CL, ModuleScript referenced v8::Module via SharedPersistent<> in ScriptModule.
This created cycle of v8::Context -> V8PerContextData -> Modulator -> ModuleMap -> ModuleScript -(persistent)-> v8::Module -> v8::Context.

This CL breaks the cycle (partially) by making ModuleScript use TraceWrapperV8Reference to reference v8::Module, and create ScriptModule on the fly.
More CLs will follow to break other parts of the cycle graph.

BUG= 594639 , 725816

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

[add] https://crrev.com/7fa31ddf3b196d9873d615e7aabe4285fed4cef8/third_party/WebKit/LayoutTests/fast/dom/script-module-with-export-leak-expected.txt
[add] https://crrev.com/7fa31ddf3b196d9873d615e7aabe4285fed4cef8/third_party/WebKit/LayoutTests/fast/dom/script-module-with-export-leak.html
[modify] https://crrev.com/7fa31ddf3b196d9873d615e7aabe4285fed4cef8/third_party/WebKit/Source/bindings/core/v8/ScriptModule.cpp
[modify] https://crrev.com/7fa31ddf3b196d9873d615e7aabe4285fed4cef8/third_party/WebKit/Source/bindings/core/v8/ScriptModule.h
[modify] https://crrev.com/7fa31ddf3b196d9873d615e7aabe4285fed4cef8/third_party/WebKit/Source/core/dom/Modulator.h
[modify] https://crrev.com/7fa31ddf3b196d9873d615e7aabe4285fed4cef8/third_party/WebKit/Source/core/dom/ModulatorImpl.h
[modify] https://crrev.com/7fa31ddf3b196d9873d615e7aabe4285fed4cef8/third_party/WebKit/Source/core/dom/ModuleScript.cpp
[modify] https://crrev.com/7fa31ddf3b196d9873d615e7aabe4285fed4cef8/third_party/WebKit/Source/core/dom/ModuleScript.h
[modify] https://crrev.com/7fa31ddf3b196d9873d615e7aabe4285fed4cef8/third_party/WebKit/Source/core/dom/ScriptModuleResolverImplTest.cpp
[modify] https://crrev.com/7fa31ddf3b196d9873d615e7aabe4285fed4cef8/third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoaderTest.cpp
[modify] https://crrev.com/7fa31ddf3b196d9873d615e7aabe4285fed4cef8/third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinkerTest.cpp
[modify] https://crrev.com/7fa31ddf3b196d9873d615e7aabe4285fed4cef8/third_party/WebKit/Source/core/testing/DummyModulator.cpp
[modify] https://crrev.com/7fa31ddf3b196d9873d615e7aabe4285fed4cef8/third_party/WebKit/Source/core/testing/DummyModulator.h

Blocking: 718442
Blocking: 718114
Project Member

Comment 4 by bugdroid1@chromium.org, May 29 2017

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

commit 74ba772e23ab732e4d4f2c2c2bcd76fbff9c78d5
Author: Kouhei Ueno <kouhei@chromium.org>
Date: Mon May 29 13:08:43 2017

[ES6 modules] Detach ScriptModuleResolverImpl refs when detaching context.

Before this CL, we kept around strong v8::Module refs from
ScriptModuleResolverImpl after the context is detached.
v8::Module refers to the settings object Document via Modulator, so
lead to leaks detected by Blink Leak Detector.

This CL detaches the v8::Module strong refs from ScriptModuleResolverImpl
when we detach Document from the context.

This CL is meant to be a temporary fix. Ideally, we should convert
all v8::Module refs in ScriptModuleResolverImpl to weak refs.
However, it is not trivial as we need to rework ScriptModule.

Bug:  594639 , 725816,  724818 
Change-Id: If5b697bc87b9cd907333be0afe713888c1be2cf9
Reviewed-on: https://chromium-review.googlesource.com/517525
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Commit-Queue: Kouhei Ueno <kouhei@chromium.org>
Cr-Commit-Position: refs/heads/master@{#475340}
[modify] https://crrev.com/74ba772e23ab732e4d4f2c2c2bcd76fbff9c78d5/third_party/WebKit/LayoutTests/LeakExpectations
[modify] https://crrev.com/74ba772e23ab732e4d4f2c2c2bcd76fbff9c78d5/third_party/WebKit/Source/core/dom/ModulatorImpl.cpp
[modify] https://crrev.com/74ba772e23ab732e4d4f2c2c2bcd76fbff9c78d5/third_party/WebKit/Source/core/dom/ScriptModuleResolverImpl.cpp
[modify] https://crrev.com/74ba772e23ab732e4d4f2c2c2bcd76fbff9c78d5/third_party/WebKit/Source/core/dom/ScriptModuleResolverImpl.h
[modify] https://crrev.com/74ba772e23ab732e4d4f2c2c2bcd76fbff9c78d5/third_party/WebKit/Source/core/dom/ScriptModuleResolverImplTest.cpp

Comment 5 by kouhei@chromium.org, May 30 2017

Blocking: -718114 -594639 -718442
EstimatedDays: ----
Labels: -Pri-1 Pri-2
Unblocking bugs and lowering the priority, as now the leak is workaround.
Will be set to "Fixed" when the strong ref is removed from ScriptModuleResolverImpl.

Comment 6 by kouhei@chromium.org, May 31 2017

Status: Assigned (was: Started)
Project Member

Comment 7 by bugdroid1@chromium.org, Jun 26 2017

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

commit 73bb506d40e7618c72d23bb949fc37928ea68f21
Author: Kouhei Ueno <kouhei@chromium.org>
Date: Mon Jun 26 08:04:24 2017

[ES6 modules] TraceWrapper from ScriptLoader and ModuleTreeLinkerRegistry

Before this CL, TraceWrapperV8References on ModuleScript were relying on
trace from Modulator->ModuleMap. However, this is insufficient, as
inline module scripts would not have an entry on module map.

This CL fixes the issue by introducing wrapper tracing to ScriptLoader
and ModuleTreeLinkerRegistry->ModuleTreeLinker object graphs.

Bug:  594639 , 725816,  732270 
Change-Id: Id4672f3daee90ae007c1ce0c9ea3608b246b129e
Reviewed-on: https://chromium-review.googlesource.com/547157
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Commit-Queue: Kouhei Ueno <kouhei@chromium.org>
Cr-Commit-Position: refs/heads/master@{#482212}
[add] https://crrev.com/73bb506d40e7618c72d23bb949fc37928ea68f21/third_party/WebKit/LayoutTests/fast/dom/script-module-inline-error-gc-expected.txt
[add] https://crrev.com/73bb506d40e7618c72d23bb949fc37928ea68f21/third_party/WebKit/LayoutTests/fast/dom/script-module-inline-error-gc.html
[modify] https://crrev.com/73bb506d40e7618c72d23bb949fc37928ea68f21/third_party/WebKit/Source/bindings/core/v8/ScriptStreamerTest.cpp
[modify] https://crrev.com/73bb506d40e7618c72d23bb949fc37928ea68f21/third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp
[modify] https://crrev.com/73bb506d40e7618c72d23bb949fc37928ea68f21/third_party/WebKit/Source/core/dom/MockScriptElementBase.h
[modify] https://crrev.com/73bb506d40e7618c72d23bb949fc37928ea68f21/third_party/WebKit/Source/core/dom/Modulator.h
[modify] https://crrev.com/73bb506d40e7618c72d23bb949fc37928ea68f21/third_party/WebKit/Source/core/dom/ModulatorImpl.cpp
[modify] https://crrev.com/73bb506d40e7618c72d23bb949fc37928ea68f21/third_party/WebKit/Source/core/dom/ModulatorImpl.h
[modify] https://crrev.com/73bb506d40e7618c72d23bb949fc37928ea68f21/third_party/WebKit/Source/core/dom/ModuleMapTest.cpp
[modify] https://crrev.com/73bb506d40e7618c72d23bb949fc37928ea68f21/third_party/WebKit/Source/core/dom/ModulePendingScript.cpp
[modify] https://crrev.com/73bb506d40e7618c72d23bb949fc37928ea68f21/third_party/WebKit/Source/core/dom/ModulePendingScript.h
[modify] https://crrev.com/73bb506d40e7618c72d23bb949fc37928ea68f21/third_party/WebKit/Source/core/dom/ModuleScript.cpp
[modify] https://crrev.com/73bb506d40e7618c72d23bb949fc37928ea68f21/third_party/WebKit/Source/core/dom/ModuleScript.h
[modify] https://crrev.com/73bb506d40e7618c72d23bb949fc37928ea68f21/third_party/WebKit/Source/core/dom/PendingScript.h
[modify] https://crrev.com/73bb506d40e7618c72d23bb949fc37928ea68f21/third_party/WebKit/Source/core/dom/ScriptElementBase.cpp
[modify] https://crrev.com/73bb506d40e7618c72d23bb949fc37928ea68f21/third_party/WebKit/Source/core/dom/ScriptElementBase.h
[modify] https://crrev.com/73bb506d40e7618c72d23bb949fc37928ea68f21/third_party/WebKit/Source/core/dom/ScriptLoader.cpp
[modify] https://crrev.com/73bb506d40e7618c72d23bb949fc37928ea68f21/third_party/WebKit/Source/core/dom/ScriptLoader.h
[modify] https://crrev.com/73bb506d40e7618c72d23bb949fc37928ea68f21/third_party/WebKit/Source/core/html/HTMLScriptElement.cpp
[modify] https://crrev.com/73bb506d40e7618c72d23bb949fc37928ea68f21/third_party/WebKit/Source/core/html/HTMLScriptElement.h
[modify] https://crrev.com/73bb506d40e7618c72d23bb949fc37928ea68f21/third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.h
[modify] https://crrev.com/73bb506d40e7618c72d23bb949fc37928ea68f21/third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.cpp
[modify] https://crrev.com/73bb506d40e7618c72d23bb949fc37928ea68f21/third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinker.h
[modify] https://crrev.com/73bb506d40e7618c72d23bb949fc37928ea68f21/third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinkerRegistry.cpp
[modify] https://crrev.com/73bb506d40e7618c72d23bb949fc37928ea68f21/third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinkerRegistry.h
[modify] https://crrev.com/73bb506d40e7618c72d23bb949fc37928ea68f21/third_party/WebKit/Source/core/loader/modulescript/ModuleTreeLinkerTest.cpp
[modify] https://crrev.com/73bb506d40e7618c72d23bb949fc37928ea68f21/third_party/WebKit/Source/core/svg/SVGScriptElement.cpp
[modify] https://crrev.com/73bb506d40e7618c72d23bb949fc37928ea68f21/third_party/WebKit/Source/core/svg/SVGScriptElement.h
[modify] https://crrev.com/73bb506d40e7618c72d23bb949fc37928ea68f21/third_party/WebKit/Source/core/workers/WorkletModuleTreeClient.h

Project Member

Comment 8 by bugdroid1@chromium.org, Jun 26 2017

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

commit b4ef7c0fece5fdbf8d51f4a6865056a1b03298c8
Author: Kouhei Ueno <kouhei@chromium.org>
Date: Mon Jun 26 10:57:30 2017

[ES6 modules] Document ModuleScript TraceWrapper paths

Bug:  594639 , 725816,  732270 
Change-Id: I7b83a77beb3806d53adb1e15baa184646a61a4f6
Reviewed-on: https://chromium-review.googlesource.com/547380
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Kouhei Ueno <kouhei@chromium.org>
Cr-Commit-Position: refs/heads/master@{#482235}
[modify] https://crrev.com/b4ef7c0fece5fdbf8d51f4a6865056a1b03298c8/third_party/WebKit/Source/core/dom/ModuleScript.h

Project Member

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

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

commit 52965d2d823717cd3d4feb67d424138c767c510a
Author: Kouhei Ueno <kouhei@chromium.org>
Date: Tue Jul 04 06:53:30 2017

[ES6 modules] TraceWrapper ModuleScript via HTMLParserScriptRunner

This CL adds another TraceWrapper path to ModuleScript to cover case where:
- Module script is an inline script
- <script> element for the inline script is removed at the time of execution

Bug:  594639 , 725816,  732270 ,  737086 
Change-Id: I5e8d00df55ae992f272aaac1b8890c120a32f3be
Reviewed-on: https://chromium-review.googlesource.com/558536
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Kouhei Ueno <kouhei@chromium.org>
Cr-Commit-Position: refs/heads/master@{#484060}
[modify] https://crrev.com/52965d2d823717cd3d4feb67d424138c767c510a/third_party/WebKit/Source/core/dom/Document.cpp
[modify] https://crrev.com/52965d2d823717cd3d4feb67d424138c767c510a/third_party/WebKit/Source/core/dom/Document.h
[modify] https://crrev.com/52965d2d823717cd3d4feb67d424138c767c510a/third_party/WebKit/Source/core/dom/DocumentParser.h
[modify] https://crrev.com/52965d2d823717cd3d4feb67d424138c767c510a/third_party/WebKit/Source/core/dom/ModuleScript.h
[modify] https://crrev.com/52965d2d823717cd3d4feb67d424138c767c510a/third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp
[modify] https://crrev.com/52965d2d823717cd3d4feb67d424138c767c510a/third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.h
[modify] https://crrev.com/52965d2d823717cd3d4feb67d424138c767c510a/third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.cpp
[modify] https://crrev.com/52965d2d823717cd3d4feb67d424138c767c510a/third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.h

Sign in to add a comment