Bug 111802 - Make all v8 weak callbacks type safe
Summary: Make all v8 weak callbacks type safe
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Elliott Sprehn
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-03-07 18:00 PST by Elliott Sprehn
Modified: 2013-04-01 23:22 PDT (History)
5 users (show)

See Also:


Attachments
WIP (20.76 KB, patch)
2013-03-07 18:02 PST, Elliott Sprehn
no flags Details | Formatted Diff | Diff
Patch (23.97 KB, patch)
2013-03-11 22:38 PDT, Elliott Sprehn
no flags Details | Formatted Diff | Diff
Patch for landing (23.92 KB, patch)
2013-04-01 18:23 PDT, Elliott Sprehn
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Elliott Sprehn 2013-03-07 18:00:27 PST
Make all v8 weak callbacks type safe
Comment 1 Elliott Sprehn 2013-03-07 18:02:09 PST
Created attachment 192120 [details]
WIP
Comment 2 Elliott Sprehn 2013-03-07 18:03:53 PST
Note that I'm not suggesting we actually do this, but I worked through the motions of how we might do this so I figured I'd post the patch.

It'd be much nicer if we could just fix the v8 API to take a functor object with an operator() overload and then deal with the typesafe ourselves.
Comment 3 Kentaro Hara 2013-03-07 18:26:53 PST
Comment on attachment 192120 [details]
WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=192120&action=review

> Source/WebCore/ChangeLog:48
> +        * bindings/v8/V8Utilities.h:
> +        (WeakHandle):
> +        (WebCore::WeakHandle::makeWeak):
> +        (WebCore::WeakHandle::WeakHandle):
> +        (WebCore::WeakHandle::invokeWeakCallback):
> +        (WebCore::WeakHandle::weakHandleCallback):
> +        (WebCore::makeWeak):

Actually V8Utilities.h is a collection of methods that should be moved to somewhere. Could you add the template to DOMWrapperMap.h ?

> Source/WebCore/bindings/v8/DOMWrapperMap.h:111
> -    v8::NearDeathCallback m_callback;
> +    WeakCallbackType m_callback;

