Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1230033005:
WKWebView: Added cert verification API to web controller. (Closed)
|
Created:
5 years, 5 months ago by Eugene But (OOO till 7-30) Modified:
5 years, 3 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionWKWebView: Added cert verification API to web controller.
This code is just a skeleton for verification and verification method
is not used for making security decisions or presenting security UI.
The decision to use CertVerifier instead of iOS cert verification API
has not been made yet. But using CertVerifier is easier for
-[WKWebView certificateChain] verification, so this CL uses
CertVerifier.
BUG=462427, 462425
Committed: https://crrev.com/dc5f11025e1db3b7766bf5faeacc316969b519b2
Cr-Commit-Position: refs/heads/master@{#347302}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Merged with origin/master #Patch Set 3 : #
Total comments: 19
Patch Set 4 : Addressed review comments #
Total comments: 34
Patch Set 5 : Addressed review comments (round 2) #Patch Set 6 : Minor cleanup. #
Total comments: 29
Patch Set 7 : Addressed review comments (round 3) #
Total comments: 28
Patch Set 8 : Addressed review comments (round 4) #
Total comments: 2
Patch Set 9 : Updated CertVerifierBlockAdapter comments. #Patch Set 10 : Removed gtest usage #
Total comments: 53
Patch Set 11 : Addressed David's review comments #Patch Set 12 : Put CertVerifierBlockAdapter to web namespace. #
Total comments: 8
Patch Set 13 : Resolved Stuart's codereview comments #
Total comments: 4
Patch Set 14 : Fixed threading bugs #
Total comments: 8
Patch Set 15 : Updated comments, fixed threading issue #Patch Set 16 : Reverted view_controller change #
Total comments: 6
Patch Set 17 : Updated comments. #Patch Set 18 : Release Web Controller on the main thread. #
Total comments: 6
Patch Set 19 : Added DCHECK(_certVerifier); #Patch Set 20 : Moved all threading code from Web Controller to a separate class. #Patch Set 21 : Updated comments #
Total comments: 10
Patch Set 22 : s/UNKNOWN/ERROR. #Patch Set 23 : Threads are hard. #
Total comments: 26
Patch Set 24 : Resolved review comments #
Total comments: 6
Patch Set 25 : Adressed review NITs #Patch Set 26 : Updated enum names #
Total comments: 4
Patch Set 27 : Relanding CL after landing dependency; #Patch Set 28 : Updated comment (s/used/user); #Messages
Total messages: 107 (18 generated)
eugenebut@chromium.org changed reviewers: + palmer@chromium.org, rsesek@chromium.org
Chris, Robert, could you please take a look if cert verification is correct from security perspective. Would be great if you could also check CertVerifierBlockAdapter. This change does not affect production app and change only WKWebView-app behavior, which is not shipped to the users. This is a doc for Security UI proposal: https://docs.google.com/document/d/1PniWABIUZYllJZU-D--JXDzpi2CQdIsV1yjerXRxW... https://codereview.chromium.org/1230033005/diff/1/ios/web/web_state/ui/crw_wk... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1230033005/diff/1/ios/web/web_state/ui/crw_wk... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:1144: [self verifyCert:cert At the moment CertVerifier DCHECKs on null cert or empty hostname. This will be gracefully handled after the following CL landed: https://codereview.chromium.org/1231783003/
https://codereview.chromium.org/1230033005/diff/1/ios/web/web_state/ui/crw_wk... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1230033005/diff/1/ios/web/web_state/ui/crw_wk... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:1144: [self verifyCert:cert On 2015/07/10 19:42:02, eugenebut wrote: > At the moment CertVerifier DCHECKs on null cert or empty hostname. This will be > gracefully handled after the following CL landed: > https://codereview.chromium.org/1231783003/ Please ignore this comment. cl/1231783003 has landed.
I don't see any issues here, but I'm not an expert in //net usage. rsleevi may want to take a look, though.
eugenebut@chromium.org changed reviewers: + rsleevi@chromium.org
eugenebut@chromium.org changed reviewers: + stuartmorgan@chromium.org
Structure LGTM once someone familiar with the APIs is happy with it.
Chris, Ryan, could you please take a look.
palmer@chromium.org changed reviewers: + davidben@chromium.org
I think you're better off with rsleevi or davidben looking at it than me.
When this is all hooked up, it will want some tests to make sure it behaves the way we expect it to. Probably: - MockCertVerifier that accepts a particular fake cert. Make sure the CertVerifier got called and such. - MockCertVerifier that rejects a particular fake cert. Make sure the CertVerifier got called *and the request did not get sent*. That last part is very important. https://codereview.chromium.org/1230033005/diff/40001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1230033005/diff/40001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:241: completionHandler:(void (^)(scoped_ptr<net::CertVerifyResult>, int))block; [Shouldn't these be indented such that the colons line up? I dunno, I don't understand Obj-C style, so I'll defer to you. :-) ] https://codereview.chromium.org/1230033005/diff/40001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:1193: NSURLCredential *credential))completionHandler { Isn't this supposed to check the protectionSpace.authenticationMethod and make sure it's a NSURLAuthenticationMethodServerTrust? https://codereview.chromium.org/1230033005/diff/40001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:1204: // Cert is invalid. The docs point to this sample code: https://developer.apple.com/library/prerelease/ios/documentation/NetworkingIn... Which isn't quite the same API, but it does have sample code to get the NSURLCredential out. https://codereview.chromium.org/1230033005/diff/40001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:1207: completionHandler(NSURLSessionAuthChallengeRejectProtectionSpace, nil); Is this supposed to be NSURLSessionAuthChallengeRejectProtectionSpace or NSURLSessionAuthChallengePerformDefaultHandling?
Thanks for review! https://codereview.chromium.org/1230033005/diff/40001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1230033005/diff/40001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:241: completionHandler:(void (^)(scoped_ptr<net::CertVerifyResult>, int))block; On 2015/07/31 18:58:46, David Benjamin wrote: > [Shouldn't these be indented such that the colons line up? I dunno, I don't > understand Obj-C style, so I'll defer to you. :-) ] This indentation is correct. When the first keyword is shorter than the others, indent the later lines by at least four spaces, maintaining colon alignment: http://google.github.io/styleguide/objcguide.xml?showone=Method_Declarations_... https://codereview.chromium.org/1230033005/diff/40001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:1193: NSURLCredential *credential))completionHandler { On 2015/07/31 18:58:46, David Benjamin wrote: > Isn't this supposed to check the protectionSpace.authenticationMethod and make > sure it's a NSURLAuthenticationMethodServerTrust? challenge.protectionSpace.serverTrust returns nil if authenticationMethod is not NSURLAuthenticationMethodServerTrust. CreateCertFromTrust returns null for null SecTrustRef and CertVerifierAdapter calls completion handler with error if cert is null. So everything should be fine. https://codereview.chromium.org/1230033005/diff/40001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:1204: // Cert is invalid. On 2015/07/31 18:58:46, David Benjamin wrote: > The docs point to this sample code: > > https://developer.apple.com/library/prerelease/ios/documentation/NetworkingIn... > > Which isn't quite the same API, but it does have sample code to get the > NSURLCredential out. Thanks for the link. Accepting bad SSL cert is not implemented in this CL yet. With current implementation SSL icon will be red, only if webView:didFailProvisionalNavigation:withError: was called. So before implementing recoverable SSL interstitial I want to fix SSL icon coloring first, and for that I need to have cert verification method. This design doc describes the big picture of cert verification: https://docs.google.com/document/d/1o8wzJkxzG1fvhvW9Gqf-RdGjhm0btCIehDtbxDfbL... https://codereview.chromium.org/1230033005/diff/40001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:1207: completionHandler(NSURLSessionAuthChallengeRejectProtectionSpace, nil); On 2015/07/31 18:58:46, David Benjamin wrote: > Is this supposed to be NSURLSessionAuthChallengeRejectProtectionSpace or > NSURLSessionAuthChallengePerformDefaultHandling? NSURLSessionAuthChallengeRejectProtectionSpace is the current code, which accepts good certs and cancels request for bad certs. For bad certs didFailProvisionalNavigation: is called after request is cancelled. I will investigate the difference between NSURLSessionAuthChallengePerformDefaultHandling and NSURLSessionAuthChallengeRejectProtectionSpace before implementing recoverable interstitials. But I agree that NSURLSessionAuthChallengePerformDefaultHandling will be more appropriate.
https://codereview.chromium.org/1230033005/diff/40001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1230033005/diff/40001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:855: params.ocsp_response = ""; // Not provided by iOS API. = "" is unnscessary (you should have a default ctor), but even if it was, you should use either = std::string() or .clear() https://codereview.chromium.org/1230033005/diff/40001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:856: params.flags = net::CertVerifier::VERIFY_CERT_IO_ENABLED; This doesn't seem correct. See https://code.google.com/p/chromium/codesearch#chromium/src/net/ssl/ssl_config... Although you can't use the socket APIs, you still should be able to get an SSLConfig from the higher layers (e.g. the SSLConfigService) to influence behaviour here, right? https://codereview.chromium.org/1230033005/diff/40001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:1201: if (result && !net::IsCertStatusError(result->cert_status)) { This doesn't seem right either - normally you'd also want to check net::IsCertStatusMinorError as well
https://codereview.chromium.org/1230033005/diff/40001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1230033005/diff/40001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:1193: NSURLCredential *credential))completionHandler { On 2015/08/01 00:25:40, eugenebut wrote: > On 2015/07/31 18:58:46, David Benjamin wrote: > > Isn't this supposed to check the protectionSpace.authenticationMethod and make > > sure it's a NSURLAuthenticationMethodServerTrust? > challenge.protectionSpace.serverTrust returns nil if authenticationMethod is not > NSURLAuthenticationMethodServerTrust. > CreateCertFromTrust returns null for null SecTrustRef This is unreasonable to rely on. > and CertVerifierAdapter > calls completion handler with error if cert is null. As far as I can tell, it does not. The NULL gets passed all the way into the CertVerifier which does NOT check this and shouldn't. > So everything should be fine. This needs an explicit check. This method is a common sink for multiple kinds of authentication methods, so it's important to check which type you're dealing with.
eugenebut@chromium.org changed reviewers: - palmer@chromium.org, rsesek@chromium.org
PTAL. Stuart, this CL has changed significantly, so PTAL as well. I guess once all the pieces are landed I will move all the code which is related to cert verification and user decision for interstitials to a separate class. https://codereview.chromium.org/1230033005/diff/40001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1230033005/diff/40001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:855: params.ocsp_response = ""; // Not provided by iOS API. On 2015/08/01 01:36:22, Ryan Sleevi wrote: > = "" is unnscessary (you should have a default ctor), but even if it was, you > should use either = std::string() or .clear() I just want to be explicit that ocsp_response is empty. And the code which assigns empty string communicates that very well. Using std::string(), clear() or "" is a style preference. Both C++ and Chromium Style guide does now disallow assigning char* to std::string. F.e. https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Variable_and... Having said that I would prefer to leave the code as it is, both suggested and current approaches are simply a personal preference. https://codereview.chromium.org/1230033005/diff/40001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:856: params.flags = net::CertVerifier::VERIFY_CERT_IO_ENABLED; On 2015/08/01 01:36:22, Ryan Sleevi wrote: > This doesn't seem correct. > > See > https://code.google.com/p/chromium/codesearch#chromium/src/net/ssl/ssl_config... > > Although you can't use the socket APIs, you still should be able to get an > SSLConfig from the higher layers (e.g. the SSLConfigService) to influence > behaviour here, right? Done, thanks for the link. https://codereview.chromium.org/1230033005/diff/40001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:1193: NSURLCredential *credential))completionHandler { On 2015/08/03 18:06:32, David Benjamin wrote: > On 2015/08/01 00:25:40, eugenebut wrote: > > On 2015/07/31 18:58:46, David Benjamin wrote: > > > Isn't this supposed to check the protectionSpace.authenticationMethod and > make > > > sure it's a NSURLAuthenticationMethodServerTrust? > > challenge.protectionSpace.serverTrust returns nil if authenticationMethod is > not > > NSURLAuthenticationMethodServerTrust. > > > CreateCertFromTrust returns null for null SecTrustRef > > This is unreasonable to rely on. > > > and CertVerifierAdapter > > calls completion handler with error if cert is null. > > As far as I can tell, it does not. The NULL gets passed all the way into the > CertVerifier which does NOT check this and shouldn't. > > > So everything should be fine. > > This needs an explicit check. This method is a common sink for multiple kinds of > authentication methods, so it's important to check which type you're dealing > with. I guess auth method, other than NSURLAuthenticationMethodServerTrust can be a special case. Done. https://codereview.chromium.org/1230033005/diff/40001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:1201: if (result && !net::IsCertStatusError(result->cert_status)) { On 2015/08/01 01:36:22, Ryan Sleevi wrote: > This doesn't seem right either - normally you'd also want to check > net::IsCertStatusMinorError as well Done. https://codereview.chromium.org/1230033005/diff/40001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:1207: completionHandler(NSURLSessionAuthChallengeRejectProtectionSpace, nil); On 2015/08/01 00:25:40, eugenebut wrote: > On 2015/07/31 18:58:46, David Benjamin wrote: > > Is this supposed to be NSURLSessionAuthChallengeRejectProtectionSpace or > > NSURLSessionAuthChallengePerformDefaultHandling? > NSURLSessionAuthChallengeRejectProtectionSpace is the current code, which > accepts good certs and cancels request for bad certs. > > For bad certs didFailProvisionalNavigation: is called after request is > cancelled. I will investigate the difference between > NSURLSessionAuthChallengePerformDefaultHandling and > NSURLSessionAuthChallengeRejectProtectionSpace before implementing recoverable > interstitials. > > But I agree that NSURLSessionAuthChallengePerformDefaultHandling will be more > appropriate. > Changed to NSURLSessionAuthChallengePerformDefaultHandling.
The CQ bit was checked by eugenebut@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1230033005/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1230033005/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1230033005/diff/40001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1230033005/diff/40001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:855: params.ocsp_response = ""; // Not provided by iOS API. On 2015/08/05 16:13:43, eugenebut wrote: > Having said that I would prefer to leave the code as it is, both suggested and > current approaches are simply a personal preference. We developed a clang tool to excise this pattern from the codebase. It's certainly less efficient than either of the alternatives I suggested. While I can appreciate personal preference, efficiency seems a reasonable argument over unnecessary code. If you do feel strongly, it seems like simply documenting it is empty is better to calling attention than adding code. https://codereview.chromium.org/1230033005/diff/60001/ios/web/net/cert_verifi... File ios/web/net/cert_verifier_block_adapter.cc (left): https://codereview.chromium.org/1230033005/diff/60001/ios/web/net/cert_verifi... ios/web/net/cert_verifier_block_adapter.cc:48: scoped_ptr<CertVerifier>(CertVerifier::CreateDefault())) { Thanks for removing this; I would have nacked this constructor if I'd seen it earlier. Separately, it may help to make sure iOS code isn't calling this. https://codereview.chromium.org/1230033005/diff/60001/ios/web/net/cert_verifi... File ios/web/net/cert_verifier_block_adapter.cc (right): https://codereview.chromium.org/1230033005/diff/60001/ios/web/net/cert_verifi... ios/web/net/cert_verifier_block_adapter.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. Doesn't the use of blocks in this file intrinsically make it objective-c++, and thus should follow .mm naming schemes? https://codereview.chromium.org/1230033005/diff/60001/ios/web/net/cert_verifi... ios/web/net/cert_verifier_block_adapter.cc:19: struct VerificationContext : public base::RefCounted<VerificationContext> { With blocks, isn't there the possibility of arbitrary thread dispatch? Does this need to be RefCountedThreadSafe? https://codereview.chromium.org/1230033005/diff/60001/ios/web/net/cert_verifi... ios/web/net/cert_verifier_block_adapter.cc:20: VerificationContext(scoped_refptr<net::X509Certificate> cert) : cert(cert) { Explicit https://codereview.chromium.org/1230033005/diff/60001/ios/web/net/cert_verifi... ios/web/net/cert_verifier_block_adapter.cc:21: result.cert_status = CERT_STATUS_INVALID; Why is this? This feels a very ad-hoc way of creating a CertVerifyResult, and runs counter to the default ctor of a CertVerifyResult as used in all the other code. https://codereview.chromium.org/1230033005/diff/60001/ios/web/net/cert_verifi... ios/web/net/cert_verifier_block_adapter.cc:61: if (!params.cert || !params.hostname.size()) { !params.hostname.empty() for empty checks with STL containers (including string). While for std::string it's equivalent, following STL container rules, .empty() will be consistently faster across all storage container types, while .size() may be O(N) [e.g. in a std::deque/std::list scenario] https://codereview.chromium.org/1230033005/diff/60001/ios/web/net/cert_verifi... ios/web/net/cert_verifier_block_adapter.cc:62: completion_handler(CertVerifyResult(), ERR_INVALID_ARGUMENT); While David explained how CertVerifier doesn't check for this, it doesn't seem right that this (as an Adapter for a CertVerifier) should introduce the check. It feels like it creates inconsistent expectations for developers about the necessary preconditions - a CertVerifierBlockAdapter no longer adheres to the same API contract as a CertVerifier. Which is to say these checks should happen at the caller site when processing external input, rather than be deferred deep down the stack. https://codereview.chromium.org/1230033005/diff/60001/ios/web/net/cert_verifi... ios/web/net/cert_verifier_block_adapter.cc:69: completion_handler(context->result, OK); BUG? It seems that you're assuming that any return of the callback will indicate success, but that's explicitly not the case. You should be using the result code from the callback to pass on to your block continuation. CompletionCallback callback = base::BindBlock(^(int result) { completion_handler(context->result, result); }); https://codereview.chromium.org/1230033005/diff/60001/ios/web/net/cert_verifi... ios/web/net/cert_verifier_block_adapter.cc:71: int status = cert_verifier_->Verify(params.cert.get(), params.hostname, This is typically called |result| in //net, rather than status, because it's a net::Error code https://codereview.chromium.org/1230033005/diff/60001/ios/web/net/cert_verifi... ios/web/net/cert_verifier_block_adapter.cc:83: completion_handler(context->result, status); BUG? Nothing in here seems like it actually keeps |context| alive (that is, takes a reference). Certainly, lines 71-74 don't, nor would lines 69/83, AFAICT, so how does this not cause use-after-frees? Does the Block_copy of BindBlock actually bind |context| as well, thus presumably creating a reference (although the interaction of Obj-C blocks and C++ ctor semantics seem weird) or does it just bind to |context->result|? If it binds to |context|, then you've seemingly made this impossible to cancel, which further doesn't seem right. That's because |callback| will keep |context| alive (line 68/69), and |context| will keep the overall verification alive through context->request (line 74), and so you end up uncancelable. https://codereview.chromium.org/1230033005/diff/60001/ios/web/net/cert_verifi... File ios/web/net/cert_verifier_block_adapter.h (right): https://codereview.chromium.org/1230033005/diff/60001/ios/web/net/cert_verifi... ios/web/net/cert_verifier_block_adapter.h:23: CertVerifierBlockAdapter(CertVerifier* cert_verifier); BUG/STYLE: This should be explicit. https://codereview.chromium.org/1230033005/diff/60001/ios/web/net/cert_verifi... ios/web/net/cert_verifier_block_adapter.h:50: CertVerifier::VerifyFlags flags; It's a bitwise or. It shouldn't be stored as an enum, but as an int. https://codereview.chromium.org/1230033005/diff/60001/ios/web/net/cert_verifi... ios/web/net/cert_verifier_block_adapter.h:59: typedef void (^CompletionHandler)(CertVerifyResult, int status); Do blocks not allow you to forward declare such structures? https://codereview.chromium.org/1230033005/diff/60001/ios/web/net/cert_verifi... ios/web/net/cert_verifier_block_adapter.h:67: // Underlying weak CertVerifier. Weak has a particular meaning in Chromium (c.f. WeakPtr); this does not appear to be a common term in Chromium code, although I'm guessing you're using the iOS terminology. https://codereview.chromium.org/1230033005/diff/60001/ios/web/net/cert_verifi... ios/web/net/cert_verifier_block_adapter.h:69: // Net Log required by CertVerifier. This comment doesn't seem to help documentation much :/ Why are you creating your own NetLog? Why is the adapter-caller not supplying it as part of the Params / Verify call? https://codereview.chromium.org/1230033005/diff/60001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1230033005/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:65: auto contextGetter = webState->GetBrowserState()->GetRequestContext(); This is not a valid (Chromium) use of auto See the discussion on automatic types linked from https://chromium-cpp.appspot.com/ , particularly https://groups.google.com/a/chromium.org/d/msg/chromium-dev/5-Bt3BJzAo0/EWcU2... This entirely hides the ownership semantics, and while some argued that's a feature, the //base and style owners seemed to agree it was a bug, and thus you shouldn't use auto like this. It's also not consistent with the admonition of "use manifest type declarations when it helps readability" of http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#auto https://codereview.chromium.org/1230033005/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:551: if (!_certVerifier) { Why do you lazily create this?
Apologies, forgot to include the "Message" With the full caveat that I don't "grok blocks", the Adapter seems like a lot of weird anti-patterns compared to a lot of the Chromium code I'm used to seeing. It also makes it hard to apply the existing knowledge of CertVerifier and map these onto the iOS concepts, which I think may be damaging towards longer-term maintenance. Hopefully none of this is taken too personally. A lot of this is just being frank with my own ignorance and confusion, so that it can be understood how it looks to an outsider who isn't used to this code, but is trying to understand it.
I understand that Adapter is very different from the rest of Chromium code. Same statement is true for WebController, mostly because it is a facade around iOS API. The goal of the adapter is to provide interface which is convenient for Web Controller (Adapter is not intended to be used anywhere else): 1.) Single point of return (vs. return value + base::Callback in CertVerifier) 2.) Blocks-based API, which is very common pattern in Web Controller 3.) Ability to easily perform multiple simultaneous verification 4.) Hiding lifetime management of CertVerifyResult/CertVerifier::Request People who familiar with SecTrust iOS API should not have any problems with using Adapter. For, example SecTrustEvaluateAsync takes SecTrustRef (which encapsulates verification params, like cert and hostname) and SecTrustCallback (which is a Block). So knowledge of SecTrustEvaluateAsync can be applied to execute CertVerifierBlockAdapter::Verify, which takes Params (cert, hostname, etc...) and Block callback. If we decide to go with iOS for certs verification then transition should be very easy. If you have any concrete concerns about CertVerifierBlockAdapter I will do my best to address them at the same time keeping Adapter convenient for Web Controller. Please note that both Web Controller and Adapter are not public interfaces of ios/web component. https://codereview.chromium.org/1230033005/diff/40001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1230033005/diff/40001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:855: params.ocsp_response = ""; // Not provided by iOS API. On 2015/08/06 03:07:08, Ryan Sleevi wrote: > On 2015/08/05 16:13:43, eugenebut wrote: > > Having said that I would prefer to leave the code as it is, both suggested and > > current approaches are simply a personal preference. > > We developed a clang tool to excise this pattern from the codebase. It's > certainly less efficient than either of the alternatives I suggested. While I > can appreciate personal preference, efficiency seems a reasonable argument over > unnecessary code. If you do feel strongly, it seems like simply documenting it > is empty is better to calling attention than adding code. Thanks, I did not know about clang changes. Replaced with a comment. https://codereview.chromium.org/1230033005/diff/60001/ios/web/net/cert_verifi... File ios/web/net/cert_verifier_block_adapter.cc (right): https://codereview.chromium.org/1230033005/diff/60001/ios/web/net/cert_verifi... ios/web/net/cert_verifier_block_adapter.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2015/08/06 03:07:08, Ryan Sleevi wrote: > Doesn't the use of blocks in this file intrinsically make it objective-c++, and > thus should follow .mm naming schemes? Clang and GCC implement blocks for C/C++ as a language extension. So there is no need to make this file mm (unless we want to compile it with other compilers). https://codereview.chromium.org/1230033005/diff/60001/ios/web/net/cert_verifi... ios/web/net/cert_verifier_block_adapter.cc:19: struct VerificationContext : public base::RefCounted<VerificationContext> { On 2015/08/06 03:07:08, Ryan Sleevi wrote: > With blocks, isn't there the possibility of arbitrary thread dispatch? Does this > need to be RefCountedThreadSafe? Good catch. Done. https://codereview.chromium.org/1230033005/diff/60001/ios/web/net/cert_verifi... ios/web/net/cert_verifier_block_adapter.cc:20: VerificationContext(scoped_refptr<net::X509Certificate> cert) : cert(cert) { On 2015/08/06 03:07:08, Ryan Sleevi wrote: > Explicit Done. https://codereview.chromium.org/1230033005/diff/60001/ios/web/net/cert_verifi... ios/web/net/cert_verifier_block_adapter.cc:21: result.cert_status = CERT_STATUS_INVALID; On 2015/08/06 03:07:08, Ryan Sleevi wrote: > Why is this? > > This feels a very ad-hoc way of creating a CertVerifyResult, and runs counter to > the default ctor of a CertVerifyResult as used in all the other code. Removed. https://codereview.chromium.org/1230033005/diff/60001/ios/web/net/cert_verifi... ios/web/net/cert_verifier_block_adapter.cc:61: if (!params.cert || !params.hostname.size()) { On 2015/08/06 03:07:08, Ryan Sleevi wrote: > !params.hostname.empty() for empty checks with STL containers (including > string). While for std::string it's equivalent, following STL container rules, > .empty() will be consistently faster across all storage container types, while > .size() may be O(N) [e.g. in a std::deque/std::list scenario] Done. https://codereview.chromium.org/1230033005/diff/60001/ios/web/net/cert_verifi... ios/web/net/cert_verifier_block_adapter.cc:62: completion_handler(CertVerifyResult(), ERR_INVALID_ARGUMENT); On 2015/08/06 03:07:08, Ryan Sleevi wrote: > While David explained how CertVerifier doesn't check for this, Actually CertVerifier DOES check for this: https://code.google.com/p/chromium/codesearch#chromium/src/net/cert/multi_thr... It is not documented in the header so I added this check to adapter as well. > it doesn't seem > right that this (as an Adapter for a CertVerifier) should introduce the check. > It feels like it creates inconsistent expectations for developers about the > necessary preconditions - a CertVerifierBlockAdapter no longer adheres to the > same API contract as a CertVerifier. CertVerifierBlockAdapter has very different API comparing to CertVerifier. F.e. CertVerifierBlockAdapter::Verify does not return value, so I think that different preconditions (though in practice they are not actually different) is totally fine. Moreover this class has Adapter in its name which suggests the difference in the interface. > Which is to say these checks should happen at the caller site when processing > external input, rather than be deferred deep down the stack. Forcing clients to check for input arguments will lead to duplication of error handling code. The whole point of CertVerifierBlockAdapter is to hide complexity from WebController and provide simple API with a single completion block, which has the same failure point. I assume that there is no difference in handling error cases if iOS did not provide a hostname vs. network error. Please let me know if that assumption is not correct. https://codereview.chromium.org/1230033005/diff/60001/ios/web/net/cert_verifi... ios/web/net/cert_verifier_block_adapter.cc:69: completion_handler(context->result, OK); On 2015/08/06 03:07:08, Ryan Sleevi wrote: > BUG? It seems that you're assuming that any return of the callback will indicate > success, but that's explicitly not the case. > > You should be using the result code from the callback to pass on to your block > continuation. > > CompletionCallback callback = base::BindBlock(^(int result) { > completion_handler(context->result, result); > }); Done. https://codereview.chromium.org/1230033005/diff/60001/ios/web/net/cert_verifi... ios/web/net/cert_verifier_block_adapter.cc:71: int status = cert_verifier_->Verify(params.cert.get(), params.hostname, On 2015/08/06 03:07:08, Ryan Sleevi wrote: > This is typically called |result| in //net, rather than status, because it's a > net::Error code I named this |status| to differentiate from |certVerifyResult|: typedef void (^CompletionHandler)(CertVerifyResult result, int status); I can use the following naming: typedef void (^CompletionHandler)(CertVerifyResult certVerifyResult, int statusResult); But it will look weird. Do you think it is ok to leave |status| name to avoid confusion with |certVerifyResult|? https://codereview.chromium.org/1230033005/diff/60001/ios/web/net/cert_verifi... ios/web/net/cert_verifier_block_adapter.cc:83: completion_handler(context->result, status); On 2015/08/06 03:07:08, Ryan Sleevi wrote: > BUG? Nothing in here seems like it actually keeps |context| alive (that is, > takes a reference). Certainly, lines 71-74 don't, nor would lines 69/83, AFAICT, > so how does this not cause use-after-frees? > > Does the Block_copy of BindBlock actually bind |context| as well, thus > presumably creating a reference (although the interaction of Obj-C blocks and > C++ ctor semantics seem weird) or does it just bind to |context->result|? > > If it binds to |context|, then you've seemingly made this impossible to cancel, > which further doesn't seem right. That's because |callback| will keep |context| > alive (line 68/69), and |context| will keep the overall verification alive > through context->request (line 74), and so you end up uncancelable. |context| is kept alive by the block (blocks retain Objective-C objects and copy C++ objects), block itself is kept alive by CompletionCallback. So yes, requests were uncancelable. I added a pool of requests, so they will be cancelled once CertVerifierAdapter is destroyed. https://codereview.chromium.org/1230033005/diff/60001/ios/web/net/cert_verifi... File ios/web/net/cert_verifier_block_adapter.h (right): https://codereview.chromium.org/1230033005/diff/60001/ios/web/net/cert_verifi... ios/web/net/cert_verifier_block_adapter.h:23: CertVerifierBlockAdapter(CertVerifier* cert_verifier); On 2015/08/06 03:07:08, Ryan Sleevi wrote: > BUG/STYLE: This should be explicit. Acknowledged. This constructor now accepts 2 arguments. https://codereview.chromium.org/1230033005/diff/60001/ios/web/net/cert_verifi... ios/web/net/cert_verifier_block_adapter.h:50: CertVerifier::VerifyFlags flags; On 2015/08/06 03:07:09, Ryan Sleevi wrote: > It's a bitwise or. It shouldn't be stored as an enum, but as an int. Done. https://codereview.chromium.org/1230033005/diff/60001/ios/web/net/cert_verifi... ios/web/net/cert_verifier_block_adapter.h:59: typedef void (^CompletionHandler)(CertVerifyResult, int status); On 2015/08/06 03:07:09, Ryan Sleevi wrote: > Do blocks not allow you to forward declare such structures? No you can't forward declare this block without including cert_verify_result.h, because block takes CertVerifyResult by value. https://codereview.chromium.org/1230033005/diff/60001/ios/web/net/cert_verifi... ios/web/net/cert_verifier_block_adapter.h:67: // Underlying weak CertVerifier. On 2015/08/06 03:07:08, Ryan Sleevi wrote: > Weak has a particular meaning in Chromium (c.f. WeakPtr); this does not appear > to be a common term in Chromium code, although I'm guessing you're using the iOS > terminology. Good catch. Changed to "unowned", because this is not a weak pointer even by iOS terminology. https://codereview.chromium.org/1230033005/diff/60001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1230033005/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:65: auto contextGetter = webState->GetBrowserState()->GetRequestContext(); On 2015/08/06 03:07:09, Ryan Sleevi wrote: > This is not a valid (Chromium) use of auto > > See the discussion on automatic types linked from > https://chromium-cpp.appspot.com/ , particularly > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/5-Bt3BJzAo0/EWcU2... > > This entirely hides the ownership semantics, and while some argued that's a > feature, the //base and style owners seemed to agree it was a bug, and thus you > shouldn't use auto like this. > > It's also not consistent with the admonition of "use manifest type declarations > when it helps readability" of > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#auto I used auto to avoid a linebreak. But I'm not very attached to this code. Done. https://codereview.chromium.org/1230033005/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:551: if (!_certVerifier) { On 2015/08/06 03:07:09, Ryan Sleevi wrote: > Why do you lazily create this? I can't create this in -init, because GetURLRequestContext can be called on IO thread. What I can do is to create a separate net::CertVerifierBlockAdapter object for every cert evaluation, but I don't see why it would be better. https://codereview.chromium.org/1230033005/diff/100001/ios/web/net/cert_verif... File ios/web/net/cert_verifier_block_adapter.cc (right): https://codereview.chromium.org/1230033005/diff/100001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter.cc:41: net_log_(BoundNetLog::Make(net_log, NetLog::SOURCE_URL_REQUEST)) { Not sure if SOURCE_URL_REQUEST is ok here, or I should create a separate SOURCE_CERT_VERIFICATION
The CQ bit was checked by eugenebut@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1230033005/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1230033005/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I defer to Stuart here for the style, but my point of view is that if you're going to call the file .cc, then it should file the C++ style guide with respect to variable naming and conventions. This seems consistent with the Objective-C style guide, http://google.github.io/styleguide/objcguide.xml?showone=File_Names#File_Names https://codereview.chromium.org/1230033005/diff/60001/ios/web/net/cert_verifi... File ios/web/net/cert_verifier_block_adapter.cc (right): https://codereview.chromium.org/1230033005/diff/60001/ios/web/net/cert_verifi... ios/web/net/cert_verifier_block_adapter.cc:71: int status = cert_verifier_->Verify(params.cert.get(), params.hostname, On 2015/08/07 02:27:20, eugenebut wrote: > But it will look weird. Do you think it is ok to leave |status| name to avoid > confusion with |certVerifyResult|? Well, you're not naming things statusResult/certVerifyResult - at least, not by my read of line 59. So you shouldn't have any naming confusion. In the C++ code, we tend to call this "verify_result", so CertVerifyResult verifyResult would seem to be acceptable naming for the Objective C++ side. https://codereview.chromium.org/1230033005/diff/100001/ios/web/net/cert_verif... File ios/web/net/cert_verifier_block_adapter.cc (right): https://codereview.chromium.org/1230033005/diff/100001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter.cc:41: net_log_(BoundNetLog::Make(net_log, NetLog::SOURCE_URL_REQUEST)) { On 2015/08/07 02:27:20, eugenebut wrote: > Not sure if SOURCE_URL_REQUEST is ok here, or I should create a separate > SOURCE_CERT_VERIFICATION I don't understand why you're creating it, versus taking it from the context of the caller. Is this because it's not compatible with the Block adapter? Then we should fix that, because you really want to use the BoundNetLog that is the callers, since that will contextually be associated with the right URLRequest flow. Of course, for WKWebView, not using //net, we have to decide how we want NetLog to behave. I don't know what's been done by the iOS team in this respect, but having a NetLog with any information you can get from the WKWebView seems _extremely_ valuable for diagnostics and debugging in the field. When using Chrome's network stack, the NetLog comes from the Socket ( https://code.google.com/p/chromium/codesearch#chromium/src/net/socket/ssl_cli... / https://code.google.com/p/chromium/codesearch#chromium/src/net/socket/ssl_cli... ), which is setup here - https://code.google.com/p/chromium/codesearch#chromium/src/net/socket/tcp_soc... Although I'll readily admit, I'm not sure the full implications of how this should look for iOS, so I'm gonna tag in mmenke@ for this. https://codereview.chromium.org/1230033005/diff/100001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter.cc:55: void CertVerifierBlockAdapter::Verify( To confirm: This method will always and only be called on the IO thread, correct? https://codereview.chromium.org/1230033005/diff/100001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter.cc:67: completion_handler(context->result, status); BUG? In //net, callbacks are always the terminal state for a return; this allows callbacks to delete the relevant objects, if appropriate, for example. The implications of this is that invoking a callback should always be the final state of the function. I believe you want to delete the pending request _prior_ to invoking the completion_handler. Note: It's perfectly OK to delete context->request in the CompletionCallback of Verify(), as CertVerifier::Verify() adheres to this contract (that is, that CompletionCallback is the last thing called and all state is cleaned up) https://codereview.chromium.org/1230033005/diff/100001/ios/web/net/cert_verif... File ios/web/net/cert_verifier_block_adapter.h (right): https://codereview.chromium.org/1230033005/diff/100001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter.h:65: // asynchronously. Does this API requirement match SecTrust's? If the goal is to have these APIs be easily convertable between the two, then it seems you should try to go for the same guarantees. For example, if SecTrustEvaluateAsync is always going to return asynchronously, perhaps your Adapter should handle converting the synchronous results from the CertVerifier into asynchronous dispatches for consistency. https://codereview.chromium.org/1230033005/diff/100001/ios/web/net/cert_verif... File ios/web/net/cert_verifier_block_adapter_unittest.cc (right): https://codereview.chromium.org/1230033005/diff/100001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter_unittest.cc:30: MOCK_METHOD9(Verify, There's a strong anti-GMock position from Chrome leads (darin@, brettw@, jam@) https://groups.google.com/a/chromium.org/d/msg/chromium-dev/-KH_IP0rIWQ/HynAL... We also already have a MockCertVerifier ( https://code.google.com/p/chromium/codesearch#chromium/src/net/cert/mock_cert... ) that is the preferred method, although I suppose it's more a Fake than a Mock. https://codereview.chromium.org/1230033005/diff/100001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter_unittest.cc:52: }); This doesn't seem to be emulating CertVerifier behaviour, but your Adapter's behaviour. CertVerifier would use base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE, base::Bind(arg6, status)); https://codereview.chromium.org/1230033005/diff/100001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter_unittest.cc:61: : cert_(new X509Certificate("test", "test", base::Time(), base::Time())), You should not create certificates like this for tests. This is a valid-only-for-certain-minimal-cases ctor that isn't compatible (because it creates an invalid OSCertHandle). You should use ImportCertFromFile to create a real X509Certificate. https://codereview.chromium.org/1230033005/diff/100001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter_unittest.cc:98: Verify(cert_.get(), kHostName, "", 0, nullptr, _, _, _, _)) These "" should be std::string() as well. https://codereview.chromium.org/1230033005/diff/100001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter_unittest.cc:156: CertVerifierBlockAdapter::Params params(cert_.get(), ""); std::string() https://codereview.chromium.org/1230033005/diff/100001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1230033005/diff/100001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:924: net::CertVerifierBlockAdapter::Params params(cert, hostname); Shouldn't this be cert.Pass()? You should either pass scoped_refptr<> by const-ref (line 910), or you should use .Pass(), to avoid unnecessarily refcount churns. https://codereview.chromium.org/1230033005/diff/100001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:931: dispatch_async(dispatch_get_main_queue(), ^{ Why is this double dispatch needed? Why can't you just pass completionHandler?
Thanks. PTAL. https://codereview.chromium.org/1230033005/diff/60001/ios/web/net/cert_verifi... File ios/web/net/cert_verifier_block_adapter.cc (right): https://codereview.chromium.org/1230033005/diff/60001/ios/web/net/cert_verifi... ios/web/net/cert_verifier_block_adapter.cc:71: int status = cert_verifier_->Verify(params.cert.get(), params.hostname, On 2015/08/07 21:52:11, Ryan Sleevi wrote: > On 2015/08/07 02:27:20, eugenebut wrote: > > But it will look weird. Do you think it is ok to leave |status| name to avoid > > confusion with |certVerifyResult|? > > Well, you're not naming things statusResult/certVerifyResult - at least, not by > my read of line 59. So you shouldn't have any naming confusion. > > In the C++ code, we tend to call this "verify_result", so CertVerifyResult > verifyResult would seem to be acceptable naming for the Objective C++ side. Sorry for confusion, this file is .cc and a subject to C++ Style Guide in terms of naming. I did no rename variables in the previous round, because wanted to ask if you ok with olds names (result/status), but I guess you are not. Done. https://codereview.chromium.org/1230033005/diff/100001/ios/web/net/cert_verif... File ios/web/net/cert_verifier_block_adapter.cc (right): https://codereview.chromium.org/1230033005/diff/100001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter.cc:41: net_log_(BoundNetLog::Make(net_log, NetLog::SOURCE_URL_REQUEST)) { > I don't understand why you're creating it, versus taking it from the context of > the caller. > > Is this because it's not compatible with the Block adapter? Caller (Web Controller) does not have BoundNetLog, but it has NetLog, so I passed |NetLog| here. There is no value in sharing BoundNetLog between multiple WebViews, because MultiThreaderCertVerifier will recreate BoundNet log anyway: https://code.google.com/p/chromium/codesearch#chromium/src/net/cert/multi_thr... Per discussion with mmenke@ I will create one BoundNetLog per CertVerifier::Verify invocation, rather than share them. BoundNetLog just groups related events together, and we don't want events from a bunch of different verify invocations all to be lumped together without being sorted. https://codereview.chromium.org/1230033005/diff/100001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter.cc:55: void CertVerifierBlockAdapter::Verify( On 2015/08/07 21:52:12, Ryan Sleevi wrote: > To confirm: This method will always and only be called on the IO thread, > correct? Technically it *can* be called on any thread, as long as if its the same thread where CertVerifier was created. In unit tests it is called on main thread, but in web controller it will *always* be called on IO thread. https://codereview.chromium.org/1230033005/diff/100001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter.cc:67: completion_handler(context->result, status); On 2015/08/07 21:52:12, Ryan Sleevi wrote: > BUG? > > In //net, callbacks are always the terminal state for a return; this allows > callbacks to delete the relevant objects, if appropriate, for example. > > The implications of this is that invoking a callback should always be the final > state of the function. I believe you want to delete the pending request _prior_ > to invoking the completion_handler. > > Note: It's perfectly OK to delete context->request in the CompletionCallback of > Verify(), as CertVerifier::Verify() adheres to this contract (that is, that > CompletionCallback is the last thing called and all state is cleaned up) Done. https://codereview.chromium.org/1230033005/diff/100001/ios/web/net/cert_verif... File ios/web/net/cert_verifier_block_adapter.h (right): https://codereview.chromium.org/1230033005/diff/100001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter.h:65: // asynchronously. On 2015/08/07 21:52:12, Ryan Sleevi wrote: > Does this API requirement match SecTrust's? > > If the goal is to have these APIs be easily convertable between the two, then it > seems you should try to go for the same guarantees. For example, if > SecTrustEvaluateAsync is always going to return asynchronously, perhaps your > Adapter should handle converting the synchronous results from the CertVerifier > into asynchronous dispatches for consistency. SecTrustEvaluateAsync always calls block asynchronously on GCD queue (similar to thread concept) provided by the caller. Verify always calls block on the same thread where it was called, however it calls it synchronously in case if result is retrieved from caches. In that sense Adapter behavior is different from SecTrustEvaluateAsync. However I believe it's acceptable tradeoff, because calling Adapter::Verify completion handler asynchronously on the same thread would add additional performance overhead and will make code a little bit more complex. WDYT? https://codereview.chromium.org/1230033005/diff/100001/ios/web/net/cert_verif... File ios/web/net/cert_verifier_block_adapter_unittest.cc (right): https://codereview.chromium.org/1230033005/diff/100001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter_unittest.cc:30: MOCK_METHOD9(Verify, On 2015/08/07 21:52:12, Ryan Sleevi wrote: > There's a strong anti-GMock position from Chrome leads (darin@, brettw@, jam@) > > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/-KH_IP0rIWQ/HynAL... > > We also already have a MockCertVerifier ( > https://code.google.com/p/chromium/codesearch#chromium/src/net/cert/mock_cert... > ) that is the preferred method, although I suppose it's more a Fake than a Mock. MockCertVerifier does not allow to test OCSP and CRL inputs, as well as asynchronous verification. Switching this test to use MockCertVerifier will lead to loosing significant test coverage. Having said that I believe that gMock is better option comparing to MockCertVerifier, even if in the future we'll need to replace gMock with something different. https://codereview.chromium.org/1230033005/diff/100001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter_unittest.cc:52: }); On 2015/08/07 21:52:12, Ryan Sleevi wrote: > This doesn't seem to be emulating CertVerifier behaviour, but your Adapter's > behaviour. > > CertVerifier would use > > base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE, base::Bind(arg6, > status)); Done. https://codereview.chromium.org/1230033005/diff/100001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter_unittest.cc:61: : cert_(new X509Certificate("test", "test", base::Time(), base::Time())), On 2015/08/07 21:52:12, Ryan Sleevi wrote: > You should not create certificates like this for tests. > > This is a valid-only-for-certain-minimal-cases ctor that isn't compatible > (because it creates an invalid OSCertHandle). You should use ImportCertFromFile > to create a real X509Certificate. Done. https://codereview.chromium.org/1230033005/diff/100001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter_unittest.cc:98: Verify(cert_.get(), kHostName, "", 0, nullptr, _, _, _, _)) On 2015/08/07 21:52:12, Ryan Sleevi wrote: > These "" should be std::string() as well. Done. https://codereview.chromium.org/1230033005/diff/100001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter_unittest.cc:156: CertVerifierBlockAdapter::Params params(cert_.get(), ""); On 2015/08/07 21:52:12, Ryan Sleevi wrote: > std::string() Done. https://codereview.chromium.org/1230033005/diff/100001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1230033005/diff/100001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:924: net::CertVerifierBlockAdapter::Params params(cert, hostname); On 2015/08/07 21:52:12, Ryan Sleevi wrote: > Shouldn't this be cert.Pass()? > > You should either pass scoped_refptr<> by const-ref (line 910), or you should > use .Pass(), to avoid unnecessarily refcount churns. scoped_refptr does not have |Pass|, so that's not possible. Using const reference will not work either, because |cert| is used inside the block. Blocks capture C++ objects by copying them and references by copying their pointer addresses. So passing |cert| as a const ref will lead to dereferencing a dangling pointer inside the block. However I changed CertVerifierBlockAdapter::Params constructor to avoid refcount churns where it's really unnecessary. https://codereview.chromium.org/1230033005/diff/100001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:931: dispatch_async(dispatch_get_main_queue(), ^{ On 2015/08/07 21:52:12, Ryan Sleevi wrote: > Why is this double dispatch needed? Why can't you just pass completionHandler? UIKit is not thread safe so I want to call |completionHandler| on the main thread (where verifyCert:forHost:completionHandler:) will always be called. ::Verify calls it's block on the same thread, which is IO.
I'm still really shaky on the threading bits, and things have changed since Stuart reviewed, so it'd help to get another re-review. I'm OOO for the next week+, so I'm going to defer to David to pick up this review. I don't think I'm comfortable with an L-G in the present form, but I'll fully defer to David's judgement and his should be seen as sufficient. https://codereview.chromium.org/1230033005/diff/100001/ios/web/net/cert_verif... File ios/web/net/cert_verifier_block_adapter_unittest.cc (right): https://codereview.chromium.org/1230033005/diff/100001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter_unittest.cc:30: MOCK_METHOD9(Verify, On 2015/08/12 22:00:38, eugenebut wrote: > MockCertVerifier does not allow to test OCSP and CRL inputs, as well as > asynchronous verification. > Switching this test to use MockCertVerifier will lead to loosing significant > test coverage. I'm sorry, I simply have trouble parsing this. I haven't seen any of your tests using that. > > Having said that I believe that gMock is better option comparing to > MockCertVerifier, even if in the future we'll need to replace gMock with > something different. I think you missed the point of those threads. There was strong disagreement that using GMock is a better option, from many of the senior Chrome leads. https://codereview.chromium.org/1230033005/diff/100001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1230033005/diff/100001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:924: net::CertVerifierBlockAdapter::Params params(cert, hostname); On 2015/08/12 22:00:38, eugenebut wrote: > scoped_refptr does not have |Pass|, so that's not possible. Yes it does. https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/ref_co... > Using const reference will not work either, because |cert| is used inside the > block. Blocks capture C++ objects by copying them and references by copying > their pointer addresses. So passing |cert| as a const ref will lead to > dereferencing a dangling pointer inside the block. Yet another unfortunate thing about using blocks :( https://codereview.chromium.org/1230033005/diff/120001/ios/web/ios_web_unitte... File ios/web/ios_web_unittests.gyp (right): https://codereview.chromium.org/1230033005/diff/120001/ios/web/ios_web_unitte... ios/web/ios_web_unittests.gyp:88: '../../net/data/ssl/certificates/2029_globalsign_com_cert.pem', This does not seem the right/appropriate way to express this dependency. Is just copying files from other targets accepted on iOS? This incidentally bit two different coworkers this week with respect to how iOS handles copy_test_data, and it seems less error prone that if copying from other targets is accepted, that you look at doing the entirity of net/data/ssl/certificates ( c.f. https://code.google.com/p/chromium/codesearch#chromium/src/net/net.gyp&l=367 ) https://codereview.chromium.org/1230033005/diff/120001/ios/web/net/cert_verif... File ios/web/net/cert_verifier_block_adapter.cc (right): https://codereview.chromium.org/1230033005/diff/120001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter.cc:24: cert(cert), cert.Pass() https://codereview.chromium.org/1230033005/diff/120001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter.cc:39: friend class base::RefCountedThreadSafe<VerificationContext>; This should be the first (e.g. on line 37, after line 36). Comment is unnecessary (and out of date) https://codereview.chromium.org/1230033005/diff/120001/ios/web/net/cert_verif... File ios/web/net/cert_verifier_block_adapter_unittest.cc (right): https://codereview.chromium.org/1230033005/diff/120001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter_unittest.cc:30: const char kHostName[] = "2029.globalsign.com"; use ok_cert.pem, if you're just looking for a random file. The README in that directory is meant to guide to well-supported files for different purposes :) https://codereview.chromium.org/1230033005/diff/120001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter_unittest.cc:56: web::WebThread::PostTask(web::WebThread::IO, FROM_HERE, This doesn't seem right. Nothing in your test asserts it's on the IO thread (e.g. the test starting at line 124). base::ThreadTaskRunnerHandle::Get()->PostTask(...) runs the callback on the same thread you called Verify() on, and is consistent from an API contract. Put differently, what if someone made the WebThread::IO a different thread than your main test? It would reveal that your test is encoding the wrong expectations. https://codereview.chromium.org/1230033005/diff/120001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter_unittest.cc:67: cert_(ImportCertFromFile(GetTestCertsDirectory(), kCertFileName)), Shouldn't this be in SetUp() so that you can ASSERT_TRUE(cert_) to make sure you're not going to crash the test harness? https://codereview.chromium.org/1230033005/diff/120001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter_unittest.cc:87: // IO Thread bundle. Seems unnecessarily verbose https://codereview.chromium.org/1230033005/diff/120001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter_unittest.cc:89: // Fake certificate created for testing. It's not really a fake certificate, is it? https://codereview.chromium.org/1230033005/diff/120001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter_unittest.cc:91: // CertVerifier mock. As does this https://codereview.chromium.org/1230033005/diff/120001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter_unittest.cc:93: // NetLog object required by CertVerifierBlockAdapter. As does this https://codereview.chromium.org/1230033005/diff/120001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter_unittest.cc:95: // Testable |CertVerifierBlockAdapter| object. As does this https://codereview.chromium.org/1230033005/diff/120001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter_unittest.cc:152: int actualStatus = -1; naming (throughout this file) doesn't follow the C++ style guide. https://codereview.chromium.org/1230033005/diff/120001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1230033005/diff/120001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:1300: !net::IsCertStatusMinorError(certVerifyResult.cert_status); net will never return OK if IsCertStatusError() is true (and thus also impossible for minor error to be true)
Ryan, thank you for detailed feedback. David, Stuart, could you please take a look. https://codereview.chromium.org/1230033005/diff/100001/ios/web/net/cert_verif... File ios/web/net/cert_verifier_block_adapter_unittest.cc (right): https://codereview.chromium.org/1230033005/diff/100001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter_unittest.cc:30: MOCK_METHOD9(Verify, On 2015/08/14 02:29:43, Ryan Sleevi (out until 8-24) wrote: > On 2015/08/12 22:00:38, eugenebut wrote: > > MockCertVerifier does not allow to test OCSP and CRL inputs, as well as > > asynchronous verification. > > Switching this test to use MockCertVerifier will lead to loosing significant > > test coverage. > > I'm sorry, I simply have trouble parsing this. I haven't seen any of your tests > using that. > > > > > Having said that I believe that gMock is better option comparing to > > MockCertVerifier, even if in the future we'll need to replace gMock with > > something different. > > I think you missed the point of those threads. There was strong disagreement > that using GMock is a better option, from many of the senior Chrome leads. I've tried using MockCertVerifier for testing CertVerifierBlockAdapter but for this specific class MockCertVerifier does not allow to test the following scenarios: 1.) Async Cert Evaluation: In this file tested by DefaultParamsAndAsync and DefaultParamsAndAsyncError. MockCertVerifier emulates only synchronous response https://code.google.com/p/chromium/codesearch#chromium/src/net/cert/mock_cert... 2.) CRL_Set, OCSP response and flags: In this file tested by AllParamsAndSync. MockCertVerifier does not use passed |ocsp_response|, |crl_set| and |flags| arguments. While I guess loosing coverage for CRL_Set, OCSP response and flags is acceptable, I strongly believe that testing async evaluation in this unit test is necessary. How about I extend MockCertVerifier to support async evaluation and then I will rewrite this test file to use MockCertVerifier? https://codereview.chromium.org/1230033005/diff/100001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1230033005/diff/100001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:924: net::CertVerifierBlockAdapter::Params params(cert, hostname); On 2015/08/14 02:29:43, Ryan Sleevi (out until 8-24) wrote: > On 2015/08/12 22:00:38, eugenebut wrote: > > scoped_refptr does not have |Pass|, so that's not possible. > > Yes it does. > > https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/ref_co... I'm sorry, the code with .Pass() did not compile because it was used inside the block, not because Pass() does not exist. I guess I was not very attentive when read the error message. The way to make it compile is ugly, but Done. https://codereview.chromium.org/1230033005/diff/120001/ios/web/ios_web_unitte... File ios/web/ios_web_unittests.gyp (right): https://codereview.chromium.org/1230033005/diff/120001/ios/web/ios_web_unitte... ios/web/ios_web_unittests.gyp:88: '../../net/data/ssl/certificates/2029_globalsign_com_cert.pem', On 2015/08/14 02:29:43, Ryan Sleevi (out until 8-24) wrote: > This does not seem the right/appropriate way to express this dependency. Is just > copying files from other targets accepted on iOS? > > This incidentally bit two different coworkers this week with respect to how iOS > handles copy_test_data, and it seems less error prone that if copying from other > targets is accepted, that you look at doing the entirity of > net/data/ssl/certificates ( c.f. > https://code.google.com/p/chromium/codesearch#chromium/src/net/net.gyp&l=367 ) Copying files from another target is not ideal, but ok. Done. https://codereview.chromium.org/1230033005/diff/120001/ios/web/net/cert_verif... File ios/web/net/cert_verifier_block_adapter.cc (right): https://codereview.chromium.org/1230033005/diff/120001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter.cc:24: cert(cert), On 2015/08/14 02:29:43, Ryan Sleevi (out until 8-24) wrote: > cert.Pass() Done. https://codereview.chromium.org/1230033005/diff/120001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter.cc:39: friend class base::RefCountedThreadSafe<VerificationContext>; On 2015/08/14 02:29:43, Ryan Sleevi (out until 8-24) wrote: > This should be the first (e.g. on line 37, after line 36). Comment is > unnecessary (and out of date) Done. https://codereview.chromium.org/1230033005/diff/120001/ios/web/net/cert_verif... File ios/web/net/cert_verifier_block_adapter_unittest.cc (right): https://codereview.chromium.org/1230033005/diff/120001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter_unittest.cc:30: const char kHostName[] = "2029.globalsign.com"; On 2015/08/14 02:29:44, Ryan Sleevi (out until 8-24) wrote: > use ok_cert.pem, if you're just looking for a random file. > > The README in that directory is meant to guide to well-supported files for > different purposes :) Done. ok_cert.pem works for me. https://codereview.chromium.org/1230033005/diff/120001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter_unittest.cc:56: web::WebThread::PostTask(web::WebThread::IO, FROM_HERE, On 2015/08/14 02:29:44, Ryan Sleevi (out until 8-24) wrote: > This doesn't seem right. Nothing in your test asserts it's on the IO thread > (e.g. the test starting at line 124). > > base::ThreadTaskRunnerHandle::Get()->PostTask(...) runs the callback on the same > thread you called Verify() on, and is consistent from an API contract. > > Put differently, what if someone made the WebThread::IO a different thread than > your main test? It would reveal that your test is encoding the wrong > expectations. Done. https://codereview.chromium.org/1230033005/diff/120001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter_unittest.cc:67: cert_(ImportCertFromFile(GetTestCertsDirectory(), kCertFileName)), On 2015/08/14 02:29:43, Ryan Sleevi (out until 8-24) wrote: > Shouldn't this be in SetUp() so that you can ASSERT_TRUE(cert_) to make sure > you're not going to crash the test harness? I added DCHECK to constructor. In GTest Constructors are more preferable than SetUp: https://code.google.com/p/googletest/wiki/FAQ#Should_I_use_the_constructor/de... https://codereview.chromium.org/1230033005/diff/120001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter_unittest.cc:87: // IO Thread bundle. On 2015/08/14 02:29:44, Ryan Sleevi (out until 8-24) wrote: > Seems unnecessarily verbose Done. https://codereview.chromium.org/1230033005/diff/120001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter_unittest.cc:89: // Fake certificate created for testing. On 2015/08/14 02:29:43, Ryan Sleevi (out until 8-24) wrote: > It's not really a fake certificate, is it? Removed. https://codereview.chromium.org/1230033005/diff/120001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter_unittest.cc:91: // CertVerifier mock. On 2015/08/14 02:29:44, Ryan Sleevi (out until 8-24) wrote: > As does this Done. https://codereview.chromium.org/1230033005/diff/120001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter_unittest.cc:93: // NetLog object required by CertVerifierBlockAdapter. On 2015/08/14 02:29:43, Ryan Sleevi (out until 8-24) wrote: > As does this Done. https://codereview.chromium.org/1230033005/diff/120001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter_unittest.cc:95: // Testable |CertVerifierBlockAdapter| object. On 2015/08/14 02:29:44, Ryan Sleevi (out until 8-24) wrote: > As does this Done. https://codereview.chromium.org/1230033005/diff/120001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter_unittest.cc:152: int actualStatus = -1; On 2015/08/14 02:29:44, Ryan Sleevi (out until 8-24) wrote: > naming (throughout this file) doesn't follow the C++ style guide. Fixed actual_status name. I believe other names are correct. https://codereview.chromium.org/1230033005/diff/120001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1230033005/diff/120001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:1300: !net::IsCertStatusMinorError(certVerifyResult.cert_status); On 2015/08/14 02:29:44, Ryan Sleevi (out until 8-24) wrote: > net will never return OK if IsCertStatusError() is true (and thus also > impossible for minor error to be true) Changing to bool isCertValid = statusResult == net::OK;
Just a few notes; not a re-review https://codereview.chromium.org/1230033005/diff/100001/ios/web/net/cert_verif... File ios/web/net/cert_verifier_block_adapter_unittest.cc (right): https://codereview.chromium.org/1230033005/diff/100001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter_unittest.cc:30: MOCK_METHOD9(Verify, On 2015/08/14 21:18:19, eugenebut wrote: > 2.) CRL_Set, OCSP response and flags: > In this file tested by AllParamsAndSync. > MockCertVerifier does not use passed |ocsp_response|, |crl_set| and |flags| > arguments. So, I can understand and appreciate that, but that you're testing that at all is part of why there's such a strong opposition to GMock in Chromium - because people end up using it to test interactions, rather than state. If you look on the internal Google resources for "mocks fakes stubs", you can see the best practices guide for each, including the admonitions against testing interactions. https://codereview.chromium.org/1230033005/diff/120001/ios/web/net/cert_verif... File ios/web/net/cert_verifier_block_adapter_unittest.cc (right): https://codereview.chromium.org/1230033005/diff/120001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter_unittest.cc:67: cert_(ImportCertFromFile(GetTestCertsDirectory(), kCertFileName)), On 2015/08/14 21:18:20, eugenebut wrote: > On 2015/08/14 02:29:43, Ryan Sleevi (out until 8-24) wrote: > > Shouldn't this be in SetUp() so that you can ASSERT_TRUE(cert_) to make sure > > you're not going to crash the test harness? > I added DCHECK to constructor. In GTest Constructors are more preferable than > SetUp: > https://code.google.com/p/googletest/wiki/FAQ#Should_I_use_the_constructor/de... I specifically said SetUp because that's what it's for. DCHECK should _never_ be used in unit tests. https://codereview.chromium.org/1230033005/diff/140001/ios/web/net/cert_verif... File ios/web/net/cert_verifier_block_adapter_unittest.cc (right): https://codereview.chromium.org/1230033005/diff/140001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter_unittest.cc:8: #include "base/message_loop/message_loop.h" Blergh. It seems the iOS test code still uses the deprecated interface. Filed https://code.google.com/p/chromium/issues/detail?id=521138 so that you shouldn't have to use base::MessageLoop
Stuart, PTAL. https://codereview.chromium.org/1230033005/diff/100001/ios/web/net/cert_verif... File ios/web/net/cert_verifier_block_adapter_unittest.cc (right): https://codereview.chromium.org/1230033005/diff/100001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter_unittest.cc:30: MOCK_METHOD9(Verify, On 2015/08/14 21:43:53, Ryan Sleevi (out until 8-24) wrote: > On 2015/08/14 21:18:19, eugenebut wrote: > > 2.) CRL_Set, OCSP response and flags: > > In this file tested by AllParamsAndSync. > > MockCertVerifier does not use passed |ocsp_response|, |crl_set| and |flags| > > arguments. > > So, I can understand and appreciate that, but that you're testing that at all is > part of why there's such a strong opposition to GMock in Chromium - because > people end up using it to test interactions, rather than state. > > If you look on the internal Google resources for "mocks fakes stubs", you can > see the best practices guide for each, including the admonitions against testing > interactions. Ryan, I my previous comment I did not argue that we should continue using gmock. I'm trying to keep coverage for async verification and using MockCertVerifier as it is will make this test case very weak. For the sake of moving forward I removed gmock usage. For async scenario testing I'm using real net::CertVerifier (test takes 12ms) and for the remaining scenarios I use MockCertVerifier. https://codereview.chromium.org/1230033005/diff/120001/ios/web/net/cert_verif... File ios/web/net/cert_verifier_block_adapter_unittest.cc (right): https://codereview.chromium.org/1230033005/diff/120001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter_unittest.cc:67: cert_(ImportCertFromFile(GetTestCertsDirectory(), kCertFileName)), On 2015/08/14 21:43:53, Ryan Sleevi (out until 8-24) wrote: > On 2015/08/14 21:18:20, eugenebut wrote: > > On 2015/08/14 02:29:43, Ryan Sleevi (out until 8-24) wrote: > > > Shouldn't this be in SetUp() so that you can ASSERT_TRUE(cert_) to make sure > > > you're not going to crash the test harness? > > I added DCHECK to constructor. In GTest Constructors are more preferable than > > SetUp: > > > https://code.google.com/p/googletest/wiki/FAQ#Should_I_use_the_constructor/de... > > I specifically said SetUp because that's what it's for. DCHECK should _never_ be > used in unit tests. Done. https://codereview.chromium.org/1230033005/diff/140001/ios/web/net/cert_verif... File ios/web/net/cert_verifier_block_adapter_unittest.cc (right): https://codereview.chromium.org/1230033005/diff/140001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter_unittest.cc:8: #include "base/message_loop/message_loop.h" On 2015/08/14 21:43:54, Ryan Sleevi (out until 8-24) wrote: > Blergh. It seems the iOS test code still uses the deprecated interface. Filed > https://code.google.com/p/chromium/issues/detail?id=521138 so that you shouldn't > have to use base::MessageLoop Acknowledged.
Sorry we //net people are so finicky and aren't the fastest reviewers. :-) I have to swap in a lot of Obj-C stuff to even understand this CL and Apple's documentation is not the best in the world. Mostly just nits, but a couple of lifetime and threading questions that are pretty important. https://codereview.chromium.org/1230033005/diff/180001/ios/web/net/cert_verif... File ios/web/net/cert_verifier_block_adapter.cc (right): https://codereview.chromium.org/1230033005/diff/180001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter.cc:22: VerificationContext(scoped_refptr<net::X509Certificate> cert, NetLog* net_log) Nit: stray net:: prefix? https://codereview.chromium.org/1230033005/diff/180001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter.cc:31: // Certificate being verificated. verificated -> verified https://codereview.chromium.org/1230033005/diff/180001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter.cc:32: scoped_refptr<net::X509Certificate> cert; Ditto. https://codereview.chromium.org/1230033005/diff/180001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter.cc:77: pending_requests_.begin(), pending_requests_.end(), context->request); [I'm not familiar with blocks and ObjC++. How do blocks capture scoped_refptrs? I'm assuming this makes a copy, thus capturing a reference to |context|. Is this correct?] https://codereview.chromium.org/1230033005/diff/180001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter.cc:79: pending_requests_.erase(request_iterator); [I am assuming that the block captures a raw pointer to |this| and doesn't try to do anything cleverer than that.] https://codereview.chromium.org/1230033005/diff/180001/ios/web/net/cert_verif... File ios/web/net/cert_verifier_block_adapter.h (right): https://codereview.chromium.org/1230033005/diff/180001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter.h:26: // null. CertVerifierBlockAdapter does NOT take ownership over |cert_verifier| Nit: over -> of? https://codereview.chromium.org/1230033005/diff/180001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter.h:41: Params(const scoped_refptr<net::X509Certificate>& cert, Nit: Stray net:: prefix? (Arguably having something in //ios/web squat in //net's namespace is already kinda weird, but I'm not going to insist you unnamespace it or anything.) https://codereview.chromium.org/1230033005/diff/180001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter.h:65: int status_result); This name tripped me up rather badly because of the word "status" and the overlap with cert_status. Why not just result? status_result is a pretty unhelpful name I think. Or maybe just error. (Ditto throughout the tests and elsewhere.) https://codereview.chromium.org/1230033005/diff/180001/ios/web/net/cert_verif... File ios/web/net/cert_verifier_block_adapter_unittest.cc (right): https://codereview.chromium.org/1230033005/diff/180001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter_unittest.cc:36: cert_ = ImportCertFromFile(GetTestCertsDirectory(), kCertFileName).Pass(); I think the .Pass() isn't necessary here since it's already a temporary. https://codereview.chromium.org/1230033005/diff/180001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter_unittest.cc:68: const int kExpectedStatus = OK; This doesn't make any sense. It should return ERR_CERT_AUTHORITY_INVALID if that's the cert_status. https://codereview.chromium.org/1230033005/diff/180001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter_unittest.cc:69: net::CertVerifyResult expected_cert_verify_result; Nit: stray net:: prefix? Also elsewhere in the file. https://codereview.chromium.org/1230033005/diff/180001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1230033005/diff/180001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:144: scoped_ptr<net::CertVerifierBlockAdapter> _certVerifier; Unfortunately, the lifetimes for URLRequestContextGetter is kind of stupid. Just because the URLRequestContextGetter is alive, doesn't mean the URLRequestContext is alive, depending on what the getter does. (The getter has effectively lifetime infinity due to ref-counting.) At some point I need to sit down with it and try to fix this. It's complicated due to there being separate threads and junk. The important question here is how does shutdown work? You access it on an UI -> IO PostTask. Can you be sure that the underlying URLRequestContext is, in turn destroyed on a UI -> IO PostTask of some object (like Profile or something)? If yes, then this should be safe. If no, we have a problem. Given that iOS is itself reference counted, I'm actually not sure what kind of lifetime assumptions you get to make. It'll depend a lot on what your URLRequestContextGetter implementation is. URLRequestContextGetter does have an observer interface for finding out when the context is gone, but that's mostly the "I give up, lifetimes are too hard and I'll write really fragile code instead" option. https://codereview.chromium.org/1230033005/diff/180001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:147: net::URLRequestContextGetter* _contextGetter; This should probably be scoped_refptr<....>, no? https://codereview.chromium.org/1230033005/diff/180001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:549: - (int)certVerifyFlags { DCHECK that you're on the IO thread? https://codereview.chromium.org/1230033005/diff/180001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:560: - (net::CertVerifierBlockAdapter*)certVerifier { DCHECK that you're on the IO thread? https://codereview.chromium.org/1230033005/diff/180001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:924: __block scoped_refptr<net::X509Certificate> blockCert = cert; This __block means that you and the block both point to the scoped_refptr itself rather than make a copy, right? How does that work when this is a C++ type with a destructor? When multiple threads are involved?? If it saves a pointer to the stack rather than popping the object into its own random little Obj-C-ref-counted box, this cannot possibly work. https://codereview.chromium.org/1230033005/diff/180001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:928: // retains self. I'm assuming ObjC reference counting is thread-safe? https://codereview.chromium.org/1230033005/diff/180001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:937: params.crl_set = net::SSLConfigService::GetCRLSet().Pass(); This returns a temporary, so I don't think the .Pass() is needed here? https://codereview.chromium.org/1230033005/diff/180001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:1288: completionHandler(NSURLSessionAuthChallengePerformDefaultHandling, nil); I realize I'm the one who asked about whether Reject should have been changed to Default, but did you come to a conclusion as to what NSURLSessionAuthChallengeRejectProtectionSpace vs. NSURLSessionAuthChallengePerformDefaultHandling was? The API docs say: "If you do not implement this method, the web view will respond to the authentication challenge with the NSURLSessionAuthChallengeRejectProtectionSpace disposition." which suggests to me that NSURLSessionAuthChallengeRejectProtectionSpace when you have no clue what this protection space is and perhaps NSURLSessionAuthChallengePerformDefaultHandling is what you do get the default??? Except what the difference there could possibly mean, I haven't the foggiest idea. This API is weird. And the documentation seems to be basically crap. :-/ It all references a *different* API with a slightly different shape. https://codereview.chromium.org/1230033005/diff/180001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:1298: if (isCertValid) { Nit: Why not just if (statusResult == net::OK) ?
David thank you for very detailed code review. Really appreciate. PTAL. https://codereview.chromium.org/1230033005/diff/180001/ios/web/net/cert_verif... File ios/web/net/cert_verifier_block_adapter.cc (right): https://codereview.chromium.org/1230033005/diff/180001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter.cc:22: VerificationContext(scoped_refptr<net::X509Certificate> cert, NetLog* net_log) On 2015/08/19 18:51:45, David Benjamin (slow) wrote: > Nit: stray net:: prefix? Done. https://codereview.chromium.org/1230033005/diff/180001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter.cc:31: // Certificate being verificated. On 2015/08/19 18:51:45, David Benjamin (slow) wrote: > verificated -> verified Done. https://codereview.chromium.org/1230033005/diff/180001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter.cc:32: scoped_refptr<net::X509Certificate> cert; On 2015/08/19 18:51:45, David Benjamin (slow) wrote: > Ditto. Done. https://codereview.chromium.org/1230033005/diff/180001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter.cc:77: pending_requests_.begin(), pending_requests_.end(), context->request); On 2015/08/19 18:51:45, David Benjamin (slow) wrote: > [I'm not familiar with blocks and ObjC++. How do blocks capture scoped_refptrs? > I'm assuming this makes a copy, thus capturing a reference to |context|. Is this > correct?] Correct, scoped_refptrs is captured as a const copy. https://codereview.chromium.org/1230033005/diff/180001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter.cc:79: pending_requests_.erase(request_iterator); On 2015/08/19 18:51:45, David Benjamin (slow) wrote: > [I am assuming that the block captures a raw pointer to |this| and doesn't try > to do anything cleverer than that.] Correct, for compiler this code looks this: this->pending_requests_.erase(request_iterator); And since |this| is a raw pointer, block will just capture its address. https://codereview.chromium.org/1230033005/diff/180001/ios/web/net/cert_verif... File ios/web/net/cert_verifier_block_adapter.h (right): https://codereview.chromium.org/1230033005/diff/180001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter.h:26: // null. CertVerifierBlockAdapter does NOT take ownership over |cert_verifier| On 2015/08/19 18:51:45, David Benjamin (slow) wrote: > Nit: over -> of? Done. https://codereview.chromium.org/1230033005/diff/180001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter.h:41: Params(const scoped_refptr<net::X509Certificate>& cert, On 2015/08/19 18:51:45, David Benjamin (slow) wrote: > Nit: Stray net:: prefix? > > (Arguably having something in //ios/web squat in //net's namespace is already > kinda weird, but I'm not going to insist you unnamespace it or anything.) This lives in //ios/web/net, and it's more net than web, I think. https://codereview.chromium.org/1230033005/diff/180001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter.h:65: int status_result); On 2015/08/19 18:51:45, David Benjamin (slow) wrote: > This name tripped me up rather badly because of the word "status" and the > overlap with cert_status. Why not just result? status_result is a pretty > unhelpful name I think. Or maybe just error. (Ditto throughout the tests and > elsewhere.) Just |result| will overlap with CertVerifyResult. Changed to |error| (as far as I can see this name is used in other places as well). https://codereview.chromium.org/1230033005/diff/180001/ios/web/net/cert_verif... File ios/web/net/cert_verifier_block_adapter_unittest.cc (right): https://codereview.chromium.org/1230033005/diff/180001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter_unittest.cc:36: cert_ = ImportCertFromFile(GetTestCertsDirectory(), kCertFileName).Pass(); On 2015/08/19 18:51:45, David Benjamin (slow) wrote: > I think the .Pass() isn't necessary here since it's already a temporary. Done. https://codereview.chromium.org/1230033005/diff/180001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter_unittest.cc:68: const int kExpectedStatus = OK; On 2015/08/19 18:51:45, David Benjamin (slow) wrote: > This doesn't make any sense. It should return ERR_CERT_AUTHORITY_INVALID if > that's the cert_status. Done. https://codereview.chromium.org/1230033005/diff/180001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter_unittest.cc:69: net::CertVerifyResult expected_cert_verify_result; On 2015/08/19 18:51:45, David Benjamin (slow) wrote: > Nit: stray net:: prefix? Also elsewhere in the file. Done. https://codereview.chromium.org/1230033005/diff/180001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1230033005/diff/180001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:144: scoped_ptr<net::CertVerifierBlockAdapter> _certVerifier; On 2015/08/19 18:51:46, David Benjamin (slow) wrote: > Unfortunately, the lifetimes for URLRequestContextGetter is kind of stupid. Just > because the URLRequestContextGetter is alive, doesn't mean the URLRequestContext > is alive, depending on what the getter does. (The getter has effectively > lifetime infinity due to ref-counting.) At some point I need to sit down with it > and try to fix this. It's complicated due to there being separate threads and > junk. > > The important question here is how does shutdown work? You access it on an UI -> > IO PostTask. Can you be sure that the underlying URLRequestContext is, in turn > destroyed on a UI -> IO PostTask of some object (like Profile or something)? If > yes, then this should be safe. If no, we have a problem. > > Given that iOS is itself reference counted, I'm actually not sure what kind of > lifetime assumptions you get to make. It'll depend a lot on what your > URLRequestContextGetter implementation is. URLRequestContextGetter does have an > observer interface for finding out when the context is gone, but that's mostly > the "I give up, lifetimes are too hard and I'll write really fragile code > instead" option. Both URLRequestContextGetter and URLRequestContext are owned by web::BrowserState. web::BrowserState always outlives WebController (this class). WebController will not access URLRequestContextGetter if it is being destroyed ([self isBeingDestroyed]). web::BrowserState for incognito profile is destroyed after the last Tab is closed and at that moment all WebControllers are already deallocated. Stuart, please correct me if I'm wrong. https://codereview.chromium.org/1230033005/diff/180001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:147: net::URLRequestContextGetter* _contextGetter; On 2015/08/19 18:51:46, David Benjamin (slow) wrote: > This should probably be scoped_refptr<....>, no? No, web::BrowserState (ios analogue of content::BrowserContext) returns a raw pointer: https://code.google.com/p/chromium/codesearch#chromium/src/ios/web/public/bro... https://codereview.chromium.org/1230033005/diff/180001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:549: - (int)certVerifyFlags { On 2015/08/19 18:51:46, David Benjamin (slow) wrote: > DCHECK that you're on the IO thread? Done. https://codereview.chromium.org/1230033005/diff/180001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:560: - (net::CertVerifierBlockAdapter*)certVerifier { On 2015/08/19 18:51:46, David Benjamin (slow) wrote: > DCHECK that you're on the IO thread? Done. https://codereview.chromium.org/1230033005/diff/180001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:924: __block scoped_refptr<net::X509Certificate> blockCert = cert; On 2015/08/19 18:51:45, David Benjamin (slow) wrote: > This __block means that you and the block both point to the scoped_refptr itself > rather than make a copy, right? > > How does that work when this is a C++ type with a destructor? When multiple > threads are involved?? If it saves a pointer to the stack rather than popping > the object into its own random little Obj-C-ref-counted box, this cannot > possibly work. __block means that |blockCert| variable will be mutable inside the block. Without |__block| modifier |blockCert| will be copied as a const object and calling |blockCert.Pass()| will not be possible. https://codereview.chromium.org/1230033005/diff/180001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:928: // retains self. On 2015/08/19 18:51:46, David Benjamin (slow) wrote: > I'm assuming ObjC reference counting is thread-safe? Correct. https://codereview.chromium.org/1230033005/diff/180001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:937: params.crl_set = net::SSLConfigService::GetCRLSet().Pass(); On 2015/08/19 18:51:46, David Benjamin (slow) wrote: > This returns a temporary, so I don't think the .Pass() is needed here? Removed. https://codereview.chromium.org/1230033005/diff/180001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:1288: completionHandler(NSURLSessionAuthChallengePerformDefaultHandling, nil); On 2015/08/19 18:51:46, David Benjamin (slow) wrote: > I realize I'm the one who asked about whether Reject should have been changed to > Default, but did you come to a conclusion as to what > NSURLSessionAuthChallengeRejectProtectionSpace vs. > NSURLSessionAuthChallengePerformDefaultHandling was? The API docs say: > > "If you do not implement this method, the web view will respond to the > authentication challenge with the NSURLSessionAuthChallengeRejectProtectionSpace > disposition." > > which suggests to me that NSURLSessionAuthChallengeRejectProtectionSpace when > you have no clue what this protection space is and perhaps > NSURLSessionAuthChallengePerformDefaultHandling is what you do get the > default??? Except what the difference there could possibly mean, I haven't the > foggiest idea. > > This API is weird. And the documentation seems to be basically crap. :-/ It all > references a *different* API with a slightly different shape. The documentation for NSURLSessionAuthChallengePerformDefaultHandling says: "Default handling for the challenge - as if this delegate were not implemented; the credential parameter is ignored.". However this doc belongs to NSURLSession.h, so I probably should switch back to NSURLSessionAuthChallengeRejectProtectionSpace, and trust WKWebView comments. P.S. I tested both constants with different certs and did not see any difference. https://codereview.chromium.org/1230033005/diff/180001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:1298: if (isCertValid) { On 2015/08/19 18:51:46, David Benjamin (slow) wrote: > Nit: Why not just > > if (statusResult == net::OK) ? Sure, this is just an example of API usage.
https://codereview.chromium.org/1230033005/diff/180001/ios/web/net/cert_verif... File ios/web/net/cert_verifier_block_adapter.h (right): https://codereview.chromium.org/1230033005/diff/180001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter.h:41: Params(const scoped_refptr<net::X509Certificate>& cert, On 2015/08/20 01:14:41, eugenebut wrote: > On 2015/08/19 18:51:45, David Benjamin (slow) wrote: > > Nit: Stray net:: prefix? > > > > (Arguably having something in //ios/web squat in //net's namespace is already > > kinda weird, but I'm not going to insist you unnamespace it or anything.) > This lives in //ios/web/net, and it's more net than web, I think. That's true, but stomping in another module's namespace is very much not reasonable. Anyway, this is an existing bug, so we should leave it as it is for this CL. But I do not think this is correct. https://codereview.chromium.org/1230033005/diff/180001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1230033005/diff/180001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:147: net::URLRequestContextGetter* _contextGetter; On 2015/08/20 01:14:41, eugenebut wrote: > On 2015/08/19 18:51:46, David Benjamin (slow) wrote: > > This should probably be scoped_refptr<....>, no? > No, web::BrowserState (ios analogue of content::BrowserContext) returns a raw > pointer: > https://code.google.com/p/chromium/codesearch#chromium/src/ios/web/public/bro... > Our reference counts are intrusive. You can and often do go from T* to scoped_refptr<T>. I think this is rather obnoxious for lots of reasons, but those are mostly because reference-counting is obnoxious. Holding a T* without a reference count tends to be rather questionable. https://codereview.chromium.org/1230033005/diff/180001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:924: __block scoped_refptr<net::X509Certificate> blockCert = cert; On 2015/08/20 01:14:41, eugenebut wrote: > On 2015/08/19 18:51:45, David Benjamin (slow) wrote: > > This __block means that you and the block both point to the scoped_refptr > itself > > rather than make a copy, right? > > > > How does that work when this is a C++ type with a destructor? When multiple > > threads are involved?? If it saves a pointer to the stack rather than popping > > the object into its own random little Obj-C-ref-counted box, this cannot > > possibly work. > __block means that |blockCert| variable will be mutable inside the block. > Without |__block| modifier |blockCert| will be copied as a const object and > calling |blockCert.Pass()| will not be possible. What does mutable inside the block mean? That's not sufficient to ensure this is safe. Is it: - blockCert is allocated on the stack and the block gets a pointer to the stack? - This is BAD BAD BAD. Do NOT do this. There is no guarantee that the block runs before verifyCert returns. This is a critical security bug. - blockCert is popped to a thread-unsafe reference-counted blob shared by both the caller and the block. - This does not work either. The outer reference-count MUST be thread-safe. - blockCert is popped to a thread-safe reference-counted blob shared by both the caller and the block. - This does actually work. It is, however, not clear to me you have gained anything in that exercise since we're still bouncing on a reference count. If you could .Pass() into the block's storage and from there .Pass() to the Params, this would be valuable. But I doubt blocks let you do that. https://codereview.chromium.org/1230033005/diff/180001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:1288: completionHandler(NSURLSessionAuthChallengePerformDefaultHandling, nil); On 2015/08/20 01:14:41, eugenebut wrote: > On 2015/08/19 18:51:46, David Benjamin (slow) wrote: > > I realize I'm the one who asked about whether Reject should have been changed > to > > Default, but did you come to a conclusion as to what > > NSURLSessionAuthChallengeRejectProtectionSpace vs. > > NSURLSessionAuthChallengePerformDefaultHandling was? The API docs say: > > > > "If you do not implement this method, the web view will respond to the > > authentication challenge with the > NSURLSessionAuthChallengeRejectProtectionSpace > > disposition." > > > > which suggests to me that NSURLSessionAuthChallengeRejectProtectionSpace when > > you have no clue what this protection space is and perhaps > > NSURLSessionAuthChallengePerformDefaultHandling is what you do get the > > default??? Except what the difference there could possibly mean, I haven't the > > foggiest idea. > > > > This API is weird. And the documentation seems to be basically crap. :-/ It > all > > references a *different* API with a slightly different shape. > The documentation for NSURLSessionAuthChallengePerformDefaultHandling says: > "Default handling for the challenge - as if this delegate were not implemented; > the credential parameter is ignored.". > However this doc belongs to NSURLSession.h, so I probably should switch back to > NSURLSessionAuthChallengeRejectProtectionSpace, and trust WKWebView comments. > > P.S. I tested both constants with different certs and did not see any > difference. Mmm. I'll defer to you here because I have no clue what in the world this API is doing. :-) It would be good to get a definitive answer from Apple though.
https://codereview.chromium.org/1230033005/diff/180001/ios/web/net/cert_verif... File ios/web/net/cert_verifier_block_adapter.h (right): https://codereview.chromium.org/1230033005/diff/180001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter.h:41: Params(const scoped_refptr<net::X509Certificate>& cert, On 2015/08/20 01:25:27, David Benjamin (slow) wrote: > On 2015/08/20 01:14:41, eugenebut wrote: > > On 2015/08/19 18:51:45, David Benjamin (slow) wrote: > > > Nit: Stray net:: prefix? > > > > > > (Arguably having something in //ios/web squat in //net's namespace is > already > > > kinda weird, but I'm not going to insist you unnamespace it or anything.) > > This lives in //ios/web/net, and it's more net than web, I think. > > That's true, but stomping in another module's namespace is very much not > reasonable. Anyway, this is an existing bug, so we should leave it as it is for > this CL. But I do not think this is correct. Changed to web:: namespace, because all ios/web/net symbols are in web::. https://codereview.chromium.org/1230033005/diff/180001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1230033005/diff/180001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:147: net::URLRequestContextGetter* _contextGetter; On 2015/08/20 01:25:27, David Benjamin (slow) wrote: > On 2015/08/20 01:14:41, eugenebut wrote: > > On 2015/08/19 18:51:46, David Benjamin (slow) wrote: > > > This should probably be scoped_refptr<....>, no? > > No, web::BrowserState (ios analogue of content::BrowserContext) returns a raw > > pointer: > > > https://code.google.com/p/chromium/codesearch#chromium/src/ios/web/public/bro... > > > > Our reference counts are intrusive. You can and often do go from T* to > scoped_refptr<T>. I think this is rather obnoxious for lots of reasons, but > those are mostly because reference-counting is obnoxious. Holding a T* without a > reference count tends to be rather questionable. Yeah, but in Chrome for iOS BrowserState::GetRequestContext() returns a raw pointer, which receiver is not suppose to release. https://codereview.chromium.org/1230033005/diff/180001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:924: __block scoped_refptr<net::X509Certificate> blockCert = cert; On 2015/08/20 01:25:27, David Benjamin (slow) wrote: > On 2015/08/20 01:14:41, eugenebut wrote: > > On 2015/08/19 18:51:45, David Benjamin (slow) wrote: > > > This __block means that you and the block both point to the scoped_refptr > > itself > > > rather than make a copy, right? > > > > > > How does that work when this is a C++ type with a destructor? When multiple > > > threads are involved?? If it saves a pointer to the stack rather than > popping > > > the object into its own random little Obj-C-ref-counted box, this cannot > > > possibly work. > > __block means that |blockCert| variable will be mutable inside the block. > > Without |__block| modifier |blockCert| will be copied as a const object and > > calling |blockCert.Pass()| will not be possible. > > What does mutable inside the block mean? That's not sufficient to ensure this is > safe. Is it: > > - blockCert is allocated on the stack and the block gets a pointer to the stack? > - This is BAD BAD BAD. Do NOT do this. There is no guarantee that the block > runs before verifyCert returns. This is a critical security bug. > > - blockCert is popped to a thread-unsafe reference-counted blob shared by both > the caller and the block. > - This does not work either. The outer reference-count MUST be thread-safe. > > - blockCert is popped to a thread-safe reference-counted blob shared by both the > caller and the block. > - This does actually work. It is, however, not clear to me you have gained > anything in that exercise since we're still bouncing on a reference count. > > If you could .Pass() into the block's storage and from there .Pass() to the > Params, this would be valuable. But I doubt blocks let you do that. The behavior is: "blockCert is popped to a thread-safe reference-counted blob shared by both the caller and the block.". And I can't .Pass() into the block's storage. This method (verifyCert:forHost:completionHandler:) bounces reference count twice: - when creating a copy from reference (otherwise block will receive a dangling dereference to |cert|) - when block captures |cert| by copying it. .Pass() inside the block avoids third bouncing of reference count. https://codereview.chromium.org/1230033005/diff/180001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:1288: completionHandler(NSURLSessionAuthChallengePerformDefaultHandling, nil); On 2015/08/20 01:25:27, David Benjamin (slow) wrote: > On 2015/08/20 01:14:41, eugenebut wrote: > > On 2015/08/19 18:51:46, David Benjamin (slow) wrote: > > > I realize I'm the one who asked about whether Reject should have been > changed > > to > > > Default, but did you come to a conclusion as to what > > > NSURLSessionAuthChallengeRejectProtectionSpace vs. > > > NSURLSessionAuthChallengePerformDefaultHandling was? The API docs say: > > > > > > "If you do not implement this method, the web view will respond to the > > > authentication challenge with the > > NSURLSessionAuthChallengeRejectProtectionSpace > > > disposition." > > > > > > which suggests to me that NSURLSessionAuthChallengeRejectProtectionSpace > when > > > you have no clue what this protection space is and perhaps > > > NSURLSessionAuthChallengePerformDefaultHandling is what you do get the > > > default??? Except what the difference there could possibly mean, I haven't > the > > > foggiest idea. > > > > > > This API is weird. And the documentation seems to be basically crap. :-/ It > > all > > > references a *different* API with a slightly different shape. > > The documentation for NSURLSessionAuthChallengePerformDefaultHandling says: > > "Default handling for the challenge - as if this delegate were not > implemented; > > the credential parameter is ignored.". > > However this doc belongs to NSURLSession.h, so I probably should switch back > to > > NSURLSessionAuthChallengeRejectProtectionSpace, and trust WKWebView comments. > > > > P.S. I tested both constants with different certs and did not see any > > difference. > > Mmm. I'll defer to you here because I have no clue what in the world this API is > doing. :-) It would be good to get a definitive answer from Apple though. I will ask WebKit folks about this and will correct this code if necessary.
As before, LGTM once domain experts are happy with the details. https://codereview.chromium.org/1230033005/diff/220001/ios/web/net/cert_verif... File ios/web/net/cert_verifier_block_adapter.h (right): https://codereview.chromium.org/1230033005/diff/220001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter.h:24: // |CertVerifier| was created. This sounds a bit "any color so long as it's black"; how about "This class must be created and used on the same thread where the CertVerifier was created". https://codereview.chromium.org/1230033005/diff/220001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1230033005/diff/220001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:171: // Cert verification flags. Must be called on IO Thread. s/called/used/ https://codereview.chromium.org/1230033005/diff/220001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:174: // Cert verification object which wraps net::CertVerifier. Must be called on same https://codereview.chromium.org/1230033005/diff/220001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:274: // on completion. |completionHandler| can not be null and will be called cannot
https://codereview.chromium.org/1230033005/diff/180001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1230033005/diff/180001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:144: scoped_ptr<net::CertVerifierBlockAdapter> _certVerifier; On 2015/08/20 01:14:41, eugenebut wrote: > On 2015/08/19 18:51:46, David Benjamin (slow) wrote: > > Unfortunately, the lifetimes for URLRequestContextGetter is kind of stupid. > Just > > because the URLRequestContextGetter is alive, doesn't mean the > URLRequestContext > > is alive, depending on what the getter does. (The getter has effectively > > lifetime infinity due to ref-counting.) At some point I need to sit down with > it > > and try to fix this. It's complicated due to there being separate threads and > > junk. > > > > The important question here is how does shutdown work? You access it on an UI > -> > > IO PostTask. Can you be sure that the underlying URLRequestContext is, in turn > > destroyed on a UI -> IO PostTask of some object (like Profile or something)? > If > > yes, then this should be safe. If no, we have a problem. > > > > Given that iOS is itself reference counted, I'm actually not sure what kind of > > lifetime assumptions you get to make. It'll depend a lot on what your > > URLRequestContextGetter implementation is. URLRequestContextGetter does have > an > > observer interface for finding out when the context is gone, but that's mostly > > the "I give up, lifetimes are too hard and I'll write really fragile code > > instead" option. > Both URLRequestContextGetter and URLRequestContext are owned by > web::BrowserState. web::BrowserState always outlives WebController (this class). > WebController will not access URLRequestContextGetter if it is being destroyed > ([self isBeingDestroyed]). web::BrowserState for incognito profile is destroyed > after the last Tab is closed and at that moment all WebControllers are already > deallocated. > > Stuart, please correct me if I'm wrong. Sorry, missed this. This is a complicated question, and it depends on what some of the words mean: > web::BrowserState always outlives WebController If this means "BrowserState's destructor is always called after WebController's dealloc", this is not a statement we can make. WC is reference-counted, and could be referenced by who-knows-what. This is why we have the explicit -close method. If you replace 'dealloc' with 'close' in that sentence, that should be true. > web::BrowserState for incognito profile is destroyed after the last Tab is closed True. > and at that moment all WebControllers are already deallocated. deallocated: We can never really make claims about deallocation order of ObjC objects closed: True. Since you're kicking off the _certVerifier removal in -close, we can make claims about how that post will be ordered relative to other things.
https://codereview.chromium.org/1230033005/diff/180001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1230033005/diff/180001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:144: scoped_ptr<net::CertVerifierBlockAdapter> _certVerifier; On 2015/08/20 20:57:43, stuartmorgan wrote: > On 2015/08/20 01:14:41, eugenebut wrote: > > On 2015/08/19 18:51:46, David Benjamin (slow) wrote: > > > Unfortunately, the lifetimes for URLRequestContextGetter is kind of stupid. > > Just > > > because the URLRequestContextGetter is alive, doesn't mean the > > URLRequestContext > > > is alive, depending on what the getter does. (The getter has effectively > > > lifetime infinity due to ref-counting.) At some point I need to sit down > with > > it > > > and try to fix this. It's complicated due to there being separate threads > and > > > junk. > > > > > > The important question here is how does shutdown work? You access it on an > UI > > -> > > > IO PostTask. Can you be sure that the underlying URLRequestContext is, in > turn > > > destroyed on a UI -> IO PostTask of some object (like Profile or something)? > > If > > > yes, then this should be safe. If no, we have a problem. > > > > > > Given that iOS is itself reference counted, I'm actually not sure what kind > of > > > lifetime assumptions you get to make. It'll depend a lot on what your > > > URLRequestContextGetter implementation is. URLRequestContextGetter does have > > an > > > observer interface for finding out when the context is gone, but that's > mostly > > > the "I give up, lifetimes are too hard and I'll write really fragile code > > > instead" option. > > Both URLRequestContextGetter and URLRequestContext are owned by > > web::BrowserState. web::BrowserState always outlives WebController (this > class). > > WebController will not access URLRequestContextGetter if it is being destroyed > > ([self isBeingDestroyed]). web::BrowserState for incognito profile is > destroyed > > after the last Tab is closed and at that moment all WebControllers are already > > deallocated. > > > > Stuart, please correct me if I'm wrong. > > Sorry, missed this. This is a complicated question, and it depends on what some > of the words mean: > > web::BrowserState always outlives WebController > If this means "BrowserState's destructor is always called after WebController's > dealloc", this is not a statement we can make. WC is reference-counted, and > could be referenced by who-knows-what. This is why we have the explicit -close > method. If you replace 'dealloc' with 'close' in that sentence, that should be > true. > > > web::BrowserState for incognito profile is destroyed after the last Tab is > closed > > True. > > > and at that moment all WebControllers are already deallocated. > > deallocated: We can never really make claims about deallocation order of ObjC > objects > closed: True. > > > Since you're kicking off the _certVerifier removal in -close, we can make claims > about how that post will be ordered relative to other things. Sorry for misleading statements. What I should have said is "BrowserState is not destructed earlier than WebController's -close is called". https://codereview.chromium.org/1230033005/diff/220001/ios/web/net/cert_verif... File ios/web/net/cert_verifier_block_adapter.h (right): https://codereview.chromium.org/1230033005/diff/220001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter.h:24: // |CertVerifier| was created. On 2015/08/20 20:42:20, stuartmorgan wrote: > This sounds a bit "any color so long as it's black"; how about "This class must > be created and used on the same thread where the CertVerifier was created". Done. :) https://codereview.chromium.org/1230033005/diff/220001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1230033005/diff/220001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:171: // Cert verification flags. Must be called on IO Thread. On 2015/08/20 20:42:21, stuartmorgan wrote: > s/called/used/ Done. https://codereview.chromium.org/1230033005/diff/220001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:174: // Cert verification object which wraps net::CertVerifier. Must be called on On 2015/08/20 20:42:21, stuartmorgan wrote: > same Done. https://codereview.chromium.org/1230033005/diff/220001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:274: // on completion. |completionHandler| can not be null and will be called On 2015/08/20 20:42:21, stuartmorgan wrote: > cannot Done.
Last comment, I promise! :-) Otherwise looks good. Sorry, I really should have noticed that one earlier. https://codereview.chromium.org/1230033005/diff/180001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1230033005/diff/180001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:147: net::URLRequestContextGetter* _contextGetter; On 2015/08/20 03:08:01, eugenebut wrote: > On 2015/08/20 01:25:27, David Benjamin (slow) wrote: > > On 2015/08/20 01:14:41, eugenebut wrote: > > > On 2015/08/19 18:51:46, David Benjamin (slow) wrote: > > > > This should probably be scoped_refptr<....>, no? > > > No, web::BrowserState (ios analogue of content::BrowserContext) returns a > raw > > > pointer: > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/ios/web/public/bro... > > > > > > > Our reference counts are intrusive. You can and often do go from T* to > > scoped_refptr<T>. I think this is rather obnoxious for lots of reasons, but > > those are mostly because reference-counting is obnoxious. Holding a T* without > a > > reference count tends to be rather questionable. > Yeah, but in Chrome for iOS BrowserState::GetRequestContext() returns a raw > pointer, which receiver is not suppose to release. Alright, I will leave that to you then. Although I'll note that this is already not true. URLFetcherBlockAdapter takes a URLRequestContextGetter*, but when passing into URLFetcher, the URLFetcher actually takes a reference to the getter. https://codereview.chromium.org/1230033005/diff/180001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:924: __block scoped_refptr<net::X509Certificate> blockCert = cert; On 2015/08/20 03:08:01, eugenebut wrote: > On 2015/08/20 01:25:27, David Benjamin (slow) wrote: > > On 2015/08/20 01:14:41, eugenebut wrote: > > > On 2015/08/19 18:51:45, David Benjamin (slow) wrote: > > > > This __block means that you and the block both point to the scoped_refptr > > > itself > > > > rather than make a copy, right? > > > > > > > > How does that work when this is a C++ type with a destructor? When > multiple > > > > threads are involved?? If it saves a pointer to the stack rather than > > popping > > > > the object into its own random little Obj-C-ref-counted box, this cannot > > > > possibly work. > > > __block means that |blockCert| variable will be mutable inside the block. > > > Without |__block| modifier |blockCert| will be copied as a const object and > > > calling |blockCert.Pass()| will not be possible. > > > > What does mutable inside the block mean? That's not sufficient to ensure this > is > > safe. Is it: > > > > - blockCert is allocated on the stack and the block gets a pointer to the > stack? > > - This is BAD BAD BAD. Do NOT do this. There is no guarantee that the block > > runs before verifyCert returns. This is a critical security bug. > > > > - blockCert is popped to a thread-unsafe reference-counted blob shared by both > > the caller and the block. > > - This does not work either. The outer reference-count MUST be thread-safe. > > > > - blockCert is popped to a thread-safe reference-counted blob shared by both > the > > caller and the block. > > - This does actually work. It is, however, not clear to me you have gained > > anything in that exercise since we're still bouncing on a reference count. > > > > If you could .Pass() into the block's storage and from there .Pass() to the > > Params, this would be valuable. But I doubt blocks let you do that. > The behavior is: "blockCert is popped to a thread-safe reference-counted blob > shared by both the > caller and the block.". And I can't .Pass() into the block's storage. > > This method (verifyCert:forHost:completionHandler:) bounces reference count > twice: > - when creating a copy from reference (otherwise block will receive a dangling > dereference to |cert|) > - when block captures |cert| by copying it. > > .Pass() inside the block avoids third bouncing of reference count. Gotcha. It's not clear to me that paying a malloc (it's got to allocate a ref-counted pointer-sized bit of storage, right?) to save a reference count bump makes much sense, but whatever. https://codereview.chromium.org/1230033005/diff/230001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1230033005/diff/230001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:1031: if ([self isBeingDestroyed]) { Is this thread-safe? Looks like it just queries something that gets set in the IO thread on close. Are WebThread's PostTasks also ordered? In that case, I expect you can remove this (if you're guaranteed that verifyCert will not be called after close) or do the isBeingDestroyed check before the PostTask rather than after (if you aren't guaranteed this). That would mean that you can schedule a verification up until one UI -> IO PostTask just before close is called. So long as, correspondingly, all the IO thread stuff is destroyed no earlier than one UI -> IO PostTask just after close is called, that works out. (Also looks like completionHandler is called on the wrong thread here.) https://codereview.chromium.org/1230033005/diff/230001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:1388: NSURLCredential *credential))completionHandler { [Deferring to you all about what Default vs Reject means here.]
https://codereview.chromium.org/1230033005/diff/230001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1230033005/diff/230001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:1031: if ([self isBeingDestroyed]) { On 2015/08/21 20:12:34, David Benjamin (slow) wrote: > Are WebThread's PostTasks also ordered? WebThread has exactly the same semantics as BrowserThread.
Per offline discussion, not LGTM. I didn't look closely at the threading, and there are some issues (e.g., _certVerifier is actually nulled out on the UI thread)
On 2015/08/21 21:15:31, stuartmorgan wrote: > Per offline discussion, not LGTM. I didn't look closely at the threading, and > there are some issues (e.g., _certVerifier is actually nulled out on the UI > thread) Oof, yeah. So I think combine my PostTask suggestion with replacing the web::WebThread::DeleteSoon with an explicit PostTask? Alternatively, split the UI -> IO thread gadget into its own thing so that CRWWKWebViewWebController can continue to be entirely single-threaded, rather than half on one thread and half on another. That will probably be easier to reason about and I think match Chromium better.
On 2015/08/21 21:18:24, David Benjamin (slow) wrote: > On 2015/08/21 21:15:31, stuartmorgan wrote: > > Per offline discussion, not LGTM. I didn't look closely at the threading, and > > there are some issues (e.g., _certVerifier is actually nulled out on the UI > > thread) > > Oof, yeah. So I think combine my PostTask suggestion with replacing the > web::WebThread::DeleteSoon with an explicit PostTask? > > Alternatively, split the UI -> IO thread gadget into its own thing so that > CRWWKWebViewWebController can continue to be entirely single-threaded, rather > than half on one thread and half on another. That will probably be easier to > reason about and I think match Chromium better. Yet another thought, since this is for WKWebView: is there actually any need for the CertVerifier to live on the IO thread or even come from the URLRequestContextGetter? I thought the whole complication with WKWebView was that we don't get our own net stack anyway. You should be able to instantiate things on the UI thread; it's just that one thread's set of objects can't cross-polinate with another one's.
On 2015/08/21 21:20:09, David Benjamin (slow) wrote: > On 2015/08/21 21:18:24, David Benjamin (slow) wrote: > > On 2015/08/21 21:15:31, stuartmorgan wrote: > > > Per offline discussion, not LGTM. I didn't look closely at the threading, > and > > > there are some issues (e.g., _certVerifier is actually nulled out on the UI > > > thread) > > > > Oof, yeah. So I think combine my PostTask suggestion with replacing the > > web::WebThread::DeleteSoon with an explicit PostTask? > > > > Alternatively, split the UI -> IO thread gadget into its own thing so that > > CRWWKWebViewWebController can continue to be entirely single-threaded, rather > > than half on one thread and half on another. That will probably be easier to > > reason about and I think match Chromium better. > > Yet another thought, since this is for WKWebView: is there actually any need for > the CertVerifier to live on the IO thread or even come from the > URLRequestContextGetter? I thought the whole complication with WKWebView was > that we don't get our own net stack anyway. You should be able to instantiate > things on the UI thread; it's just that one thread's set of objects can't > cross-polinate with another one's. certVerifyFlags can be read only on IO thread anyways, so there is no value in using CertVerifier on the main thread.
On 2015/08/21 21:25:01, eugenebut wrote: > On 2015/08/21 21:20:09, David Benjamin (slow) wrote: > > On 2015/08/21 21:18:24, David Benjamin (slow) wrote: > > > On 2015/08/21 21:15:31, stuartmorgan wrote: > > > > Per offline discussion, not LGTM. I didn't look closely at the threading, > > and > > > > there are some issues (e.g., _certVerifier is actually nulled out on the > UI > > > > thread) > > > > > > Oof, yeah. So I think combine my PostTask suggestion with replacing the > > > web::WebThread::DeleteSoon with an explicit PostTask? > > > > > > Alternatively, split the UI -> IO thread gadget into its own thing so that > > > CRWWKWebViewWebController can continue to be entirely single-threaded, > rather > > > than half on one thread and half on another. That will probably be easier to > > > reason about and I think match Chromium better. > > > > Yet another thought, since this is for WKWebView: is there actually any need > for > > the CertVerifier to live on the IO thread or even come from the > > URLRequestContextGetter? I thought the whole complication with WKWebView was > > that we don't get our own net stack anyway. You should be able to instantiate > > things on the UI thread; it's just that one thread's set of objects can't > > cross-polinate with another one's. > certVerifyFlags can be read only on IO thread anyways, so there is no value in > using CertVerifier on the main thread. Sure, but that's only because you're getting it out of an IO-thread URLRequestContext and whatnot. Given that you're not actually using //net with WKWebView, just tiny pieces of it, it's not clear to me it makes much sense to pull in URLRequestContext and the whole IO thread baggage here. (I don't know the history of Chromium's IO thread, but I suspect one of the motivations for it even existing is due to deadlock complications with sync IPCs and windowed NPAPI plugins on Windows. This is something that is insanely far from applying to a WKWebView Chrome.) //net itself hasn't the slightest clue what an IO thread is. It just want individual piles of objects to be single-threaded. It doesn't even care if you instantiate two sets of unconnected objects on different threads so long as each set is only used on its corresponding thread. (Note: we probably have bugs around doing this here and there, but those are bugs.)
On 2015/08/21 21:34:46, David Benjamin (slow) wrote: > Sure, but that's only because you're getting it out of an IO-thread > URLRequestContext and whatnot. Given that you're not actually using //net with > WKWebView [...] Non-webview-originated requests (e.g., URLFetcher) will use //net in the usual way, so we don't want to start ignoring conventions.
On 2015/08/21 22:46:13, stuartmorgan wrote: > On 2015/08/21 21:34:46, David Benjamin (slow) wrote: > > Sure, but that's only because you're getting it out of an IO-thread > > URLRequestContext and whatnot. Given that you're not actually using //net with > > WKWebView [...] > > Non-webview-originated requests (e.g., URLFetcher) will use //net in the usual > way, so we don't want to start ignoring conventions. Also shared net::CertVerifier object is used for omnibox suggestions, translate, and other web services verification. Initially this CL created net::CertVerifier on stack and when I changed that to use shared net::CertVerifier, Ryan was very supportive for the change. Threading is messed up here in 2 places (Adapter destruction and calling -isBeignDestroyed) I will fix both in the next patch.
Fixed threading bugs. PTAL https://codereview.chromium.org/1230033005/diff/180001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1230033005/diff/180001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:924: __block scoped_refptr<net::X509Certificate> blockCert = cert; On 2015/08/21 20:12:33, David Benjamin (slow) wrote: > On 2015/08/20 03:08:01, eugenebut wrote: > > On 2015/08/20 01:25:27, David Benjamin (slow) wrote: > > > On 2015/08/20 01:14:41, eugenebut wrote: > > > > On 2015/08/19 18:51:45, David Benjamin (slow) wrote: > > > > > This __block means that you and the block both point to the > scoped_refptr > > > > itself > > > > > rather than make a copy, right? > > > > > > > > > > How does that work when this is a C++ type with a destructor? When > > multiple > > > > > threads are involved?? If it saves a pointer to the stack rather than > > > popping > > > > > the object into its own random little Obj-C-ref-counted box, this cannot > > > > > possibly work. > > > > __block means that |blockCert| variable will be mutable inside the block. > > > > Without |__block| modifier |blockCert| will be copied as a const object > and > > > > calling |blockCert.Pass()| will not be possible. > > > > > > What does mutable inside the block mean? That's not sufficient to ensure > this > > is > > > safe. Is it: > > > > > > - blockCert is allocated on the stack and the block gets a pointer to the > > stack? > > > - This is BAD BAD BAD. Do NOT do this. There is no guarantee that the > block > > > runs before verifyCert returns. This is a critical security bug. > > > > > > - blockCert is popped to a thread-unsafe reference-counted blob shared by > both > > > the caller and the block. > > > - This does not work either. The outer reference-count MUST be > thread-safe. > > > > > > - blockCert is popped to a thread-safe reference-counted blob shared by both > > the > > > caller and the block. > > > - This does actually work. It is, however, not clear to me you have gained > > > anything in that exercise since we're still bouncing on a reference count. > > > > > > If you could .Pass() into the block's storage and from there .Pass() to the > > > Params, this would be valuable. But I doubt blocks let you do that. > > The behavior is: "blockCert is popped to a thread-safe reference-counted blob > > shared by both the > > caller and the block.". And I can't .Pass() into the block's storage. > > > > This method (verifyCert:forHost:completionHandler:) bounces reference count > > twice: > > - when creating a copy from reference (otherwise block will receive a dangling > > dereference to |cert|) > > - when block captures |cert| by copying it. > > > > .Pass() inside the block avoids third bouncing of reference count. > > Gotcha. It's not clear to me that paying a malloc (it's got to allocate a > ref-counted pointer-sized bit of storage, right?) to save a reference count bump > makes much sense, but whatever. Copying object here is necessary, because block can not capture references in a safe manner. To use |cert| inside the block it is should be either copied on stack or passed to verifyCert:forHost:completionHandler: by value (which is also a copy). https://codereview.chromium.org/1230033005/diff/230001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1230033005/diff/230001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:1388: NSURLCredential *credential))completionHandler { On 2015/08/21 20:12:34, David Benjamin (slow) wrote: > [Deferring to you all about what Default vs Reject means here.] Still waiting for the answer from WebKit folks.
https://codereview.chromium.org/1230033005/diff/250001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1230033005/diff/250001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:368: [[self retain] autorelease]; I don't see how this does what the comment says. Did you mean to have this repost to the main thread? https://codereview.chromium.org/1230033005/diff/250001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:643: net::URLRequestContext* context = _contextGetter->GetURLRequestContext(); Since there was a fair amount of discussion, it's probably worth commenting why this is safe, from an ordering perspective.
https://codereview.chromium.org/1230033005/diff/250001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1230033005/diff/250001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:1041: completionHandler(net::CertVerifyResult(), net::ERR_FAILED); This needs a dispatch_async, etc., right?
PTAL https://codereview.chromium.org/1230033005/diff/250001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1230033005/diff/250001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:368: [[self retain] autorelease]; On 2015/08/24 17:14:19, stuartmorgan wrote: > I don't see how this does what the comment says. Did you mean to have this > repost to the main thread? This block will be destroyed on IO thread, so in case if self is only retained by the block then [self dealloc] will happen on IO thread. Autorelease pool is always drained on the main thread. Updated the comments. https://codereview.chromium.org/1230033005/diff/250001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:643: net::URLRequestContext* context = _contextGetter->GetURLRequestContext(); On 2015/08/24 17:14:19, stuartmorgan wrote: > Since there was a fair amount of discussion, it's probably worth commenting why > this is safe, from an ordering perspective. Done. https://codereview.chromium.org/1230033005/diff/250001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:1041: completionHandler(net::CertVerifyResult(), net::ERR_FAILED); On 2015/08/24 17:43:31, David Benjamin wrote: > This needs a dispatch_async, etc., right? D'oh. Thanks for catching that. I see one concrete scenario where we want to call verifyCert:forHost:completionHandler: completionHandler on IO thread: Cert is invalid and request is main document request. In this case we want to check CertPolicyCache for user decision, which can be done only on IO thread. Because of that I will always call completionHandler on IO thread and clients bounce to main thread only if necessary.
https://codereview.chromium.org/1230033005/diff/290001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1230033005/diff/290001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:280: // asynchronously on IO thread. This is a very strange API. Having functions which get called on the UI thread respond on the IO thread seems... bizarre. If there's other IO work to be done like checking a CertPolicyCache, perhaps that should happen inside verifyCert? Or does that layering not work? https://codereview.chromium.org/1230033005/diff/290001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:369: // thread. Autorelease pool is always drained on the main thread and will Nit: Autorelease pool -> The autorelease pool https://codereview.chromium.org/1230033005/diff/290001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:370: // bounce self deallocation to the main thread. Hrm. That seems kind of weird and not consistent with http://stackoverflow.com/questions/12575010/using-arc-is-it-fatal-not-to-have..., but I know nothing about ObjC and will defer to you and Stuart.
https://codereview.chromium.org/1230033005/diff/290001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1230033005/diff/290001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:280: // asynchronously on IO thread. On 2015/08/24 19:21:36, David Benjamin wrote: > This is a very strange API. Having functions which get called on the UI thread > respond on the IO thread seems... bizarre. > > If there's other IO work to be done like checking a CertPolicyCache, perhaps > that should happen inside verifyCert? Or does that layering not work? This API will also be used for determining icon color/status (where using CertPolicyCache is unnecessary) and making load / no load decision. After I implement everything (interstitials, lock coloring, lock popover) I plan to move all cert verification stuff to a separate class and make verifyCert:forHost:completionHandler: private. So the fact that this API will call completionHandler on IO thread will be encapsulated. Do you think it would be an acceptable tradeoff? https://codereview.chromium.org/1230033005/diff/290001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:369: // thread. Autorelease pool is always drained on the main thread and will On 2015/08/24 19:21:37, David Benjamin wrote: > Nit: Autorelease pool -> The autorelease pool Done.
https://codereview.chromium.org/1230033005/diff/250001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1230033005/diff/250001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:368: [[self retain] autorelease]; On 2015/08/24 18:56:50, eugenebut wrote: > Autorelease pool is always drained on the main thread. I'm 99.9% sure this is not true; that would mean all Cocoa objects had to be thread safe.
https://codereview.chromium.org/1230033005/diff/250001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1230033005/diff/250001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:368: [[self retain] autorelease]; On 2015/08/24 19:55:24, stuartmorgan wrote: > On 2015/08/24 18:56:50, eugenebut wrote: > > Autorelease pool is always drained on the main thread. > > I'm 99.9% sure this is not true; that would mean all Cocoa objects had to be > thread safe. I think |autorelease| is type-agnostic. Anyhow it is safer to not use NSAutoreleasePool, so Done.
lgtm https://codereview.chromium.org/1230033005/diff/330001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1230033005/diff/330001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:649: // _contextGetter is alive at least until -close is called. DCHECK(_certVerifier) so that the requirement that close hasn't been called is checked?
The CQ bit was checked by eugenebut@chromium.org to run a CQ dry run
https://codereview.chromium.org/1230033005/diff/330001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1230033005/diff/330001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:649: // _contextGetter is alive at least until -close is called. On 2015/08/24 20:34:24, stuartmorgan wrote: > DCHECK(_certVerifier) so that the requirement that close hasn't been called is > checked? Sure.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1230033005/350001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1230033005/350001
https://codereview.chromium.org/1230033005/diff/290001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1230033005/diff/290001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:280: // asynchronously on IO thread. On 2015/08/24 19:51:28, eugenebut wrote: > On 2015/08/24 19:21:36, David Benjamin wrote: > > This is a very strange API. Having functions which get called on the UI thread > > respond on the IO thread seems... bizarre. > > > > If there's other IO work to be done like checking a CertPolicyCache, perhaps > > that should happen inside verifyCert? Or does that layering not work? > This API will also be used for determining icon color/status (where using > CertPolicyCache is unnecessary) and making load / no load decision. After I > implement everything (interstitials, lock coloring, lock popover) I plan to move > all cert verification stuff to a separate class and make > verifyCert:forHost:completionHandler: private. So the fact that this API will > call completionHandler on IO thread will be encapsulated. > > Do you think it would be an acceptable tradeoff? It's hard for me to tell without a sense of what you're doing with this code. Note, by the way, that that this function on its own would easily result in deleting self on the IO thread. See my other comment for how. If all the cert verification stuff is being split to a separate class, can the thread-hopping not move in there too? And anything involving CertPolicyCache and whatever else also be in there. Then CRWWKWebViewWebController can stay entirely on the UI thread and not risk being deleted on IO. That class, of course, would still need to ensure that any UI completionHandlers passed into it are deleted on UI and whatnot. The best way to do that is to have your logic be implemented all in one thread (if CertPolicyCache is also an IO thread object, it sounds like IO is that thread). Then have a separate object which only proxies things. Of course, this is much easier with scoped_ptr and WeakPtr rather than reference-counting. https://codereview.chromium.org/1230033005/diff/330001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1230033005/diff/330001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:372: }); I don't think this works. The block itself retains a reference. There's no guarantee that the function will return before the UI thread runs [self release] and returns from its block. There's a similar kind of problem with the verifyCert code below. I think the only way to avoid deleting this on the wrong thread is to ensure no other threads ever hold a reference to it. That means getting all the IO stuff out of this class and into something smaller that can be deleted anywhere because everything is so pervasively reference-counted. This sort of mess is why Chromium tries not to have objects span threads so much. And why we avoid reference-counting and use base::WeakPtr for everything. This way you can easily ensure that stray references don't cause your objects to be deleted on weird threads.
https://codereview.chromium.org/1230033005/diff/330001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1230033005/diff/330001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:372: }); On 2015/08/24 21:23:02, David Benjamin wrote: > I don't think this works. The block itself retains a reference. There's no > guarantee that the function will return before the UI thread runs [self release] > and returns from its block. Ugh, right. Someday I hope to actually learn to reason correctly about threads :P It may even be slightly worse, because I'm not sure there's any guarantee that the block is cleaned up immediately, vs. deferred slightly.
https://codereview.chromium.org/1230033005/diff/330001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1230033005/diff/330001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:372: }); On 2015/08/24 21:29:43, stuartmorgan wrote: > On 2015/08/24 21:23:02, David Benjamin wrote: > > I don't think this works. The block itself retains a reference. There's no > > guarantee that the function will return before the UI thread runs [self > release] > > and returns from its block. > > Ugh, right. Someday I hope to actually learn to reason correctly about threads > :P And that's why we //net folks are so allergic to reference-counting and multi-threaded objects! :-) How does the iOS code typically deal with threads? Reference-counting is slightly harder to avoid in ObjC. The only thing I can think of is to eschew all ObjC at thread-boundaries and write scoped_ptr- and WeakPtr-style code.
https://codereview.chromium.org/1230033005/diff/330001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1230033005/diff/330001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:372: }); On 2015/08/24 21:33:42, David Benjamin wrote: > How does the iOS code typically deal with threads? Reference-counting is > slightly harder to avoid in ObjC. A common pattern for us is explicit teardowns that do thread-specific work, called from a known thread, so that the dealloc thread doesn't matter. For instance, that's exactly why we have -close in WebController in the first place. There's a certain amount of finding things that should be there the hard way, unfortunately.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
David, I moved all threading to a separate class (CRWCertVerificationController). If you ok with this change i will write unit tests for it. Thank you for your patience, PTAL. https://codereview.chromium.org/1230033005/diff/390001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.mm (right): https://codereview.chromium.org/1230033005/diff/390001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:75: } else if (net::IsCertStatusError(result.cert_status)) { David, could you please confirm that the logic is correct here.
https://codereview.chromium.org/1230033005/diff/390001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.mm (right): https://codereview.chromium.org/1230033005/diff/390001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:75: } else if (net::IsCertStatusError(result.cert_status)) { On 2015/08/25 18:05:54, eugenebut wrote: > David, could you please confirm that the logic is correct here. I would maybe UNKNOWN -> ERROR to make it clear this is an error state (i.e. the verifier hit some kind of fatal error internally), but otherwise I think this is correct. This is Ryan's code though, so he'd know more canonically. https://codereview.chromium.org/1230033005/diff/390001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:83: handler(policy); Hrm. I think we *still* have the threading problem. :'-( So, handler internally has a reference to the outer completionHandler, right? This IO thread outer block must have a reference to handler so it may pass it off to the main thread inner block. Which means we again can't be sure the IO thread outer block returns before the main thread inner block runs, so the last reference to handler may go away on the IO thread and so far down the line... Although, given how absurdly viral this problem is when blocks get involved, I wonder if it actually doesn't matter where objects get deleted on or something. This basically means that absolutely everything which might end up on another thread bleeds this into all other objects it touches. Does this mean iOS apps just literally never use threads, or is it secretly expected that every object has thread-safe destruction. (I'm not familiar with Obj-C conventions.) I guess completionHandler does avoid taking a reference to self. Does it matter that WKWebView might internally make it take references to who knows what? If this constraint is important, I think what you want is something like... you instead of passing the completionHandlers to the IO thread, you keep them in a UI thread list or so and then pass a WeakPtr over to the IO thread. Then in shutDown, you invalidate all the WeakPtrs (and perhaps fail every pending callback?). But that smells extremely non-idiomatic. I feel like there must be a better way, but I'm probably not qualified to find the right Obj-C way.
https://codereview.chromium.org/1230033005/diff/390001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.mm (right): https://codereview.chromium.org/1230033005/diff/390001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:75: } else if (net::IsCertStatusError(result.cert_status)) { On 2015/08/26 20:19:09, David Benjamin wrote: > On 2015/08/25 18:05:54, eugenebut wrote: > > David, could you please confirm that the logic is correct here. > > I would maybe UNKNOWN -> ERROR to make it clear this is an error state (i.e. the > verifier hit some kind of fatal error internally), but otherwise I think this is > correct. This is Ryan's code though, so he'd know more canonically. Done. https://codereview.chromium.org/1230033005/diff/390001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:83: handler(policy); On 2015/08/26 20:19:09, David Benjamin wrote: > Hrm. I think we *still* have the threading problem. :'-( > > So, handler internally has a reference to the outer completionHandler, right? > This IO thread outer block must have a reference to handler so it may pass it > off to the main thread inner block. Which means we again can't be sure the IO > thread outer block returns before the main thread inner block runs, so the last > reference to handler may go away on the IO thread and so far down the line... > > Although, given how absurdly viral this problem is when blocks get involved, I > wonder if it actually doesn't matter where objects get deleted on or something. > This basically means that absolutely everything which might end up on another > thread bleeds this into all other objects it touches. Does this mean iOS apps > just literally never use threads, or is it secretly expected that every object > has thread-safe destruction. (I'm not familiar with Obj-C conventions.) > > I guess completionHandler does avoid taking a reference to self. Does it matter > that WKWebView might internally make it take references to who knows what? > > > If this constraint is important, I think what you want is something like... you > instead of passing the completionHandlers to the IO thread, you keep them in a > UI thread list or so and then pass a WeakPtr over to the IO thread. Then in > shutDown, you invalidate all the WeakPtrs (and perhaps fail every pending > callback?). But that smells extremely non-idiomatic. I feel like there must be a > better way, but I'm probably not qualified to find the right Obj-C way. You are right, this very block (Line 83) retains, |handler| on IO thread and |handler| indirectly retains WebController on UI thread. However this block will release |handler| on the UI thread, because the dispatch is performed on UI thread. (I also checked that in practice). Please note that outer block (Line 72) does not retain |handler|. Answering your questions: UIKit classes are not generally thread-safe and I would avoid destructing them on background threads. None of the blocks inside this method retain self. WKWebView may potentially retain some references (like WebController), however I will not lead to retain cycle, because WKWebView released in WebController's close (not dealloc). In any case this code should also wait for Stuart's review when he's back from vacation (next Tuesday).
Ryan, could you please take a look at this code, which makes load/no-load decision for request with cert: https://codereview.chromium.org/1230033005/diff/390001/ios/web/net/crw_cert_v...
https://codereview.chromium.org/1230033005/diff/390001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.mm (right): https://codereview.chromium.org/1230033005/diff/390001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:83: handler(policy); On 2015/08/26 21:12:48, eugenebut wrote: > On 2015/08/26 20:19:09, David Benjamin wrote: > > Hrm. I think we *still* have the threading problem. :'-( > > > > So, handler internally has a reference to the outer completionHandler, right? > > This IO thread outer block must have a reference to handler so it may pass it > > off to the main thread inner block. Which means we again can't be sure the IO > > thread outer block returns before the main thread inner block runs, so the > last > > reference to handler may go away on the IO thread and so far down the line... > > > > Although, given how absurdly viral this problem is when blocks get involved, I > > wonder if it actually doesn't matter where objects get deleted on or > something. > > This basically means that absolutely everything which might end up on another > > thread bleeds this into all other objects it touches. Does this mean iOS apps > > just literally never use threads, or is it secretly expected that every object > > has thread-safe destruction. (I'm not familiar with Obj-C conventions.) > > > > I guess completionHandler does avoid taking a reference to self. Does it > matter > > that WKWebView might internally make it take references to who knows what? > > > > > > If this constraint is important, I think what you want is something like... > you > > instead of passing the completionHandlers to the IO thread, you keep them in a > > UI thread list or so and then pass a WeakPtr over to the IO thread. Then in > > shutDown, you invalidate all the WeakPtrs (and perhaps fail every pending > > callback?). But that smells extremely non-idiomatic. I feel like there must be > a > > better way, but I'm probably not qualified to find the right Obj-C way. > > You are right, this very block (Line 83) retains, |handler| on IO thread > and |handler| indirectly retains WebController on UI thread. > > However this block will release |handler| on the UI thread, because the dispatch > is performed on UI thread. > (I also checked that in practice). Please note that outer block (Line 72) does > not retain |handler|. The outer block must retain |handler| so that it may pass it into the inner block when that gets created, no? Perhaps Obj-C is clever enough to do the equivalent of .Pass(), but otherwise both inner and outer would have a reference I would think. > Answering your questions: > UIKit classes are not generally thread-safe and I would avoid destructing them > on background threads. > None of the blocks inside this method retain self. > WKWebView may potentially retain some references (like WebController), however I > will not lead to retain cycle, because WKWebView released in WebController's > close (not dealloc). > > > In any case this code should also wait for Stuart's review when he's back from > vacation (next Tuesday).
https://codereview.chromium.org/1230033005/diff/390001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.mm (right): https://codereview.chromium.org/1230033005/diff/390001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:83: handler(policy); On 2015/08/26 21:20:46, David Benjamin wrote: > On 2015/08/26 21:12:48, eugenebut wrote: > > On 2015/08/26 20:19:09, David Benjamin wrote: > > > Hrm. I think we *still* have the threading problem. :'-( > > > > > > So, handler internally has a reference to the outer completionHandler, > right? > > > This IO thread outer block must have a reference to handler so it may pass > it > > > off to the main thread inner block. Which means we again can't be sure the > IO > > > thread outer block returns before the main thread inner block runs, so the > > last > > > reference to handler may go away on the IO thread and so far down the > line... > > > > > > Although, given how absurdly viral this problem is when blocks get involved, > I > > > wonder if it actually doesn't matter where objects get deleted on or > > something. > > > This basically means that absolutely everything which might end up on > another > > > thread bleeds this into all other objects it touches. Does this mean iOS > apps > > > just literally never use threads, or is it secretly expected that every > object > > > has thread-safe destruction. (I'm not familiar with Obj-C conventions.) > > > > > > I guess completionHandler does avoid taking a reference to self. Does it > > matter > > > that WKWebView might internally make it take references to who knows what? > > > > > > > > > If this constraint is important, I think what you want is something like... > > you > > > instead of passing the completionHandlers to the IO thread, you keep them in > a > > > UI thread list or so and then pass a WeakPtr over to the IO thread. Then in > > > shutDown, you invalidate all the WeakPtrs (and perhaps fail every pending > > > callback?). But that smells extremely non-idiomatic. I feel like there must > be > > a > > > better way, but I'm probably not qualified to find the right Obj-C way. > > > > You are right, this very block (Line 83) retains, |handler| on IO thread > > and |handler| indirectly retains WebController on UI thread. > > > > However this block will release |handler| on the UI thread, because the > dispatch > > is performed on UI thread. > > (I also checked that in practice). Please note that outer block (Line 72) does > > not retain |handler|. > > The outer block must retain |handler| so that it may pass it into the inner > block when that gets created, no? Perhaps Obj-C is clever enough to do the > equivalent of .Pass(), but otherwise both inner and outer would have a reference > I would think. > > > Answering your questions: > > UIKit classes are not generally thread-safe and I would avoid destructing them > > on background threads. > > None of the blocks inside this method retain self. > > WKWebView may potentially retain some references (like WebController), however > I > > will not lead to retain cycle, because WKWebView released in WebController's > > close (not dealloc). > > > > > > In any case this code should also wait for Stuart's review when he's back from > > vacation (next Tuesday). > Oh, right, I reproduced the case you described and handler was indeed deallocated on IO thread. So: 1.) |handler| can be released on IO thread 2.) handler itself does not retain web controller (and will not), but it retains WKWebView's completion handler which may be a problem I will take a look what can be done here.
David, I apologize for such a long review. PTAL, I addressed the latest threading concern. https://codereview.chromium.org/1230033005/diff/390001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.mm (right): https://codereview.chromium.org/1230033005/diff/390001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:83: handler(policy); On 2015/08/26 21:56:29, eugenebut wrote: > On 2015/08/26 21:20:46, David Benjamin wrote: > > On 2015/08/26 21:12:48, eugenebut wrote: > > > On 2015/08/26 20:19:09, David Benjamin wrote: > > > > Hrm. I think we *still* have the threading problem. :'-( > > > > > > > > So, handler internally has a reference to the outer completionHandler, > > right? > > > > This IO thread outer block must have a reference to handler so it may pass > > it > > > > off to the main thread inner block. Which means we again can't be sure the > > IO > > > > thread outer block returns before the main thread inner block runs, so the > > > last > > > > reference to handler may go away on the IO thread and so far down the > > line... > > > > > > > > Although, given how absurdly viral this problem is when blocks get > involved, > > I > > > > wonder if it actually doesn't matter where objects get deleted on or > > > something. > > > > This basically means that absolutely everything which might end up on > > another > > > > thread bleeds this into all other objects it touches. Does this mean iOS > > apps > > > > just literally never use threads, or is it secretly expected that every > > object > > > > has thread-safe destruction. (I'm not familiar with Obj-C conventions.) > > > > > > > > I guess completionHandler does avoid taking a reference to self. Does it > > > matter > > > > that WKWebView might internally make it take references to who knows what? > > > > > > > > > > > > If this constraint is important, I think what you want is something > like... > > > you > > > > instead of passing the completionHandlers to the IO thread, you keep them > in > > a > > > > UI thread list or so and then pass a WeakPtr over to the IO thread. Then > in > > > > shutDown, you invalidate all the WeakPtrs (and perhaps fail every pending > > > > callback?). But that smells extremely non-idiomatic. I feel like there > must > > be > > > a > > > > better way, but I'm probably not qualified to find the right Obj-C way. > > > > > > You are right, this very block (Line 83) retains, |handler| on IO thread > > > and |handler| indirectly retains WebController on UI thread. > > > > > > However this block will release |handler| on the UI thread, because the > > dispatch > > > is performed on UI thread. > > > (I also checked that in practice). Please note that outer block (Line 72) > does > > > not retain |handler|. > > > > The outer block must retain |handler| so that it may pass it into the inner > > block when that gets created, no? Perhaps Obj-C is clever enough to do the > > equivalent of .Pass(), but otherwise both inner and outer would have a > reference > > I would think. > > > > > Answering your questions: > > > UIKit classes are not generally thread-safe and I would avoid destructing > them > > > on background threads. > > > None of the blocks inside this method retain self. > > > WKWebView may potentially retain some references (like WebController), > however > > I > > > will not lead to retain cycle, because WKWebView released in WebController's > > > close (not dealloc). > > > > > > > > > In any case this code should also wait for Stuart's review when he's back > from > > > vacation (next Tuesday). > > > Oh, right, I reproduced the case you described and handler was indeed > deallocated on IO thread. > > So: > 1.) |handler| can be released on IO thread > 2.) handler itself does not retain web controller (and will not), but it retains > WKWebView's completion handler which may be a problem > > I will take a look what can be done here. > Alright, |handler| is not retained by the blocks anymore and its lifetime is now managed by| BlockHolder|.
At this point I can clearly see that CertVerifierBlockAdapter is a useless abstraction. I created it because I wanted WebController to work with block-based interface (which is convenient for WKNavigation delegate callback), but this CL evolved so WC works with CRWCertVerificationController. CertVerifierBlockAdapter does not add any value, but instead makes memory management more difficult and convoluted. With that I think I want to replace Adapter with a simpler class CertVerificationRequest (which purpose will be bookkeeping various CertVerification params, like verification result, cert, verification request, completionHandler). CertVerificationRequest will communicate back to CRWCertVerificationController using delegation and CRWCertVerificationController will keep track pending CertVerificationRequest objects. David, if you in general happy with threading and security in this CL I would like to make mentioned refactoring in a separate CL, to unblock my other work (like SSL Lock coloring and making load/no-load decision for a cert). Please let me know if you want to see the refactoring in this CL.
On 2015/08/31 14:22:23, eugenebut wrote: > At this point I can clearly see that CertVerifierBlockAdapter is a useless > abstraction. I created it because I wanted WebController to work with > block-based interface (which is convenient for WKNavigation delegate callback), > but this CL evolved so WC works with CRWCertVerificationController. > > CertVerifierBlockAdapter does not add any value, but instead makes memory > management more difficult and convoluted. With that I think I want to replace > Adapter with a simpler class CertVerificationRequest (which purpose will be > bookkeeping various CertVerification params, like verification result, cert, > verification request, completionHandler). CertVerificationRequest will > communicate back to CRWCertVerificationController using delegation and > CRWCertVerificationController will keep track pending CertVerificationRequest > objects. > > David, if you in general happy with threading and security in this CL I would > like to make mentioned refactoring in a separate CL, to unblock my other work > (like SSL Lock coloring and making load/no-load decision for a cert). Please let > me know if you want to see the refactoring in this CL. (Sorry, about the delay, I'll get to this tomorrow. Without having looked at the code yet, if the current iteration works, I'm fine with doing that separately.)
Oh, you missed that we take a reference over on line... Nah, I think this is finally good, modulo the RefCounted vs RefCountedThreadSafe thing. :-) https://codereview.chromium.org/1230033005/diff/430001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.mm (right): https://codereview.chromium.org/1230033005/diff/430001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:18: namespace { https://codereview.chromium.org/1230033005/diff/430001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:19: // This class takes ownerhip of block and releases it on UI thread, even if Nit: ownerhip -> ownership https://codereview.chromium.org/1230033005/diff/430001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:22: class BlockHolder : public base::RefCounted<BlockHolder<T>> { RefCountedThreadSafe https://codereview.chromium.org/1230033005/diff/430001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:25: explicit BlockHolder(T block) : block_([block copy]) { DCHECK(block_); } I probably wouldn't bother templating this since it isn't used outside the one type. https://codereview.chromium.org/1230033005/diff/430001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:28: template <typename... Arguments> Ditto. https://codereview.chromium.org/1230033005/diff/430001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:29: void call(Arguments... Args) { Style: Call? https://codereview.chromium.org/1230033005/diff/430001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:53: T block_; I assume automatic reference-counting doesn't kick in here? I'm really not clear on how ARC works. (Or do we not use ARC? I sort of figured we did given blocks capturing stuff, but maybe that's orthogonal? I really don't know anything about ObjC. :-) ) https://codereview.chromium.org/1230033005/diff/430001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:54: }; } // namespace https://codereview.chromium.org/1230033005/diff/430001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:85: DCHECK(!_certVerifier); // This is not a thread safe check. What's this comment for? https://codereview.chromium.org/1230033005/diff/430001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:108: // IO thread and then bounses back to UI thread. As a result all objects Nit: bounces https://codereview.chromium.org/1230033005/diff/430001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:109: // captured by completionHandler may be realeases on either UI or IO thread. Nit: released https://codereview.chromium.org/1230033005/diff/430001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:152: net::SSLConfigService* SSLConfigService = context->ssl_config_service(); Style: ssl_config_service
Also written a unit test for CRWCertVerificationController. PTAL. https://codereview.chromium.org/1230033005/diff/430001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.mm (right): https://codereview.chromium.org/1230033005/diff/430001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:18: On 2015/09/01 22:30:38, David Benjamin wrote: > namespace { Done. https://codereview.chromium.org/1230033005/diff/430001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:19: // This class takes ownerhip of block and releases it on UI thread, even if On 2015/09/01 22:30:38, David Benjamin wrote: > Nit: ownerhip -> ownership Done. https://codereview.chromium.org/1230033005/diff/430001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:22: class BlockHolder : public base::RefCounted<BlockHolder<T>> { On 2015/09/01 22:30:39, David Benjamin wrote: > RefCountedThreadSafe Done. https://codereview.chromium.org/1230033005/diff/430001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:25: explicit BlockHolder(T block) : block_([block copy]) { DCHECK(block_); } On 2015/09/01 22:30:39, David Benjamin wrote: > I probably wouldn't bother templating this since it isn't used outside the one > type. I will use BlockHolder for another public method of CRWCertVerificationController, that's why I need template. I will refactor this code to remove BlockHolder and CertVerifierBlockAdapter after I have CLs for SSL Lock Coloring and Recoverable Interstitials. https://codereview.chromium.org/1230033005/diff/430001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:28: template <typename... Arguments> On 2015/09/01 22:30:39, David Benjamin wrote: > Ditto. Acknowledged. https://codereview.chromium.org/1230033005/diff/430001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:29: void call(Arguments... Args) { On 2015/09/01 22:30:38, David Benjamin wrote: > Style: Call? Both |Call| and |call| are correct names according to Google C++ Style guide: "You may also use lowercase letters for other very short inlined functions." https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Names https://codereview.chromium.org/1230033005/diff/430001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:53: T block_; On 2015/09/01 22:30:38, David Benjamin wrote: > I assume automatic reference-counting doesn't kick in here? I'm really not clear > on how ARC works. (Or do we not use ARC? I sort of figured we did given blocks > capturing stuff, but maybe that's orthogonal? I really don't know anything about > ObjC. :-) ) Fortunately for this CL we don't support ARC :) https://codereview.chromium.org/1230033005/diff/430001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:54: }; On 2015/09/01 22:30:38, David Benjamin wrote: > } // namespace Done. https://codereview.chromium.org/1230033005/diff/430001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:85: DCHECK(!_certVerifier); // This is not a thread safe check. On 2015/09/01 22:30:39, David Benjamin wrote: > What's this comment for? Removed. https://codereview.chromium.org/1230033005/diff/430001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:108: // IO thread and then bounses back to UI thread. As a result all objects On 2015/09/01 22:30:39, David Benjamin wrote: > Nit: bounces Done. https://codereview.chromium.org/1230033005/diff/430001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:109: // captured by completionHandler may be realeases on either UI or IO thread. On 2015/09/01 22:30:38, David Benjamin wrote: > Nit: released Done. https://codereview.chromium.org/1230033005/diff/430001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:152: net::SSLConfigService* SSLConfigService = context->ssl_config_service(); On 2015/09/01 22:30:38, David Benjamin wrote: > Style: ssl_config_service This is Objective-C method and for that reason should follow Objective-C style guide. And in Objective-C style guide all acronyms are all caps :)
lgtm https://codereview.chromium.org/1230033005/diff/450001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.h (right): https://codereview.chromium.org/1230033005/diff/450001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.h:30: // Caller can present recoverable SSL interstitial and ask used if they want Minor nit; I'd prefer we not refer to specific UI (e.g., interstitial) in lower-level classes, as it tends to rot. https://codereview.chromium.org/1230033005/diff/450001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.mm (right): https://codereview.chromium.org/1230033005/diff/450001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:142: // dealloc to happen on IO thread (which is fine for this class). Couldn't hurt to mention in dealloc itself that it could happen on either thread.
lgtm. That was quite a review. :-) https://codereview.chromium.org/1230033005/diff/430001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.mm (right): https://codereview.chromium.org/1230033005/diff/430001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:152: net::SSLConfigService* SSLConfigService = context->ssl_config_service(); On 2015/09/02 20:17:45, eugenebut wrote: > On 2015/09/01 22:30:38, David Benjamin wrote: > > Style: ssl_config_service > This is Objective-C method and for that reason should follow Objective-C style > guide. > And in Objective-C style guide all acronyms are all caps :) Oh, I see. This is camelCase + first word happens to be an acronym. I would have thought the "first word is lowercase" rule trumps the "acronyms are uppercase" rule. (In which case, it'd be sslConfigService.) But I don't know ObjC style, so I'll defer to you and Stuart here. https://codereview.chromium.org/1230033005/diff/450001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller_unittest.mm (right): https://codereview.chromium.org/1230033005/diff/450001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller_unittest.mm:21: namespace { Nit: newline here (or remove the newline in line 26)
Huge than you to everyone! And I really apologies for the number of rounds, now I should be more careful with threading issues. Ryan could you please review this line: https://codereview.chromium.org/1230033005/diff/390001/ios/web/net/crw_cert_v... https://codereview.chromium.org/1230033005/diff/430001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.mm (right): https://codereview.chromium.org/1230033005/diff/430001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:152: net::SSLConfigService* SSLConfigService = context->ssl_config_service(); On 2015/09/03 18:34:27, David Benjamin wrote: > On 2015/09/02 20:17:45, eugenebut wrote: > > On 2015/09/01 22:30:38, David Benjamin wrote: > > > Style: ssl_config_service > > This is Objective-C method and for that reason should follow Objective-C style > > guide. > > And in Objective-C style guide all acronyms are all caps :) > > Oh, I see. This is camelCase + first word happens to be an acronym. I would have > thought the "first word is lowercase" rule trumps the "acronyms are uppercase" > rule. (In which case, it'd be sslConfigService.) But I don't know ObjC style, so > I'll defer to you and Stuart here. Yes, this is not something that is very intuitive. And rationale for starting variable names with all caps is the following: 1.) In method/property name acronyms must always be all caps (Apple frameworks are very consistent with that). 2.) Since property is all caps, then instance variable that backs that property up should be _SSLConfigService (or SSLConfigService). Otherwise Key Value Coding technology will not work. 3.) Finally local variables should have the same style as ivars so they may be started with all caps. In Chromium code you will probably see sslConfigService more often than SSLConfigService. https://codereview.chromium.org/1230033005/diff/450001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.h (right): https://codereview.chromium.org/1230033005/diff/450001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.h:30: // Caller can present recoverable SSL interstitial and ask used if they want On 2015/09/03 18:07:57, stuartmorgan wrote: > Minor nit; I'd prefer we not refer to specific UI (e.g., interstitial) in > lower-level classes, as it tends to rot. Done. https://codereview.chromium.org/1230033005/diff/450001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.mm (right): https://codereview.chromium.org/1230033005/diff/450001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:142: // dealloc to happen on IO thread (which is fine for this class). On 2015/09/03 18:07:57, stuartmorgan wrote: > Couldn't hurt to mention in dealloc itself that it could happen on either > thread. Done. https://codereview.chromium.org/1230033005/diff/450001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller_unittest.mm (right): https://codereview.chromium.org/1230033005/diff/450001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller_unittest.mm:21: namespace { On 2015/09/03 18:34:27, David Benjamin wrote: > Nit: newline here (or remove the newline in line 26) Removed the newline in line 26.
https://codereview.chromium.org/1230033005/diff/390001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.mm (right): https://codereview.chromium.org/1230033005/diff/390001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:75: } else if (net::IsCertStatusError(result.cert_status)) { On 2015/08/26 20:19:09, David Benjamin wrote: > On 2015/08/25 18:05:54, eugenebut wrote: > > David, could you please confirm that the logic is correct here. > > I would maybe UNKNOWN -> ERROR to make it clear this is an error state (i.e. the > verifier hit some kind of fatal error internally), but otherwise I think this is > correct. This is Ryan's code though, so he'd know more canonically. Right, I don't think it's correct :) error can be any valid net::error :) if (error == net::OK) { allow } else if (net::IsCertStatusError() && net::IsCertStatusMinorError()) { decide whether you want to allow or reject these, based on the sort of UI surface you can/want to expose. } else { reject }
https://codereview.chromium.org/1230033005/diff/390001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.mm (right): https://codereview.chromium.org/1230033005/diff/390001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:75: } else if (net::IsCertStatusError(result.cert_status)) { On 2015/09/03 21:56:04, Ryan Sleevi wrote: > On 2015/08/26 20:19:09, David Benjamin wrote: > > On 2015/08/25 18:05:54, eugenebut wrote: > > > David, could you please confirm that the logic is correct here. > > > > I would maybe UNKNOWN -> ERROR to make it clear this is an error state (i.e. > the > > verifier hit some kind of fatal error internally), but otherwise I think this > is > > correct. This is Ryan's code though, so he'd know more canonically. > > Right, I don't think it's correct :) error can be any valid net::error :) > > if (error == net::OK) { > allow > } else if (net::IsCertStatusError() && net::IsCertStatusMinorError()) { > decide whether you want to allow or reject these, based on the sort of UI > surface you can/want to expose. > } else { > reject > } Hm... not sure if I understand this... If cert cert has expired then net::IsCertStatusError() returns true, but IsCertStatusMinorError() returns false, in this case we want to show SSL interstitial and ask user's decision. If both net::IsCertStatusError() and IsCertStatusMinorError() return true, I think we want to allow load, w/o showing interstitial. If error != net::OK and net::IsCertStatusError() returns false then CertVerifier has actually failed to verify given cert and we don't even want to show interstitial, but show net error. Please let me know if any of these statements are incorrect.
On 2015/09/03 22:26:38, eugenebut wrote: > If cert cert has expired then net::IsCertStatusError() returns true, but > IsCertStatusMinorError() returns false, > in this case we want to show SSL interstitial and ask user's decision. > > If both net::IsCertStatusError() and IsCertStatusMinorError() return true, I > think we want to allow load, w/o showing interstitial. But what you described here is a tristate - Accept (no errors) - Interstitial - Fail Not every net::IsCertStatusError() results in an interstitial, FWIW. That's handled elsewhere. And some net::IsCertStatusErrors may, depending on other circumstances (e.g. HSTS) result in no interstitial. It sounds like there's definitely room for improving documentation in this code, but as I see it (and check me if I'm wrong), you have - Accept (CERT_ACCEPT_POLICY_ALLOW) - Interstitial (CERT_ACCEPT_POLICY_DENY) - Fail (CERT_ACCEPT_POLICY_UNKNOWN) Is this correct? If so, I think that's confusing, because DENY != FAIL is ... weird :) But I'll hold further comments to make sure we're on the same page there. > > If error != net::OK and net::IsCertStatusError() returns false then CertVerifier > has actually failed to verify given cert and we don't even want to show > interstitial, but show net error. Correct
On 2015/09/03 22:38:11, Ryan Sleevi wrote: > On 2015/09/03 22:26:38, eugenebut wrote: > > If cert cert has expired then net::IsCertStatusError() returns true, but > > IsCertStatusMinorError() returns false, > > in this case we want to show SSL interstitial and ask user's decision. > > > > If both net::IsCertStatusError() and IsCertStatusMinorError() return true, I > > think we want to allow load, w/o showing interstitial. > > But what you described here is a tristate > > - Accept (no errors) > - Interstitial > - Fail > > Not every net::IsCertStatusError() results in an interstitial, FWIW. That's > handled elsewhere. And some net::IsCertStatusErrors may, depending on other > circumstances (e.g. HSTS) result in no interstitial. > > It sounds like there's definitely room for improving documentation in this code, > but as I see it (and check me if I'm wrong), you have > > - Accept (CERT_ACCEPT_POLICY_ALLOW) > - Interstitial (CERT_ACCEPT_POLICY_DENY) > - Fail (CERT_ACCEPT_POLICY_UNKNOWN) > > Is this correct? If so, I think that's confusing, because DENY != FAIL is ... > weird :) > > But I'll hold further comments to make sure we're on the same page there. > > > > > If error != net::OK and net::IsCertStatusError() returns false then > CertVerifier > > has actually failed to verify given cert and we don't even want to show > > interstitial, but show net error. > > Correct Ryan, I'm pretty sure we are on the same page. I've updated enum names to avoid confusion (as I now can clearly see why it was confusing).
LGTM. There's still something wrong with the test, but I don't know if it's worth blocking this CL (I'd like to, but I'm not sure if that'd be reasonable given how long this CL has gone on) From an //ios/web/net perspective, there's a number of things I'd call concerning, but that's mostly due to my own questions regarding the conceptual layering of this. It's not things we'd want in //net, but they might be acceptable for //content or for //chrome, so it really depends on which that directory is trying to reflect. I suspect I'm ultimately not in a good position to determine if this is a good direction for the code, and defer that to stuart-et-al, but as far as *functional*, I think it LGTM. Thanks for working out the (very tricky) threading issues. https://codereview.chromium.org/1230033005/diff/490001/ios/web/net/cert_verif... File ios/web/net/cert_verifier_block_adapter_unittest.cc (right): https://codereview.chromium.org/1230033005/diff/490001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter_unittest.cc:90: scoped_ptr<net::CertVerifier> verifier(net::CertVerifier::CreateDefault()); There's no guarantee this will be async; I just mentioned to another dev that tests shouldn't use CertVerifier::CreateDefault; it can add latencies of up to 10-15 seconds. I don't know what the iOS cost is, but using the real verifier, there's zero API contract that line 98 or line 99 will remain true.
Thank you! https://codereview.chromium.org/1230033005/diff/490001/ios/web/net/cert_verif... File ios/web/net/cert_verifier_block_adapter_unittest.cc (right): https://codereview.chromium.org/1230033005/diff/490001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter_unittest.cc:90: scoped_ptr<net::CertVerifier> verifier(net::CertVerifier::CreateDefault()); On 2015/09/03 23:48:03, Ryan Sleevi wrote: > There's no guarantee this will be async; > > I just mentioned to another dev that tests shouldn't use > CertVerifier::CreateDefault; it can add latencies of up to 10-15 seconds. > I don't know what the iOS cost is, but using the real verifier, there's zero API > contract that line 98 or line 99 will remain true. This test takes 15 ms on Simulator, slower that Mock, but in acceptable range. As for Async concert: would you be OK if I extend net::MockCertVerifier with SetAsync() API? I do realize that there is no guarantee if this call to be async, but it should be unlikely that the code will execute synchronously and |error| will be net::ERR_CERT_AUTHORITY_INVALID.
The CQ bit was checked by eugenebut@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1230033005/490001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1230033005/490001
https://codereview.chromium.org/1230033005/diff/490001/ios/web/net/cert_verif... File ios/web/net/cert_verifier_block_adapter_unittest.cc (right): https://codereview.chromium.org/1230033005/diff/490001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter_unittest.cc:90: scoped_ptr<net::CertVerifier> verifier(net::CertVerifier::CreateDefault()); On 2015/09/03 23:55:37, eugenebut wrote: > As for Async concert: would you be OK if I extend net::MockCertVerifier with > SetAsync() API? Yup; preferably for a follow-up CL :)
https://codereview.chromium.org/1230033005/diff/490001/ios/web/net/cert_verif... File ios/web/net/cert_verifier_block_adapter_unittest.cc (right): https://codereview.chromium.org/1230033005/diff/490001/ios/web/net/cert_verif... ios/web/net/cert_verifier_block_adapter_unittest.cc:90: scoped_ptr<net::CertVerifier> verifier(net::CertVerifier::CreateDefault()); On 2015/09/04 00:01:07, Ryan Sleevi wrote: > On 2015/09/03 23:55:37, eugenebut wrote: > > As for Async concert: would you be OK if I extend net::MockCertVerifier with > > SetAsync() API? > > Yup; preferably for a follow-up CL :) Definitely in a follow up CL and a little bit later (I will file a bug for myself).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by eugenebut@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stuartmorgan@chromium.org, davidben@chromium.org Link to the patchset: https://codereview.chromium.org/1230033005/#ps490001 (title: "Updated enum names")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1230033005/490001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1230033005/490001
Message was sent while issue was closed.
Committed patchset #26 (id:490001)
Message was sent while issue was closed.
Patchset 26 (id:??) landed as https://crrev.com/dc5f11025e1db3b7766bf5faeacc316969b519b2 Cr-Commit-Position: refs/heads/master@{#347302}
Message was sent while issue was closed.
thestig@chromium.org changed reviewers: + thestig@chromium.org
Message was sent while issue was closed.
... and tree's red. Revert time?
Message was sent while issue was closed.
A revert of this CL (patchset #26 id:490001) has been created in https://codereview.chromium.org/1306733006/ by thestig@chromium.org. The reason for reverting is: Compile failed ios/web/net/cert_verifier_block_adapter.cc:28:26: error: no member named 'SOURCE_IOS_WEB_VIEW_CERT_VERIFIER' in 'net::NetLog'.
The CQ bit was checked by eugenebut@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1230033005/530001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1230033005/530001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2015/09/03 23:48:03, Ryan Sleevi wrote:
> From an //ios/web/net perspective, there's a number of things I'd call
> concerning, but that's mostly due to my own questions regarding the conceptual
> layering of this. It's not things we'd want in //net, but they might be
> acceptable for //content or for //chrome, so it really depends on which that
> directory is trying to reflect.
//ios/web ~=~ //content
//ios/web/net ~=~ //content/{browser,common}/net (we don't have a common/browser
distinction on account of our code being in one process)
|
