New issue
Advanced search Search tips

Issue 501866 link

Starred by 8 users

Issue metadata

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

Blocking:
issue 700680
issue 701695



Sign in to add a comment

Events that have ScriptValues leak memory

Project Member Reported by jochen@chromium.org, Jun 18 2015

Issue description

I'd expect that

new CustomEvent({detail: new Error()}); leads to a memory leak of the entire context

same for PopStateEvent({state: new Error()}) and new ErrorEvent({error: new Error()}) and MessageEvent({data: new Error()});
 

Comment 1 by jochen@chromium.org, Jun 18 2015

PromiseRejectionEvent had the same problem, I did this: https://codereview.chromium.org/1189513004
Cc: bashi@chromium.org blink-reviews-bindings@chromium.org yukishiino@chromium.org
+bashi, +yukishino

This looks like a fundamental problem. In general, ScriptValue from Blink to V8 always has a risk of creating a cycle crossing the binding boundary (and thus cause a leak). We can avoid the cycle by making the Blink => V8 reference weak (just like Jochen did in https://codereview.chromium.org/1189513004), but if we do that, the Blink object will fail at keeping the V8 object alive.


Cc: yhirano@chromium.org
I may be wrong but this looks very reminiscent of the problem that caused yhirano@ to create ScriptPromiseProperty? See https://docs.google.com/document/d/1WphFrSM18-m6b4RFaBxwLL_zNlpOdCtEbuRclQ-S_ts/edit

Comment 4 by jochen@chromium.org, Jun 18 2015

that's similar to my work-around, and fails to solve the problem in a similar fashion - the value can get garbage collected, so events would forget their properties over time.
> #4

Sorry I don't understand: a promise property installed with ScriptPromiseProperty will be valid as long as the wrapper is valid, I think.

Comment 6 by jochen@chromium.org, Jun 19 2015

right, but that's not the desired behavior. It should keep the promise alive, without leaking the entire window
ScriptPromiseProperty doesn't extend the wrapper / window lifetime. We just store the promise (JS) object in the wrapper as a hidden value. Both the promise object and the wrapper are governed by V8 GC.


Comment 8 by jochen@chromium.org, Jun 19 2015

what happens if the C++ holder of the ScriptPromiseProperty doesn't have a wrapper (yet) and the promise gets collected?
A ScriptPromiseProperty holds the resolved / rejected value as a C++ object. ScriptPromiseProperty is not designed to resolve a promise with *arbitrary* value, we assume the value can be held as a C++ object (e.g. undefined, numbers, ScriptWrappable objects, ...). 

The original problem is holding an arbitrary value (i.e. ScriptValue / v8::Value) in a C++ Event object and ScriptPromiseProperty is not designed for the (corresponding) use case.

I think the systematic fix here is:

- Every* ScriptValue is provided an "lifetime" JavaScript object (LJO) on creation.
- The ScriptValue establishes a hidden reference from the LJO to the referent. (It does not hold on to the LJO, of course.)
- Something else in the system (eg the DOM wrapper table, JS closure, etc.) holds onto the LJO.
- The ScriptValue only has a weak handle to its referent.
- ASSERT when dereferencing a ScriptValue whose referent was collected, except when the thread is terminating, because evidently it had the wrong LJO.

(Incidentally, why do we set hidden references from wrappers to property values? eg. the generated V8UIEvent.cpp sourceDeviceAttributeGetter? Most of the calls to V8HiddenValue::setHiddenValue in gen in fact?)

* We will need some ScriptValues with strong references/without LJOs, but those should only be used on the stack; uses on the heap should be audited carefully. There should be approx. one of these per world--very, very rare.
Re: comment 3--I wrote ScriptPromiseProperty (and sadly had to fight to do that...) yhirano is just author of that linked doc.

Re: comment 8--ScriptPromiseProperty's holder can be any object you like. If you want it to be a wrapper, but the wrapper does not exist, you just create the wrapper at that point. Think it through--this is how it should work (ScriptPromiseProperty is for a Promise you can repeatedly retrieve through a property.)
> - Every* ScriptValue is provided an "lifetime" JavaScript object (LJO) on creation.
> - The ScriptValue establishes a hidden reference from the LJO to the referent. (It does not hold on to the LJO, of course.)
> - Something else in the system (eg the DOM wrapper table, JS closure, etc.) holds onto the LJO.
> - The ScriptValue only has a weak handle to its referent.
> - ASSERT when dereferencing a ScriptValue whose referent was collected, except when the thread is terminating, because evidently it had the wrong LJO.

How is the LJO kept alive as long as the DOM object that holds the ScriptValue wants to keep the LJO alive?

> (Incidentally, why do we set hidden references from wrappers to property values? eg. the generated V8UIEvent.cpp sourceDeviceAttributeGetter? Most of the calls to V8HiddenValue::setHiddenValue in gen in fact?)

This is a performance optimization for readonly attributes (which never changes). See the comment here:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/bindings/scripts/v8_attributes.py&q=cached_attribute_validation_method&sq=package:chromium&type=cs&l=291


Thanks for the link re: readonly attributes.

Re: keeping the LJO alive, that's a design question. You need to look at specific examples.

For example, ScriptPromiseProperty, the object that has the property is the LJO. The property is not accessible if you don't have the object. (Of course, script can hold on to just the Promise, which is fine--we will collect everything else and not leak.)

In the case of CustomEvent in the original post, the CustomEvent wrapper should be the LJO for the value of the detail property.
Oh, you asked about DOM objects specifically. That is also a case-by-case thing. But we have ActiveDOMObject, for example; DOM objects that need to stay alive keep themselves alive, which keeps their wrapper (if any) alive.
> But we have ActiveDOMObject, for example; DOM objects that need to stay alive keep themselves alive, which keeps their wrapper (if any) alive.

I think that's the trick part. CustomEvent can keep the wrapper alive by returning true to ActiveDOMObject::hasPendingActivity. However, CustomEvent wants to keep the wrapper alive as long as it's referenced by somebody -- in other words, CustomEvent wants to keep the wrapper alive as long as it has a wrapper. This is exactly the cycle we have to break -- getting back to the original cycle problem :)