Sorry, I don't fully understand the magic. After this patch, "v8::NearDeathCallback" is completely gone away from V8 binding. Then how does it work as v8::NearDeathCallback ?
Comment 4 Elliott Sprehn 2013-03-07 18:41:39 PST
(In reply to comment #3)
> (From update of attachment 192120 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=192120&action=review
> 
> > Source/WebCore/ChangeLog:48
> > +        * bindings/v8/V8Utilities.h:
> > +        (WeakHandle):
> > +        (WebCore::WeakHandle::makeWeak):
> > +        (WebCore::WeakHandle::WeakHandle):
> > +        (WebCore::WeakHandle::invokeWeakCallback):
> > +        (WebCore::WeakHandle::weakHandleCallback):
> > +        (WebCore::makeWeak):
> 
> Actually V8Utilities.h is a collection of methods that should be moved to somewhere. Could you add the template to DOMWrapperMap.h ?

Hmm, that would mean anyone using a v8::Persistent and calling makeWeak() would need to include DOMWrapperMap even if they don't use the wrapper map which is weird, but maybe it's okay.

> 
> > Source/WebCore/bindings/v8/DOMWrapperMap.h:111
> > -    v8::NearDeathCallback m_callback;
> > +    WeakCallbackType m_callback;
> 
> Sorry, I don't fully understand the magic. After this patch, "v8::NearDeathCallback" is completely gone away from V8 binding. Then how does it work as v8::NearDeathCallback ?

WeakHandle<T, H>::invokeWeakCallback is a v8::NearDeathCallback. Each call to makeWeak for each type creates a template instantiation with an invokeWeakCallback that calls the type safe version.

A major downside of this patch is that you couldn't have a class that has two different weakCallbacks. Right now we never do that so it's not an issue.
Comment 5 Kentaro Hara 2013-03-07 18:45:43 PST
(In reply to comment #4)
> > Actually V8Utilities.h is a collection of methods that should be moved to somewhere. Could you add the template to DOMWrapperMap.h ?
> 
> Hmm, that would mean anyone using a v8::Persistent and calling makeWeak() would need to include DOMWrapperMap even if they don't use the wrapper map which is weird, but maybe it's okay.

Maybe it would be better to create a new header file.

> > Sorry, I don't fully understand the magic. After this patch, "v8::NearDeathCallback" is completely gone away from V8 binding. Then how does it work as v8::NearDeathCallback ?
> 
> WeakHandle<T, H>::invokeWeakCallback is a v8::NearDeathCallback. Each call to makeWeak for each type creates a template instantiation with an invokeWeakCallback that calls the type safe version.

Thanks, makes sense!
 
> A major downside of this patch is that you couldn't have a class that has two different weakCallbacks. Right now we never do that so it's not an issue.

This wouldn't be an issue in practice. But we should have abarth look at the patch, as I don't understand the template magic well enough to review it:)
Comment 6 Adam Barth 2013-03-08 11:37:41 PST
Comment on attachment 192120 [details]
WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=192120&action=review

The type safety part of this patch is nice, but the static dependency in WeakHandle is really nasty.  I can see how having a real callback object in V8 would make this much nicer.

> Source/WebCore/bindings/v8/DOMWrapperWorld.cpp:92
> -    static_cast<DOMWrapperWorld*>(parameter)->deref();
> +    world->deref();

That's nice!

> Source/WebCore/bindings/v8/V8Utilities.h:89
> +            static CallbackType s_callback = 0;

static!  Oh my.  This seems like a bad idea...
Comment 7 Elliott Sprehn 2013-03-11 22:38:08 PDT
Created attachment 192642 [details]
Patch
Comment 8 Elliott Sprehn 2013-03-11 22:42:35 PDT
(In reply to comment #7)
> Created an attachment (id=192642) [details]
> Patch

Here's a way that doesn't use the weird static locals. If anyone has ideas for better naming I'm happy to change things.

Now we can add type safe weak listeners with:

WeakHandleListener<Foo>::makeWeak(isolate, handle, fooPtr);

or if the argument is something different than the thing creating the type we can use:

WeakHandleListener<Bar, Foo>::makeWeak(isolate, handle, fooPtr); // Foo handling specific to Bar.

This lets things like StringCache create a listener for StringImpl.
Comment 9 Adam Barth 2013-03-12 11:28:42 PDT
Comment on attachment 192642 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=192642&action=review

This is way better!

> Source/WebCore/bindings/v8/V8Utilities.h:67
> +    template<class OwnerType, typename ArgumentType = OwnerType>

Any reason why one of these arguments is "class" and the other "typename"?
Comment 10 Adam Klein 2013-03-12 13:03:34 PDT
Comment on attachment 192642 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=192642&action=review

Super minor nits, happy to see the lack of a static.

> Source/WebCore/bindings/v8/V8Utilities.h:66
> +    // FIXME: We might move this to it's own file.

Tiny nit: s/it's/its/

> Source/WebCore/bindings/v8/V8Utilities.h:79
> +        explicit WeakHandleListener() { }

"explicit" doesn't have meaning here. Also, can you leave out the implementation of the constructor altogether?
Comment 11 Elliott Sprehn 2013-03-12 14:37:14 PDT
Comment on attachment 192642 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=192642&action=review

>> Source/WebCore/bindings/v8/V8Utilities.h:67
>> +    template<class OwnerType, typename ArgumentType = OwnerType>
> 
> Any reason why one of these arguments is "class" and the other "typename"?

OwnerType is the class which is creating the weak handles, ArgumentType is just the thing that's passed into the callback. It could be class but it would prevent you from passing an int* or some such to the callback. Probably not a useful feature so I'll switch to class.
Comment 12 Elliott Sprehn 2013-04-01 18:21:40 PDT
(In reply to comment #10)
> (From update of attachment 192642 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=192642&action=review
> 
> Super minor nits, happy to see the lack of a static.
> 
> > Source/WebCore/bindings/v8/V8Utilities.h:66
> > +    // FIXME: We might move this to it's own file.
> 
> Tiny nit: s/it's/its/

Fixed.

> 
> > Source/WebCore/bindings/v8/V8Utilities.h:79
> > +        explicit WeakHandleListener() { }
> 
> "explicit" doesn't have meaning here. Also, can you leave out the implementation of the constructor altogether?

Fixed.
Comment 13 Elliott Sprehn 2013-04-01 18:23:33 PDT
Created attachment 196049 [details]
Patch for landing
Comment 14 WebKit Review Bot 2013-04-01 23:22:17 PDT
Comment on attachment 196049 [details]
Patch for landing

Clearing flags on attachment: 196049

Committed r147391: <http://trac.webkit.org/changeset/147391>
Comment 15 WebKit Review Bot 2013-04-01 23:22:21 PDT
All reviewed patches have been landed.  Closing bug.