New issue
Advanced search Search tips

Issue 918913 link

Starred by 1 user
Status: Available
Owner: ----
Cc:
mlippautz@chromium.org
yhirano@chromium.org
peria@chromium.org
haraken@chromium.org
verwa...@chromium.org
lfg@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac , Fuchsia
Pri: 2
Type: Bug



Sign in to add a comment

ScriptPromisePropertyBase destructor calling into V8

Project Member Reported by mlippautz@chromium.org, Jan 3 
ScriptPromisePropertyBase destructor calls into V8. This is problematic as it can in general cause
a) script execution and
b) allocations.

Both are problematic. 

The way this currently works is because SetPrivate does *not* enter a proper V8 scope via V8_ENTER or V8_ENTER_NO_SCRIPT. Because of this, the Blink callback for on entering script execution is not called and the containing check not fired.

Since the finalizer executing the destructor is already called during sweeping, I assume that the properties that are to be cleared are not actually used anymore. In particular, I would assume that any set promise or resolver stays unused in that time.

In that case, we could just schedule a foreground task that clears out all cached promises and resolvers from the wrappers, no?

https://cs.chromium.org/chromium/src/third_party/blink/renderer/bindings/core/v8/script_promise_property_base.cc?q=ScriptPromisePropertyBase&sq=package:chromium&g=0&l=26

Comment 1 by haraken@chromium.org, Jan 4 

I'm getting a bit confused.

In the first place why do we not simply wrapper-trace the following path?

  holder wrapper -> ScriptPromiseProperty -> Promise and Resolver

assuming that the intention is to keep returning the same promise and resolver while the holder wrapper is alive?

Comment 2 by mlippautz@chromium.org, Jan 8 

I hope somebody else can chime in here. I'd like to get this sorted out but don't know the details of the initial design.

Comment 3 by haraken@chromium.org, Jan 9

Cc: yhirano@chromium.org
yhirano@: Any thoughts on this?

Comment 4 by yhirano@chromium.org, Jan 9 

I'm confused: Is this different from issue 916022?

Comment 5 by mlippautz@chromium.org, Jan 9 

Issue 916022 was about a specific crash that happened because of recursive GC calls triggered from V8 API usage in the destructor. That crasher is fixed.

The mitigation I landed is very careful in the usage of V8's API so that no GC is triggered (as far as I know). This works now but is very fragile and may break at any time without anybody realizing :/

The more general issue seems that we should not call into V8 from destructors to avoid this problem on that level.

Comment 6 by yhirano@chromium.org, Jan 9 

Hm, the destructor has never been trivial; See https://codereview.chromium.org/361863003.

- I think making the destructor trivial is safe. Some references will be left but I don't think they're super harmful. 
- Haraken's solution also sounds good. But we need EnsureWrapper as the wrapper may not be instantiated. We also need to ensure that TraceWrapperMember is used instead of Member.
- Or can we wait for the unified GC if it comes soon?

Comment 7 by mlippautz@chromium.org, Jan 9 

> Or can we wait for the unified GC if it comes soon?

Unified GC does not solve the problem of calling into V8 from destructors that may have arbitrary side effects. For this particular issue it solves the need for using TraceWrapperMember at some point. Until we fully launch we need it though.
►

Sign in to add a comment