Bug 125241 - Add RetainPtr::autorelease to combine leakPtr() + autorelease
Summary: Add RetainPtr::autorelease to combine leakPtr() + autorelease
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac Unspecified
: P2 Normal
Assignee: Pratik Solanki
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-12-04 12:21 PST by Brian Burg
Modified: 2016-08-04 14:18 PDT (History)
6 users (show)

See Also:


Attachments
Patch (12.81 KB, patch)
2014-03-02 12:43 PST, Pratik Solanki
darin: review+
Details | Formatted Diff | Diff
For the mountain lion bots (12.71 KB, patch)
2014-03-03 11:00 PST, Pratik Solanki
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.