[Cronet] Continue UrlRequest with NULL certificate if Client cert is requested.

[Cronet] Continue UrlRequest with NULL certificate if Client cert is requested.

TEST=CronetUrlRequestTest.testMockClientCertificateRequested
BUG=558420
Committed: https://crrev.com/abc0bae34bdc9456d7e4a1f1aa2c6aa51a4bad4d
Cr-Commit-Position: refs/heads/master@{#361130}

[mef, 2015-11-20 16:19:51]

mef@chromium.org changed reviewers:
+ svaldez@chromium.org, xunjieli@chromium.org

[mef, 2015-11-20 16:19:52]

PTAL

[svaldez, 2015-11-20 16:30:11]

https://codereview.chromium.org/1459993003/diff/40001/components/cronet/andro...
File components/cronet/android/cronet_url_request_adapter.cc (right):

https://codereview.chromium.org/1459993003/diff/40001/components/cronet/andro...
components/cronet/android/cronet_url_request_adapter.cc:274: void
CronetURLRequestAdapter::OnCertificateRequested(
Move above OnSSLCertificateError to match header ordering.

https://codereview.chromium.org/1459993003/diff/40001/components/cronet/andro...
File components/cronet/android/cronet_url_request_adapter.h (right):

https://codereview.chromium.org/1459993003/diff/40001/components/cronet/andro...
components/cronet/android/cronet_url_request_adapter.h:122: virtual void
OnCertificateRequested(
virtual?

https://codereview.chromium.org/1459993003/diff/40001/components/cronet/andro...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java
(right):

https://codereview.chromium.org/1459993003/diff/40001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:579:
public void testMockCertRequested() throws Exception {
nit: Move above testMockSSLCertificateError to match URLRequest ordering.

https://codereview.chromium.org/1459993003/diff/40001/net/test/url_request/ur...
File net/test/url_request/url_request_failed_job.cc (right):

https://codereview.chromium.org/1459993003/diff/40001/net/test/url_request/ur...
net/test/url_request/url_request_failed_job.cc:74: CHECK_LE(phase,
URLRequestFailedJob::FailurePhase::CERT_REQUESTED);
Shouldn't this just be CHECK_LT(..., MAX_FAILURE_PHASE)?

https://codereview.chromium.org/1459993003/diff/40001/net/test/url_request/ur...
net/test/url_request/url_request_failed_job.cc:91: CHECK_LE(phase,
URLRequestFailedJob::FailurePhase::CERT_REQUESTED);
see above.

https://codereview.chromium.org/1459993003/diff/40001/net/test/url_request/ur...
File net/test/url_request/url_request_failed_job.h (right):

https://codereview.chromium.org/1459993003/diff/40001/net/test/url_request/ur...
net/test/url_request/url_request_failed_job.h:17: class SSLCertRequestInfo;
unnecessary? Not used in header?

[mef, 2015-11-20 16:44:06]

Thanks, PTAL.

https://codereview.chromium.org/1459993003/diff/40001/components/cronet/andro...
File components/cronet/android/cronet_url_request_adapter.cc (right):

https://codereview.chromium.org/1459993003/diff/40001/components/cronet/andro...
components/cronet/android/cronet_url_request_adapter.cc:274: void
CronetURLRequestAdapter::OnCertificateRequested(
On 2015/11/20 16:30:10, svaldez wrote:
> Move above OnSSLCertificateError to match header ordering.

Done.

https://codereview.chromium.org/1459993003/diff/40001/components/cronet/andro...
File components/cronet/android/cronet_url_request_adapter.h (right):

https://codereview.chromium.org/1459993003/diff/40001/components/cronet/andro...
components/cronet/android/cronet_url_request_adapter.h:122: virtual void
OnCertificateRequested(
On 2015/11/20 16:30:10, svaldez wrote:
> virtual?

Oops. Done.

https://codereview.chromium.org/1459993003/diff/40001/components/cronet/andro...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java
(right):

https://codereview.chromium.org/1459993003/diff/40001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:579:
public void testMockCertRequested() throws Exception {
On 2015/11/20 16:30:10, svaldez wrote:
> nit: Move above testMockSSLCertificateError to match URLRequest ordering.

Done.

https://codereview.chromium.org/1459993003/diff/40001/net/test/url_request/ur...
File net/test/url_request/url_request_failed_job.cc (right):

https://codereview.chromium.org/1459993003/diff/40001/net/test/url_request/ur...
net/test/url_request/url_request_failed_job.cc:74: CHECK_LE(phase,
URLRequestFailedJob::FailurePhase::CERT_REQUESTED);
On 2015/11/20 16:30:10, svaldez wrote:
> Shouldn't this just be CHECK_LT(..., MAX_FAILURE_PHASE)?

Done.

https://codereview.chromium.org/1459993003/diff/40001/net/test/url_request/ur...
net/test/url_request/url_request_failed_job.cc:91: CHECK_LE(phase,
URLRequestFailedJob::FailurePhase::CERT_REQUESTED);
On 2015/11/20 16:30:10, svaldez wrote:
> see above.

Done.

https://codereview.chromium.org/1459993003/diff/40001/net/test/url_request/ur...
File net/test/url_request/url_request_failed_job.h (right):

https://codereview.chromium.org/1459993003/diff/40001/net/test/url_request/ur...
net/test/url_request/url_request_failed_job.h:17: class SSLCertRequestInfo;
On 2015/11/20 16:30:10, svaldez wrote:
> unnecessary? Not used in header?

Done.

[xunjieli, 2015-11-20 18:59:59]

https://codereview.chromium.org/1459993003/diff/60001/components/cronet/andro...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java
(right):

https://codereview.chromium.org/1459993003/diff/60001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:563:
final int arbitraryNetError = -3;
It is a bit counterintuitive that we are reporting ERR_ABORTED even though we
decide to ContinueWithCertificate. 

Is it possible to make URLRequestMockDataJob or some other mock job to override
ContinueWithCertificate to continue with the request, and actually fetch some
response data?

That way the ContinueWithCertificate will actually mean ignore the cert error
and carry on with the request. Might be easier to understand.

[davidben, 2015-11-20 19:02:35]

davidben@chromium.org changed reviewers:
+ davidben@chromium.org

[davidben, 2015-11-20 19:02:36]

https://codereview.chromium.org/1459993003/diff/60001/components/cronet/andro...
File components/cronet/android/cronet_url_request_adapter.cc (right):

https://codereview.chromium.org/1459993003/diff/60001/components/cronet/andro...
components/cronet/android/cronet_url_request_adapter.cc:265:
request->ContinueWithCertificate(NULL, NULL);
(Drive-by nit: nullptr instead of NULL for new code.)

[davidben, 2015-11-20 19:02:43]

Description was changed from

==========
[Cronet] Continue UrlRequest with NULL certificate if Client cert is requested.

BUG=558420
==========

to

==========
[Cronet] Continue UrlRequest with NULL certificate if Client cert is requested.

BUG=558420
==========

[davidben, 2015-11-20 19:02:43]

davidben@chromium.org changed reviewers:
- davidben@chromium.org

[mef, 2015-11-20 20:51:49]

Thanks, PTAL!

https://codereview.chromium.org/1459993003/diff/60001/components/cronet/andro...
File components/cronet/android/cronet_url_request_adapter.cc (right):

https://codereview.chromium.org/1459993003/diff/60001/components/cronet/andro...
components/cronet/android/cronet_url_request_adapter.cc:265:
request->ContinueWithCertificate(NULL, NULL);
On 2015/11/20 19:02:35, davidben (slow) wrote:
> (Drive-by nit: nullptr instead of NULL for new code.)

Done.

https://codereview.chromium.org/1459993003/diff/60001/components/cronet/andro...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java
(right):

https://codereview.chromium.org/1459993003/diff/60001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:563:
final int arbitraryNetError = -3;
On 2015/11/20 18:59:59, xunjieli wrote:
> It is a bit counterintuitive that we are reporting ERR_ABORTED even though we
> decide to ContinueWithCertificate. 
> 
> Is it possible to make URLRequestMockDataJob or some other mock job to
override
> ContinueWithCertificate to continue with the request, and actually fetch some
> response data?
> 
> That way the ContinueWithCertificate will actually mean ignore the cert error
> and carry on with the request. Might be easier to understand.

Good point. Done.

[xunjieli, 2015-11-20 21:04:25]

https://codereview.chromium.org/1459993003/diff/80001/components/cronet/andro...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java
(right):

https://codereview.chromium.org/1459993003/diff/80001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:563:
final int arbitraryNetError = -606;
nit: unused, and in comment.

https://codereview.chromium.org/1459993003/diff/80001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:566:
assertNotNull(callback.mResponseInfo);
Maybe assert on getHttpStatusCode and mResponseAsString.

https://codereview.chromium.org/1459993003/diff/80001/components/cronet/andro...
File
components/cronet/android/test/src/org/chromium/net/MockUrlRequestJobFactory.java
(right):

https://codereview.chromium.org/1459993003/diff/80001/components/cronet/andro...
components/cronet/android/test/src/org/chromium/net/MockUrlRequestJobFactory.java:57:
* Constructs a mock URL that will request Client Certificate.
nit: maybe not capitalize client certificate?
s/Client Certificate/a client certificate.

https://codereview.chromium.org/1459993003/diff/80001/net/test/url_request/ur...
File net/test/url_request/url_request_mock_data_job.h (right):

https://codereview.chromium.org/1459993003/diff/80001/net/test/url_request/ur...
net/test/url_request/url_request_mock_data_job.h:18: // Mock data job, which
synchronously returns data repeated multiple times.
Maybe also say that when |request_client_certificate| is true, this job will ask
for a client certificate before proceeding.

https://codereview.chromium.org/1459993003/diff/80001/net/test/url_request/ur...
net/test/url_request/url_request_mock_data_job.h:44: // Constructs Mock URL
which will request client certificate.
Maybe mention that the response body is the word "data"

[mef, 2015-11-20 22:01:23]

Thanks, PTAL.

https://codereview.chromium.org/1459993003/diff/80001/components/cronet/andro...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java
(right):

https://codereview.chromium.org/1459993003/diff/80001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:563:
final int arbitraryNetError = -606;
On 2015/11/20 21:04:25, xunjieli wrote:
> nit: unused, and in comment.

Done.

https://codereview.chromium.org/1459993003/diff/80001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java:566:
assertNotNull(callback.mResponseInfo);
On 2015/11/20 21:04:25, xunjieli wrote:
> Maybe assert on getHttpStatusCode and mResponseAsString.

Done.

https://codereview.chromium.org/1459993003/diff/80001/components/cronet/andro...
File
components/cronet/android/test/src/org/chromium/net/MockUrlRequestJobFactory.java
(right):

https://codereview.chromium.org/1459993003/diff/80001/components/cronet/andro...
components/cronet/android/test/src/org/chromium/net/MockUrlRequestJobFactory.java:57:
* Constructs a mock URL that will request Client Certificate.
On 2015/11/20 21:04:25, xunjieli wrote:
> nit: maybe not capitalize client certificate?
> s/Client Certificate/a client certificate.

Done.

https://codereview.chromium.org/1459993003/diff/80001/net/test/url_request/ur...
File net/test/url_request/url_request_mock_data_job.h (right):

https://codereview.chromium.org/1459993003/diff/80001/net/test/url_request/ur...
net/test/url_request/url_request_mock_data_job.h:18: // Mock data job, which
synchronously returns data repeated multiple times.
On 2015/11/20 21:04:25, xunjieli wrote:
> Maybe also say that when |request_client_certificate| is true, this job will
ask
> for a client certificate before proceeding.

Done.

https://codereview.chromium.org/1459993003/diff/80001/net/test/url_request/ur...
net/test/url_request/url_request_mock_data_job.h:44: // Constructs Mock URL
which will request client certificate.
On 2015/11/20 21:04:25, xunjieli wrote:
> Maybe mention that the response body is the word "data"

Done.

[xunjieli, 2015-11-20 22:09:33]

lgtm

[mef, 2015-11-20 22:26:43]

Description was changed from

==========
[Cronet] Continue UrlRequest with NULL certificate if Client cert is requested.

BUG=558420
==========

to

==========
[Cronet] Continue UrlRequest with NULL certificate if Client cert is requested.

TEST=CronetUrlRequestTest.testMockClientCertificateRequested
BUG=558420
==========

[svaldez, 2015-11-23 15:45:11]

lgtm

[svaldez, 2015-11-23 15:45:15]

lgtm

[mef, 2015-11-23 15:47:29]

The CQ bit was checked by mef@chromium.org

[commit-bot: I haz the power, 2015-11-23 15:48:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1459993003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1459993003/100001

[commit-bot: I haz the power, 2015-11-23 17:21:49]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-11-23 17:22:59]

Patchset 6 (id:??) landed as
https://crrev.com/abc0bae34bdc9456d7e4a1f1aa2c6aa51a4bad4d
Cr-Commit-Position: refs/heads/master@{#361130}