Gave the GetOriginBoundCertificate an asynchronous interface because certificate

Gave the GetOriginBoundCertificate an asynchronous interface because certificate
generation is a blocking operation.

BUG=88782
TEST=None

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=96229

[rkn, 2011-08-04 01:26:32]

Please review.

The files origin_bound_cert_service.{h,cc} and
origin_bound_cert_service.temp.{h,cc} are identical, but the latter is the
result of an "svn cp" from cert_verifier.{h,cc} to see the diffs. I will remove
the "*temp*" files before committing.

[wtc, 2011-08-05 01:48:21]

Thanks for creating the "*temp*" files to help me code review.
I may ask you to walk me through the CL tomorrow or on Monday.

I skimmed through the CL.  Here are some suggested changes.

http://codereview.chromium.org/7565023/diff/6002/net/base/origin_bound_cert_s...
File net/base/origin_bound_cert_service.h (right):

http://codereview.chromium.org/7565023/diff/6002/net/base/origin_bound_cert_s...
net/base/origin_bound_cert_service.h:22: class OriginBoundCertServiceWorker;
I wonder if these two types should be Job and Worker
nested inside the OriginBoundCertService class.

http://codereview.chromium.org/7565023/diff/6002/net/base/origin_bound_cert_s...
net/base/origin_bound_cert_service.h:45: // certificate.
Add comments to describe the async behavior, out_req, and
the return value.

|private_key_result| and |cert_result| have been renamed.
Make the same change to the comments on lines 65-66 below.

http://codereview.chromium.org/7565023/diff/6002/net/base/origin_bound_cert_s...
net/base/origin_bound_cert_service.h:52: // Cancels the specified request. |req|
is the handle returned by Verify().
Verify => GetOriginBoundCert

http://codereview.chromium.org/7565023/diff/6002/net/base/origin_bound_cert_s...
net/base/origin_bound_cert_service.h:67: // OK if successful and ERR_FAILED
otherwise.
If the only possible return values are OK and ERR_FAILED,
please have the function return bool.

[rkn, 2011-08-08 22:08:32]

agl: Please do a high-level review. wtc and I will go through the details.

The files origin_bound_cert_service.{h,cc} and
origin_bound_cert_service.temp.{h,cc} are identical, but the latter is the
result of an "svn cp" from cert_verifier.{h,cc} to see the diffs. I will remove
the "*temp*" files before committing.

http://codereview.chromium.org/7565023/diff/21008/net/base/origin_bound_cert_...
File net/base/origin_bound_cert_service.temp.cc (right):

http://codereview.chromium.org/7565023/diff/21008/net/base/origin_bound_cert_...
net/base/origin_bound_cert_service.temp.cc:391:
DISABLE_RUNNABLE_METHOD_REFCOUNT(net::OriginBoundCertServiceWorker);
The only requirement for this macro is that the OriginBoundCertServiceWorker
outlive the RunnableMethod object. I'm still looking into whether this is the
case.

[rkn, 2011-08-08 22:09:05]

wtc: I have made the changes we talked about earlier today.

[agl, 2011-08-08 22:36:27]

I'm afraid that I don't have enough time to give this a good review and I'll be
traveling for the rest of the week.

Using the DNS cache code again means that it probably functions, although I
suspect that it's about time that it was abstracted out. (No need in this CL
however.)

http://codereview.chromium.org/7565023/diff/21008/net/base/origin_bound_cert_...
File net/base/origin_bound_cert_service.h (right):