> #10
That sounds good.
Allowing to hold a ScriptValue to any ScriptWrappable is not needed though, I think. There are two kinds of ScriptWrappable objects. One is world-neutral ones (most of Nodes and Events) and the other is ones bound to a world (e.g. XHR, ReadableStream). I assume PromiseRejectionEvent is latter.
Providing a way to distinguish between the former and the latter, and encouraging to hold a ScriptValue only in latter ones are helpful, I think.

Why are the ScriptWrappables of Nodes and Events world-neutral?

Nodes are referred from multiple worlds, aren't they?
if that's the case that would be a P0 UXSS bug...
Then I misunderstood, sorry.
The security contract the world must provide is that JavaScript objects (including wrappers) are completely isolated per world.

Hmm, I talked about C++ classes (I should have prefixed them with "blink::").
ok, note however that the ScriptWrappable must only ever be used in the main world. For non-main world wrappers, we have to go through the wrapper map
> #23
Yes, and what I wanted to say was that usually holding a ScriptValue in a C++ ScriptWrappable which is exposed to multiple worlds using the wrapper map is not right, because the ScriptValue is not compatible with the wrapper map.
Holding |reason| as a ScriptValue in blink::PromiseRejectionEvent is correct because the event is dispatched only to the world on which |reason| is created.

Does that make sense?
Yes, it is not right to expose a ScriptValue to another world. We need to either of:

- Don't return anything if the ScriptValue is requested from another world (e.g., PromiseRejectionEvent.reason)
- Return a new ScriptValue after serializing & deserializing the original ScriptValue (e.g., CustomEvent.detail)

Sorry, I'm missing your point but what's your proposal for preventing the leaks caused by the ScriptValues? (e.g., PromiseRejectionEvent.reason, CustomEvent.detail)

> #25
Sorry for not being clear.

