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

[bugzilla-daemon at webkit.org]

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
https://bugs.webkit.org/show_bug.cgi?id=222760

Attachment 422310: Patch

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




--- Comment #9 from Darin Adler <darin at apple.com> ---
Comment on attachment 422310
  --> https://bugs.webkit.org/attachment.cgi?id=422310
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_i
nputBufferQueue.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/ResourceHandleCFURLConnectionDelegateWithOpe
rationQueue.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.