Bug 222237 - Use adoptNS() as soon as we [[ObjcClass alloc] init] to avoid leaks
Summary: Use adoptNS() as soon as we [[ObjcClass alloc] init] to avoid leaks
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-02-20 13:54 PST by Chris Dumez
Modified: 2021-02-22 14:45 PST (History)
19 users (show)

See Also:


Attachments
Patch (137.19 KB, patch)
2021-02-20 15:02 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (137.72 KB, patch)
2021-02-22 08:29 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (135.82 KB, patch)
2021-02-22 11:30 PST, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2021-02-20 13:54:57 PST
Use adoptNS() as soon as we [[ObjcClass alloc] init] to avoid leaks.
Comment 1 Chris Dumez 2021-02-20 15:02:58 PST
Created attachment 421116 [details]
Patch
Comment 2 Chris Dumez 2021-02-22 08:29:50 PST
Created attachment 421196 [details]
Patch
Comment 3 Darin Adler 2021-02-22 09:55:51 PST
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?
Comment 4 Chris Dumez 2021-02-22 11:06:22 PST
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.
Comment 5 Chris Dumez 2021-02-22 11:08:27 PST
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];
Comment 6 Chris Dumez 2021-02-22 11:30:36 PST
Created attachment 421215 [details]
Patch
Comment 7 EWS 2021-02-22 13:59:07 PST
Committed r273276: <https://commits.webkit.org/r273276>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 421215 [details].
Comment 8 Radar WebKit Bug Importer 2021-02-22 14:01:43 PST
<rdar://problem/74612721>