Bug 222760 – Reduce use of CFRetain() / CFRelease() / CFAutoRelease() in WebKit

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Chris Dumez]

Reduce use of CFRetain() / CFRelease() / CFAutoRelease() in WebKit by using RetainPtr<>.

[Chris Dumez]

Created attachment 422280 [details]
Patch

[Chris Dumez]

Created attachment 422283 [details]
Patch

[Chris Dumez]

Created attachment 422287 [details]
Patch

[Chris Dumez]

Created attachment 422290 [details]
Patch

[Chris Dumez]

Created attachment 422292 [details]
Patch

[Chris Dumez]

Created attachment 422301 [details]
Patch

[Chris Dumez]

Created attachment 422303 [details]
Patch

[Chris Dumez]

Created attachment 422310 [details]
Patch

[Darin Adler]

Comment on attachment 422310 [details]
Patch

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

> Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:606
> +    return (AXTextMarkerRangeRef)adoptCF(AXTextMarkerRangeCreate(kCFAllocatorDefault, startMarker, endMarker)).bridgingAutorelease();

I’m pretty sure that CFBridgingRelease was wrong here; instead it would be correct to use CFAutorelease. AXTextMarkerRangeRef is not an Objective-C pointer type and doesn’t work with ARC.

Separately, we should change this internal function to return a RetainPtr<> and not do either kind of autorelease, do them at the call sites instead. But I’m pretty sure we want autorelease(), not bridgingAutorelease().

Same applies for the rest of this file.

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm:435
> +    PathConversionInfo conversion = { self, adoptCF(CGPathCreateMutable()).autorelease() };

The autorelease() should be at the return statement. We can refactor to add a local variable that holds the RetainPtr, and use a get() here, not autorelease().

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:745
> +    return adoptCF(AXTextMarkerCreate(kCFAllocatorDefault, (const UInt8*)&textMarkerData, sizeof(textMarkerData))).bridgingAutorelease();

For this internal function, we should use RetainPtr<id>, not autorelease. The call to autorelease should go up to the callers level. However, that does requires we convert RetainPtr<AXTextMarkerRef> to RetainPtr<id>. Maybe this should return RetainPtr<AXTextMarkerRef> and put bridgingAutorelease() at the call sites that need to convert it to an id.

Same applies to other functions in this file.

> Source/WebCore/loader/cocoa/DiskCacheMonitorCocoa.mm:108
> -        CFRetain(response);
> +        auto strongResponse = retainPtr(response);
>          WebThreadRun(^{
> -            block(response);
> -            CFRelease(response);
> +            block(strongResponse.get());
>          });

A lambda could make this a little more elegant since we could write the strongResponse inside the capture expression. But certainly OK like this.

> Source/WebCore/page/mac/EventHandlerMac.mm:471
> +    RetainPtr<NSScrollView> strongSelf;

Given the name "retaining" here, I think this should be named "retainedSelf".

> Source/WebCore/page/mac/EventHandlerMac.mm:473
>      bool shouldRetainSelf = isMainThread() && nsScrollViewScrollWheelShouldRetainSelf();
> -
>      if (shouldRetainSelf)

Don’t need the bool here any more. Should just put it inside the if statement now.

> Source/WebCore/page/mac/EventHandlerMac.mm:474
> +        strongSelf = retainPtr(self);

Nice to be explicit, I guess, but the call to retainPtr is not needed.

> Source/WebCore/platform/mediarecorder/cocoa/AudioSampleBufferCompressor.mm:348
>                  auto sampleBuffer = adoptCF((CMSampleBufferRef)(const_cast<void*>(CMBufferQueueDequeueAndRetain(m_inputBufferQueue.get()))));

Messy, wonder why this CMSampleBufferRef cast is safe.

> Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:207
> +    auto streamProperties  = adoptCF(CFDictionaryCreateMutable(0, 0, &kCFTypeDictionaryKeyCallBacks, &kCFTypeDictionaryValueCallBacks));

Extra space here before the "=".

> Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegateWithOperationQueue.cpp:140
> +            return cfRequest.leakRef();

I was really puzzled by this, and had to study a lot of code to find out why this is OK.

