Bug 125241

Summary: Add RetainPtr::autorelease to combine leakPtr() + autorelease
Product: WebKit Reporter: Brian Burg <burg>
Component: PlatformAssignee: Pratik Solanki <psolanki>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, benjamin, cmarcelo, commit-queue, darin, psolanki
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: Unspecified   
Attachments:
Description Flags
Patch
darin: review+
For the mountain lion bots none

Description Brian Burg 2013-12-04 12:21:15 PST
Client code should not have to worry about calling leakPtr() manually.
Comment 1 Pratik Solanki 2014-03-02 12:43:44 PST
Created attachment 225606 [details]
Patch
Comment 2 Pratik Solanki 2014-03-02 14:39:47 PST
/Volumes/Data/EWS/WebKit/WebKitBuild/Release/usr/local/include/wtf/RetainPtr.h:195:39: error: use of undeclared identifier 'CFAutorelease'
        PtrType ptr = fromStorageType(CFAutorelease(m_ptr));
                                      ^
/Volumes/Data/EWS/WebKit/Source/WebCore/bindings/objc/DOM.mm:558:50: note: in instantiation of member function 'WTF::RetainPtr<NSImage>::autorelease' requested here
    return createDragImageForNode(*frame, *node).autorelease();
                                                 ^
1 error generated.

No CFAutorelease on Mountain Lion. :(
Comment 3 Darin Adler 2014-03-02 15:23:46 PST
Comment on attachment 225606 [details]
Patch

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

Looks like CFAutorelease is new to Mavericks. We’ll have to do something different to support pre-Mavericks versions of Mac. And maybe something different for Windows too.

r=me, but we need to get this compiling

> Source/WTF/wtf/RetainPtr.h:198
> +    template<typename T> inline typename RetainPtr<T>::PtrType RetainPtr<T>::autorelease()
> +    {
> +        if (!m_ptr)
> +            return 0;
> +        PtrType ptr = fromStorageType(CFAutorelease(m_ptr));
> +        m_ptr = 0;
> +        return ptr;
> +    }

The 0 here should be nullptr.

I am not sure that fromStorageType will always do the right thing with the return value of CFAutorelease. I suggest putting the CFAutorelease on a separate line and ignoring the return value.
Comment 4 Darin Adler 2014-03-02 15:36:20 PST
(In reply to comment #2)
> No CFAutorelease on Mountain Lion. :(

For MountainLion, we can put a function in some .mm file in WTF that casts the pointer to (id) and then calls -[NSObject autorelease] on it.
Comment 5 Pratik Solanki 2014-03-03 11:00:44 PST
Created attachment 225671 [details]
For the mountain lion bots

This adds autorelease implementation for Mountain Lion to AutodrainedMac.mm. Lets see how this one fares.
Comment 6 Darin Adler 2014-03-04 09:05:45 PST
Comment on attachment 225671 [details]
For the mountain lion bots

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

> Source/WTF/wtf/AutodrainedPoolMac.mm:57
> +#if !PLATFORM(IOS) && __MAC_OS_X_VERSION_MIN_REQUIRED < 1090
> +template<typename T> inline typename RetainPtr<T>::PtrType RetainPtr<T>::autorelease()
> +{
> +    if (!m_ptr)
> +        return nullptr;
> +    [(id)m_ptr autorelease];
> +    PtrType ptr = fromStorageType(m_ptr);
> +    m_ptr = nullptr;
> +    return ptr;
> +}
> +#endif

A function template has to be in a header, not an implementation file. I was talking about putting a non-inline function that calls autorelease in the .mm file, which the function template would call.
Comment 7 Anders Carlsson 2016-08-04 14:18:20 PDT
We already did this some time ago.