Add v8::Object::CreationContext that works for a persistent handle

Add v8::Object::CreationContext that works for a persistent handle

I need this API for https://codereview.chromium.org/1609343002/.

BUG=483722
Committed: https://crrev.com/9285e66630c4173d80929038f3862b6910b580b8
Cr-Commit-Position: refs/heads/master@{#39775}

[haraken, 2016-01-25 04:36:11]

haraken@chromium.org changed reviewers:
+ jochen@chromium.org

[haraken, 2016-01-25 04:36:11]

PTAL

[jochen (gone - plz use gerrit), 2016-01-25 10:01:11]

we're actively moving away from CreationContext

[haraken, 2016-01-25 10:55:21]

On 2016/01/25 10:01:11, jochen wrote:
> we're actively moving away from CreationContext

But until we have a way to get a creation context from ScriptWrappable, we need
to have the way on the V8 side...

[jochen (gone - plz use gerrit), 2016-01-25 11:01:14]

On 2016/01/25 at 10:55:21, haraken wrote:
> On 2016/01/25 10:01:11, jochen wrote:
> > we're actively moving away from CreationContext
> 
> But until we have a way to get a creation context from ScriptWrappable, we
need to have the way on the V8 side...

It's still just making it more difficult to remove the API later

[haraken, 2016-01-25 11:21:39]

On 2016/01/25 11:01:14, jochen wrote:
> On 2016/01/25 at 10:55:21, haraken wrote:
> > On 2016/01/25 10:01:11, jochen wrote:
> > > we're actively moving away from CreationContext
> > 
> > But until we have a way to get a creation context from ScriptWrappable, we
> need to have the way on the V8 side...
> 
> It's still just making it more difficult to remove the API later

I'll consider another work-around.

But I don't think it makes sense to ask the embedder not to use/introduce a V8
API before providing an alternative way (even though the binding team has a
responsibility of providing the way).

[jochen (gone - plz use gerrit), 2016-01-25 11:37:40]

On 2016/01/25 at 11:21:39, haraken wrote:
> On 2016/01/25 11:01:14, jochen wrote:
> > On 2016/01/25 at 10:55:21, haraken wrote:
> > > On 2016/01/25 10:01:11, jochen wrote:
> > > > we're actively moving away from CreationContext
> > > 
> > > But until we have a way to get a creation context from ScriptWrappable, we
> > need to have the way on the V8 side...
> > 
> > It's still just making it more difficult to remove the API later
> 
> I'll consider another work-around.
> 
> But I don't think it makes sense to ask the embedder not to use/introduce a V8
API before providing an alternative way (even though the binding team has a
responsibility of providing the way).

yeah, I can see where you're coming from.

The API, however, is still in a state where we have several deprecated versions
of a single API, and before that pile of work is gone, I'd rather err on the
side of pushing back against changes like this.

[haraken, 2016-04-20 01:40:44]

(Reviving this old thread...)

If we don't have an immediate plan to replace the CreationContext API with
something better (I wrote some doc here:
https://docs.google.com/document/d/1Hdys-DfDMK6i9s7O8tPwyerZW1epb93fMu1GnRdy7...),
I'd like to land this API.

I'm now trying to make hasPendingActivity return false when the associated
ExecutionContext is gone, and there I need the API.

void VisitPersistentHandle(...) {
  v8::Local<v8::Object> wrapper = ...;
  if (toWrapperTypeInfo(wrapper)->hasPendingActivity() &&
!toExecutionContext(wrapper->CreationContext())->isDetached()) {
    wrapper->MarkActive();
  }
}

[jochen (gone - plz use gerrit), 2016-04-20 07:47:13]

On 2016/04/20 at 01:40:44, haraken wrote:
> (Reviving this old thread...)
> 
> If we don't have an immediate plan to replace the CreationContext API with
something better (I wrote some doc here:
https://docs.google.com/document/d/1Hdys-DfDMK6i9s7O8tPwyerZW1epb93fMu1GnRdy7...),
I'd like to land this API.
> 
> I'm now trying to make hasPendingActivity return false when the associated
ExecutionContext is gone, and there I need the API.
> 
> void VisitPersistentHandle(...) {
>   v8::Local<v8::Object> wrapper = ...;
>   if (toWrapperTypeInfo(wrapper)->hasPendingActivity() &&
!toExecutionContext(wrapper->CreationContext())->isDetached()) {
>     wrapper->MarkActive();
>   }
> }

shouldn't the objects just be lifecycle observers for their execution context?

[haraken, 2016-04-20 07:51:14]

On 2016/04/20 07:47:13, jochen wrote:
> On 2016/04/20 at 01:40:44, haraken wrote:
> > (Reviving this old thread...)
> > 
> > If we don't have an immediate plan to replace the CreationContext API with
> something better (I wrote some doc here:
>
https://docs.google.com/document/d/1Hdys-DfDMK6i9s7O8tPwyerZW1epb93fMu1GnRdy7...),
> I'd like to land this API.
> > 
> > I'm now trying to make hasPendingActivity return false when the associated
> ExecutionContext is gone, and there I need the API.
> > 
> > void VisitPersistentHandle(...) {
> >   v8::Local<v8::Object> wrapper = ...;
> >   if (toWrapperTypeInfo(wrapper)->hasPendingActivity() &&
> !toExecutionContext(wrapper->CreationContext())->isDetached()) {
> >     wrapper->MarkActive();
> >   }
> > }
> 
> shouldn't the objects just be lifecycle observers for their execution context?

ContextLifecycleNotifier needs to keep track of weak pointers to all
ContextLifecycleObservers, and the notifier needs to iterate over all observers
when sending various notifications. So I don't really want to increase # of
ContextLifecycleObservers (that's one of the reasons I moved hasPendingActivity
from ActiveDOMObject to ScriptWrappable).

[jochen (gone - plz use gerrit), 2016-04-20 12:50:26]

On 2016/04/20 at 07:51:14, haraken wrote:
> On 2016/04/20 07:47:13, jochen wrote:
> > On 2016/04/20 at 01:40:44, haraken wrote:
> > > (Reviving this old thread...)
> > > 
> > > If we don't have an immediate plan to replace the CreationContext API with
> > something better (I wrote some doc here:
> >
https://docs.google.com/document/d/1Hdys-DfDMK6i9s7O8tPwyerZW1epb93fMu1GnRdy7...),
> > I'd like to land this API.
> > > 
> > > I'm now trying to make hasPendingActivity return false when the associated
> > ExecutionContext is gone, and there I need the API.
> > > 
> > > void VisitPersistentHandle(...) {
> > >   v8::Local<v8::Object> wrapper = ...;
> > >   if (toWrapperTypeInfo(wrapper)->hasPendingActivity() &&
> > !toExecutionContext(wrapper->CreationContext())->isDetached()) {
> > >     wrapper->MarkActive();
> > >   }
> > > }
> > 
> > shouldn't the objects just be lifecycle observers for their execution
context?
> 
> ContextLifecycleNotifier needs to keep track of weak pointers to all
ContextLifecycleObservers, and the notifier needs to iterate over all observers
when sending various notifications. So I don't really want to increase # of
ContextLifecycleObservers (that's one of the reasons I moved hasPendingActivity
from ActiveDOMObject to ScriptWrappable).

well, but by using the creationcontext, you're essentially storing this weak
pointer in v8, and you iterate over all persistent handles instead of just over
all observers of a given context.

[haraken, 2016-04-20 13:09:20]

On 2016/04/20 12:50:26, jochen wrote:
> On 2016/04/20 at 07:51:14, haraken wrote:
> > On 2016/04/20 07:47:13, jochen wrote:
> > > On 2016/04/20 at 01:40:44, haraken wrote:
> > > > (Reviving this old thread...)
> > > > 
> > > > If we don't have an immediate plan to replace the CreationContext API
with
> > > something better (I wrote some doc here:
> > >
>
https://docs.google.com/document/d/1Hdys-DfDMK6i9s7O8tPwyerZW1epb93fMu1GnRdy7...),
> > > I'd like to land this API.
> > > > 
> > > > I'm now trying to make hasPendingActivity return false when the
associated
> > > ExecutionContext is gone, and there I need the API.
> > > > 
> > > > void VisitPersistentHandle(...) {
> > > >   v8::Local<v8::Object> wrapper = ...;
> > > >   if (toWrapperTypeInfo(wrapper)->hasPendingActivity() &&
> > > !toExecutionContext(wrapper->CreationContext())->isDetached()) {
> > > >     wrapper->MarkActive();
> > > >   }
> > > > }
> > > 
> > > shouldn't the objects just be lifecycle observers for their execution
> context?
> > 
> > ContextLifecycleNotifier needs to keep track of weak pointers to all
> ContextLifecycleObservers, and the notifier needs to iterate over all
observers
> when sending various notifications. So I don't really want to increase # of
> ContextLifecycleObservers (that's one of the reasons I moved
hasPendingActivity
> from ActiveDOMObject to ScriptWrappable).
> 
> well, but by using the creationcontext, you're essentially storing this weak
> pointer in v8

Right. As mentioned in the doc, we anyway need to store the CreationContext
pointer somewhere. Currently we store it in V8, and our plan is to move it to
Blink. My point is just that I want to use V8 until we actually finish the
migration (which seems not easy).

> and you iterate over all persistent handles instead of just over
> all observers of a given context.

Are you talking about V8GCController::hasPendingActivity? It is used only by
worker's postMessage as a hack to check if the worker's context has any pending
activity (we need to remove the hack somehow). So I think what matters more
would be the performance of (a) hasPendingActivity used in GC callbacks and (b)
LifecycleNotifier's methods (e.g.,
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...).

(a) iterates over all persistent handles anyway (by the V8 GC Visitor). (b)
iterates over all LifecycleObservers, and I want to reduce the number of
ContextLifecycleObservers for (b).

[jochen (gone - plz use gerrit), 2016-04-21 14:47:41]

jochen@chromium.org changed reviewers:
+ verwaest@chromium.org

[jochen (gone - plz use gerrit), 2016-04-21 14:49:40]

On 2016/04/20 at 13:09:20, haraken wrote:
> On 2016/04/20 12:50:26, jochen wrote:
> > On 2016/04/20 at 07:51:14, haraken wrote:
> > > On 2016/04/20 07:47:13, jochen wrote:
> > > > On 2016/04/20 at 01:40:44, haraken wrote:
> > > > > (Reviving this old thread...)
> > > > > 
> > > > > If we don't have an immediate plan to replace the CreationContext API
with
> > > > something better (I wrote some doc here:
> > > >
> >
https://docs.google.com/document/d/1Hdys-DfDMK6i9s7O8tPwyerZW1epb93fMu1GnRdy7...),
> > > > I'd like to land this API.
> > > > > 
> > > > > I'm now trying to make hasPendingActivity return false when the
associated
> > > > ExecutionContext is gone, and there I need the API.
> > > > > 
> > > > > void VisitPersistentHandle(...) {
> > > > >   v8::Local<v8::Object> wrapper = ...;
> > > > >   if (toWrapperTypeInfo(wrapper)->hasPendingActivity() &&
> > > > !toExecutionContext(wrapper->CreationContext())->isDetached()) {
> > > > >     wrapper->MarkActive();
> > > > >   }
> > > > > }
> > > > 
> > > > shouldn't the objects just be lifecycle observers for their execution
> > context?
> > > 
> > > ContextLifecycleNotifier needs to keep track of weak pointers to all
> > ContextLifecycleObservers, and the notifier needs to iterate over all
observers
> > when sending various notifications. So I don't really want to increase # of
> > ContextLifecycleObservers (that's one of the reasons I moved
hasPendingActivity
> > from ActiveDOMObject to ScriptWrappable).
> > 
> > well, but by using the creationcontext, you're essentially storing this weak
> > pointer in v8
> 
> Right. As mentioned in the doc, we anyway need to store the CreationContext
pointer somewhere. Currently we store it in V8, and our plan is to move it to
Blink. My point is just that I want to use V8 until we actually finish the
migration (which seems not easy).
> 
> > and you iterate over all persistent handles instead of just over
> > all observers of a given context.
> 
> Are you talking about V8GCController::hasPendingActivity? It is used only by
worker's postMessage as a hack to check if the worker's context has any pending
activity (we need to remove the hack somehow). So I think what matters more
would be the performance of (a) hasPendingActivity used in GC callbacks and (b)
LifecycleNotifier's methods (e.g.,
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...).
> 
> (a) iterates over all persistent handles anyway (by the V8 GC Visitor). (b)
iterates over all LifecycleObservers, and I want to reduce the number of
ContextLifecycleObservers for (b).

once we have incremental marking of dom objects, (a) won't happen anymore.

I can understand that you want to get rid of lifecycle observers, but I don't
think it's a good solution to address this by increasing the dependencies on a
deprecated system.

[jochen (gone - plz use gerrit), 2016-04-21 14:49:40]

On 2016/04/20 at 13:09:20, haraken wrote:
> On 2016/04/20 12:50:26, jochen wrote:
> > On 2016/04/20 at 07:51:14, haraken wrote:
> > > On 2016/04/20 07:47:13, jochen wrote:
> > > > On 2016/04/20 at 01:40:44, haraken wrote:
> > > > > (Reviving this old thread...)
> > > > > 
> > > > > If we don't have an immediate plan to replace the CreationContext API
with
> > > > something better (I wrote some doc here:
> > > >
> >
https://docs.google.com/document/d/1Hdys-DfDMK6i9s7O8tPwyerZW1epb93fMu1GnRdy7...),
> > > > I'd like to land this API.
> > > > > 
> > > > > I'm now trying to make hasPendingActivity return false when the
associated
> > > > ExecutionContext is gone, and there I need the API.
> > > > > 
> > > > > void VisitPersistentHandle(...) {
> > > > >   v8::Local<v8::Object> wrapper = ...;
> > > > >   if (toWrapperTypeInfo(wrapper)->hasPendingActivity() &&
> > > > !toExecutionContext(wrapper->CreationContext())->isDetached()) {
> > > > >     wrapper->MarkActive();
> > > > >   }
> > > > > }
> > > > 
> > > > shouldn't the objects just be lifecycle observers for their execution
> > context?
> > > 
> > > ContextLifecycleNotifier needs to keep track of weak pointers to all
> > ContextLifecycleObservers, and the notifier needs to iterate over all
observers
> > when sending various notifications. So I don't really want to increase # of
> > ContextLifecycleObservers (that's one of the reasons I moved
hasPendingActivity
> > from ActiveDOMObject to ScriptWrappable).
> > 
> > well, but by using the creationcontext, you're essentially storing this weak
> > pointer in v8
> 
> Right. As mentioned in the doc, we anyway need to store the CreationContext
pointer somewhere. Currently we store it in V8, and our plan is to move it to
Blink. My point is just that I want to use V8 until we actually finish the
migration (which seems not easy).
> 
> > and you iterate over all persistent handles instead of just over
> > all observers of a given context.
> 
> Are you talking about V8GCController::hasPendingActivity? It is used only by
worker's postMessage as a hack to check if the worker's context has any pending
activity (we need to remove the hack somehow). So I think what matters more
would be the performance of (a) hasPendingActivity used in GC callbacks and (b)
LifecycleNotifier's methods (e.g.,
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...).
> 
> (a) iterates over all persistent handles anyway (by the V8 GC Visitor). (b)
iterates over all LifecycleObservers, and I want to reduce the number of
ContextLifecycleObservers for (b).

once we have incremental marking of dom objects, (a) won't happen anymore.

I can understand that you want to get rid of lifecycle observers, but I don't
think it's a good solution to address this by increasing the dependencies on a
deprecated system.

[haraken, 2016-09-23 06:35:00]

I'm now hitting another case that needs the CreationContext API.

I noticed that hasPendingActivity() has been causing memory leaks on multiple
objects, so I want to make a change to make
ActiveScriptWrappable::hasPendingActivity() return false after
ActiveScriptWrappable's associated window gets detached. To get the associated
window object from a given wrapper, I need this API.

I think the situation is:

- It's important to fix the leaks asap.

- traceWrapper will replace most of hasPendingActivity()'s but not everything.
Some hasPendingActivity() will remain (e.g., XHR's hasPendingActivity()).

- We have concluded that the concept of creation context is necessary (it's the
relevant realm in the spec terms). We should probably move the concept from V8
to Blink but don't yet have a clear plan for it.

Any thoughts?

[Toon Verwaest, 2016-09-23 07:01:06]

I have no problem with keeping track of the creation context of specific objects
in blink, we should just not require it for all objects.

If we add something to fix something now, but plan on replacing it, shouldn't we
make that clear in the API?

[haraken, 2016-09-23 07:10:40]

On 2016/09/23 07:01:06, Toon Verwaest wrote:
> I have no problem with keeping track of the creation context of specific
objects
> in blink, we should just not require it for all objects.
> 
> If we add something to fix something now, but plan on replacing it, shouldn't
we
> make that clear in the API?

Any suggestion? Adding more comment?

[jochen (gone - plz use gerrit), 2016-09-23 10:31:22]

On 2016/09/23 at 07:10:40, haraken wrote:
> On 2016/09/23 07:01:06, Toon Verwaest wrote:
> > I have no problem with keeping track of the creation context of specific
objects
> > in blink, we should just not require it for all objects.
> > 
> > If we add something to fix something now, but plan on replacing it,
shouldn't we
> > make that clear in the API?
> 
> Any suggestion? Adding more comment?

why is stuff like XHR not observing the window if it depends on its lifetime?

[haraken, 2016-09-23 10:47:17]

On 2016/09/23 10:31:22, jochen (slow) wrote:
> On 2016/09/23 at 07:10:40, haraken wrote:
> > On 2016/09/23 07:01:06, Toon Verwaest wrote:
> > > I have no problem with keeping track of the creation context of specific
> objects
> > > in blink, we should just not require it for all objects.
> > > 
> > > If we add something to fix something now, but plan on replacing it,
> shouldn't we
> > > make that clear in the API?
> > 
> > Any suggestion? Adding more comment?
> 
> why is stuff like XHR not observing the window if it depends on its lifetime?

XHR wants to collect the wrapper earlier than the window object gets collected.
XHR's wrapper can be collected when XHR finishes dispatching all events. There
we want to use hasPendingActivity.

Once we enable traceWrapper, hasPendingActivity will become a way to represent
reachability that can be collected earlier than the window object. Hence it's
important to make hasPendingActivity not return true after the window object is
detached. To implement the logic, I need the CreationContext API.

[jochen (gone - plz use gerrit), 2016-09-27 10:13:55]

lgtm

[haraken, 2016-09-27 13:40:07]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2016-09-27 13:40:12]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-27 14:19:25]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-09-27 14:19:59]

Description was changed from

==========
Add v8::Object::CreationContext that works for a persistent handle

I need this API for https://codereview.chromium.org/1609343002/.

BUG=483722
==========

to

==========
Add v8::Object::CreationContext that works for a persistent handle

I need this API for https://codereview.chromium.org/1609343002/.

BUG=483722

Committed: https://crrev.com/9285e66630c4173d80929038f3862b6910b580b8
Cr-Commit-Position: refs/heads/master@{#39775}
==========

[commit-bot: I haz the power, 2016-09-27 14:20:00]

Patchset 1 (id:??) landed as
https://crrev.com/9285e66630c4173d80929038f3862b6910b580b8
Cr-Commit-Position: refs/heads/master@{#39775}