Bug 222401 – WKRemoteObjectCoder should be able to handle NSErrors from TLS failures

• Component: New Bugs
• Status: RESOLVED FIXED

[Alex Christensen]

WKRemoteObjectCoder should be able to handle NSErrors from TLS failures

[Alex Christensen]

Created attachment 421496 [details]
Patch

[Alex Christensen]

<rdar://problem/72103865>

[Chris Dumez]

Comment on attachment 421496 [details]
Patch

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

> Source/WebKit/Shared/API/Cocoa/WKRemoteObjectCoder.mm:317
> +    return [result autorelease];

I spent a lot of time getting rid of all the explicit [objc autorelease] in our internal code. Please use RetainPtr<>.

> Source/WebKit/Shared/API/Cocoa/WKRemoteObjectCoder.mm:343
> +    return (NSData *)data.autorelease();

I cc'd Darin because I seem to remember that this sort of things (casting from CF to NS and autoreleasing) makes ARC transition complicated.

Overall, we should be using RetainPtr<>.

[Darin Adler]

Comment on attachment 421496 [details]
Patch

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

I suggest using a lot more RetainPtr.

> Source/WebKit/Shared/API/Cocoa/WKRemoteObjectCoder.mm:303
> +static id decodeObjCObject(WKRemoteObjectDecoder *decoder, Class objectClass)

This should return a RetainPtr<id>.

> Source/WebKit/Shared/API/Cocoa/WKRemoteObjectCoder.mm:322
> +NSString *peerCertificateKey = @"NSErrorPeerCertificateChainKey";
> +NSString *peerTrustKey = @"NSURLErrorFailingURLPeerTrustErrorKey";
> +NSString *clientCertificateKey = @"NSErrorClientCertificateChainKey";

Should these have static constexpr in front of them?

> Source/WebKit/Shared/API/Cocoa/WKRemoteObjectCoder.mm:324
> +static NSArray *transformCertificatesToData(NSArray *input)

This should return a RetainPtr<NSArray>.

> Source/WebKit/Shared/API/Cocoa/WKRemoteObjectCoder.mm:335
> +static NSData *transformTrustToData(SecTrustRef trust)

This should return a RetainPtr<CFDataRef>.

>> Source/WebKit/Shared/API/Cocoa/WKRemoteObjectCoder.mm:343
>> +    return (NSData *)data.autorelease();
> 
> I cc'd Darin because I seem to remember that this sort of things (casting from CF to NS and autoreleasing) makes ARC transition complicated.
> 
> Overall, we should be using RetainPtr<>.

For our own functions we should use RetainPtr for return values, not autoreleased objects. However, I don’t know exactly how to change a RetainPtr<CFDataRef> into a RetainPtr<NSData>. I suggest we sidestep that issue by having this function return RetainPtr<CFDataRef>.

> Source/WebKit/Shared/API/Cocoa/WKRemoteObjectCoder.mm:348
> +    NSMutableDictionary *copy = nil;

This should be a RetainPtr.

> Source/WebKit/Shared/API/Cocoa/WKRemoteObjectCoder.mm:374
> +static NSArray *transformDataToCertificates(NSArray *input)

This should return a RetainPtr<NSArray>.

> Source/WebKit/Shared/API/Cocoa/WKRemoteObjectCoder.mm:388
> +static id transformDataToTrust(NSData *data)

This should probably return a RetainPtr<CFTypeRef>.

> Source/WebKit/Shared/API/Cocoa/WKRemoteObjectCoder.mm:399
> +static NSError *decodeError(WKRemoteObjectDecoder *decoder)

This should return a RetainPtr<NSError>.

[Alex Christensen]

Created attachment 422041 [details]
Patch

[Alex Christensen]

Created attachment 422042 [details]
Patch

[Alex Christensen]

(In reply to Darin Adler from comment #4)
> > Source/WebKit/Shared/API/Cocoa/WKRemoteObjectCoder.mm:322
> > +NSString *peerCertificateKey = @"NSErrorPeerCertificateChainKey";
> > +NSString *peerTrustKey = @"NSURLErrorFailingURLPeerTrustErrorKey";
> > +NSString *clientCertificateKey = @"NSErrorClientCertificateChainKey";
> 
> Should these have static constexpr in front of them?

This blew my mind a little bit.  Static I get, but I definitely need to do more thinking about exactly what a constexpr NSString* is.

[Darin Adler]

(In reply to Alex Christensen from comment #7)
> This blew my mind a little bit.  Static I get, but I definitely need to do
> more thinking about exactly what a constexpr NSString* is.

Roughly speaking, it's an NSString * with a value known at compile time and that never changes.

[Chris Dumez]

Comment on attachment 422042 [details]
Patch

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

r=me with a couple of changes.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/RemoteObjectRegistry.mm:237
> +    auto webView = [[[WKWebView alloc] initWithFrame:CGRectZero configuration:[WKWebViewConfiguration _test_configurationWithTestPlugInClassName:@"RemoteObjectRegistryPlugIn"]] autorelease];

no autorelease] please. Just use adoptNS().

> Tools/TestWebKitAPI/Tests/WebKitCocoa/RemoteObjectRegistry.mm:238
> +    auto delegate = [[TestNavigationDelegate new] autorelease];

ditto.

[Alex Christensen]

Created attachment 422183 [details]
Patch

[EWS]

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

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