Bug 177389 - WeakPtrFactory should allow downcasting
Summary: WeakPtrFactory should allow downcasting
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: zalan
URL:
Keywords: InRadar
Depends on: 177375
Blocks: 177429
  Show dependency treegraph
 
Reported: 2017-09-22 15:07 PDT by Jiewen Tan
Modified: 2017-10-16 12:32 PDT (History)
11 users (show)

See Also:


Attachments
Patch (3.55 KB, patch)
2017-09-25 15:26 PDT, zalan
no flags Details | Formatted Diff | Diff
Patch (10.88 KB, patch)
2017-09-26 23:54 PDT, Jiewen Tan
no flags Details | Formatted Diff | Diff
Second Solution (10.54 KB, patch)
2017-09-27 11:18 PDT, Jiewen Tan
no flags Details | Formatted Diff | Diff
First Solution (6.97 KB, patch)
2017-09-27 12:00 PDT, Jiewen Tan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jiewen Tan 2017-09-22 15:07:18 PDT
WeakPtrFactory should allow downcasting, i.e. creating a weak pointer of the current class' derived classes.
Comment 1 Radar WebKit Bug Importer 2017-09-22 15:09:48 PDT
<rdar://problem/34604174>
Comment 2 Jiewen Tan 2017-09-22 15:15:47 PDT
Geoff's proposal:

The Problem we have currently:
WeakPtr<Element> ptr = parent.createWeakPtr(); // fails: returns WeakPtr<Node>
WeakPtr<Element> ptr = parent.createWeakPtr<Element>(); // succeeds: returns WeakPtr<Element>