http://codereview.chromium.org/7565023/diff/21008/net/base/origin_bound_cert_...
net/base/origin_bound_cert_service.h:69: uint64 cache_hits() const {return
cache_hits_; }
space before {

http://codereview.chromium.org/7565023/diff/21008/net/base/origin_bound_cert_...
net/base/origin_bound_cert_service.h:70: uint64 inflight_joins() const {return
inflight_joins_; }
ditto

http://codereview.chromium.org/7565023/diff/21008/net/base/origin_bound_cert_...
net/base/origin_bound_cert_service.h:82: uint32 serial_number,
A 32-bit serial number is really small. I know NSS will reject certificates with
duplicate issuer+serial number pairs. This might be a concern.

http://codereview.chromium.org/7565023/diff/21008/net/base/origin_bound_cert_...
File net/base/origin_bound_cert_service.temp.cc (right):

http://codereview.chromium.org/7565023/diff/21008/net/base/origin_bound_cert_...
net/base/origin_bound_cert_service.temp.cc:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
Should this file be included in the CL?

http://codereview.chromium.org/7565023/diff/21008/net/base/origin_bound_cert_...
File net/base/origin_bound_cert_service.temp.h (right):

http://codereview.chromium.org/7565023/diff/21008/net/base/origin_bound_cert_...
net/base/origin_bound_cert_service.temp.h:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
Should this file be included in the CL?

[wtc, 2011-08-09 00:43:33]

agl: thank you for the review comments.  I am reviewing the
CL carefully, so you just need to skim through it.

rkn: here are my review comments on Patch Set 5.

http://codereview.chromium.org/7565023/diff/21008/net/base/net_error_list.h
File net/base/net_error_list.h (right):

http://codereview.chromium.org/7565023/diff/21008/net/base/net_error_list.h#n...
net/base/net_error_list.h:577: // Origin-bound certificate generation failed.
This comment should say:
  Key pair generation failed.

Consider naming this error KEY_GENERATION_FAILED so that it
can also be used for symmetric key generation failure.

http://codereview.chromium.org/7565023/diff/21008/net/base/net_error_list.h#n...
net/base/net_error_list.h:586: // Failure to get DER-encoded cert.
The comments in this file are the default error messages
for these error codes, so they should be clearly written
and avoid abbreviations.

I wonder if we should name this error code
GET_CERT_BYTES_FAILED, or GET_ENCODED_CERT_FAILED, to avoid
mentioning "DER".

http://codereview.chromium.org/7565023/diff/21008/net/base/origin_bound_cert_...
File net/base/origin_bound_cert_service.cc (right):

http://codereview.chromium.org/7565023/diff/21008/net/base/origin_bound_cert_...
net/base/origin_bound_cert_service.cc:280: // No cache hit. See if an identical
request is currently in flight.
"No cache hit" should be updated because we aren't using
a cache.

http://codereview.chromium.org/7565023/diff/21008/net/base/origin_bound_cert_...
File net/base/origin_bound_cert_service.h (right):

http://codereview.chromium.org/7565023/diff/21008/net/base/origin_bound_cert_...
net/base/origin_bound_cert_service.h:45: // On success, |private_key| stores a
DER-encoded PrivateKeyInfo
On success => On successful completion

http://codereview.chromium.org/7565023/diff/21008/net/base/origin_bound_cert_...
net/base/origin_bound_cert_service.h:49: // could not be completed
synchronously, in which case the result code will
synchronously => immediately

http://codereview.chromium.org/7565023/diff/21008/net/base/origin_bound_cert_...
File net/base/origin_bound_cert_service.temp.cc (right):

http://codereview.chromium.org/7565023/diff/21008/net/base/origin_bound_cert_...
net/base/origin_bound_cert_service.temp.cc:54: // and calls the callback.
Perhaps "fields" should be changed to "output arguments".

http://codereview.chromium.org/7565023/diff/21008/net/base/origin_bound_cert_...
net/base/origin_bound_cert_service.temp.cc:113: &cert_);
Fix the indentation of the function arguments.

serial number does not need to be a member.  It can be a
local variable in this method; it is not used in any other
method.

http://codereview.chromium.org/7565023/diff/21008/net/base/origin_bound_cert_...
net/base/origin_bound_cert_service.temp.cc:138: origin_, error_, private_key_,
cert_);
Nit: I think it's better to format this as:
  origin_bound_cert_service_->HandleResult(origin_, error_,
                                           private_key_, cert_);

http://codereview.chromium.org/7565023/diff/21008/net/base/origin_bound_cert_...
File net/base/origin_bound_cert_service.temp.h (right):

http://codereview.chromium.org/7565023/diff/21008/net/base/origin_bound_cert_...
net/base/origin_bound_cert_service.temp.h:52: // If |out_req| is non_NULL, then
|*out_req| will be filled with a handle to
"non-NULL" is correct, right?  We should not use an underscore
here.

http://codereview.chromium.org/7565023/diff/21008/net/base/origin_bound_cert_...
net/base/origin_bound_cert_service.temp.h:67: int get_cert_count();
Remove "get_".  The getters don't need the "get_" prefix
when in this form.

