WKRemoteObjectCoder should be able to handle NSErrors from TLS failures
Created attachment 421496 [details] Patch
<rdar://problem/72103865>
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<>.
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>.
Created attachment 422041 [details] Patch
Created attachment 422042 [details] Patch
(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.
(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.
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.
Created attachment 422183 [details] Patch
Committed r273875: <https://commits.webkit.org/r273875> All reviewed patches have been landed. Closing bug and clearing flags on attachment 422183 [details].