Issue 725816 in chromium: <script type=module> leaks v8::Context and blink::Document

kou… via monorail

unread,

May 24, 2017, 4:16:55 AM5/24/17

to modul...@chromium.org

Status: Started
Owner: kou...@chromium.org
CC: modul...@chromium.org
Labels: Arch-All
Components: Blink>HTML>Script

New issue 725816 by kou...@chromium.org: <script type=module> leaks v8::Context and blink::Document
https://bugs.chromium.org/p/chromium/issues/detail?id=725816

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.

--
You received this message because:
1. You were specifically CC'd on the issue

You may adjust your notification preferences at:
https://bugs.chromium.org/hosting/settings

Reply to this email to add a comment.

bugdro… via monorail

unread,

May 24, 2017, 11:34:16 PM5/24/17

to modul...@chromium.org

hirosh… via monorail

unread,

May 26, 2017, 6:08:15 PM5/26/17

to modul...@chromium.org

Issue 725816: <script type=module> leaks v8::Context and blink::Document
https://bugs.chromium.org/p/chromium/issues/detail?id=725816

This issue is now blocking issue 718442.
See https://bugs.chromium.org/p/chromium/issues/detail?id=718442

hirosh… via monorail

unread,

May 26, 2017, 6:10:45 PM5/26/17

to modul...@chromium.org

Issue 725816: <script type=module> leaks v8::Context and blink::Document
https://bugs.chromium.org/p/chromium/issues/detail?id=725816

This issue is now blocking issue 718114.
See https://bugs.chromium.org/p/chromium/issues/detail?id=718114

bugdro… via monorail

unread,

May 29, 2017, 9:09:46 AM5/29/17

to modul...@chromium.org

Comment #4 on issue 725816 by bugd...@chromium.org: <script type=module> leaks v8::Context and blink::Document
https://bugs.chromium.org/p/chromium/issues/detail?id=725816#c4

The following revision refers to this bug:

https://chromium.googlesource.com/chromium/src.git/+/74ba772e23ab732e4d4f2c2c2bcd76fbff9c78d5

commit 74ba772e23ab732e4d4f2c2c2bcd76fbff9c78d5
Author: Kouhei Ueno <kou...@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 <yhi...@chromium.org>
Commit-Queue: Kouhei Ueno <kou...@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

kou… via monorail

unread,

May 30, 2017, 2:42:09 AM5/30/17

to modul...@chromium.org

Updates:
Blocking: -718114 -594639 -718442
EstimatedDays: ----
Labels: -Pri-1 Pri-2

Comment #5 on issue 725816 by kou...@chromium.org: <script type=module> leaks v8::Context and blink::Document
https://bugs.chromium.org/p/chromium/issues/detail?id=725816#c5

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.

kou… via monorail

unread,

May 31, 2017, 8:32:51 AM5/31/17

to modul...@chromium.org

Updates:
Status: Assigned

Comment #6 on issue 725816 by kou...@chromium.org: <script type=module> leaks v8::Context and blink::Document
https://bugs.chromium.org/p/chromium/issues/detail?id=725816#c6

(No comment was entered for this change.)

bugdro… via monorail

unread,

Jun 26, 2017, 4:05:27 AM6/26/17

to modul...@chromium.org

Comment #7 on issue 725816 by bugd...@chromium.org: <script type=module> leaks v8::Context and blink::Document
https://bugs.chromium.org/p/chromium/issues/detail?id=725816#c7

The following revision refers to this bug:

https://chromium.googlesource.com/chromium/src.git/+/73bb506d40e7618c72d23bb949fc37928ea68f21

commit 73bb506d40e7618c72d23bb949fc37928ea68f21
Author: Kouhei Ueno <kou...@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 <yhi...@chromium.org>
Commit-Queue: Kouhei Ueno <kou...@chromium.org>

bugdro… via monorail

unread,

Jun 26, 2017, 6:57:45 AM6/26/17

to modul...@chromium.org

Comment #8 on issue 725816 by bugd...@chromium.org: <script type=module> leaks v8::Context and blink::Document
https://bugs.chromium.org/p/chromium/issues/detail?id=725816#c8

The following revision refers to this bug:

https://chromium.googlesource.com/chromium/src.git/+/b4ef7c0fece5fdbf8d51f4a6865056a1b03298c8

commit b4ef7c0fece5fdbf8d51f4a6865056a1b03298c8
Author: Kouhei Ueno <kou...@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 <har...@chromium.org>
Commit-Queue: Kouhei Ueno <kou...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#482235}
[modify] https://crrev.com/b4ef7c0fece5fdbf8d51f4a6865056a1b03298c8/third_party/WebKit/Source/core/dom/ModuleScript.h

bugdro… via monorail

unread,

Jul 4, 2017, 2:54:31 AM7/4/17

to modul...@chromium.org

Comment #9 on issue 725816 by bugd...@chromium.org: <script type=module> leaks v8::Context and blink::Document
https://bugs.chromium.org/p/chromium/issues/detail?id=725816#c9

The following revision refers to this bug:

https://chromium.googlesource.com/chromium/src.git/+/52965d2d823717cd3d4feb67d424138c767c510a

commit 52965d2d823717cd3d4feb67d424138c767c510a
Author: Kouhei Ueno <kou...@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 <har...@chromium.org>
Commit-Queue: Kouhei Ueno <kou...@chromium.org>

hiroshige via monorail

unread,

Jul 7, 2022, 2:19:52 PM7/7/22

to modul...@chromium.org

Comment #11 on issue 725816 by hiro...@chromium.org: <script type=module> leaks v8::Context and blink::Document
https://bugs.chromium.org/p/chromium/issues/detail?id=725816#c11

Kouhei, do you know whether this is still a problem? (Checking after several CLs landed and Oilpan GC has been largely updated)

-- 
You received this message because:
  1. You were specifically CC'd on the issue

You may adjust your notification preferences at:
https://bugs.chromium.org/hosting/settings

Reply to this email to add a comment or make updates.

Reply all

Reply to author

Forward