Bug 222237 – Use adoptNS() as soon as we [[ObjcClass alloc] init] to avoid leaks

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Chris Dumez]

Use adoptNS() as soon as we [[ObjcClass alloc] init] to avoid leaks.

[Chris Dumez]

Created attachment 421116 [details]
Patch

[Chris Dumez]

Created attachment 421196 [details]
Patch

[Darin Adler]

Comment on attachment 421196 [details]
Patch

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

> Source/WebKit/UIProcess/mac/WebColorPickerMac.mm:133
>  + (NSPopover *)_colorPopoverCreateIfNecessary:(BOOL)forceCreation

This is a very strange interface for a method. It is really two separate methods. One always creates if passed YES, overwriting the old one, and the other never creates if passed NO, so what does "if necessary" even mean?

> Source/WebKit/WebProcess/InjectedBundle/API/mac/WKDOMInternals.mm:87
> +static RetainPtr<WKDOMNode> initWithImpl(WebCore::Node* impl)

Not a good name for a function that creates an object. I think createWrapper would be a better name.

> Source/WebKit/WebProcess/InjectedBundle/API/mac/WKDOMInternals.mm:134
> +static RetainPtr<WKDOMRange> initWithImpl(WebCore::Range* impl)

Ditto.

> Source/WebKitLegacy/mac/Plugins/WebPluginController.mm:155
> +    auto& views = pluginViews();

Not sure we need a local variable. Could just call pluginViews() twice.

> Source/WebKitLegacy/mac/Plugins/WebPluginController.mm:163
>  #if PLATFORM(IOS_FAMILY)

Do we still need plug-in machinery on iOS? Is it used for video or something?

> Source/WebKitLegacy/mac/Plugins/WebPluginController.mm:166
> +    auto& views = pluginViews();

Ditto.

> Source/WebKitLegacy/mac/Plugins/WebPluginDatabase.mm:66
> +    static NeverDestroyed<RetainPtr<WebPluginDatabase>> _sharedDatabase;

Didn’t comment elsewhere, but are you sure these leading underscore names are available? I seem to recall that C++ reserved those for the standard library or something.

> Source/WebKitLegacy/mac/WebView/WebPreferences.mm:380
> +    // FIXME: This check is necessary to avoid recursion (see <rdar://problem/9564337>), but it also makes standardPreferences() construction not thread safe.
> +    if (auto& preferences = standardPreferences())
> +        return preferences.get();

I really do not understand how this helps with anything given how we use dispatch_once below.

My guess is that it does not prevent recursion.

> Source/WebKitLegacy/mac/WebView/WebView.mm:1352
> +static RetainPtr<NSString> outlookQuirksUserScriptContents()

I’d call this "createXXX".

> Source/WebKitLegacy/mac/WebView/WebView.mm:1374
> +static RetainPtr<NSString> laBanquePostaleQuirksScript()

Ditto.

> Tools/TestRunnerShared/mac/NSPasteboardAdditions.mm:41
>          if (auto legacyType = adoptNS((__bridge NSString *)UTTypeCopyPreferredTagWithClass((__bridge CFStringRef)type, kUTTagClassNSPboardType)))

This is wrong. Must not bridge cast and then do adoptNS. Instead need to do adoptCF.

> Tools/TestWebKitAPI/Tests/mac/WindowlessWebViewWithMedia.mm:67
> +        RetainPtr<WebView> webView = adoptNS([[WebView alloc] initWithFrame:NSMakeRect(0, 0, 120, 200) frameName:nil groupName:nil]);

Can we do auto here?

> Tools/TestWebKitAPI/Tests/mac/WindowlessWebViewWithMedia.mm:68
> +        RetainPtr<WindowlessWebViewWithMediaFrameLoadDelegate> testController = adoptNS([WindowlessWebViewWithMediaFrameLoadDelegate new]);

Can we do auto here?

[Chris Dumez]

Comment on attachment 421196 [details]
Patch

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

>> Source/WebKit/UIProcess/mac/WebColorPickerMac.mm:133
>>  + (NSPopover *)_colorPopoverCreateIfNecessary:(BOOL)forceCreation
> 
> This is a very strange interface for a method. It is really two separate methods. One always creates if passed YES, overwriting the old one, and the other never creates if passed NO, so what does "if necessary" even mean?

I asked about it and this is apparently overriding a method from NSPopoverColorWell. We don't really have flexibility here.

[Chris Dumez]

Comment on attachment 421196 [details]
Patch

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

>> Source/WebKitLegacy/mac/Plugins/WebPluginController.mm:155
>> +    auto& views = pluginViews();
> 
> Not sure we need a local variable. Could just call pluginViews() twice.

I think I like it better with the local. It would get call 3 times:
if (!pluginViews())
    pluginViews() = adoptNS([[NSMutableSet alloc] init]);
[pluginViews() addObject:view];

[Chris Dumez]

Created attachment 421215 [details]
Patch

[EWS]

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

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

[Radar WebKit Bug Importer]

<rdar://problem/74612721>