Bug 231467 - Switch WTF::bridge_cast to use type traits
Summary: Switch WTF::bridge_cast to use type traits
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Kilzer (:ddkilzer)
URL:
Keywords: InRadar
Depends on: 230406 231251
Blocks:
  Show dependency treegraph
 
Reported: 2021-10-08 17:37 PDT by David Kilzer (:ddkilzer)
Modified: 2021-10-12 14:57 PDT (History)
15 users (show)

See Also:


Attachments
Patch v1 (8.53 KB, patch)
2021-10-08 17:40 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch v2 (13.44 KB, patch)
2021-10-09 10:19 PDT, David Kilzer (:ddkilzer)
darin: review+
Details | Formatted Diff | Diff
Patch for landing (12.25 KB, patch)
2021-10-11 19:14 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch for landing v2 (12.25 KB, patch)
2021-10-12 12:36 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Kilzer (:ddkilzer) 2021-10-08 17:37:40 PDT
Switch WTF::bridge_cast to use type traits.

This replaces some macro-generated code with template code.

Not sure the new code is more readable, but error messages should be much less verbose.

Defining the type traits in WebKit will also let us remove some duplicate code in Safari.

This is based on some code written in November 2011 by Anders Carlsson.
Comment 1 Radar WebKit Bug Importer 2021-10-08 17:38:20 PDT
<rdar://problem/84050614>
Comment 2 David Kilzer (:ddkilzer) 2021-10-08 17:40:47 PDT
Created attachment 440693 [details]
Patch v1
Comment 3 David Kilzer (:ddkilzer) 2021-10-08 17:48:36 PDT
Darin, I tried my best to use this to fix the return type of RetainPtr<>::bridgingAutorelease(), but I couldn't figure out how to make this work:

 #ifdef __OBJC__
+    using BridgingPtrType = typename std::conditional_t<!std::is_convertible_v<T, id> && IsTypeComplete<CFTollFreeBridgingTraits<T>>, typename CFTollFreeBridgingTraits<T>::BridgedType, id>;
     using HelperPtrType = typename std::conditional_t<std::is_convertible_v<T, id> && !std::is_same_v<T, id>, std::remove_pointer_t<T>, T>;
 #else
+    using BridgingPtrType = id;
     using HelperPtrType = PtrType;
 #endif

