Use adoptNS() right away after calling [obj copy] / [obj mutableCopy] to minimize the chance of leaks.
Created attachment 422020 [details] Patch
Comment on attachment 422020 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=422020&action=review > Source/WebCore/platform/cocoa/SystemVersion.mm:40 > - return [[systemVersionInfo objectForKey:@"ProductVersion"] copy]; > + return [systemVersionInfo objectForKey:@"ProductVersion"]; What’s the rationale for moving from copy to retain here? I’m not saying it’s wrong, but we are removing the copy. That means if it happens to be a mutable dictionary we will keep a pointer to it rather than making an immutable copy. I could imagine making the copy in an effort to avoid things being mutated under us or perhaps to optimize memory use since the immutable copy could be smaller. > Source/WebKit/Shared/ApplePay/ApplePayPaymentSetupFeatures.mm:74 > + allowedClasses.get() = WTFMove(allowed); What’s the rationale for moving from copy to retain here? > Source/WebKit/Shared/ApplePay/PaymentSetupConfiguration.mm:93 > + allowedClasses.get() = WTFMove(allowed); What’s the rationale for moving from copy to retain here? > Source/WebKit/Shared/ApplePay/cocoa/PaymentSetupConfiguration.mm:70 > + allowedClasses.get() = WTFMove(allowed); What’s the rationale for moving from copy to retain here? > Source/WebKit/UIProcess/Launcher/mac/ProcessLauncherMac.mm:110 > - }() copy]; > + }()); What’s the rationale for moving from copy to retain here?
(In reply to Darin Adler from comment #2) > Comment on attachment 422020 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=422020&action=review > > > Source/WebCore/platform/cocoa/SystemVersion.mm:40 > > - return [[systemVersionInfo objectForKey:@"ProductVersion"] copy]; > > + return [systemVersionInfo objectForKey:@"ProductVersion"]; > > What’s the rationale for moving from copy to retain here? > > I’m not saying it’s wrong, but we are removing the copy. That means if it > happens to be a mutable dictionary we will keep a pointer to it rather than > making an immutable copy. I could imagine making the copy in an effort to > avoid things being mutated under us or perhaps to optimize memory use since > the immutable copy could be smaller. I did not realize the benefits of calling [obj copy] here. I will re-introduce those then. > > > Source/WebKit/Shared/ApplePay/ApplePayPaymentSetupFeatures.mm:74 > > + allowedClasses.get() = WTFMove(allowed); > > What’s the rationale for moving from copy to retain here? > > > Source/WebKit/Shared/ApplePay/PaymentSetupConfiguration.mm:93 > > + allowedClasses.get() = WTFMove(allowed); > > What’s the rationale for moving from copy to retain here? > > > Source/WebKit/Shared/ApplePay/cocoa/PaymentSetupConfiguration.mm:70 > > + allowedClasses.get() = WTFMove(allowed); > > What’s the rationale for moving from copy to retain here? > > > Source/WebKit/UIProcess/Launcher/mac/ProcessLauncherMac.mm:110 > > - }() copy]; > > + }()); > > What’s the rationale for moving from copy to retain here?
Comment on attachment 422020 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=422020&action=review >>> Source/WebCore/platform/cocoa/SystemVersion.mm:40 >>> + return [systemVersionInfo objectForKey:@"ProductVersion"]; >> >> What’s the rationale for moving from copy to retain here? >> >> I’m not saying it’s wrong, but we are removing the copy. That means if it happens to be a mutable dictionary we will keep a pointer to it rather than making an immutable copy. I could imagine making the copy in an effort to avoid things being mutated under us or perhaps to optimize memory use since the immutable copy could be smaller. > > I did not realize the benefits of calling [obj copy] here. I will re-introduce those then. These are *possible* benefits. Also possible that someone had a choice of typing "retain" or "copy" and selected copy without thinking deeply about tradeoffs.
Created attachment 422142 [details] Patch
Committed r273848: <https://commits.webkit.org/r273848> All reviewed patches have been landed. Closing bug and clearing flags on attachment 422142 [details].
<rdar://problem/75005651>