[webkit-reviews] review granted: [Bug 222760] Reduce use of CFRetain() / CFRelease() / CFAutoRelease() in WebKit : [Attachment 422310] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Mar 4 18:44:03 PST 2021

Darin Adler <darin at apple.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 222760: Reduce use of CFRetain() / CFRelease() / CFAutoRelease() in WebKit

Attachment 422310: Patch


--- Comment #9 from Darin Adler <darin at apple.com> ---
Comment on attachment 422310
  --> https://bugs.webkit.org/attachment.cgi?id=422310

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

> Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:606
> +    return
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

> 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() &&
> -
>      if (shouldRetainSelf)

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

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

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

>		   auto sampleBuffer =

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 "=".

> +	       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

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

> 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,

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

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

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.

More information about the webkit-reviews mailing list