The challenge is that even if `IsTypeComplete<CFTollFreeBridgingTraits<T>` is false, `typename CFTollFreeBridgingTraits<T>::BridgedType` must still evaluate to something (even if it's not used).  Or at least that's the conclusion I came to when I tried it.
Comment 4 Darin Adler 2021-10-08 18:09:41 PDT
I haven’t lost confidence that we can do it yet.

We do not want bridgingAutorelease to do less type checking when the type is not complete, so I think that part of the solution isn’t heading in the right direction.

What we want instead is that calls to bridgingAutorelease fail to compile successfully, maybe because of SFINAE, when the type is not complete. So this means we may not be able to use a using at the class level; the type expression may need to be entirely within the bridgingAutorelease function definition.
Comment 5 Darin Adler 2021-10-08 18:09:41 PDT
I haven’t lost confidence that we can do it yet.

We do not want bridgingAutorelease to do less type checking when the type is not complete, so I think that part of the solution isn’t heading in the right direction.

What we want instead is that calls to bridgingAutorelease fail to compile successfully, maybe because of SFINAE, when the type is not complete. So this means we may not be able to use a using at the class level; the type expression may need to be entirely within the bridgingAutorelease function definition.
Comment 6 David Kilzer (:ddkilzer) 2021-10-08 20:53:08 PDT
(In reply to Darin Adler from comment #5)
> I haven’t lost confidence that we can do it yet.
> 
> We do not want bridgingAutorelease to do less type checking when the type is
> not complete, so I think that part of the solution isn’t heading in the
> right direction.

Okay.

> What we want instead is that calls to bridgingAutorelease fail to compile
> successfully, maybe because of SFINAE, when the type is not complete. So
> this means we may not be able to use a using at the class level; the type
> expression may need to be entirely within the bridgingAutorelease function
> definition.

I don't think SFINAE will work because we can't make an instance method of RetainPtr<> optional (that I'm aware of), but maybe there's an SFINAE pattern I'm not thinking of.  We'd have to move bridgingAutorelease() out of the RetainPtr<> class (to a stand-alone function) to use SFINAE.

This does work:

template<typename T> inline id RetainPtr<T>::bridgingAutorelease()
{
    static_assert(!std::is_convertible_v<PtrType, id> && IsTypeComplete<CFTollFreeBridgingTraits<PtrType>>, "bridgingAutorelease() can only be used for toll-free bridged CF types.");
    return CFBridgingRelease(leakRef());
}

And it already caught an "error":

WebKitBuild/Debug/usr/local/include/wtf/RetainPtr.h:223:5: error: static_assert failed due to requirement 'IsTypeComplete<WTF::CFTollFreeBridgingTraits<const __AXTextMarker *>>' "bridgingAutorelease() can only be used for toll-free bridged CF types."
    static_assert(!std::is_convertible_v<PtrType, id> && IsTypeComplete<CFTollFreeBridgingTraits<PtrType>>, "bridgingAutorelease() can only be used for toll-free bridged CF types.");
    ^                                                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from WebKitBuild/Debug/DerivedSources/WebCore/unified-sources/UnifiedSource5-mm.mm:5:
./accessibility/mac/AXObjectCacheMac.mm:494:38: note: in instantiation of member function 'WTF::RetainPtr<const __AXTextMarker *>::bridgingAutorelease' requested here
        [change setObject:textMarker.bridgingAutorelease() forKey:NSAccessibilityTextChangeValueStartMarker];
                                     ^
1 error generated.

Is this the type of issue we want to catch?  I thought it was generally okay to autorelease CF objects.

Looks like this should probably just be a .get() with a bridge_id_cast() to prevent the autorelease in the first place:

 static void addTextMarkerFor(NSMutableDictionary* change, AXCoreObject& object, HTMLTextFormControlElement& textControl)
 {
     if (auto textMarker = [object.wrapper() textMarkerForFirstPositionInTextControl:textControl])
-        [change setObject:textMarker.bridgingAutorelease() forKey:NSAccessibilityTextChangeValueStartMarker];
+        [change setObject:bridge_id_cast(textMarker.get()) forKey:NSAccessibilityTextChangeValueStartMarker];
 }
Comment 7 David Kilzer (:ddkilzer) 2021-10-09 10:19:50 PDT
Created attachment 440712 [details]
Patch v2
Comment 8 David Kilzer (:ddkilzer) 2021-10-11 08:50:11 PDT
Review ping.
Comment 9 Darin Adler 2021-10-11 15:00:24 PDT
(In reply to David Kilzer (:ddkilzer) from comment #6)
> Is this the type of issue we want to catch?  I thought it was generally okay
> to autorelease CF objects.

Once bridgingAutorelease chooses the type correctly, it might need to be replaced by two different functions, for the same reason we added both bridge_cast and bridge_id_cast; the former can be used when the class is toll-free bridged with a specific Objective-C class, while the latter can be used for any CF object that we choose to use with Objective-C, even if there is no Objective-C counterpart.

Thinking about this, I want us to think about which cast function one should use in the other direction, to convert the "id" back to a CoreFoundation type. For object ownership we would use adoptNS, but then how would we convert the RetainPtr<id> into RetainPtr<AXTextMarkerRef>.

It seems this particular case calls for the bridge_id_cast style.

> Looks like this should probably just be a .get() with a bridge_id_cast() to
> prevent the autorelease in the first place

Yes, this code does not need autorelease; it needs bridging.

> -        [change setObject:textMarker.bridgingAutorelease()
> forKey:NSAccessibilityTextChangeValueStartMarker];
> +        [change setObject:bridge_id_cast(textMarker.get())
> forKey:NSAccessibilityTextChangeValueStartMarker];

I’m noticing that this will be slightly suboptimal under ARC, requiring an extra retain/release pair. Kind of annoying that RetainPtr messes up ARC optimizations.
Comment 10 Darin Adler 2021-10-11 16:50:50 PDT
Comment on attachment 440712 [details]
Patch v2

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

> Source/WTF/wtf/RetainPtr.h:224
> +    static_assert(!std::is_convertible_v<PtrType, id> && IsTypeComplete<CFTollFreeBridgingTraits<PtrType>>, "bridgingAutorelease() may only be used for toll-free bridged CF types.");

The idea behind my saying "base the return type on the type that is bridged" was not to generate compile time errors when we wanted to bridge to just "id". So I’m not sure it’s worth making this change yet.

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:4134
> -            return [protectedSelf nextTextMarkerForCharacterOffset:characterOffset].bridgingAutorelease();
> +            return bridge_id_cast([protectedSelf nextTextMarkerForCharacterOffset:characterOffset]);

Is this really right? We need autorelease here. Does bridge_id_cast implicitly do an autorelease?

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:4149
> -            return [protectedSelf previousTextMarkerForCharacterOffset:characterOffset].bridgingAutorelease();
> +            return bridge_id_cast([protectedSelf previousTextMarkerForCharacterOffset:characterOffset]);

Same.
Comment 11 David Kilzer (:ddkilzer) 2021-10-11 17:46:51 PDT
Comment on attachment 440712 [details]
Patch v2

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

>> Source/WTF/wtf/RetainPtr.h:224
>> +    static_assert(!std::is_convertible_v<PtrType, id> && IsTypeComplete<CFTollFreeBridgingTraits<PtrType>>, "bridgingAutorelease() may only be used for toll-free bridged CF types.");
> 
> The idea behind my saying "base the return type on the type that is bridged" was not to generate compile time errors when we wanted to bridge to just "id". So I’m not sure it’s worth making this change yet.

Okay, I'll revert this.  We don't have to solve fixing the return type of bridgingAutorelease() before landing the patch, correct?

>> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:4134
>> +            return bridge_id_cast([protectedSelf nextTextMarkerForCharacterOffset:characterOffset]);
> 
> Is this really right? We need autorelease here. Does bridge_id_cast implicitly do an autorelease?

I'm pretty sure this lambda returns a RetainPtr<id>, so this is way more efficient than autoreleasing it and then re-retaining it, correct?

        return Accessibility::retrieveAutoreleasedValueFromMainThread<id>([&textMarker, protectedSelf = retainPtr(self)] () -> RetainPtr<id> {

Maybe I amI missing something?  Is there some unexpected requirement that this be autoreleased before returning it?

>> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:4149
>> +            return bridge_id_cast([protectedSelf previousTextMarkerForCharacterOffset:characterOffset]);
> 
> Same.

Same:

        return Accessibility::retrieveAutoreleasedValueFromMainThread<id>([&textMarker, protectedSelf = retainPtr(self)] () -> RetainPtr<id> {
Comment 12 Darin Adler 2021-10-11 17:48:45 PDT
Comment on attachment 440712 [details]
Patch v2

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

>>> Source/WTF/wtf/RetainPtr.h:224
>>> +    static_assert(!std::is_convertible_v<PtrType, id> && IsTypeComplete<CFTollFreeBridgingTraits<PtrType>>, "bridgingAutorelease() may only be used for toll-free bridged CF types.");
>> 
>> The idea behind my saying "base the return type on the type that is bridged" was not to generate compile time errors when we wanted to bridge to just "id". So I’m not sure it’s worth making this change yet.
> 
> Okay, I'll revert this.  We don't have to solve fixing the return type of bridgingAutorelease() before landing the patch, correct?

That’s right. A separate thought that we can pursue in its own time.

>>> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:4134
>>> +            return bridge_id_cast([protectedSelf nextTextMarkerForCharacterOffset:characterOffset]);
>> 
>> Is this really right? We need autorelease here. Does bridge_id_cast implicitly do an autorelease?
> 
> I'm pretty sure this lambda returns a RetainPtr<id>, so this is way more efficient than autoreleasing it and then re-retaining it, correct?
> 
>         return Accessibility::retrieveAutoreleasedValueFromMainThread<id>([&textMarker, protectedSelf = retainPtr(self)] () -> RetainPtr<id> {
> 
> Maybe I amI missing something?  Is there some unexpected requirement that this be autoreleased before returning it?

I did not see retrieveAutoreleasedValueFromMainThread, clearly that’s what does the autorelease. Thanks, my mistake.
Comment 13 David Kilzer (:ddkilzer) 2021-10-11 19:14:19 PDT
Created attachment 440870 [details]
Patch for landing
Comment 14 Darin Adler 2021-10-12 08:36:29 PDT
Comment on attachment 440870 [details]
Patch for landing

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

> Source/WTF/wtf/cocoa/TollFreeBridging.h:42
> +template<> struct CFTollFreeBridgingTraits<CFClassName##Ref> { typedef NSClassName *BridgedType; }; \
> +template<> struct NSTollFreeBridgingTraits<NSClassName> { typedef CFClassName##Ref BridgedType; };

Realized that to be more modern we could use "using" instead of typedef here.

> Source/WTF/wtf/cocoa/TollFreeBridging.h:60
> +template<> struct CFTollFreeBridgingTraits<CFBooleanRef> { typedef NSNumber *BridgedType; };

Ditto.
Comment 15 David Kilzer (:ddkilzer) 2021-10-12 12:36:27 PDT
Created attachment 440970 [details]
Patch for landing v2
Comment 16 David Kilzer (:ddkilzer) 2021-10-12 13:52:30 PDT
Comment on attachment 440970 [details]
Patch for landing v2

Marking cq+ since the only changes from "Patch for landing" were to change `typedef` to `using`, which would have appeared as a build failure if they were an issue.
Comment 17 EWS 2021-10-12 14:56:59 PDT
Committed r284038 (242868@main): <https://commits.webkit.org/242868@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 440970 [details].