We should change ResourceHandleCFURLConnectionDelegate::willSendRequest and ResourceHandleCFURLConnectionDelegate::willCacheResponse to return a RetainPtr so the leakRef can be where it belongs, in ResourceHandleCFURLConnectionDelegate::willSendRequestCallback and ResourceHandleCFURLConnectionDelegate::willCacheResponseCallback.

At this point, it’s Apple-Windows-port-only code, but probably still worth the effort.

> Source/WebCore/platform/network/cf/ResourceRequestCFNet.cpp:286
> +        m_httpMethod = method.get();

Can this be WTFMove(method) instead of method.get()?

> Source/WebCore/testing/cocoa/WebViewVisualIdentificationOverlay.mm:127
> +    return adoptCF(CTFontCreateWithName(CFSTR("Helvetica"), 20, &matrix)).autorelease();

I think we should change this to return RetainPtr<CTFontRef> and put get() calls at the two call sites. No need for autorelease.

> Source/WebKit/PluginProcess/mac/PluginProcessMac.mm:314
> +    // We leak the two observers so that they observe notifications for the lifetime of the process.

We could consider sticking these in global variables just to make leak checkers less confused. static NeverDestroyed<RetainPtr<id>>, I suppose. Would waste a few bytes of global storage, but why not? Or maybe leak checkers are not confused?

> Source/WebKitLegacy/mac/WebView/WebView.mm:8406
> +    NSString *nsurlCacheDirectory = adoptCF(_CFURLCacheCopyCacheDirectory([[NSURLCache sharedURLCache] _CFURLCache])).bridgingAutorelease();

Can we use RetainPtr<CFStringRef> for the local variable instead? I know we’d need to add two type casts, but at least no autorelease.

[Chris Dumez]

Comment on attachment 422310 [details]
Patch

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

>> Source/WebCore/platform/network/cf/ResourceRequestCFNet.cpp:286
>> +        m_httpMethod = method.get();
> 
> Can this be WTFMove(method) instead of method.get()?

I don't think so because m_httpMethod is a String, not a RetainPtr<>.

[Chris Dumez]

Comment on attachment 422310 [details]
Patch

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

>> Source/WebKit/PluginProcess/mac/PluginProcessMac.mm:314
>> +    // We leak the two observers so that they observe notifications for the lifetime of the process.
> 
> We could consider sticking these in global variables just to make leak checkers less confused. static NeverDestroyed<RetainPtr<id>>, I suppose. Would waste a few bytes of global storage, but why not? Or maybe leak checkers are not confused?

Actually, looking at https://developer.apple.com/documentation/foundation/nsnotificationcenter/1411723-addobserverforname it says:
"""
Return Value
An opaque object to act as the observer. Notification center strongly holds this return value until you remove the observer registration.
"""

So I don't think we need to do any leaking on our side.

[Chris Dumez]

Created attachment 422328 [details]
Patch

[EWS]

Committed r273966: <https://commits.webkit.org/r273966>

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

[Radar WebKit Bug Importer]

<rdar://problem/75094347>

[Alex Christensen]

This broke the Windows build.
See https://build.webkit.org/#/builders/67/builds/828

[Chris Dumez]

(In reply to Alex Christensen from comment #15)
> This broke the Windows build.
> See https://build.webkit.org/#/builders/67/builds/828

Looking now. thanks.

[Chris Dumez]

(In reply to Chris Dumez from comment #16)
> (In reply to Alex Christensen from comment #15)
> > This broke the Windows build.
> > See https://build.webkit.org/#/builders/67/builds/828
> 
> Looking now. thanks.

Landed a fix in <https://commits.webkit.org/r273969>. I'll monitor this bot since the win-ews somehow did not process the patch overnight.

[Chris Dumez]

(In reply to Chris Dumez from comment #17)
> (In reply to Chris Dumez from comment #16)
> > (In reply to Alex Christensen from comment #15)
> > > This broke the Windows build.
> > > See https://build.webkit.org/#/builders/67/builds/828
> > 
> > Looking now. thanks.
> 
> Landed a fix in <https://commits.webkit.org/r273969>. I'll monitor this bot
> since the win-ews somehow did not process the patch overnight.

Also <https://commits.webkit.org/r273971>