Bug 29077 – [v8] Share persistent context handles between events

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Christian Plesner Hansen]

Change events to use a single ref-counted shared persistent handle to the context in which to run, rather than create a new persistent handle for each new event which puts pressure on the gc.

[Christian Plesner Hansen]

Created attachment 39253 [details]
initial

[Adam Barth]

Comment on attachment 39253 [details]
initial

SharedV8Context doesn't really have anything to do with contexts.  We should make it more general (templated?) and put it in its own header, like OwnHandle.  You might want to call it something like RefCountedHandle or something.

Aside from that, this patch looks good.

[Adam Barth]

Also, why not make the destructor Dispose and Clear the handle if that hasn't happened already?

[Adam Barth]

Actually, SharedHandle is a good name.  You should also considering defining a typedef that defines RefHandle<T> as RefPtr<SharedHandle<T> >.

[Christian Plesner Hansen]

Created attachment 39340 [details]
updated

[Adam Barth]

Comment on attachment 39340 [details]
updated

+ PassRefPtr<SharedPersistent<v8::Context> > context = proxy->shared_context();

We don't usually hold PassRefPtrs as local variables.  They're a bit tricky to use like this b/c they like to null themselves out.  In this case, we're fine, but I wanted to make sure you're realized what you were doing was somewhat subtle.

+        No new tests. (OOPS!)

This line will be rejected by the pre-submit checks.  We can either land this manually, or we can land it automatically if you upload a new patch without this line.

[Christian Plesner Hansen]

(In reply to comment #6)
> (From update of attachment 39340 [details])
> + PassRefPtr<SharedPersistent<v8::Context> > context = proxy->shared_context();
> 
> We don't usually hold PassRefPtrs as local variables.  They're a bit tricky to
> use like this b/c they like to null themselves out.  In this case, we're fine,
> but I wanted to make sure you're realized what you were doing was somewhat
> subtle.

No, it should have been a RefPtr.  I'd prefer to fix it even though it happens to work here.

> +        No new tests. (OOPS!)
> 
> This line will be rejected by the pre-submit checks.  We can either land this
> manually, or we can land it automatically if you upload a new patch without
> this line.

I'll upload a new patch with the above change and this line removed.

[Christian Plesner Hansen]

Created attachment 39413 [details]
tweaks

[Adam Barth]

Comment on attachment 39413 [details]
tweaks

I'm worried about how this interacts with weak handles.  For example, we lose

- m_context.makeWeak();

in V8AbstractEventListener.cpp.  That seems like it might cause problems...

Also,

-    m_context.MakeWeak(this, &contextWeakReferenceCallback);
+    m_context->get().MakeWeak(this, &contextWeakReferenceCallback);

in V8IsolatedWorld.cpp.  Won't that make the shared handle weak?  Doesn't V8Proxy need to hold a strong reference to the context?

[Christian Plesner Hansen]

> I'm worried about how this interacts with weak handles.  For example, we lose
> 
> - m_context.makeWeak();
> 
> in V8AbstractEventListener.cpp.  That seems like it might cause problems...

Before, each OwnHandle had its own persistent handle it needed to manager.  Now the event is sharing the handle of the frame or isolated world to which it belongs the owner takes care of that.

> Also,
> 
> -    m_context.MakeWeak(this, &contextWeakReferenceCallback);
> +    m_context->get().MakeWeak(this, &contextWeakReferenceCallback);
> 
> in V8IsolatedWorld.cpp.  Won't that make the shared handle weak?  Doesn't
> V8Proxy need to hold a strong reference to the context?

Yes, it will make the shared handle weak.  But that's no different from how it was before really except that before there was as many handles as there were event listeners, all of them weak, whereas now there is just one.

[Adam Barth]

> Before, each OwnHandle had its own persistent handle it needed to manager.  Now
> the event is sharing the handle of the frame or isolated world to which it
> belongs the owner takes care of that.

But you see that the lifetime of the handle has changed right?  We need to think carefully and make sure that the new lifetime is ok for the event listeners.  The key point is that the event listeners won't fire when they ought not to fire.  We have some amount of test coverage for this, but it's not complete.

Maybe we're ok because V8Proxy explicitly Disposes / Clears the shared context when it would have dropped its strong handle before?

> > Also,
> > 
> > -    m_context.MakeWeak(this, &contextWeakReferenceCallback);
> > +    m_context->get().MakeWeak(this, &contextWeakReferenceCallback);
> > 
> > in V8IsolatedWorld.cpp.  Won't that make the shared handle weak?  Doesn't
> > V8Proxy need to hold a strong reference to the context?
> 
> Yes, it will make the shared handle weak.  But that's no different from how it
> was before really except that before there was as many handles as there were
> event listeners, all of them weak, whereas now there is just one.

The whole thing is super confusing though.  If you're in the main world, it just so happens that the shared context is a strong reference to the context, but when you're in an isolated world, the shared handle is weak.  All the clients of V8Proxy::context(Frame*) now have to be prepared to deal with either case.

Maybe we're ok because these clients already have to worry about this issue?

Anyway, it's after 2 a.m. here.  I'll take a look at it again tomorrow when I'm more awake.

Ager, any insight that you have would be appreciated.

[Christian Plesner Hansen]

> But you see that the lifetime of the handle has changed right?  We need to
> think carefully and make sure that the new lifetime is ok for the event
> listeners.  The key point is that the event listeners won't fire when they
> ought not to fire.  We have some amount of test coverage for this, but it's not
> complete.
> 
> Maybe we're ok because V8Proxy explicitly Disposes / Clears the shared context
> when it would have dropped its strong handle before?

The handle is disposed by the proxy at the same time now as before.  What is different is that before event would hang on to the context from the V8Proxy had disposed its handle and until the next gc (which, incidentally, made event behavior non-deterministic) whereas now events lose access to the context immediately.  I would expect that to be 

> The whole thing is super confusing though.  If you're in the main world, it
> just so happens that the shared context is a strong reference to the context,
> but when you're in an isolated world, the shared handle is weak.  All the
> clients of V8Proxy::context(Frame*) now have to be prepared to deal with either
> case.
> 
> Maybe we're ok because these clients already have to worry about this issue?

The handle returned here will behave exactly the same as before since it's returned directly from the owner who has the same behavior as before.

[Adam Barth]

Comment on attachment 39413 [details]
tweaks

Ok.  I'm sold.  Thanks for explaining this to me.

[WebKit Commit Bot]

Comment on attachment 39413 [details]
tweaks

Clearing flags on attachment: 39413

Committed r48306: <http://trac.webkit.org/changeset/48306>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Dimitri Glazkov (Google)]

Christian, I am undoing most of this work in bug 30648, but not because your code was wrong -- we no longer have to hold on to the context in an event listener. Can you take a look/see?