Bug 222634 – Use adoptNS() right away after calling [obj copy] / [obj mutableCopy]

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Chris Dumez]

Use adoptNS() right away after calling [obj copy] / [obj mutableCopy] to minimize the chance of leaks.

[Chris Dumez]

Created attachment 422020 [details]
Patch

[Darin Adler]

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?

[Chris Dumez]

(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?

[Darin Adler]

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.

[Chris Dumez]

Created attachment 422142 [details]
Patch

[EWS]

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

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

[Radar WebKit Bug Importer]

<rdar://problem/75005651>