At #16 and #24, I was concerned about Handle leakage between worlds and blink code readability. Not all blink developers understand bindings and worlds mechanism. Currently we have a very simple rule: Do not hold a ScriptValue in a ScriptWrappable. I like Dominic's proposal (#10), but just allowing to hold a ScriptValue in ScriptWrappable may also encourage people to write wrong or confusing code, I think. We need a mechanism or a rule that encourages blink developers to write correct code that is easy to understand / review.

That's what I wanted say at #16.

(from #16)
> Providing a way to distinguish between the former and the latter, and encouraging to hold a ScriptValue only in latter ones are helpful, I think.

I don't have a shiny idea, though.

I thought about defining another ScriptWrappable, say ScriptWrappableWithoutWrapperMap, but it has a problem that PromiseRejectionEvent is an Event subclass.


once the "v8 marks through blink heap" thing is done, all those cycles aren't a problem anymore
Yes, that's the long-term solution.

Re: Comment 15:

> However, CustomEvent wants to keep the wrapper alive as long as it's referenced by
> somebody -- in other words, CustomEvent wants to keep the wrapper alive as long as
> it has a wrapper.

Doesn't the wrapper table handle that? ie the wrapper retains the C++ CustomEvent. In the proposal in Comment 10, 'detail' would be weak from C++ Custom Event to JS object, and strong from wrapper to JS object. Then any cycle formed by detail to Custom Event would be in the JavaScript heap and visible to the V8 GC.

Today it can leak because the C++ Custom Event -> JS object for 'detail' is a strong ScriptValue. Then a reference from detail to the wrapper causes a cycle, but the cycle straddles C++ and V8 heaps and is not visible to the V8 GC.
Thanks, I understand -- it will work!

it won't work for events like PromiseRejectionEvent that don't necessarily have a wrapper
Can we create a wrapper for PromiseRejectionEvent? Given that PromiseRejectionEvent wants to store a ScriptValue, we should have a context on which we can create a wrapper of PromiseRejectionEvent.

(FWIW, we're doing a similar thing in SVG elements etc -- forcibly create a wrapper and represent a DOM-side reference in the JS side.)

the wrapper would be garbage and as such not suitable to hold the script value alive
> the wrapper would be garbage and as such not suitable to hold the script value alive

I think that's the tricky part of dominicc's proposal -- if the wrapper becomes unreachable, it's ok to collect the ScriptValue even if the Event object is still reachable in the DOM side. If you really need to keep the ScriptValue (or the wrapper) alive as long as the DOM object is reachable from somewhere, you need to explicitly annotate the reachability with [SetWrapperReferenceTo/From]. That is what we're currently doing for other wrappers and what we'll keep doing in the future even after V8 GC gets an ability to trace DOM objects (See my comment here: https://groups.google.com/a/chromium.org/d/msg/blink-dev/khUceiDiXtk/eyiF1EHSp5sJ).



the setReference thing doesn't work here.

The parent would be the wrapper of the event, but there's no way to get a const ref to the wrapper. Also, there's no callback or anything when a wrapper gets added.
I think we can call setReference in visitDOMWrapper (where we know the event wrapper). We don't need to handle the reachability when the wrapper gets added/removed -- we just need to tell the reachability when the wrapper is visited.

Comment 37 by tkent@chromium.org, Jul 15 2015

Labels: -Cr-Blink-Events Cr-Blink-Bindings
Owner: haraken@chromium.org
I proposed a solution: traceWrapper()

See the design document: https://docs.google.com/document/d/1gu7NwxuQ9fLWRRBjbH1MGKr7RZEx9ESRCZ3P4KwvP68/edit

Owner: jochen@chromium.org

Comment 41 by bashi@chromium.org, Jan 20 2016

Status: Assigned
Has there been any progress on traceWrapper()?
still blocked on oilpan not being launched
Any movement on this?
hlopko@ is now implementing traceWrapper.

Owner: hpayer@chromium.org
Cc: tdres...@chromium.org
Cc: hpayer@chromium.org
Owner: mlippautz@chromium.org
Cc: jochen@chromium.org
Components: Blink>JavaScript>GC
fyi: I see a problem with
- PopStateEvent (m_state)
- ErrorEvent (m_error)
- MessageEvent (m_dataAsScriptValue)

CustomEvent should be fine as it doesn't keep a ScriptValue around.

PromiseRejectionEvent looks good as it stores the world it was created in, keeps wrapper tracing references to relevant members, and only creates ScriptValues back from those for the creation context (world). 

I think this approach should be used for other Events too.
Re #49: Yes, that's a right way forward! In general, we should replace all ScriptValues held by an on-heap object with the wrapper tracing.


Blocking: 700680
Cc: kouhei@chromium.org
Blocking: 701695
Project Member

Comment 54 by bugdroid1@chromium.org, Apr 28 2017

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

commit fa3fcf6eb2ec5bfb3504ab2c118bc8406fd5ef57
Author: Kenichi Ishibashi <bashi@chromium.org>
Date: Fri Apr 28 08:44:43 2017

Trace CustomEvent.detail instead of keeping ScriptValue

Before this CL we used V8PrivateProperty to avoid
cyclic references between V8 and Blink. Now we have
trace wrapper and DOM object can have a reference to
V8 objects. Use trace wrapper for CustomEvent

BUG=501866,700680

Change-Id: I9089d9cf0828bb979467cc5bd859e460847b6c28
Reviewed-on: https://chromium-review.googlesource.com/484162
Commit-Queue: Kenichi Ishibashi <bashi@chromium.org>
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#467934}
[modify] https://crrev.com/fa3fcf6eb2ec5bfb3504ab2c118bc8406fd5ef57/third_party/WebKit/Source/bindings/bindings.gni
[delete] https://crrev.com/f32671c0f445b58612ebba5c7a72de7c7981e8e9/third_party/WebKit/Source/bindings/core/v8/custom/V8CustomEventCustom.cpp
[modify] https://crrev.com/fa3fcf6eb2ec5bfb3504ab2c118bc8406fd5ef57/third_party/WebKit/Source/core/events/CustomEvent.cpp
[modify] https://crrev.com/fa3fcf6eb2ec5bfb3504ab2c118bc8406fd5ef57/third_party/WebKit/Source/core/events/CustomEvent.h
[modify] https://crrev.com/fa3fcf6eb2ec5bfb3504ab2c118bc8406fd5ef57/third_party/WebKit/Source/core/events/CustomEvent.idl
[modify] https://crrev.com/fa3fcf6eb2ec5bfb3504ab2c118bc8406fd5ef57/third_party/WebKit/Source/platform/bindings/V8PrivateProperty.h

Project Member

Comment 56 by bugdroid1@chromium.org, May 2 2017

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

commit 1df4bbd0744720e49bcd98deb70753d4c1602205
Author: bashi <bashi@chromium.org>
Date: Tue May 02 05:24:48 2017

Revert of Use trace wrapper in PopStateEvent (patchset #1 id:1 of https://codereview.chromium.org/2850133002/ )

Reason for revert:
Broke builds

Original issue's description:
> Don't store ScriptValue in PopStateEvent
>
> Use TraceWrapperV8Reference instead. This CL doesn't remove
> custom getter of PopStateEvent.state as more work will be
> needed to remove custom bindings.
>
> BUG=501866
>
> Review-Url: https://codereview.chromium.org/2850133002
> Cr-Commit-Position: refs/heads/master@{#468565}
> Committed: https://chromium.googlesource.com/chromium/src/+/77301cfe98de369b7d72659d4a95014f1d4dc4b6

TBR=haraken@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=501866

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

[modify] https://crrev.com/1df4bbd0744720e49bcd98deb70753d4c1602205/third_party/WebKit/Source/bindings/core/v8/custom/V8PopStateEventCustom.cpp
[modify] https://crrev.com/1df4bbd0744720e49bcd98deb70753d4c1602205/third_party/WebKit/Source/core/events/PopStateEvent.cpp
[modify] https://crrev.com/1df4bbd0744720e49bcd98deb70753d4c1602205/third_party/WebKit/Source/core/events/PopStateEvent.h
[modify] https://crrev.com/1df4bbd0744720e49bcd98deb70753d4c1602205/third_party/WebKit/Source/core/events/PopStateEvent.idl

Project Member

Comment 57 by bugdroid1@chromium.org, May 2 2017

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

commit ccffbeec870b3c375c0dbb9f3368b87766a7d6e5
Author: bashi <bashi@chromium.org>
Date: Tue May 02 08:07:20 2017

Reland: Don't store ScriptValue in PopStateEvent

Fix compilation errors (rebased).

Original description:

Use TraceWrapperV8Reference instead. This CL doesn't remove
custom getter of PopStateEvent.state as more work will be
needed to remove custom bindings.

BUG=501866

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

[modify] https://crrev.com/ccffbeec870b3c375c0dbb9f3368b87766a7d6e5/third_party/WebKit/Source/bindings/core/v8/custom/V8PopStateEventCustom.cpp
[modify] https://crrev.com/ccffbeec870b3c375c0dbb9f3368b87766a7d6e5/third_party/WebKit/Source/core/events/PopStateEvent.cpp
[modify] https://crrev.com/ccffbeec870b3c375c0dbb9f3368b87766a7d6e5/third_party/WebKit/Source/core/events/PopStateEvent.h
[modify] https://crrev.com/ccffbeec870b3c375c0dbb9f3368b87766a7d6e5/third_party/WebKit/Source/core/events/PopStateEvent.idl

Cc: mlippautz@chromium.org
Owner: ----
Status: Available (was: Assigned)
Currently not working on this. I think this is likely to happen on-demand when somebody has cycles.
Project Member

Comment 60 by sheriffbot@chromium.org, Aug 21

Labels: Hotlist-Recharge-Cold
Status: Untriaged (was: Available)
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue.

Sorry for the inconvenience if the bug really should have been left as Available.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Cc: peria@chromium.org
Status: Available (was: Untriaged)
Project Member

Comment 62 by bugdroid1@chromium.org, Dec 12 (2 days ago)

Project Member

Comment 63 by bugdroid1@chromium.org, Yesterday (32 hours ago)

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

commit 8d579fc15d93d5fd9677bbfda8c5144569d27240
Author: Yuki Shiino <yukishiino@chromium.org>
Date: Thu Dec 13 06:06:01 2018

v8binding: Unify ScriptValue::ToWorldSafeScriptValue to WorldSafeV8Reference

Remove ScriptValue::ToWorldSafeScriptValue and replace its usage
with WorldSafeV8Reference.

Bug: 501866, 755520, 803478
Change-Id: I75f38dd4d4dd8c3730bec1e9d002fadaa236395b
Reviewed-on: https://chromium-review.googlesource.com/c/1373894
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Hitoshi Yoshida <peria@chromium.org>
Commit-Queue: Yuki Shiino <yukishiino@chromium.org>
Cr-Commit-Position: refs/heads/master@{#616228}
[modify] https://crrev.com/8d579fc15d93d5fd9677bbfda8c5144569d27240/third_party/blink/renderer/bindings/core/v8/script_value.cc
[modify] https://crrev.com/8d579fc15d93d5fd9677bbfda8c5144569d27240/third_party/blink/renderer/bindings/core/v8/script_value.h
[modify] https://crrev.com/8d579fc15d93d5fd9677bbfda8c5144569d27240/third_party/blink/renderer/bindings/core/v8/world_safe_v8_reference.h
[modify] https://crrev.com/8d579fc15d93d5fd9677bbfda8c5144569d27240/third_party/blink/renderer/modules/payments/payment_method_change_event.cc
[modify] https://crrev.com/8d579fc15d93d5fd9677bbfda8c5144569d27240/third_party/blink/renderer/modules/payments/payment_method_change_event.h
[modify] https://crrev.com/8d579fc15d93d5fd9677bbfda8c5144569d27240/third_party/blink/renderer/modules/payments/payment_response.cc
[modify] https://crrev.com/8d579fc15d93d5fd9677bbfda8c5144569d27240/third_party/blink/renderer/modules/payments/payment_response.h

Sign in to add a comment