http://codereview.chromium.org/7565023/diff/21008/net/base/origin_bound_cert_...
net/base/origin_bound_cert_service.temp.h:70: uint64 inflight_joins() const
{return inflight_joins_; }
Add a space after the opening curly brace '{' on lines 69
and 70.

http://codereview.chromium.org/7565023/diff/21008/net/base/origin_bound_cert_...
net/base/origin_bound_cert_service.temp.h:79: // base::RandInt, which opens the
file /dev/urandom. This is done with a
This is done => /dev/urandom is opened

http://codereview.chromium.org/7565023/diff/21008/net/base/origin_bound_cert_...
net/base/origin_bound_cert_service.temp.h:98: uint64 cache_hits_;
"cache hits" is not right because we aren't using a cache.

http://codereview.chromium.org/7565023/diff/21008/net/base/origin_bound_cert_...
File net/base/origin_bound_cert_service_unittest.cc (right):

http://codereview.chromium.org/7565023/diff/21008/net/base/origin_bound_cert_...
net/base/origin_bound_cert_service_unittest.cc:78: std::string
origin1("https://encrypted.google.com:443");
Is the explicit :433 port required in an origin?  Just wondering.

http://codereview.chromium.org/7565023/diff/21008/net/base/origin_bound_cert_...
net/base/origin_bound_cert_service_unittest.cc:204: std::string(1, (char) ('a' +
i)),
Should we construct more realistic origins?

http://codereview.chromium.org/7565023/diff/21008/net/base/origin_bound_cert_...
net/base/origin_bound_cert_service_unittest.cc:208: &request_handle);
Align the function arguments.

[rkn, 2011-08-09 03:25:38]

http://codereview.chromium.org/7565023/diff/21008/net/base/origin_bound_cert_...
File net/base/origin_bound_cert_service.temp.cc (right):

http://codereview.chromium.org/7565023/diff/21008/net/base/origin_bound_cert_...
net/base/origin_bound_cert_service.temp.cc:113: &cert_);
On 2011/08/09 00:43:34, wtc wrote:
> Fix the indentation of the function arguments.
> 
> serial number does not need to be a member.  It can be a
> local variable in this method; it is not used in any other
> method.

Isn't the problem that the entire "Run()" function is done on the worker thread?
So if serial_number_ is a local variable, then we will still be calling
RandInt() on the worker thread.

http://codereview.chromium.org/7565023/diff/21008/net/base/origin_bound_cert_...
File net/base/origin_bound_cert_service_unittest.cc (right):

http://codereview.chromium.org/7565023/diff/21008/net/base/origin_bound_cert_...
net/base/origin_bound_cert_service_unittest.cc:78: std::string
origin1("https://encrypted.google.com:443");
On 2011/08/09 00:43:34, wtc wrote:
> Is the explicit :433 port required in an origin?  Just wondering.

I don't believe so.

[wtc, 2011-08-09 18:09:47]

LGTM.  I reviewed Patch Set 6.

http://codereview.chromium.org/7565023/diff/21008/net/base/origin_bound_cert_...
File net/base/origin_bound_cert_service.temp.cc (right):

http://codereview.chromium.org/7565023/diff/21008/net/base/origin_bound_cert_...
net/base/origin_bound_cert_service.temp.cc:113: &cert_);
On 2011/08/09 03:25:38, rkn wrote:
>
> Isn't the problem that the entire "Run()" function is done on the worker
thread?
> So if serial_number_ is a local variable, then we will still be calling
> RandInt() on the worker thread.

You are right.  I am confused.  It may be worthwhile to
add a comment to note that serial_number_ has to be
initialized on a non-worker thread to avoid calling
base::RandInt on a worker thread.

http://codereview.chromium.org/7565023/diff/22028/net/base/origin_bound_cert_...
File net/base/origin_bound_cert_service.h (right):

http://codereview.chromium.org/7565023/diff/22028/net/base/origin_bound_cert_...
net/base/origin_bound_cert_service.h:98: uint64 synchronous_completions_;
Let's call this member cert_store_hits_.  "synchronous
completions" is too indirect.

[commit-bot: I haz the power, 2011-08-10 22:31:56]

Change committed as 96229