Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1239543003:
[Cronet] Surface SSL cert error through CronetURLRequestAdapter::OnSSLCertificateError (Closed)
Description[Cronet] Surface SSL cert error through CronetURLRequestAdapter::OnSSLCertificateError
Committed: https://crrev.com/885de3126c2f1569f54134419ed24998f3c7fe91
Cr-Commit-Position: refs/heads/master@{#339455}
Patch Set 1 : #
Total comments: 1
Patch Set 2 : Remove unneeded variable #
Total comments: 3
Patch Set 3 : Added a test #
Total comments: 13
Patch Set 4 : Address comments #
Total comments: 14
Patch Set 5 : Address Matt's comments #
Total comments: 2
Patch Set 6 : Add include #Messages
Total messages: 25 (8 generated)
Patchset #1 (id:1) has been deleted
xunjieli@chromium.org changed reviewers: + mmenke@chromium.org
Delegate::OnResponseStarted is invoked after we called the embedder with the cert error. So to avoid CronetUrlRequest_onError being called twice, I added a boolean. Not sure if it's okay or if there's a better way. Let me know what you think.
https://codereview.chromium.org/1239543003/diff/20001/components/cronet/andro... File components/cronet/android/cronet_url_request_adapter.cc (right): https://codereview.chromium.org/1239543003/diff/20001/components/cronet/andro... components/cronet/android/cronet_url_request_adapter.cc:282: if (has_ssl_cert_error_ || MaybeReportError(request)) Why is this needed? OnSSLCertificateError calls CronetUrlRequest::onError which calls CronetUrlRequest::failWithException, which calls CronetUrlRequest::destroyRequestAdapter. So then when this code calls MaybeReportError, which calls CronetUrlRequest::onError again, it notices the request already fails, and does nothing...Right? That was done to solve a previous problem where we were also double-reporting errors.
https://codereview.chromium.org/1239543003/diff/40001/components/cronet/andro... File components/cronet/android/cronet_url_request_adapter.cc (right): https://codereview.chromium.org/1239543003/diff/40001/components/cronet/andro... components/cronet/android/cronet_url_request_adapter.cc:379: << " on chromium request: " << initial_url_.possibly_invalid_spec(); You are totally right! Sorry I was confused by this log message here that I thought the second error (ERR_ABORTED) was being reported. Since we are reporting error in OnSSLCertificateError as well, should we remove this log message in case more people get confused by it?
Have you thought about how to test this? That's the main reason why I didn't do this myself earlier. https://codereview.chromium.org/1239543003/diff/40001/components/cronet/andro... File components/cronet/android/cronet_url_request_adapter.cc (right): https://codereview.chromium.org/1239543003/diff/40001/components/cronet/andro... components/cronet/android/cronet_url_request_adapter.cc:379: << " on chromium request: " << initial_url_.possibly_invalid_spec(); On 2015/07/15 22:21:14, xunjieli wrote: > You are totally right! Sorry I was confused by this log message here that I > thought the second error (ERR_ABORTED) was being reported. Since we are > reporting error in OnSSLCertificateError as well, should we remove this log > message in case more people get confused by it? I'm fine with removing it. Worth noting I mostly do breakpoint debugging (Don't do much with Android, don't do much with printf/LOG), so others may disagree.
Can we have a mock url request job, and make it trigger URLRequestJob::NotifySSLCertificateError in URLRequestJob::Start? We don't have a real https server, so we can't simulate real cert errors. Is this mock url request job route good? https://codereview.chromium.org/1239543003/diff/40001/components/cronet/andro... File components/cronet/android/cronet_url_request_adapter.cc (right): https://codereview.chromium.org/1239543003/diff/40001/components/cronet/andro... components/cronet/android/cronet_url_request_adapter.cc:379: << " on chromium request: " << initial_url_.possibly_invalid_spec(); On 2015/07/16 14:48:30, mmenke wrote: > On 2015/07/15 22:21:14, xunjieli wrote: > > You are totally right! Sorry I was confused by this log message here that I > > thought the second error (ERR_ABORTED) was being reported. Since we are > > reporting error in OnSSLCertificateError as well, should we remove this log > > message in case more people get confused by it? > > I'm fine with removing it. Worth noting I mostly do breakpoint debugging (Don't > do much with Android, don't do much with printf/LOG), so others may disagree. Hmm.. I guess I will leave it for now, since the java side does not log the error.
mmenke@chromium.org changed reviewers: + davidben@chromium.org
On 2015/07/16 18:36:48, xunjieli wrote: > Can we have a mock url request job, and make it trigger > URLRequestJob::NotifySSLCertificateError in URLRequestJob::Start? We don't have > a real https server, so we can't simulate real cert errors. Is this mock url > request job route good? I don't believe we have any MockURLRequestJob's that return cert errors, but I'm unaware of any reason why we shouldn't just do that. [+davidben]: Any gotchas when mimicking URLRequestJob's failing with cert errors? We really don't care about the specific error, though since the plan is to move away for the current test server, could see such a mock seeing wider use in the future.
On 2015/07/16 18:48:53, mmenke wrote: > On 2015/07/16 18:36:48, xunjieli wrote: > > Can we have a mock url request job, and make it trigger > > URLRequestJob::NotifySSLCertificateError in URLRequestJob::Start? We don't > have > > a real https server, so we can't simulate real cert errors. Is this mock url > > request job route good? > > I don't believe we have any MockURLRequestJob's that return cert errors, but I'm > unaware of any reason why we shouldn't just do that. > > [+davidben]: Any gotchas when mimicking URLRequestJob's failing with cert > errors? We really don't care about the specific error, though since the plan is > to move away for the current test server, could see such a mock seeing wider use > in the future. There's a lot of crazy stuff at the HttpTransaction layer where the error codes corresponding to certificate errors are assumed to come with a working GetResponseInfo()->ssl_info, but the URLRequestJob layer has less of that. The one weird thing I can think of is that notifying OnSSLCertificateError means that ContinueDespiteLastError is supposed to work if the certificate error isn't fatal. Perhaps you could just always re-assert the certificate error all the time or NotifyStartError or something. Or maybe just only ever do fatal ones. The certificate error plumbing is sorta weird and easy to accidentally infinite loop (see that socket pool bug).
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:80001) has been deleted
On 2015/07/16 20:05:38, David Benjamin slow until 7-21 wrote: > On 2015/07/16 18:48:53, mmenke wrote: > > On 2015/07/16 18:36:48, xunjieli wrote: > > > Can we have a mock url request job, and make it trigger > > > URLRequestJob::NotifySSLCertificateError in URLRequestJob::Start? We don't > > have > > > a real https server, so we can't simulate real cert errors. Is this mock url > > > request job route good? > > > > I don't believe we have any MockURLRequestJob's that return cert errors, but > I'm > > unaware of any reason why we shouldn't just do that. > > > > [+davidben]: Any gotchas when mimicking URLRequestJob's failing with cert > > errors? We really don't care about the specific error, though since the plan > is > > to move away for the current test server, could see such a mock seeing wider > use > > in the future. > > There's a lot of crazy stuff at the HttpTransaction layer where the error codes > corresponding to certificate errors are assumed to come with a working > GetResponseInfo()->ssl_info, but the URLRequestJob layer has less of that. > > The one weird thing I can think of is that notifying OnSSLCertificateError means > that ContinueDespiteLastError is supposed to work if the certificate error isn't > fatal. Perhaps you could just always re-assert the certificate error all the > time or NotifyStartError or something. Or maybe just only ever do fatal ones. > The certificate error plumbing is sorta weird and easy to accidentally infinite > loop (see that socket pool bug). Thanks, David! For the purpose of this CL, we just want to add a test to see if a cert error is propagated to the Java side (which happens when URLRequestJob::NotifySSLCertificateError is invoked). Cronet doesn't use ContinueDespiteLastError, I think. Don't really understand other implications. I chose net::ERR_CERT_DATE_INVALID for testing. Mock url request job seems to do the job. PTAL.
Just nits https://codereview.chromium.org/1239543003/diff/100001/components/cronet/andr... File components/cronet/android/cronet_url_request_adapter.cc (right): https://codereview.chromium.org/1239543003/diff/100001/components/cronet/andr... components/cronet/android/cronet_url_request_adapter.cc:290: int ssl_cert_net_error = net::MapCertStatusToNetError(ssl_info.cert_status); optional: May want to just call this net_error (Mostly so that "ConvertUTF8ToJavaString(...).obj()" can all fit on one line). https://codereview.chromium.org/1239543003/diff/100001/components/cronet/andr... File components/cronet/android/test/mock_url_request_job_factory.cc (right): https://codereview.chromium.org/1239543003/diff/100001/components/cronet/andr... components/cronet/android/test/mock_url_request_job_factory.cc:12: #include "ssl_certificate_error_job.h" This should be compontents/cronet/... (Though see other comment about moving this to net/) https://codereview.chromium.org/1239543003/diff/100001/components/cronet/andr... File components/cronet/android/test/ssl_certificate_error_job.cc (right): https://codereview.chromium.org/1239543003/diff/100001/components/cronet/andr... components/cronet/android/test/ssl_certificate_error_job.cc:65: // static "// static" is no longer needed before static methods. It was in the old version of the Google style guide, isn't in the current one. Not worth going back and removing them, just may as well stop doing it in new files. https://codereview.chromium.org/1239543003/diff/100001/components/cronet/andr... components/cronet/android/test/ssl_certificate_error_job.cc:70: SSLCertificateErrorJob::~SSLCertificateErrorJob() { nit: Definition order should match the declaration order in the header. https://codereview.chromium.org/1239543003/diff/100001/components/cronet/andr... File components/cronet/android/test/ssl_certificate_error_job.h (right): https://codereview.chromium.org/1239543003/diff/100001/components/cronet/andr... components/cronet/android/test/ssl_certificate_error_job.h:16: class SSLCertificateErrorJob : public net::URLRequestTestJob { Suggest putting this in net/test/url_request, to make it less likely we end up with another similar class. https://codereview.chromium.org/1239543003/diff/100001/components/cronet/andr... components/cronet/android/test/ssl_certificate_error_job.h:30: ~SSLCertificateErrorJob() override; Can make this private, since there are no subclasses.
Thanks for the feedback! PTAL https://codereview.chromium.org/1239543003/diff/100001/components/cronet/andr... File components/cronet/android/cronet_url_request_adapter.cc (right): https://codereview.chromium.org/1239543003/diff/100001/components/cronet/andr... components/cronet/android/cronet_url_request_adapter.cc:290: int ssl_cert_net_error = net::MapCertStatusToNetError(ssl_info.cert_status); On 2015/07/17 19:29:59, mmenke wrote: > optional: May want to just call this net_error (Mostly so that > "ConvertUTF8ToJavaString(...).obj()" can all fit on one line). Done. https://codereview.chromium.org/1239543003/diff/100001/components/cronet/andr... File components/cronet/android/test/ssl_certificate_error_job.cc (right): https://codereview.chromium.org/1239543003/diff/100001/components/cronet/andr... components/cronet/android/test/ssl_certificate_error_job.cc:65: // static On 2015/07/17 19:29:59, mmenke wrote: > "// static" is no longer needed before static methods. It was in the old > version of the Google style guide, isn't in the current one. > > Not worth going back and removing them, just may as well stop doing it in new > files. Done. https://codereview.chromium.org/1239543003/diff/100001/components/cronet/andr... components/cronet/android/test/ssl_certificate_error_job.cc:70: SSLCertificateErrorJob::~SSLCertificateErrorJob() { On 2015/07/17 19:29:59, mmenke wrote: > nit: Definition order should match the declaration order in the header. Done. https://codereview.chromium.org/1239543003/diff/100001/components/cronet/andr... File components/cronet/android/test/ssl_certificate_error_job.h (right): https://codereview.chromium.org/1239543003/diff/100001/components/cronet/andr... components/cronet/android/test/ssl_certificate_error_job.h:16: class SSLCertificateErrorJob : public net::URLRequestTestJob { On 2015/07/17 19:29:59, mmenke wrote: > Suggest putting this in net/test/url_request, to make it less likely we end up > with another similar class. Done. My initial concern was the use case of this class is pretty narrow. But I guess it doesn't hurt to move this class there. https://codereview.chromium.org/1239543003/diff/100001/components/cronet/andr... components/cronet/android/test/ssl_certificate_error_job.h:30: ~SSLCertificateErrorJob() override; On 2015/07/17 19:29:59, mmenke wrote: > Can make this private, since there are no subclasses. Done.
https://codereview.chromium.org/1239543003/diff/100001/components/cronet/andr... File components/cronet/android/test/ssl_certificate_error_job.h (right): https://codereview.chromium.org/1239543003/diff/100001/components/cronet/andr... components/cronet/android/test/ssl_certificate_error_job.h:16: class SSLCertificateErrorJob : public net::URLRequestTestJob { On 2015/07/17 20:15:21, xunjieli wrote: > On 2015/07/17 19:29:59, mmenke wrote: > > Suggest putting this in net/test/url_request, to make it less likely we end up > > with another similar class. > > Done. My initial concern was the use case of this class is pretty narrow. But I > guess it doesn't hurt to move this class there. There are a bunch of browser tests that need to get cert errors of various sorts, at least some of which don't need real failing servers. Using this class would be one way to get them off the spawned test server. Emily's hooking up a way to mock out the cert verifier, so maybe this won't end up not being needed. We'll see. https://codereview.chromium.org/1239543003/diff/120001/components/cronet/andr... File components/cronet/android/cronet_url_request_adapter.cc (right): https://codereview.chromium.org/1239543003/diff/120001/components/cronet/andr... components/cronet/android/cronet_url_request_adapter.cc:285: void CronetURLRequestAdapter::OnSSLCertificateError( Definition order here should match order in the header. https://codereview.chromium.org/1239543003/diff/120001/components/cronet/andr... File components/cronet/android/cronet_url_request_adapter.h (right): https://codereview.chromium.org/1239543003/diff/120001/components/cronet/andr... components/cronet/android/cronet_url_request_adapter.h:127: bool fatal) override; Order here should match the order they're declared in URLRequest::Delegate. OnSSLCertificateError should go just before OnResponseStarted (That also roughly corresponds to the order they're called in, though OnSSLCertificateError can be before or after redirects) https://codereview.chromium.org/1239543003/diff/120001/net/test/url_request/s... File net/test/url_request/ssl_certificate_error_job.cc (right): https://codereview.chromium.org/1239543003/diff/120001/net/test/url_request/s... net/test/url_request/ssl_certificate_error_job.cc:59: return GURL(base::StringPrintf("https://%s", kMockHostname)); Should probably include <string> https://codereview.chromium.org/1239543003/diff/120001/net/test/url_request/s... net/test/url_request/ssl_certificate_error_job.cc:66: SSLInfo info; Should include whatever header declares SSLInfo. https://codereview.chromium.org/1239543003/diff/120001/net/test/url_request/s... File net/test/url_request/ssl_certificate_error_job.h (right): https://codereview.chromium.org/1239543003/diff/120001/net/test/url_request/s... net/test/url_request/ssl_certificate_error_job.h:16: class SSLCertificateErrorJob : public URLRequestTestJob { Suggest using URLRequestJob instead. URLRequestTestJob is just kinda weird, and I don't think you're using anything from it. https://codereview.chromium.org/1239543003/diff/120001/net/test/url_request/s... net/test/url_request/ssl_certificate_error_job.h:19: NetworkDelegate* network_delegate); Nit: Should forward declare NetworkDelegate and URLRequest. https://codereview.chromium.org/1239543003/diff/120001/net/test/url_request/s... net/test/url_request/ssl_certificate_error_job.h:31: void NotifyError(); nit: Blank line after destructor declaration.
Patchset #5 (id:140001) has been deleted
Thanks for the feedback! https://codereview.chromium.org/1239543003/diff/100001/components/cronet/andr... File components/cronet/android/test/ssl_certificate_error_job.h (right): https://codereview.chromium.org/1239543003/diff/100001/components/cronet/andr... components/cronet/android/test/ssl_certificate_error_job.h:16: class SSLCertificateErrorJob : public net::URLRequestTestJob { On 2015/07/17 21:09:18, mmenke wrote: > On 2015/07/17 20:15:21, xunjieli wrote: > > On 2015/07/17 19:29:59, mmenke wrote: > > > Suggest putting this in net/test/url_request, to make it less likely we end > up > > > with another similar class. > > > > Done. My initial concern was the use case of this class is pretty narrow. But > I > > guess it doesn't hurt to move this class there. > > There are a bunch of browser tests that need to get cert errors of various > sorts, at least some of which don't need real failing servers. Using this class > would be one way to get them off the spawned test server. Emily's hooking up a > way to mock out the cert verifier, so maybe this won't end up not being needed. > We'll see. Acknowledged. I see! Makes sense. Good to know. Thanks! https://codereview.chromium.org/1239543003/diff/120001/components/cronet/andr... File components/cronet/android/cronet_url_request_adapter.cc (right): https://codereview.chromium.org/1239543003/diff/120001/components/cronet/andr... components/cronet/android/cronet_url_request_adapter.cc:285: void CronetURLRequestAdapter::OnSSLCertificateError( On 2015/07/17 21:09:18, mmenke wrote: > Definition order here should match order in the header. Done. https://codereview.chromium.org/1239543003/diff/120001/components/cronet/andr... File components/cronet/android/cronet_url_request_adapter.h (right): https://codereview.chromium.org/1239543003/diff/120001/components/cronet/andr... components/cronet/android/cronet_url_request_adapter.h:127: bool fatal) override; On 2015/07/17 21:09:18, mmenke wrote: > Order here should match the order they're declared in URLRequest::Delegate. > OnSSLCertificateError should go just before OnResponseStarted (That also roughly > corresponds to the order they're called in, though OnSSLCertificateError can be > before or after redirects) Done. https://codereview.chromium.org/1239543003/diff/120001/net/test/url_request/s... File net/test/url_request/ssl_certificate_error_job.cc (right): https://codereview.chromium.org/1239543003/diff/120001/net/test/url_request/s... net/test/url_request/ssl_certificate_error_job.cc:59: return GURL(base::StringPrintf("https://%s", kMockHostname)); On 2015/07/17 21:09:18, mmenke wrote: > Should probably include <string> Done. https://codereview.chromium.org/1239543003/diff/120001/net/test/url_request/s... net/test/url_request/ssl_certificate_error_job.cc:66: SSLInfo info; On 2015/07/17 21:09:18, mmenke wrote: > Should include whatever header declares SSLInfo. Done. https://codereview.chromium.org/1239543003/diff/120001/net/test/url_request/s... File net/test/url_request/ssl_certificate_error_job.h (right): https://codereview.chromium.org/1239543003/diff/120001/net/test/url_request/s... net/test/url_request/ssl_certificate_error_job.h:16: class SSLCertificateErrorJob : public URLRequestTestJob { On 2015/07/17 21:09:18, mmenke wrote: > Suggest using URLRequestJob instead. URLRequestTestJob is just kinda weird, and > I don't think you're using anything from it. Done. https://codereview.chromium.org/1239543003/diff/120001/net/test/url_request/s... net/test/url_request/ssl_certificate_error_job.h:19: NetworkDelegate* network_delegate); On 2015/07/17 21:09:18, mmenke wrote: > Nit: Should forward declare NetworkDelegate and URLRequest. Done. https://codereview.chromium.org/1239543003/diff/120001/net/test/url_request/s... net/test/url_request/ssl_certificate_error_job.h:31: void NotifyError(); On 2015/07/17 21:09:18, mmenke wrote: > nit: Blank line after destructor declaration. Done.
LGTM! https://codereview.chromium.org/1239543003/diff/160001/components/cronet/andr... File components/cronet/android/test/mock_url_request_job_factory.cc (right): https://codereview.chromium.org/1239543003/diff/160001/components/cronet/andr... components/cronet/android/test/mock_url_request_job_factory.cc:43: GURL url(net::SSLCertificateErrorJob::GetMockUrl()); While you're here, this file should include url/gurl.h
Thanks! https://codereview.chromium.org/1239543003/diff/160001/components/cronet/andr... File components/cronet/android/test/mock_url_request_job_factory.cc (right): https://codereview.chromium.org/1239543003/diff/160001/components/cronet/andr... components/cronet/android/test/mock_url_request_job_factory.cc:43: GURL url(net::SSLCertificateErrorJob::GetMockUrl()); On 2015/07/20 15:05:35, mmenke wrote: > While you're here, this file should include url/gurl.h Done.
The CQ bit was checked by xunjieli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/1239543003/#ps180001 (title: "Add include")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1239543003/180001
Message was sent while issue was closed.
Committed patchset #6 (id:180001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/885de3126c2f1569f54134419ed24998f3c7fe91 Cr-Commit-Position: refs/heads/master@{#339455} |
