Bug 111802 – Make all v8 weak callbacks type safe

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Elliott Sprehn]

Make all v8 weak callbacks type safe

[Elliott Sprehn]

Created attachment 192120 [details]
WIP

[Elliott Sprehn]

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.

[Kentaro Hara]

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 ?

[Elliott Sprehn]

(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.

[Kentaro Hara]

(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:)

[Adam Barth]

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...

[Elliott Sprehn]

Created attachment 192642 [details]
Patch

[Elliott Sprehn]

(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.

[Adam Barth]

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"?

[Adam Klein]

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?

[Elliott Sprehn]

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.

[Elliott Sprehn]

(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.

[Elliott Sprehn]

Created attachment 196049 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 196049 [details]
Patch for landing

Clearing flags on attachment: 196049

Committed r147391: <http://trac.webkit.org/changeset/147391>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.