Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1389213003:
[Cronet] Use Https for Quic Test Server (Closed)
Description[Cronet] Use Https for Quic Test Server
|enable_insecure_quic| is removed in crrev.com/1393713003,
use a self issued cert for the QUIC test server.
BUG=514629
Committed: https://crrev.com/013145fe3f51f959616a01174ccb1fc15737bffc
Cr-Commit-Position: refs/heads/master@{#355186}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Use mock verifier and fix tests #
Total comments: 10
Patch Set 3 : Address Paul's comments #Patch Set 4 : Adopt suggestion #
Total comments: 8
Patch Set 5 : Address Misha's comments #
Total comments: 11
Patch Set 6 : Address Paul's comments #
Total comments: 12
Patch Set 7 : Address Paul's comments #
Total comments: 16
Patch Set 8 : Address Paul's comments #Patch Set 9 : Self review #Patch Set 10 : Rebased and remove dependency on rch@'s CL #Patch Set 11 : Rebased #
Total comments: 4
Patch Set 12 : Address Misha's comments #Messages
Total messages: 75 (13 generated)
Patchset #1 (id:1) has been deleted
xunjieli@chromium.org changed reviewers: + pauljensen@chromium.org
Paul, WDYT? It's pretty messy to set up a local CA cert on the device. Using a preprocessor define isn't good, but at least it will work reliably across different Android versions (although I don't like either solution). PTAL.
Hmm.. This doesn't work. http_network_session_params_.ignore_certificate_errors isn't plumbed through QUIC.
> Hmm.. This doesn't work. > http_network_session_params_.ignore_certificate_errors > isn't plumbed through QUIC. Drats. https://codereview.chromium.org/1389213003/diff/20001/components/cronet/andro... File components/cronet/android/test/assets/test/quic_certs/leaf_cert.pem (right): https://codereview.chromium.org/1389213003/diff/20001/components/cronet/andro... components/cronet/android/test/assets/test/quic_certs/leaf_cert.pem:1: Certificate: I think there may already be lots of certificates checked in, can we just use one of those instead? https://codereview.chromium.org/1389213003/diff/20001/net/url_request/url_req... File net/url_request/url_request_context_builder.h (right): https://codereview.chromium.org/1389213003/diff/20001/net/url_request/url_req... net/url_request/url_request_context_builder.h:176: void set_ignore_certificate_errors(bool ignore_certificate_errors) { This needs a comment
xunjieli@chromium.org changed reviewers: + rch@chromium.org
Ryan: is it possible to use HttpNetworkSession::Param.ignore_certificate_errors in QuicCryptoClientStream::DoVerifyProof to ignore cert errors?
On 2015/10/08 18:56:10, xunjieli wrote: > Ryan: is it possible to use HttpNetworkSession::Param.ignore_certificate_errors > in QuicCryptoClientStream::DoVerifyProof to ignore cert errors? It would probably be easier to use a MockCertVerifier in HttpNetworkSession::Params.cert_verifier. I think that will do what you what?
https://codereview.chromium.org/1389213003/diff/20001/components/cronet/url_r... File components/cronet/url_request_context_config.cc (right): https://codereview.chromium.org/1389213003/diff/20001/components/cronet/url_r... components/cronet/url_request_context_config.cc:85: // TODO(xunjieli): Figure out a reliable way to install a local CA cert. The easiest way to do this for testing is to simply inject a MockCertVerifier.
https://codereview.chromium.org/1389213003/diff/20001/components/cronet/url_r... File components/cronet/url_request_context_config.cc (right): https://codereview.chromium.org/1389213003/diff/20001/components/cronet/url_r... components/cronet/url_request_context_config.cc:85: // TODO(xunjieli): Figure out a reliable way to install a local CA cert. On 2015/10/08 21:29:17, Ryan Hamilton wrote: > The easiest way to do this for testing is to simply inject a MockCertVerifier. This is actually our production code. A block of MockCertVerifier and MockCertVerifier::AddResultForCert code in non-test code doesn't seem very clean, so I am favoring a one line flag. Paul, any thought?
On 2015/10/08 22:21:23, xunjieli wrote: > https://codereview.chromium.org/1389213003/diff/20001/components/cronet/url_r... > File components/cronet/url_request_context_config.cc (right): > > https://codereview.chromium.org/1389213003/diff/20001/components/cronet/url_r... > components/cronet/url_request_context_config.cc:85: // TODO(xunjieli): Figure > out a reliable way to install a local CA cert. > On 2015/10/08 21:29:17, Ryan Hamilton wrote: > > The easiest way to do this for testing is to simply inject a MockCertVerifier. > > This is actually our production code. A block of MockCertVerifier and > MockCertVerifier::AddResultForCert code in non-test code doesn't seem very > clean, so I am favoring a one line flag. Paul, any thought? How 'bout if the production code had an AddCertVerifier(), or some such, method which could be called from the test code to use a MockCertVerifier.
On 2015/10/08 23:00:33, Ryan Hamilton wrote: > On 2015/10/08 22:21:23, xunjieli wrote: > > > https://codereview.chromium.org/1389213003/diff/20001/components/cronet/url_r... > > File components/cronet/url_request_context_config.cc (right): > > > > > https://codereview.chromium.org/1389213003/diff/20001/components/cronet/url_r... > > components/cronet/url_request_context_config.cc:85: // TODO(xunjieli): Figure > > out a reliable way to install a local CA cert. > > On 2015/10/08 21:29:17, Ryan Hamilton wrote: > > > The easiest way to do this for testing is to simply inject a > MockCertVerifier. > > > > This is actually our production code. A block of MockCertVerifier and > > MockCertVerifier::AddResultForCert code in non-test code doesn't seem very > > clean, so I am favoring a one line flag. Paul, any thought? > > How 'bout if the production code had an AddCertVerifier(), or some such, method > which could be called from the test code to use a MockCertVerifier. I guess that's the only way around it. Since our java integration tests do not have a point of entry into Cronet binary other than the UrlRequestContextConfig. I will encode necessary params of MockCertVerifier::AddResultForCert as strings and set it using a package protected method in the java UrlRequestContextConfig. I will get to this tomorrow.
On 2015/10/08 23:26:09, xunjieli wrote: > On 2015/10/08 23:00:33, Ryan Hamilton wrote: > > On 2015/10/08 22:21:23, xunjieli wrote: > > > > > > https://codereview.chromium.org/1389213003/diff/20001/components/cronet/url_r... > > > File components/cronet/url_request_context_config.cc (right): > > > > > > > > > https://codereview.chromium.org/1389213003/diff/20001/components/cronet/url_r... > > > components/cronet/url_request_context_config.cc:85: // TODO(xunjieli): > Figure > > > out a reliable way to install a local CA cert. > > > On 2015/10/08 21:29:17, Ryan Hamilton wrote: > > > > The easiest way to do this for testing is to simply inject a > > MockCertVerifier. > > > > > > This is actually our production code. A block of MockCertVerifier and > > > MockCertVerifier::AddResultForCert code in non-test code doesn't seem very > > > clean, so I am favoring a one line flag. Paul, any thought? > > > > How 'bout if the production code had an AddCertVerifier(), or some such, > method > > which could be called from the test code to use a MockCertVerifier. > > I guess that's the only way around it. Since our java integration tests do not > have a point of entry into Cronet binary other than the UrlRequestContextConfig. > I will encode necessary params of MockCertVerifier::AddResultForCert as strings > and set it using a package protected method in the java UrlRequestContextConfig. > > I will get to this tomorrow. Let me know if I can help out here. Since the MockCertVerifier is not available in "production" code (non-test targets) I think you're going to need some piece of test-code to create the MockCertVerifier and then pass it in to the non-test code.
Patchset #2 (id:40001) has been deleted
Ryan and Paul: PTAL https://codereview.chromium.org/1389213003/diff/20001/components/cronet/andro... File components/cronet/android/test/assets/test/quic_certs/leaf_cert.pem (right): https://codereview.chromium.org/1389213003/diff/20001/components/cronet/andro... components/cronet/android/test/assets/test/quic_certs/leaf_cert.pem:1: Certificate: On 2015/10/08 18:39:25, pauljensen wrote: > I think there may already be lots of certificates checked in, can we just use > one of those instead? Done. https://codereview.chromium.org/1389213003/diff/20001/net/url_request/url_req... File net/url_request/url_request_context_builder.h (right): https://codereview.chromium.org/1389213003/diff/20001/net/url_request/url_req... net/url_request/url_request_context_builder.h:176: void set_ignore_certificate_errors(bool ignore_certificate_errors) { On 2015/10/08 18:39:25, pauljensen wrote: > This needs a comment Done.
This looks like it will work, but I see a lot of #if CRONET_TEST in what looks like otherwise production code. Is that OK? That's not kosher for code in net/ but components/cronet/android can be a different beast, of course.
xunjieli@chromium.org changed reviewers: + mef@chromium.org
xunjieli@chromium.org changed reviewers: + mmenke@chromium.org
On 2015/10/09 20:37:16, Ryan Hamilton wrote: > This looks like it will work, but I see a lot of #if CRONET_TEST in what looks > like otherwise production code. Is that OK? That's not kosher for code in net/ > but components/cronet/android can be a different beast, of course. + Misha, Matt to get their thought I am not sure either. But I don't have a better solution on injecting the mock cert verifier into Cronet, since these are integration tests that use Cronet's public APIs. I agree this is an ugly change, so I am aggressively adding all Cronet owners.
On 2015/10/09 20:52:29, xunjieli wrote: > On 2015/10/09 20:37:16, Ryan Hamilton wrote: > > This looks like it will work, but I see a lot of #if CRONET_TEST in what looks > > like otherwise production code. Is that OK? That's not kosher for code in net/ > > but components/cronet/android can be a different beast, of course. > > + Misha, Matt to get their thought > I am not sure either. But I don't have a better solution on injecting the mock > cert verifier into Cronet, since these are integration tests that use Cronet's > public APIs. I agree this is an ugly change, so I am aggressively adding all > Cronet owners. I would expect that "Add a cert verifier" would be part of he public cronet API, and a test-only method would exist to *create* a mock verifier. Then the test could call that test-only method to create the verifier and hand it to the public set_verifier API. Is this complicated because the cert verifier is a C++ object and the public cronet APIs are java? In any case, I'm super anxious for this CL to land so I can remove insecure QUIC so if this approach is good for y'all I'm happy :>
I'm tending to think a solution involving: 1. piping ignore_certificate_errors down into QUIC, and 2. exposing a package-private CronetEngine.Builder setter for ignore_certificate_errors might be a cleaner solution that doesn't involve any CRONET_TEST macros and doesn't involve pulling MockCertVerifier into production code.
https://codereview.chromium.org/1389213003/diff/60001/build/android/pylib/ins... File build/android/pylib/instrumentation/setup.py (right): https://codereview.chromium.org/1389213003/diff/60001/build/android/pylib/ins... build/android/pylib/instrumentation/setup.py:34: if test_apk in ['ChromeTest', 'ContentShellTest', 'CronetTestInstrumentation']: line wrap https://codereview.chromium.org/1389213003/diff/60001/components/cronet/andro... File components/cronet/android/test/quic_test_server.cc (right): https://codereview.chromium.org/1389213003/diff/60001/components/cronet/andro... components/cronet/android/test/quic_test_server.cc:33: const base::FilePath& key_path) { any particular reason you have a tiny function that's only called once? can we inline it? https://codereview.chromium.org/1389213003/diff/60001/components/cronet/andro... components/cronet/android/test/quic_test_server.cc:56: bool success = base::android::GetExternalStorageDirectory(&directory); can we combine this and the next line into CHECK(base::android::GetExternalStorageDirectory(&directory)); https://codereview.chromium.org/1389213003/diff/60001/components/cronet/url_r... File components/cronet/url_request_context_config.h (right): https://codereview.chromium.org/1389213003/diff/60001/components/cronet/url_r... components/cronet/url_request_context_config.h:85: // A comma delimited certs to ignore for testing. delimited->delimited list of https://codereview.chromium.org/1389213003/diff/60001/components/cronet/url_r... components/cronet/url_request_context_config.h:85: // A comma delimited certs to ignore for testing. ignore->accept
On 2015/10/09 20:57:46, pauljensen wrote: > I'm tending to think a solution involving: > 1. piping ignore_certificate_errors down into QUIC, and This is very much problematic given the way ignore_certificate_errors works. It requires the request to complete and then be retried. But that machinery will cause QUIC to be marked as broken. I spent a good chunk of time talking through this issue with sleevi in the context of testing, and the MockCertVerifier was the approach he most preferred. > 2. exposing a package-private CronetEngine.Builder setter for > ignore_certificate_errors > might be a cleaner solution that doesn't involve any CRONET_TEST macros and > doesn't involve pulling MockCertVerifier into production code. I'm not sure I understand this concern fully. The cronet tests are, of course, tests :> So it *seems* like it should be possible to run test-only code in them. But of course the java/c++ layers probably add complications that I don't appreciate.
On 2015/10/09 20:57:46, pauljensen wrote: > I'm tending to think a solution involving: > 1. piping ignore_certificate_errors down into QUIC, and > 2. exposing a package-private CronetEngine.Builder setter for > ignore_certificate_errors > might be a cleaner solution that doesn't involve any CRONET_TEST macros and > doesn't involve pulling MockCertVerifier into production code. I agree that we don't want production code to depend on MockCertVerifier. Another option would be to make some way to pass a native CertVerifier to the builder, though that gets a bit ugly.
On 2015/10/09 21:03:58, Ryan Hamilton wrote: > On 2015/10/09 20:57:46, pauljensen wrote: > > I'm tending to think a solution involving: > > 1. piping ignore_certificate_errors down into QUIC, and > > This is very much problematic given the way ignore_certificate_errors works. It > requires the request to complete and then be retried. But that machinery will > cause QUIC to be marked as broken. I spent a good chunk of time talking through > this issue with sleevi in the context of testing, and the MockCertVerifier was > the approach he most preferred. > > > 2. exposing a package-private CronetEngine.Builder setter for > > ignore_certificate_errors > > might be a cleaner solution that doesn't involve any CRONET_TEST macros and > > doesn't involve pulling MockCertVerifier into production code. > > I'm not sure I understand this concern fully. The cronet tests are, of course, > tests :> So it *seems* like it should be possible to run test-only code in them. > But of course the java/c++ layers probably add complications that I don't > appreciate. I think it's generally a good idea to keep test code out of production code as much as possible - may need test setters and getters (And maybe pass stuff in via ugly globals, if really necessary), but shouldn't go beyond that. This also alloys you not to have to rebuild everything with and without your test macros defined.
On 2015/10/09 20:55:39, Ryan Hamilton wrote: > On 2015/10/09 20:52:29, xunjieli wrote: > > On 2015/10/09 20:37:16, Ryan Hamilton wrote: > > > This looks like it will work, but I see a lot of #if CRONET_TEST in what > looks > > > like otherwise production code. Is that OK? That's not kosher for code in > net/ > > > but components/cronet/android can be a different beast, of course. > > > > + Misha, Matt to get their thought > > I am not sure either. But I don't have a better solution on injecting the mock > > cert verifier into Cronet, since these are integration tests that use Cronet's > > public APIs. I agree this is an ugly change, so I am aggressively adding all > > Cronet owners. > > I would expect that "Add a cert verifier" would be part of he public cronet API, > and a test-only method would exist to *create* a mock verifier. > > Then the test could call that test-only method to create the verifier and hand > it to the public set_verifier API. > > Is this complicated because the cert verifier is a C++ object and the public > cronet > APIs are java? What if the "Add a cert verifier" API was C++-only, perhaps we could add some testing-only C++ code to generate the MockCertVerifier and install it, identical to how we install the fake host resolver rules for NativeTestServer?
On 2015/10/09 21:07:21, mmenke wrote: > > I think it's generally a good idea to keep test code out of production code as > much as possible - may need test setters and getters (And maybe pass stuff in > via ugly globals, if really necessary), but shouldn't go beyond that. But that's exactly what I'm proposing :> I want the public / production / non-test code to *only* have an AddCertVerifier() method. Then there would be test-only code to call that method with a MockCertVerifier(). This is how we solve this problem in /net. > This also > alloys you not to have to rebuild everything with and without your test macros > defined. Yeah, agreed.
On 2015/10/09 21:07:37, pauljensen wrote: > On 2015/10/09 20:55:39, Ryan Hamilton wrote: > > On 2015/10/09 20:52:29, xunjieli wrote: > > > On 2015/10/09 20:37:16, Ryan Hamilton wrote: > > > > This looks like it will work, but I see a lot of #if CRONET_TEST in what > > looks > > > > like otherwise production code. Is that OK? That's not kosher for code in > > net/ > > > > but components/cronet/android can be a different beast, of course. > > > > > > + Misha, Matt to get their thought > > > I am not sure either. But I don't have a better solution on injecting the > mock > > > cert verifier into Cronet, since these are integration tests that use > Cronet's > > > public APIs. I agree this is an ugly change, so I am aggressively adding all > > > Cronet owners. > > > > I would expect that "Add a cert verifier" would be part of he public cronet > API, > > and a test-only method would exist to *create* a mock verifier. > > > > Then the test could call that test-only method to create the verifier and hand > > > it to the public set_verifier API. > > > > Is this complicated because the cert verifier is a C++ object and the public > > cronet > > APIs are java? > > What if the "Add a cert verifier" API was C++-only, perhaps we could add some > testing-only C++ code to generate the MockCertVerifier and install it, identical > to how we install the fake host resolver rules for NativeTestServer? SGTM!
On 2015/10/09 21:09:18, Ryan Hamilton wrote: > On 2015/10/09 21:07:21, mmenke wrote: > > > > I think it's generally a good idea to keep test code out of production code as > > much as possible - may need test setters and getters (And maybe pass stuff in > > via ugly globals, if really necessary), but shouldn't go beyond that. > > But that's exactly what I'm proposing :> I want the public / production / > non-test code > to *only* have an AddCertVerifier() method. Then there would be test-only code > to call > that method with a MockCertVerifier(). > > This is how we solve this problem in /net. Sorry, I misunderstood your comment about running test-only code. > > > This also > > alloys you not to have to rebuild everything with and without your test macros > > defined. > > Yeah, agreed.
Thanks everyone. If we have a C++ API on the CronetUrlRequestAdapter to set the cert verifier. Since Cronet builds the net::URLRequestContext immediately in CronetUrlRequestContext's constructor, We will need to have a global to pass the verifier around in CronetUrlRequestAdapter. As much as I hate globals, I guess this is the way forward. I will get to it. Please correct me if I am wrong in the meanwhile. https://codereview.chromium.org/1389213003/diff/60001/build/android/pylib/ins... File build/android/pylib/instrumentation/setup.py (right): https://codereview.chromium.org/1389213003/diff/60001/build/android/pylib/ins... build/android/pylib/instrumentation/setup.py:34: if test_apk in ['ChromeTest', 'ContentShellTest', 'CronetTestInstrumentation']: On 2015/10/09 20:58:10, pauljensen wrote: > line wrap Done. https://codereview.chromium.org/1389213003/diff/60001/components/cronet/andro... File components/cronet/android/test/quic_test_server.cc (right): https://codereview.chromium.org/1389213003/diff/60001/components/cronet/andro... components/cronet/android/test/quic_test_server.cc:33: const base::FilePath& key_path) { On 2015/10/09 20:58:10, pauljensen wrote: > any particular reason you have a tiny function that's only called once? can we > inline it? Done. https://codereview.chromium.org/1389213003/diff/60001/components/cronet/andro... components/cronet/android/test/quic_test_server.cc:56: bool success = base::android::GetExternalStorageDirectory(&directory); On 2015/10/09 20:58:10, pauljensen wrote: > can we combine this and the next line into > CHECK(base::android::GetExternalStorageDirectory(&directory)); Done. https://codereview.chromium.org/1389213003/diff/60001/components/cronet/url_r... File components/cronet/url_request_context_config.h (right): https://codereview.chromium.org/1389213003/diff/60001/components/cronet/url_r... components/cronet/url_request_context_config.h:85: // A comma delimited certs to ignore for testing. On 2015/10/09 20:58:10, pauljensen wrote: > delimited->delimited list of Done. https://codereview.chromium.org/1389213003/diff/60001/components/cronet/url_r... components/cronet/url_request_context_config.h:85: // A comma delimited certs to ignore for testing. On 2015/10/09 20:58:10, pauljensen wrote: > ignore->accept Done.
On 2015/10/09 21:19:58, xunjieli wrote: > Thanks everyone. If we have a C++ API on the CronetUrlRequestAdapter to set the > cert verifier. Since Cronet builds the net::URLRequestContext immediately in > CronetUrlRequestContext's constructor, We will need to have a global to pass the > verifier around in CronetUrlRequestAdapter. > > As much as I hate globals, I guess this is the way forward. I will get to it. > Please correct me if I am wrong in the meanwhile. I'm fine with a global (Though yea, they're hideous), but couldn't we smuggle it in through the builder?
On 2015/10/09 21:22:06, mmenke wrote: > On 2015/10/09 21:19:58, xunjieli wrote: > > Thanks everyone. If we have a C++ API on the CronetUrlRequestAdapter to set > the > > cert verifier. Since Cronet builds the net::URLRequestContext immediately in > > CronetUrlRequestContext's constructor, We will need to have a global to pass > the > > verifier around in CronetUrlRequestAdapter. > > > > As much as I hate globals, I guess this is the way forward. I will get to it. > > Please correct me if I am wrong in the meanwhile. > > I'm fine with a global (Though yea, they're hideous), but couldn't we smuggle it > in through the builder? What about just calling context_->set_cert_verifier() or that disastrous?
Ah, I didn't realize there is a set_cert_verifier() on the URLRequestContext. I guess that will work then. Thanks all. I will get to it. On Fri, Oct 9, 2015 at 5:24 PM, <pauljensen@chromium.org> wrote: > On 2015/10/09 21:22:06, mmenke wrote: > >> On 2015/10/09 21:19:58, xunjieli wrote: >> > Thanks everyone. If we have a C++ API on the CronetUrlRequestAdapter to >> set >> the >> > cert verifier. Since Cronet builds the net::URLRequestContext >> immediately in >> > CronetUrlRequestContext's constructor, We will need to have a global to >> pass >> the >> > verifier around in CronetUrlRequestAdapter. >> > >> > As much as I hate globals, I guess this is the way forward. I will get >> to >> > it. > >> > Please correct me if I am wrong in the meanwhile. >> > > I'm fine with a global (Though yea, they're hideous), but couldn't we >> smuggle >> > it > >> in through the builder? >> > > What about just calling context_->set_cert_verifier() or that disastrous? > > https://codereview.chromium.org/1389213003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
URLRequestContext::set_cert_verifier doesn't actually do what you think. The CertVerifier is mainly used used by the HttpNetworkSession, and it can't be set on that. On 2015/10/09 21:26:50, xunjieli wrote: > Ah, I didn't realize there is a set_cert_verifier() on the > URLRequestContext. I guess that will work then. Thanks all. I will get to > it. > > On Fri, Oct 9, 2015 at 5:24 PM, <mailto:pauljensen@chromium.org> wrote: > > > On 2015/10/09 21:22:06, mmenke wrote: > > > >> On 2015/10/09 21:19:58, xunjieli wrote: > >> > Thanks everyone. If we have a C++ API on the CronetUrlRequestAdapter to > >> set > >> the > >> > cert verifier. Since Cronet builds the net::URLRequestContext > >> immediately in > >> > CronetUrlRequestContext's constructor, We will need to have a global to > >> pass > >> the > >> > verifier around in CronetUrlRequestAdapter. > >> > > >> > As much as I hate globals, I guess this is the way forward. I will get > >> to > >> > > it. > > > >> > Please correct me if I am wrong in the meanwhile. > >> > > > > I'm fine with a global (Though yea, they're hideous), but couldn't we > >> smuggle > >> > > it > > > >> in through the builder? > >> > > > > What about just calling context_->set_cert_verifier() or that disastrous? > > > > https://codereview.chromium.org/1389213003/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
Patchset #5 (id:120001) has been deleted
Thanks everyone for the suggestions! I have updated the CL. Could you take a look to see if this is closer to what you had in mind? PTAL.
This looks good to me, though admittedly I'm unfamiliar with the cronet code in any level of detail. But the flow looks good as do the changes to net/
https://codereview.chromium.org/1389213003/diff/140001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java (right): https://codereview.chromium.org/1389213003/diff/140001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:77: nativeSetMockCertVerifierForTesting(mUrlRequestContextAdapter, sNativeMockCertVerifier); Would it make sense to have a 'SetMockCertVerifierForTesting' method on CronetEngine.Builder, which will pass it down through config and use it in URLRequestContextConfig::ConfigureURLRequestContextBuilder? That should work for both CronetUrlRequestContext and legacy API.
https://codereview.chromium.org/1389213003/diff/140001/components/cronet/andr... File components/cronet/android/cronet_url_request_context_adapter.h (right): https://codereview.chromium.org/1389213003/diff/140001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.h:160: scoped_ptr<net::URLRequestContextBuilder> context_builder_; The need to add context_builder_ as a member of UrlRequestContextAdapter seems wrong. https://codereview.chromium.org/1389213003/diff/140001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java (right): https://codereview.chromium.org/1389213003/diff/140001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java:38: CronetUrlRequestContext.setMockCertVerifierForTesting(mCertVerifier.getNativePointer()); Hrm, how would it work? I imagine that this code will execute in the instrumentation test process, not in the test app. Am I missing something? https://codereview.chromium.org/1389213003/diff/140001/components/cronet/andr... File components/cronet/android/test/mock_cert_verifier.cc (right): https://codereview.chromium.org/1389213003/diff/140001/components/cronet/andr... components/cronet/android/test/mock_cert_verifier.cc:32: net::ImportCertFromFile(net::GetTestCertsDirectory(), cert); Do we need to add TestCertsDirectory as resources or assets or something?
On 2015/10/12 18:35:03, mef wrote: > https://codereview.chromium.org/1389213003/diff/140001/components/cronet/andr... > File components/cronet/android/cronet_url_request_context_adapter.h (right): > > https://codereview.chromium.org/1389213003/diff/140001/components/cronet/andr... > components/cronet/android/cronet_url_request_context_adapter.h:160: > scoped_ptr<net::URLRequestContextBuilder> context_builder_; > The need to add context_builder_ as a member of UrlRequestContextAdapter seems > wrong. > > https://codereview.chromium.org/1389213003/diff/140001/components/cronet/andr... > File components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java > (right): > > https://codereview.chromium.org/1389213003/diff/140001/components/cronet/andr... > components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java:38: > CronetUrlRequestContext.setMockCertVerifierForTesting(mCertVerifier.getNativePointer()); > Hrm, how would it work? I imagine that this code will execute in the > instrumentation test process, not in the test app. Am I missing something? > > https://codereview.chromium.org/1389213003/diff/140001/components/cronet/andr... > File components/cronet/android/test/mock_cert_verifier.cc (right): > > https://codereview.chromium.org/1389213003/diff/140001/components/cronet/andr... > components/cronet/android/test/mock_cert_verifier.cc:32: > net::ImportCertFromFile(net::GetTestCertsDirectory(), cert); > Do we need to add TestCertsDirectory as resources or assets or something? Thanks for the comments! Need to leave early for an event now. Will get back to this later tonight.
Patchset #4 (id:100001) has been deleted
Patchset #5 (id:160001) has been deleted
Thanks Misha for the great suggestion! I didn't realize we can serialize a raw pointer like that. It does make the code a lot cleaner ! :) PTAL. https://codereview.chromium.org/1389213003/diff/140001/components/cronet/andr... File components/cronet/android/cronet_url_request_context_adapter.h (right): https://codereview.chromium.org/1389213003/diff/140001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.h:160: scoped_ptr<net::URLRequestContextBuilder> context_builder_; On 2015/10/12 18:35:03, mef wrote: > The need to add context_builder_ as a member of UrlRequestContextAdapter seems > wrong. Done. https://codereview.chromium.org/1389213003/diff/140001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java (right): https://codereview.chromium.org/1389213003/diff/140001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:77: nativeSetMockCertVerifierForTesting(mUrlRequestContextAdapter, sNativeMockCertVerifier); On 2015/10/12 18:27:13, mef wrote: > Would it make sense to have a 'SetMockCertVerifierForTesting' method on > CronetEngine.Builder, which will pass it down through config and use it in > URLRequestContextConfig::ConfigureURLRequestContextBuilder? > > That should work for both CronetUrlRequestContext and legacy API. Great idea! Didn't realize we can serialize a native pointer into string that way! Very smart https://codereview.chromium.org/1389213003/diff/140001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java (right): https://codereview.chromium.org/1389213003/diff/140001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java:38: CronetUrlRequestContext.setMockCertVerifierForTesting(mCertVerifier.getNativePointer()); On 2015/10/12 18:35:03, mef wrote: > Hrm, how would it work? I imagine that this code will execute in the > instrumentation test process, not in the test app. Am I missing something? It sets a static variable in CronetUrlRequestContext before the mUrlRequestContext is constructed, so mUrlRequestContext will actually use the cert verifier. But after changing to your approach, this is no longer applicable :) https://codereview.chromium.org/1389213003/diff/140001/components/cronet/andr... File components/cronet/android/test/mock_cert_verifier.cc (right): https://codereview.chromium.org/1389213003/diff/140001/components/cronet/andr... components/cronet/android/test/mock_cert_verifier.cc:32: net::ImportCertFromFile(net::GetTestCertsDirectory(), cert); On 2015/10/12 18:35:03, mef wrote: > Do we need to add TestCertsDirectory as resources or assets or something? The change in build/android/pylib/instrumentation/setup.py took care of it :)
https://codereview.chromium.org/1389213003/diff/180001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java (right): https://codereview.chromium.org/1389213003/diff/180001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java:49: mActivity = launchCronetTestAppWithUrlAndCommandLineArgs(null, commandLineArgs); So, it looks like here we create a native pointer, serialize it to a string, pass the string as an Intent argument and then deserialize the native pointer and use it. This is a little terrifying as Intents are generally used for IPC, and a native pointer shouldn't be passed via IPC. It works here because instrumentation tests run in the same process. I'm not sure I have a much better idea. A small improvement may be to create the MockCertVerifier inside the CronetTestApp and insert it into the Builder there, so at least it's not crossing an IPC boundary. Maybe I'll think of something better tomorrow. https://codereview.chromium.org/1389213003/diff/180001/net/url_request/url_re... File net/url_request/url_request_context_builder.cc (right): https://codereview.chromium.org/1389213003/diff/180001/net/url_request/url_re... net/url_request/url_request_context_builder.cc:354: storage->set_cert_verifier(CertVerifier::CreateDefault()); Can I recommend a simplification: 1. make UrlRequestContextBuilder::set_cert_verifier take a scoped_ptr 2. Pass() the CertVerifier into |storage| here 3. remove lines 385-386 that you added below 4. make MockCertVerifier simply contain a static function that returns a "long" pointer to a native CertVerifier 5. get rid of the "owned by Java" comment
https://codereview.chromium.org/1389213003/diff/180001/net/url_request/url_re... File net/url_request/url_request_context_builder.cc (right): https://codereview.chromium.org/1389213003/diff/180001/net/url_request/url_re... net/url_request/url_request_context_builder.cc:354: storage->set_cert_verifier(CertVerifier::CreateDefault()); On 2015/10/15 19:13:07, pauljensen wrote: > Can I recommend a simplification: > 1. make UrlRequestContextBuilder::set_cert_verifier take a scoped_ptr > 2. Pass() the CertVerifier into |storage| here > 3. remove lines 385-386 that you added below > 4. make MockCertVerifier simply contain a static function that returns a "long" > pointer to a native CertVerifier > 5. get rid of the "owned by Java" comment I think 385-386 need to be changed to take the cert_verifier from the URLRequestContext - the fact it doesn't currently do that looks like a bug.
https://codereview.chromium.org/1389213003/diff/180001/net/url_request/url_re... File net/url_request/url_request_context_builder.cc (right): https://codereview.chromium.org/1389213003/diff/180001/net/url_request/url_re... net/url_request/url_request_context_builder.cc:354: storage->set_cert_verifier(CertVerifier::CreateDefault()); On 2015/10/15 19:40:41, mmenke wrote: > On 2015/10/15 19:13:07, pauljensen wrote: > > Can I recommend a simplification: > > 1. make UrlRequestContextBuilder::set_cert_verifier take a scoped_ptr > > 2. Pass() the CertVerifier into |storage| here > > 3. remove lines 385-386 that you added below > > 4. make MockCertVerifier simply contain a static function that returns a > "long" > > pointer to a native CertVerifier > > 5. get rid of the "owned by Java" comment > > I think 385-386 need to be changed to take the cert_verifier from the > URLRequestContext - the fact it doesn't currently do that looks like a bug. I believe this already happens on line 367; this is why I proposed "Pass() the CertVerifier into |storage| here" so it'll be first attached to the URLRequestContext, and then copied to |network_session_params|.
https://codereview.chromium.org/1389213003/diff/180001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java (right): https://codereview.chromium.org/1389213003/diff/180001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java:49: mActivity = launchCronetTestAppWithUrlAndCommandLineArgs(null, commandLineArgs); On 2015/10/15 19:13:07, pauljensen wrote: > So, it looks like here we create a native pointer, serialize it to a string, > pass the string as an Intent argument and then deserialize the native pointer > and use it. This is a little terrifying as Intents are generally used for IPC, > and a native pointer shouldn't be passed via IPC. It works here because > instrumentation tests run in the same process. I'm not sure I have a much > better idea. A small improvement may be to create the MockCertVerifier inside > the CronetTestApp and insert it into the Builder there, so at least it's not > crossing an IPC boundary. Maybe I'll think of something better tomorrow. I guess a bigger question is why do we serialize a CronetEngine.Builder into Intent arguments? In launchCronetTestAppWithUrlAndCommandLineArgs() why don't we just cast the resulting Activity to CronetTestActivity and call a setter function that takes a CronetEngine.Builder? I just checked and the resulting Activity in launchCronetTestAppWithUrlAndCommandLineArgs() is in fact an instance of CronetTestActivity.
https://codereview.chromium.org/1389213003/diff/180001/net/url_request/url_re... File net/url_request/url_request_context_builder.cc (right): https://codereview.chromium.org/1389213003/diff/180001/net/url_request/url_re... net/url_request/url_request_context_builder.cc:354: storage->set_cert_verifier(CertVerifier::CreateDefault()); On 2015/10/16 11:17:22, pauljensen wrote: > On 2015/10/15 19:40:41, mmenke wrote: > > On 2015/10/15 19:13:07, pauljensen wrote: > > > Can I recommend a simplification: > > > 1. make UrlRequestContextBuilder::set_cert_verifier take a scoped_ptr > > > 2. Pass() the CertVerifier into |storage| here > > > 3. remove lines 385-386 that you added below > > > 4. make MockCertVerifier simply contain a static function that returns a > > "long" > > > pointer to a native CertVerifier > > > 5. get rid of the "owned by Java" comment > > > > I think 385-386 need to be changed to take the cert_verifier from the > > URLRequestContext - the fact it doesn't currently do that looks like a bug. > > I believe this already happens on line 367; this is why I proposed "Pass() the > CertVerifier into |storage| here" so it'll be first attached to the > URLRequestContext, and then copied to |network_session_params|. I'm not seeing where storage's cert_verified is set on network_session_params Ahh, you're right - I completely missed that magic line...And I reviewed the CL that added it, not that far back.
https://codereview.chromium.org/1389213003/diff/180001/net/url_request/url_re... File net/url_request/url_request_context_builder.cc (right): https://codereview.chromium.org/1389213003/diff/180001/net/url_request/url_re... net/url_request/url_request_context_builder.cc:354: storage->set_cert_verifier(CertVerifier::CreateDefault()); On 2015/10/16 13:25:35, mmenke wrote: > On 2015/10/16 11:17:22, pauljensen wrote: > > On 2015/10/15 19:40:41, mmenke wrote: > > > On 2015/10/15 19:13:07, pauljensen wrote: > > > > Can I recommend a simplification: > > > > 1. make UrlRequestContextBuilder::set_cert_verifier take a scoped_ptr > > > > 2. Pass() the CertVerifier into |storage| here > > > > 3. remove lines 385-386 that you added below > > > > 4. make MockCertVerifier simply contain a static function that returns a > > > "long" > > > > pointer to a native CertVerifier > > > > 5. get rid of the "owned by Java" comment > > > > > > I think 385-386 need to be changed to take the cert_verifier from the > > > URLRequestContext - the fact it doesn't currently do that looks like a bug. > > > > I believe this already happens on line 367; this is why I proposed "Pass() the > > CertVerifier into |storage| here" so it'll be first attached to the > > URLRequestContext, and then copied to |network_session_params|. > > I'm not seeing where storage's cert_verified is set on network_session_params (Not sure if you meant to leave this line in, or your comment was a sequence of thoughts, so I'll just answer to be absolutely sure we're on the same page:) Line 367 calling line 215. > > Ahh, you're right - I completely missed that magic line...And I reviewed the CL > that added it, not that far back.
On 2015/10/16 13:30:56, pauljensen wrote: > https://codereview.chromium.org/1389213003/diff/180001/net/url_request/url_re... > File net/url_request/url_request_context_builder.cc (right): > > https://codereview.chromium.org/1389213003/diff/180001/net/url_request/url_re... > net/url_request/url_request_context_builder.cc:354: > storage->set_cert_verifier(CertVerifier::CreateDefault()); > On 2015/10/16 13:25:35, mmenke wrote: > > On 2015/10/16 11:17:22, pauljensen wrote: > > > On 2015/10/15 19:40:41, mmenke wrote: > > > > On 2015/10/15 19:13:07, pauljensen wrote: > > > > > Can I recommend a simplification: > > > > > 1. make UrlRequestContextBuilder::set_cert_verifier take a scoped_ptr > > > > > 2. Pass() the CertVerifier into |storage| here > > > > > 3. remove lines 385-386 that you added below > > > > > 4. make MockCertVerifier simply contain a static function that returns a > > > > "long" > > > > > pointer to a native CertVerifier > > > > > 5. get rid of the "owned by Java" comment > > > > > > > > I think 385-386 need to be changed to take the cert_verifier from the > > > > URLRequestContext - the fact it doesn't currently do that looks like a > bug. > > > > > > I believe this already happens on line 367; this is why I proposed "Pass() > the > > > CertVerifier into |storage| here" so it'll be first attached to the > > > URLRequestContext, and then copied to |network_session_params|. > > > > I'm not seeing where storage's cert_verified is set on network_session_params > > (Not sure if you meant to leave this line in, or your comment was a sequence of > thoughts, so I'll just answer to be absolutely sure we're on the same page:) > Line 367 calling line 215. > > > > > Ahh, you're right - I completely missed that magic line...And I reviewed the > CL > > that added it, not that far back. Do we have an ETA on this change landing? We'd really like to get rid of insecure QUIC.
> Do we have an ETA on this change landing? We'd really like to get rid of > insecure QUIC. Helen has been at the Grace Hopper conference this week. I imagine early next week we might be able to finish it.
https://codereview.chromium.org/1389213003/diff/180001/net/url_request/url_re... File net/url_request/url_request_context_builder.cc (right): https://codereview.chromium.org/1389213003/diff/180001/net/url_request/url_re... net/url_request/url_request_context_builder.cc:354: storage->set_cert_verifier(CertVerifier::CreateDefault()); On 2015/10/16 13:30:56, pauljensen wrote: > On 2015/10/16 13:25:35, mmenke wrote: > > On 2015/10/16 11:17:22, pauljensen wrote: > > > On 2015/10/15 19:40:41, mmenke wrote: > > > > On 2015/10/15 19:13:07, pauljensen wrote: > > > > > Can I recommend a simplification: > > > > > 1. make UrlRequestContextBuilder::set_cert_verifier take a scoped_ptr > > > > > 2. Pass() the CertVerifier into |storage| here > > > > > 3. remove lines 385-386 that you added below > > > > > 4. make MockCertVerifier simply contain a static function that returns a > > > > "long" > > > > > pointer to a native CertVerifier > > > > > 5. get rid of the "owned by Java" comment > > > > > > > > I think 385-386 need to be changed to take the cert_verifier from the > > > > URLRequestContext - the fact it doesn't currently do that looks like a > bug. > > > > > > I believe this already happens on line 367; this is why I proposed "Pass() > the > > > CertVerifier into |storage| here" so it'll be first attached to the > > > URLRequestContext, and then copied to |network_session_params|. > > > > I'm not seeing where storage's cert_verified is set on network_session_params > > (Not sure if you meant to leave this line in, or your comment was a sequence of > thoughts, so I'll just answer to be absolutely sure we're on the same page:) > Line 367 calling line 215. > > > > > Ahh, you're right - I completely missed that magic line...And I reviewed the > CL > > that added it, not that far back. > I forgot I wrote that line, when I suddenly realized what was going on. :(
On Fri, Oct 16, 2015 at 11:19 AM, <pauljensen@chromium.org> wrote: > Do we have an ETA on this change landing? We'd really like to get rid of >> ​ ​ >> insecure QUIC. >> > > Helen has been at the Grace Hopper conference this week. I imagine early > next > ​ ​ > week we might be able to finish it. > ​Ah, right. I forgot about the conference. Thanks.​ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Patchset #6 (id:200001) has been deleted
PTAL. Thanks for the suggestions! https://codereview.chromium.org/1389213003/diff/180001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java (right): https://codereview.chromium.org/1389213003/diff/180001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java:49: mActivity = launchCronetTestAppWithUrlAndCommandLineArgs(null, commandLineArgs); On 2015/10/16 13:03:49, pauljensen wrote: > On 2015/10/15 19:13:07, pauljensen wrote: > > So, it looks like here we create a native pointer, serialize it to a string, > > pass the string as an Intent argument and then deserialize the native pointer > > and use it. This is a little terrifying as Intents are generally used for > IPC, > > and a native pointer shouldn't be passed via IPC. It works here because > > instrumentation tests run in the same process. I'm not sure I have a much > > better idea. A small improvement may be to create the MockCertVerifier inside > > the CronetTestApp and insert it into the Builder there, so at least it's not > > crossing an IPC boundary. Maybe I'll think of something better tomorrow. > > I guess a bigger question is why do we serialize a CronetEngine.Builder into > Intent arguments? > In launchCronetTestAppWithUrlAndCommandLineArgs() why don't we just cast the > resulting Activity to CronetTestActivity and call a setter function that takes a > CronetEngine.Builder? I just checked and the resulting Activity in > launchCronetTestAppWithUrlAndCommandLineArgs() is in fact an instance of > CronetTestActivity. With the builder serialized in intent arguments, we can create the CronetEngines in onCreate(). If we have a setter, then we would need to move the Builder.build() logic from onCreate() to somewhere else. And starting the application won't initialize the mCronetEngine, which needs to be done in a separate step. I am not sure if that's cleaner. I agree that it's a bit weird that this pointer is created in the instrumentation app. But as you mentioned, the instrumentation app runs in the same process as the test app, so there isn't a process boundary. https://codereview.chromium.org/1389213003/diff/180001/net/url_request/url_re... File net/url_request/url_request_context_builder.cc (right): https://codereview.chromium.org/1389213003/diff/180001/net/url_request/url_re... net/url_request/url_request_context_builder.cc:354: storage->set_cert_verifier(CertVerifier::CreateDefault()); On 2015/10/15 19:13:07, pauljensen wrote: > Can I recommend a simplification: > 1. make UrlRequestContextBuilder::set_cert_verifier take a scoped_ptr > 2. Pass() the CertVerifier into |storage| here > 3. remove lines 385-386 that you added below > 4. make MockCertVerifier simply contain a static function that returns a "long" > pointer to a native CertVerifier > 5. get rid of the "owned by Java" comment Done. That's really neat. Thanks!
https://codereview.chromium.org/1389213003/diff/180001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java (right): https://codereview.chromium.org/1389213003/diff/180001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java:49: mActivity = launchCronetTestAppWithUrlAndCommandLineArgs(null, commandLineArgs); On 2015/10/19 01:40:43, xunjieli wrote: > On 2015/10/16 13:03:49, pauljensen wrote: > > On 2015/10/15 19:13:07, pauljensen wrote: > > > So, it looks like here we create a native pointer, serialize it to a string, > > > pass the string as an Intent argument and then deserialize the native > pointer > > > and use it. This is a little terrifying as Intents are generally used for > > IPC, > > > and a native pointer shouldn't be passed via IPC. It works here because > > > instrumentation tests run in the same process. I'm not sure I have a much > > > better idea. A small improvement may be to create the MockCertVerifier > inside > > > the CronetTestApp and insert it into the Builder there, so at least it's not > > > crossing an IPC boundary. Maybe I'll think of something better tomorrow. > > > > I guess a bigger question is why do we serialize a CronetEngine.Builder into > > Intent arguments? > > In launchCronetTestAppWithUrlAndCommandLineArgs() why don't we just cast the > > resulting Activity to CronetTestActivity and call a setter function that takes > a > > CronetEngine.Builder? I just checked and the resulting Activity in > > launchCronetTestAppWithUrlAndCommandLineArgs() is in fact an instance of > > CronetTestActivity. > > With the builder serialized in intent arguments, we can create the CronetEngines > in onCreate(). If we have a setter, then we would need to move the > Builder.build() logic from onCreate() to somewhere else. And starting the > application won't initialize the mCronetEngine, which needs to be done in a > separate step. I am not sure if that's cleaner. > > I agree that it's a bit weird that this pointer is created in the > instrumentation app. But as you mentioned, the instrumentation app runs in the > same process as the test app, so there isn't a process boundary. I think creating it here and passing it via a setter would be substantially simpler: 1. We could hide the serialize/deserialize CronetEngine.Builder APIs 2. We could remove the serialize/deserialize logic in our tests, including: a. CronetTestActivity.createCronetEngineBuilder b. CronetTestActivity.SDCH_KEY c. CronetTestActivity.CONFIG_KEY Actually, now I'm wondering, why do we have a test activity? why don't we simply extend android.test.AndroidTestCase (a junit test with a Context)? Instrumentation tests are meant to test apps, but we just want to test Cronet (a library not an app). These are really unit tests, not app tests.
On 2015/10/19 11:43:31, pauljensen wrote: > https://codereview.chromium.org/1389213003/diff/180001/components/cronet/andr... > File components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java > (right): > > https://codereview.chromium.org/1389213003/diff/180001/components/cronet/andr... > components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java:49: > mActivity = launchCronetTestAppWithUrlAndCommandLineArgs(null, commandLineArgs); > On 2015/10/19 01:40:43, xunjieli wrote: > > On 2015/10/16 13:03:49, pauljensen wrote: > > > On 2015/10/15 19:13:07, pauljensen wrote: > > > > So, it looks like here we create a native pointer, serialize it to a > string, > > > > pass the string as an Intent argument and then deserialize the native > > pointer > > > > and use it. This is a little terrifying as Intents are generally used for > > > IPC, > > > > and a native pointer shouldn't be passed via IPC. It works here because > > > > instrumentation tests run in the same process. I'm not sure I have a much > > > > better idea. A small improvement may be to create the MockCertVerifier > > inside > > > > the CronetTestApp and insert it into the Builder there, so at least it's > not > > > > crossing an IPC boundary. Maybe I'll think of something better tomorrow. > > > > > > I guess a bigger question is why do we serialize a CronetEngine.Builder into > > > Intent arguments? > > > In launchCronetTestAppWithUrlAndCommandLineArgs() why don't we just cast the > > > resulting Activity to CronetTestActivity and call a setter function that > takes > > a > > > CronetEngine.Builder? I just checked and the resulting Activity in > > > launchCronetTestAppWithUrlAndCommandLineArgs() is in fact an instance of > > > CronetTestActivity. > > > > With the builder serialized in intent arguments, we can create the > CronetEngines > > in onCreate(). If we have a setter, then we would need to move the > > Builder.build() logic from onCreate() to somewhere else. And starting the > > application won't initialize the mCronetEngine, which needs to be done in a > > separate step. I am not sure if that's cleaner. > > > > I agree that it's a bit weird that this pointer is created in the > > instrumentation app. But as you mentioned, the instrumentation app runs in the > > same process as the test app, so there isn't a process boundary. > > I think creating it here and passing it via a setter would be substantially > simpler: > 1. We could hide the serialize/deserialize CronetEngine.Builder APIs > 2. We could remove the serialize/deserialize logic in our tests, including: > a. CronetTestActivity.createCronetEngineBuilder > b. CronetTestActivity.SDCH_KEY > c. CronetTestActivity.CONFIG_KEY > > Actually, now I'm wondering, why do we have a test activity? why don't we simply > extend android.test.AndroidTestCase (a junit test with a Context)? > Instrumentation tests are meant to test apps, but we just want to test Cronet (a > library not an app). These are really unit tests, not app tests. Thanks for the suggestions! I talked to Paul offline, and we decided to investigate this in a separate CL since this will affect other integration tests as well. I filed crbug.com/544976 to track this. PTAL.
https://codereview.chromium.org/1389213003/diff/220001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/1389213003/diff/220001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetEngine.java:298: return putString( Is there particular reason not to use putLong? https://codereview.chromium.org/1389213003/diff/220001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java (right): https://codereview.chromium.org/1389213003/diff/220001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java:36: MockCertVerifier certVerifier = new MockCertVerifier(CERTS_USED); If certVerifier is created in test process, then is it really usable in the test app process? Should it be created in test app instead?
PTAL. https://codereview.chromium.org/1389213003/diff/220001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/1389213003/diff/220001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetEngine.java:298: return putString( On 2015/10/19 15:35:21, mef wrote: > Is there particular reason not to use putLong? putLong doesn't work. The base/json/json_value_converter.h does not work with that format. I had to use putString. https://codereview.chromium.org/1389213003/diff/220001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java (right): https://codereview.chromium.org/1389213003/diff/220001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java:36: MockCertVerifier certVerifier = new MockCertVerifier(CERTS_USED); On 2015/10/19 15:35:21, mef wrote: > If certVerifier is created in test process, then is it really usable in the test > app process? > > Should it be created in test app instead? It turned out that they are in the same process. The tests all passed.
https://codereview.chromium.org/1389213003/diff/220001/components/cronet/andr... File components/cronet/android/test/src/org/chromium/net/MockCertVerifier.java (right): https://codereview.chromium.org/1389213003/diff/220001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/MockCertVerifier.java:16: private long mMockCertVerifier = 0; can we just replace mMockCertVerifier, MockCertVerifier() and getNativePointer() with a single static function CreateMockCertVerifier()? https://codereview.chromium.org/1389213003/diff/220001/components/cronet/url_... File components/cronet/url_request_context_config.cc (right): https://codereview.chromium.org/1389213003/diff/220001/components/cronet/url_... components/cronet/url_request_context_config.cc:27: net::CertVerifier* cert_verifier = reinterpret_cast<net::CertVerifier*>(val); can we combine this and the next line into: *result = reinterpret_cast<net::CertVerifier*>(val); https://codereview.chromium.org/1389213003/diff/220001/components/cronet/url_... File components/cronet/url_request_context_config.h (right): https://codereview.chromium.org/1389213003/diff/220001/components/cronet/url_... components/cronet/url_request_context_config.h:86: // Cert verifier for testing. Cert->Certificate https://codereview.chromium.org/1389213003/diff/220001/components/cronet/url_... components/cronet/url_request_context_config.h:87: net::CertVerifier* mock_cert_verifier; scoped_ptr
Thanks for the review! PTAL https://codereview.chromium.org/1389213003/diff/220001/components/cronet/andr... File components/cronet/android/test/src/org/chromium/net/MockCertVerifier.java (right): https://codereview.chromium.org/1389213003/diff/220001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/MockCertVerifier.java:16: private long mMockCertVerifier = 0; On 2015/10/19 18:52:29, pauljensen wrote: > can we just replace mMockCertVerifier, MockCertVerifier() and getNativePointer() > with a single static function CreateMockCertVerifier()? Done. https://codereview.chromium.org/1389213003/diff/220001/components/cronet/url_... File components/cronet/url_request_context_config.cc (right): https://codereview.chromium.org/1389213003/diff/220001/components/cronet/url_... components/cronet/url_request_context_config.cc:27: net::CertVerifier* cert_verifier = reinterpret_cast<net::CertVerifier*>(val); On 2015/10/19 18:52:29, pauljensen wrote: > can we combine this and the next line into: > *result = reinterpret_cast<net::CertVerifier*>(val); Done. https://codereview.chromium.org/1389213003/diff/220001/components/cronet/url_... File components/cronet/url_request_context_config.h (right): https://codereview.chromium.org/1389213003/diff/220001/components/cronet/url_... components/cronet/url_request_context_config.h:86: // Cert verifier for testing. On 2015/10/19 18:52:29, pauljensen wrote: > Cert->Certificate Done. https://codereview.chromium.org/1389213003/diff/220001/components/cronet/url_... components/cronet/url_request_context_config.h:87: net::CertVerifier* mock_cert_verifier; On 2015/10/19 18:52:29, pauljensen wrote: > scoped_ptr Done.
https://codereview.chromium.org/1389213003/diff/240001/components/cronet/andr... File components/cronet/android/test/native_test_server.cc (right): https://codereview.chromium.org/1389213003/diff/240001/components/cronet/andr... components/cronet/android/test/native_test_server.cc:43: const char kFakeQuicDomain[] = "test.example.com"; can you add a comment here? perhaps mention that this must match stuff in other files and specify the paths to the other files (e.g. the certificate, simple.txt etc) https://codereview.chromium.org/1389213003/diff/240001/components/cronet/andr... File components/cronet/android/test/quic_test_server.cc (right): https://codereview.chromium.org/1389213003/diff/240001/components/cronet/andr... components/cronet/android/test/quic_test_server.cc:55: g_quic_server->SetProofSource(proof_source); grumble grumble...this function is documented to take ownership but doesn't take a scoped_ptr...grumble grumble. Not for this CL. I filed crbug.com/545474 https://codereview.chromium.org/1389213003/diff/240001/components/cronet/andr... File components/cronet/android/test/src/org/chromium/net/MockCertVerifier.java (right): https://codereview.chromium.org/1389213003/diff/240001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/MockCertVerifier.java:11: * into {@link CronetEngine.Builder} for testing. The native pointer will be Builder->Builder#setMockCertVerifierForTesting https://codereview.chromium.org/1389213003/diff/240001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/MockCertVerifier.java:12: * freed when CronetEngine is torn down. CronetEngine->the CronetEngine https://codereview.chromium.org/1389213003/diff/240001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/MockCertVerifier.java:20: * @param certs a String array of certs to accept in testing. Please document that these are actually filenames of files in net:::GetTestCertsDirectory(), not the certificates themselves. https://codereview.chromium.org/1389213003/diff/240001/components/cronet/andr... File components/cronet/android/test/src/org/chromium/net/QuicTestServer.java (right): https://codereview.chromium.org/1389213003/diff/240001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/QuicTestServer.java:34: return "https://" + getServerHost() + ":" + getServerPort(); this is awesome https://codereview.chromium.org/1389213003/diff/240001/components/cronet/url_... File components/cronet/url_request_context_config.cc (right): https://codereview.chromium.org/1389213003/diff/240001/components/cronet/url_... components/cronet/url_request_context_config.cc:24: scoped_ptr<net::CertVerifier>* result) { passing pointers to scoped_ptr's is strictly forbade: http://dev.chromium.org/developers/smart-pointer-guidelines but in this case we're kinda forced by the semantics of RegisterCustomField. I guess I'm okay with this, knowing that we'll remove all this serializing/deserializing soon. Perhaps add a TODO with link to crbug.com/544976 https://codereview.chromium.org/1389213003/diff/240001/components/cronet/url_... components/cronet/url_request_context_config.cc:57: : mock_cert_verifier(nullptr) {} unnecessary, remove
Thanks! PTAL https://codereview.chromium.org/1389213003/diff/240001/components/cronet/andr... File components/cronet/android/test/native_test_server.cc (right): https://codereview.chromium.org/1389213003/diff/240001/components/cronet/andr... components/cronet/android/test/native_test_server.cc:43: const char kFakeQuicDomain[] = "test.example.com"; On 2015/10/20 13:53:58, pauljensen wrote: > can you add a comment here? perhaps mention that this must match stuff in other > files and specify the paths to the other files (e.g. the certificate, simple.txt > etc) Done. https://codereview.chromium.org/1389213003/diff/240001/components/cronet/andr... File components/cronet/android/test/quic_test_server.cc (right): https://codereview.chromium.org/1389213003/diff/240001/components/cronet/andr... components/cronet/android/test/quic_test_server.cc:55: g_quic_server->SetProofSource(proof_source); On 2015/10/20 13:53:58, pauljensen wrote: > grumble grumble...this function is documented to take ownership but doesn't take > a scoped_ptr...grumble grumble. Not for this CL. I filed crbug.com/545474 Acknowledged. Added the bug number here. https://codereview.chromium.org/1389213003/diff/240001/components/cronet/andr... File components/cronet/android/test/src/org/chromium/net/MockCertVerifier.java (right): https://codereview.chromium.org/1389213003/diff/240001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/MockCertVerifier.java:11: * into {@link CronetEngine.Builder} for testing. The native pointer will be On 2015/10/20 13:53:58, pauljensen wrote: > Builder->Builder#setMockCertVerifierForTesting Done. https://codereview.chromium.org/1389213003/diff/240001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/MockCertVerifier.java:12: * freed when CronetEngine is torn down. On 2015/10/20 13:53:58, pauljensen wrote: > CronetEngine->the CronetEngine Done. https://codereview.chromium.org/1389213003/diff/240001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/MockCertVerifier.java:20: * @param certs a String array of certs to accept in testing. On 2015/10/20 13:53:58, pauljensen wrote: > Please document that these are actually filenames of files in > net:::GetTestCertsDirectory(), not the certificates themselves. Done. https://codereview.chromium.org/1389213003/diff/240001/components/cronet/andr... File components/cronet/android/test/src/org/chromium/net/QuicTestServer.java (right): https://codereview.chromium.org/1389213003/diff/240001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/QuicTestServer.java:34: return "https://" + getServerHost() + ":" + getServerPort(); On 2015/10/20 13:53:58, pauljensen wrote: > this is awesome Acknowledged. Lol thanks https://codereview.chromium.org/1389213003/diff/240001/components/cronet/url_... File components/cronet/url_request_context_config.cc (right): https://codereview.chromium.org/1389213003/diff/240001/components/cronet/url_... components/cronet/url_request_context_config.cc:24: scoped_ptr<net::CertVerifier>* result) { On 2015/10/20 13:53:58, pauljensen wrote: > passing pointers to scoped_ptr's is strictly forbade: > http://dev.chromium.org/developers/smart-pointer-guidelines > but in this case we're kinda forced by the semantics of RegisterCustomField. > I guess I'm okay with this, knowing that we'll remove all this > serializing/deserializing soon. Perhaps add a TODO with link to > crbug.com/544976 Done. https://codereview.chromium.org/1389213003/diff/240001/components/cronet/url_... components/cronet/url_request_context_config.cc:57: : mock_cert_verifier(nullptr) {} On 2015/10/20 13:53:58, pauljensen wrote: > unnecessary, remove Done.
lgtm
xunjieli@chromium.org changed reviewers: + jbudorick@chromium.org
jbudorick@: could you take a look at build/android/pylib/instrumentation/setup.py ?
lgtm
Matt seems to be out. Misha, do you want to take another look? You could also defer to Paul, since this change is on test code and should have low impact.
lgtm https://codereview.chromium.org/1389213003/diff/320001/components/cronet/andr... File components/cronet/android/test/src/org/chromium/net/MockCertVerifier.java (right): https://codereview.chromium.org/1389213003/diff/320001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/MockCertVerifier.java:16: private MockCertVerifier() {} Maybe constructor should call nativeCreateMockCertVerifier() and store the pointer in member variable and getNativePointer() should be just a getter, not creator? https://codereview.chromium.org/1389213003/diff/320001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/MockCertVerifier.java:23: public static long getNativePointer(String[] certs) { Given that native cert verifier is not a singleton, would it make sense to name this something like 'createMockCertVerifier' or 'createNativeVerifier' or 'getNewNativeVerifier' or 'getNativePointerToNewVerifier'?
https://codereview.chromium.org/1389213003/diff/320001/components/cronet/andr... File components/cronet/android/test/src/org/chromium/net/MockCertVerifier.java (right): https://codereview.chromium.org/1389213003/diff/320001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/MockCertVerifier.java:16: private MockCertVerifier() {} On 2015/10/20 20:43:34, mef wrote: > Maybe constructor should call nativeCreateMockCertVerifier() and store the > pointer in member variable and getNativePointer() should be just a getter, not > creator? I had that initially, but Paul suggested using a static function to create a native pointer. Both approaches are fine with me. But using a static function avoids creating the java MockCertVerifier, which saves us a step. I agree we should change the name of getNativePointer to be more appropriate for this approach. https://codereview.chromium.org/1389213003/diff/320001/components/cronet/andr... components/cronet/android/test/src/org/chromium/net/MockCertVerifier.java:23: public static long getNativePointer(String[] certs) { On 2015/10/20 20:43:34, mef wrote: > Given that native cert verifier is not a singleton, would it make sense to name > this something like 'createMockCertVerifier' or 'createNativeVerifier' or > 'getNewNativeVerifier' or 'getNativePointerToNewVerifier'? Done. createMockCertVerifier sgtm. Agreed, the getNativePointer suggested it is a singleton which it is not.
The CQ bit was checked by xunjieli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pauljensen@chromium.org, jbudorick@chromium.org, mef@chromium.org Link to the patchset: https://codereview.chromium.org/1389213003/#ps340001 (title: "Address Misha's comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1389213003/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1389213003/340001
Message was sent while issue was closed.
Committed patchset #12 (id:340001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/013145fe3f51f959616a01174ccb1fc15737bffc Cr-Commit-Position: refs/heads/master@{#355186} |