Solution is something like this:
   template<typename U> WeakPtr<U> createWeakPtr() const { return WeakPtr<U>(static_reference_cast<U>(m_ref); }
Comment 3 zalan 2017-09-25 15:26:44 PDT
Created attachment 321758 [details]
Patch
Comment 4 Jiewen Tan 2017-09-25 17:22:23 PDT
Comment on attachment 321758 [details]
Patch

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

Thank you for making this patch. Have one question listed below. Could you explain it to me?

> Source/WTF/wtf/WeakPtr.h:108
> +        return WeakPtr<U>(Ref<WeakReference<U>>(static_cast<WeakReference<U>&>(*m_ref)));

No sure why static_cast works here. Could you elaborate it a little bit?
Comment 5 Geoffrey Garen 2017-09-25 17:27:28 PDT
> > Source/WTF/wtf/WeakPtr.h:108
> > +        return WeakPtr<U>(Ref<WeakReference<U>>(static_cast<WeakReference<U>&>(*m_ref)));
> 
> No sure why static_cast works here. Could you elaborate it a little bit?

Yeah, I didn't write it quite right here. I think Zalan has a working copy of this function now.
Comment 6 Geoffrey Garen 2017-09-25 17:43:14 PDT
> > Source/WTF/wtf/WeakPtr.h:108
> > +        return WeakPtr<U>(Ref<WeakReference<U>>(static_cast<WeakReference<U>&>(*m_ref)));
> 
> No sure why static_cast works here. Could you elaborate it a little bit?

Yeah, actually, this is wrong. It only worked in this patch because T and U are equal across all invocations. But when T and U are not equal, static casting WeakReference<T>& to WeakReference<U>& is an error.

So, we need a better version of this function.
Comment 7 zalan 2017-09-25 17:45:37 PDT
(In reply to Geoffrey Garen from comment #6)
> > > Source/WTF/wtf/WeakPtr.h:108
> > > +        return WeakPtr<U>(Ref<WeakReference<U>>(static_cast<WeakReference<U>&>(*m_ref)));
> > 
> > No sure why static_cast works here. Could you elaborate it a little bit?
> 
> Yeah, actually, this is wrong. It only worked in this patch because T and U
> are equal across all invocations. But when T and U are not equal, static
> casting WeakReference<T>& to WeakReference<U>& is an error.
> 
> So, we need a better version of this function.
Yea, and I was going to work on that. This was more of the first iteration, hence not landed yet.
Comment 8 Geoffrey Garen 2017-09-25 17:50:53 PDT
I guess there are two ways to fix this:

We could add a reinterpret_cast that static_asserts the conversion between T* and U* is OK.

Or we could add a template parameter to WeakPtr, and say that WeakPtr points to WeakReference<T>, and casts to U* when you get its contents.

The second option is probably more in line with idiomatic C++, so if it doesn't make WeakPtr construction too annoying, it might be preferred.
Comment 9 Jiewen Tan 2017-09-26 23:54:54 PDT
Created attachment 321940 [details]
Patch
Comment 10 Build Bot 2017-09-26 23:55:59 PDT
Attachment 321940 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/WeakPtr.h:68:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/WeakPtr.h:69:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 2 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 Jiewen Tan 2017-09-27 00:00:10 PDT
Comment on attachment 321940 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/WTF/WeakPtr.cpp:215
> +    WeakPtr<Derived, Base> derivedWeakPtr;

Ryosuke and I are kind of worrying this format will be annoying to use. However, I don't know how it could be prevented, i.e. using WeakPtr<Derived> instead of WeakPtr<Derived, Base>. If anyone has any ideas, please tell me. Appreciate it.
Comment 12 Geoffrey Garen 2017-09-27 08:13:01 PDT
Comment on attachment 321940 [details]
Patch

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

> Source/WTF/wtf/WeakPtr.h:88
> +        static_assert((!std::is_class<U>::value && !std::is_class<T>::value) || std::is_base_of<U, T>::value, "U should be a base class of T");

There's no need for this assertion because it's built into the static_cast. The value of the double template parameter solution is that you don't need any assertions: The language guarantees correctness.
Comment 13 Geoffrey Garen 2017-09-27 08:26:06 PDT
Comment on attachment 321940 [details]
Patch

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

>> Source/WTF/wtf/WeakPtr.h:88
>> +        static_assert((!std::is_class<U>::value && !std::is_class<T>::value) || std::is_base_of<U, T>::value, "U should be a base class of T");
> 
> There's no need for this assertion because it's built into the static_cast. The value of the double template parameter solution is that you don't need any assertions: The language guarantees correctness.

...built into the static_cast in the getter.

>> Tools/TestWebKitAPI/Tests/WTF/WeakPtr.cpp:215
>> +    WeakPtr<Derived, Base> derivedWeakPtr;
> 
> Ryosuke and I are kind of worrying this format will be annoying to use. However, I don't know how it could be prevented, i.e. using WeakPtr<Derived> instead of WeakPtr<Derived, Base>. If anyone has any ideas, please tell me. Appreciate it.

If this is too annoying, we can use the reinterpret_cast solution:

template<typename T, typename U>
WeakReference<T>& static_weak_reference_cast(WeakReference<U>& weakReference)
{
    static_assert(std::is_convertible<T*, U*>::value, "U* must be convertible to T*");
    return reinterpret_cast<WeakReference <T>&>(weakReference);
}

Then createWeakPtr would static_weak_reference_cast m_ref when creating a WeakPtr.
Comment 14 Jiewen Tan 2017-09-27 11:14:43 PDT
Comment on attachment 321940 [details]
Patch

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

Thanks Geoff for reviewing the patch. I will upload a patch addressing comments in this patch and then upload another that uses the first solution. Then we can decide which one to go.

>>> Source/WTF/wtf/WeakPtr.h:88
>>> +        static_assert((!std::is_class<U>::value && !std::is_class<T>::value) || std::is_base_of<U, T>::value, "U should be a base class of T");
>> 
>> There's no need for this assertion because it's built into the static_cast. The value of the double template parameter solution is that you don't need any assertions: The language guarantees correctness.
> 
> ...built into the static_cast in the getter.

Removed.

>>> Tools/TestWebKitAPI/Tests/WTF/WeakPtr.cpp:215
>>> +    WeakPtr<Derived, Base> derivedWeakPtr;
>> 
>> Ryosuke and I are kind of worrying this format will be annoying to use. However, I don't know how it could be prevented, i.e. using WeakPtr<Derived> instead of WeakPtr<Derived, Base>. If anyone has any ideas, please tell me. Appreciate it.
> 
> If this is too annoying, we can use the reinterpret_cast solution:
> 
> template<typename T, typename U>
> WeakReference<T>& static_weak_reference_cast(WeakReference<U>& weakReference)
> {
>     static_assert(std::is_convertible<T*, U*>::value, "U* must be convertible to T*");
>     return reinterpret_cast<WeakReference <T>&>(weakReference);
> }
> 
> Then createWeakPtr would static_weak_reference_cast m_ref when creating a WeakPtr.

Sure. The first solution will definitely resolve this issue for us. I was trying to upload two patches, one for each, yesterday, but was not able to accomplish that. Building for WTF is too costly, I am wondering if it is possible to just build WTF and corresponding API tests.
Comment 15 Jiewen Tan 2017-09-27 11:18:57 PDT
Created attachment 321978 [details]
Second Solution
Comment 16 Build Bot 2017-09-27 11:21:39 PDT
Attachment 321978 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/WeakPtr.h:66:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/WeakPtr.h:67:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/WeakPtr.h:83:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 3 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 17 Jiewen Tan 2017-09-27 11:27:28 PDT
(In reply to Jiewen Tan from comment #14)
> Comment on attachment 321940 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=321940&action=review
> 
> Thanks Geoff for reviewing the patch. I will upload a patch addressing
> comments in this patch and then upload another that uses the first solution.
> Then we can decide which one to go.
> 
> >>> Source/WTF/wtf/WeakPtr.h:88
> >>> +        static_assert((!std::is_class<U>::value && !std::is_class<T>::value) || std::is_base_of<U, T>::value, "U should be a base class of T");
> >> 
> >> There's no need for this assertion because it's built into the static_cast. The value of the double template parameter solution is that you don't need any assertions: The language guarantees correctness.
> > 
> > ...built into the static_cast in the getter.
> 
> Removed.
> 
> >>> Tools/TestWebKitAPI/Tests/WTF/WeakPtr.cpp:215
> >>> +    WeakPtr<Derived, Base> derivedWeakPtr;
> >> 
> >> Ryosuke and I are kind of worrying this format will be annoying to use. However, I don't know how it could be prevented, i.e. using WeakPtr<Derived> instead of WeakPtr<Derived, Base>. If anyone has any ideas, please tell me. Appreciate it.
> > 
> > If this is too annoying, we can use the reinterpret_cast solution:
> > 
> > template<typename T, typename U>
> > WeakReference<T>& static_weak_reference_cast(WeakReference<U>& weakReference)
> > {
> >     static_assert(std::is_convertible<T*, U*>::value, "U* must be convertible to T*");
> >     return reinterpret_cast<WeakReference <T>&>(weakReference);
> > }
> > 
> > Then createWeakPtr would static_weak_reference_cast m_ref when creating a WeakPtr.
> 
> Sure. The first solution will definitely resolve this issue for us. I was
> trying to upload two patches, one for each, yesterday, but was not able to
> accomplish that. Building for WTF is too costly, I am wondering if it is
> possible to just build WTF and corresponding API tests.

Hmm, I could build WTF and TestWebKitAPI separately.
Comment 18 Jiewen Tan 2017-09-27 12:00:27 PDT
Created attachment 321989 [details]
First Solution
Comment 19 Build Bot 2017-09-27 12:03:33 PDT
Attachment 321989 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/WeakPtr.h:71:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 20 Jiewen Tan 2017-09-27 16:58:27 PDT
Comment on attachment 321989 [details]
First Solution

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

> Tools/TestWebKitAPI/Tests/WTF/WeakPtr.cpp:228
> +        EXPECT_EQ(derivedWeakPtr->foo(), dummy1);

It should be derivedPtr.
Comment 21 Jiewen Tan 2017-09-27 16:59:24 PDT
Comment on attachment 321978 [details]
Second Solution

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

> Tools/TestWebKitAPI/Tests/WTF/WeakPtr.cpp:228
> +        EXPECT_EQ(derivedWeakPtr->foo(), dummy1);

It should be derivedPtr.
Comment 22 Ryosuke Niwa 2017-09-27 18:45:05 PDT
Comment on attachment 321989 [details]
First Solution

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

> Tools/TestWebKitAPI/Tests/WTF/WeakPtr.cpp:215
> +    WeakPtr<Base> baseWeakPtr;
> +    WeakPtr<Derived> derivedWeakPtr;

This looks much better to me.
Comment 23 zalan 2017-09-27 18:46:35 PDT
(In reply to Ryosuke Niwa from comment #22)
> Comment on attachment 321989 [details]
> First Solution
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=321989&action=review
> 
> > Tools/TestWebKitAPI/Tests/WTF/WeakPtr.cpp:215
> > +    WeakPtr<Base> baseWeakPtr;
> > +    WeakPtr<Derived> derivedWeakPtr;
> 
> This looks much better to me.
So much better.
Comment 24 Daniel Bates 2017-09-27 19:54:31 PDT
Comment on attachment 321989 [details]
First Solution

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

>>> Tools/TestWebKitAPI/Tests/WTF/WeakPtr.cpp:215
>>> +    WeakPtr<Derived> derivedWeakPtr;
>> 
>> This looks much better to me.
> 
> So much better.

Better :)
Comment 25 Geoffrey Garen 2017-09-28 14:04:10 PDT
Comment on attachment 321989 [details]
First Solution

r=me
Comment 26 WebKit Commit Bot 2017-09-28 14:31:43 PDT
Comment on attachment 321989 [details]
First Solution

Clearing flags on attachment: 321989

Committed r222633: <http://trac.webkit.org/changeset/222633>