Bug 111802

Summary: Make all v8 weak callbacks type safe
Product: WebKit Reporter: Elliott Sprehn <esprehn>
Component: New BugsAssignee: Elliott Sprehn <esprehn>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, adamk, haraken, japhet, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
WIP
none
Patch
none
Patch for landing none

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.