Bug 91222 – Make Functional bind PassOwnPtr.

• Status: UNCONFIRMED
• Component: Web Template Framework

[Dongseong Hwang]

In common, threads communicate using Functional. However, Functional causes the
segment fault in following case.

Class Foo {};

void bar(PassOwnPtr<Foo> foo);

{
    OwnPtr<Foo> foo;
    callOnMainThread(bind(bar, foo.release());
}

This patch fixed it.

[Dongseong Hwang]

Created attachment 152223 [details]
Patch

[Kwang Yul Seo]

Comment on attachment 152223 [details]
Patch

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

> Source/WTF/ChangeLog:8
> +        In common, threads communicate using Functional. However, Functional causes the

Functional is commonly used for inter-thread communication.

> Tools/TestWebKitAPI/Tests/WTF/Functional.cpp:221
> +class OwnershipClass {

This class is a own-ptr version of Number class above. Why don't we name it OwnedNumber or something instead of OwnershipClass?

[Kwang Yul Seo]

Comment on attachment 152223 [details]
Patch

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

> Source/WTF/wtf/Functional.h:322
> +    static void deref(const OwnPtr<T>&) { }

I am curious why we need const here.

[Dongseong Hwang]

(In reply to comment #2)
> (From update of attachment 152223 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=152223&action=review
> 
> > Source/WTF/ChangeLog:8
> > +        In common, threads communicate using Functional. However, Functional causes the
> 
> Functional is commonly used for inter-thread communication.
> 
> > Tools/TestWebKitAPI/Tests/WTF/Functional.cpp:221
> > +class OwnershipClass {
> 
> This class is a own-ptr version of Number class above. Why don't we name it OwnedNumber or something instead of OwnershipClass?

Good point. I will update.

[Dongseong Hwang]

(In reply to comment #3)
> (From update of attachment 152223 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=152223&action=review
> 
> > Source/WTF/wtf/Functional.h:322
> > +    static void deref(const OwnPtr<T>&) { }
> 
> I am curious why we need const here.

"const" is not necessary. Putting "const" in front of a reference is my habit. However, I think it is not bad.

[Dongseong Hwang]

Created attachment 152374 [details]
Patch

[Dongseong Hwang]

(In reply to comment #6)
> Created an attachment (id=152374) [details]
> Patch

Amended Changelog to correct the grammar.
Renamed the class name for test from OwnershipClass to OwnedNumber.

[Dongseong Hwang]

I'm waiting the review.

I filed this bug because I wanted to use the Functional with OwnPtr in parallel image decoders (Bug 90375).
Currently, I used foo(Bar*) with 'delete bar' instead of foo(PassOwnPtr<Bar>).

[Anders Carlsson]

Comment on attachment 152374 [details]
Patch

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

> Source/WTF/wtf/Functional.h:322
> +template<typename T, bool shouldRefAndDeref> struct RefAndDeref<PassOwnPtr<T>, shouldRefAndDeref> {
> +    static void ref(const OwnPtr<T>&) { }
> +    static void deref(const OwnPtr<T>&) { }
> +};

Is this really needed?

[Dongseong Hwang]

Comment on attachment 152374 [details]
Patch

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

>> Source/WTF/wtf/Functional.h:322
>> +};
> 
> Is this really needed?

Yes, it is needed because the StorageType of PassOwnPtr is OwnPtr. In "RefAndDeref<P1, FunctionWrapper::shouldRefFirstParameter>::ref(m_p1)", P1 is PassOwnPtr but m_p1 is OwnPtr, which is the StorageType of PassOwnPtr.
So if this code stub is removed, we will encounter a compile error like the following in gcc.

"
../../../Source/WTF/wtf/Functional.h: In constructor ‘WTF::BoundFunctionImpl<FunctionWrapper, R(P1)>::BoundFunctionImpl(FunctionWrapper, const P1&) [with FunctionWrapper = WTF::FunctionWrapper<int (*)(WTF::PassOwnPtr<TestWebKitAPI::OwnedNumber>)>, R = int, P1 = WTF::PassOwnPtr<TestWebKitAPI::OwnedNumber>]’:
../../../Source/WTF/wtf/Functional.h:662:240:   instantiated from ‘WTF::Function<typename WTF::FunctionWrapper<FunctionType>::ResultType()> WTF::bind(FunctionType, const A1&) [with FunctionType = int (*)(WTF::PassOwnPtr<TestWebKitAPI::OwnedNumber>), A1 = WTF::PassOwnPtr<TestWebKitAPI::OwnedNumber>, typename WTF::FunctionWrapper<FunctionType>::ResultType = int]’
../../../Tools/TestWebKitAPI/Tests/WTF/Functional.cpp:263:72:   instantiated from here
../../../Source/WTF/wtf/Functional.h:404:9: error: no matching function for call to ‘WTF::RefAndDeref<WTF::PassOwnPtr<TestWebKitAPI::OwnedNumber>, false>::ref(WTF::ParamStorageTraits<WTF::PassOwnPtr<TestWebKitAPI::OwnedNumber> >::StorageType&)’
../../../Source/WTF/wtf/Functional.h:404:9: note: candidate is:
../../../Source/WTF/wtf/Functional.h:315:17: note: static void WTF::RefAndDeref<T, shouldRefAndDeref>::ref(T) [with T = WTF::PassOwnPtr<TestWebKitAPI::OwnedNumber>, bool shouldRefAndDeref = false]
../../../Source/WTF/wtf/Functional.h:315:17: note:   no known conversion for argument 1 from ‘WTF::ParamStorageTraits<WTF::PassOwnPtr<TestWebKitAPI::OwnedNumber> >::StorageType {aka WTF::OwnPtr<TestWebKitAPI::OwnedNumber>}’ to ‘WTF::PassOwnPtr<TestWebKitAPI::OwnedNumber>’
"

[Anders Carlsson]

Comment on attachment 152374 [details]
Patch

Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.