Refactor the API for CertVerifier::Verify() and the implementation of MultiThreadedCertVerifier::Ver

Refactor the API for CertVerifier::Verify() and the implementation of MultiThreadedCertVerifier::Verify().

* CertVerifier::Verify() fills a |scoped_pointer<Request>*| rather than a |void*|

* CertVerifier::CancelRequest(void*) has been removed. Requests are instead cancelled by resetting the scoped_ptr<Request>.

* Refactored memory ownership in MultiThreadedCertVerifier so there are no manual calls to "delete".

* Removed locking and the CertVerifierWorker in favor of using WorkerPool::PostTaskAndReply()

BUG=475153, 479336
TBR=pneubeck@chromium.org,zea@chromium.org,sergeyu@chromium.org

Committed: https://crrev.com/7f9236a3b9a7c0f7eb1d231c65027d0c4c535f3c
Cr-Commit-Position: refs/heads/master@{#329234}

[eroman, 2015-04-30 05:51:24]

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

[Alexander Potapenko, 2015-04-30 10:40:51]

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

[Alexander Potapenko, 2015-04-30 10:40:52]

https://codereview.chromium.org/1115903002/diff/20001/net/cert/multi_threaded...
File net/cert/multi_threaded_cert_verifier.cc (right):

https://codereview.chromium.org/1115903002/diff/20001/net/cert/multi_threaded...
net/cert/multi_threaded_cert_verifier.cc:373: int
MultiThreadedCertVerifier::Verify(X509Certificate* cert,
Can two threads call MultiThreadedCertVerifier::Verify() concurrently?

[eroman, 2015-04-30 17:56:17]

https://codereview.chromium.org/1115903002/diff/20001/net/cert/multi_threaded...
File net/cert/multi_threaded_cert_verifier.cc (right):

https://codereview.chromium.org/1115903002/diff/20001/net/cert/multi_threaded...
net/cert/multi_threaded_cert_verifier.cc:373: int
MultiThreadedCertVerifier::Verify(X509Certificate* cert,
On 2015/04/30 10:40:52, Alexander Potapenko wrote:
> Can two threads call MultiThreadedCertVerifier::Verify() concurrently?

No. A single instance of MultiThreadedCertVerifier lives and is operated
exclusively on the "origin thread" (i.e. IO thread).

The multi-threaded part is in how it handles requests -- it posts the actual
verification work over to the worker pool.

[eroman, 2015-04-30 17:59:49]

https://codereview.chromium.org/1115903002/diff/20001/net/cert/multi_threaded...
File net/cert/multi_threaded_cert_verifier.cc (right):

https://codereview.chromium.org/1115903002/diff/20001/net/cert/multi_threaded...
net/cert/multi_threaded_cert_verifier.cc:373: int
MultiThreadedCertVerifier::Verify(X509Certificate* cert,
On 2015/04/30 17:56:17, eroman wrote:
> On 2015/04/30 10:40:52, Alexander Potapenko wrote:
> > Can two threads call MultiThreadedCertVerifier::Verify() concurrently?
> 
> No. A single instance of MultiThreadedCertVerifier lives and is operated
> exclusively on the "origin thread" (i.e. IO thread).
> 
> The multi-threaded part is in how it handles requests -- it posts the actual
> verification work over to the worker pool.

Actually let me rephrase that:

An individual instance of MultiThreadedCertVerifier lives and is operated
exclusively from 1 thread (as per the DCHECK right below).

However it should be possible to create _separate_ instances of
MultiThreadedCertVerifier and use them concurrently from different threads. Not
sure that any code is currently doing that, but should be a supported mode.

[eroman, 2015-05-06 00:34:03]

Just a reminder in case this fell off your queue :)

[davidben, 2015-05-06 14:55:12]

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

[davidben, 2015-05-06 14:55:14]

It did slip through, sorry!

I reviewed everything but the meat of it, MultiThreadedCertVerifier, which does
a lot of changes. Ryan, do you mind if I toss this your way? You're probably a
better person to review large changes to that class.

https://codereview.chromium.org/1115903002/diff/20001/tools/valgrind/memcheck...
File tools/valgrind/memcheck/suppressions.txt (left):

https://codereview.chromium.org/1115903002/diff/20001/tools/valgrind/memcheck...
tools/valgrind/memcheck/suppressions.txt:1833:
fun:_ZN3net18SSLClientSocketNSS21OnHandshakeIOCompleteEi
Guessing this'll be already applied when you rebase past
https://codereview.chromium.org/1120503003/diff/1/tools/valgrind/memcheck/sup...
?

[eroman, 2015-05-06 21:39:20]

https://codereview.chromium.org/1115903002/diff/20001/tools/valgrind/memcheck...
File tools/valgrind/memcheck/suppressions.txt (left):

https://codereview.chromium.org/1115903002/diff/20001/tools/valgrind/memcheck...
tools/valgrind/memcheck/suppressions.txt:1833:
fun:_ZN3net18SSLClientSocketNSS21OnHandshakeIOCompleteEi
On 2015/05/06 14:55:13, David Benjamin wrote:
> Guessing this'll be already applied when you rebase past
>
https://codereview.chromium.org/1120503003/diff/1/tools/valgrind/memcheck/sup...
> ?

Correct. Rebased and it is gone now.

[Ryan Sleevi, 2015-05-06 22:31:48]

My biggest concern seems to be that this has an observable change in lifetime
semantics with respect to destruction, and while that "may" be OK, I want to
make sure to discuss it first.

In the existing code, if you have outstanding requests and the CertVerifier is
deleted, their callbacks are never invoked (due to the canceled_ flag).

In the new code, if you have outstanding requests and the CertVerifier is
deleted, ALL of the callbacks are invoked - and during the CertVerifier's dtor.

This makes it exceptionally dangerous with respect to inheritance, since
anything that composed atop this may find the vtable clobbered if one of the
callbacks were to assume the CertVerifier is live.

Now, we could argue that code is "bugged" because if there were outstanding
requests, the CertVerifier shouldn't have been deleted, or the callbacks should
have ensured the CertVerifier is alive, but it's certainly a change in semantics
and behaviour and it's hard to reason that code "does the right thing" here.

https://codereview.chromium.org/1115903002/diff/40001/net/cert/multi_threaded...
File net/cert/multi_threaded_cert_verifier.cc (left):

https://codereview.chromium.org/1115903002/diff/40001/net/cert/multi_threaded...
net/cert/multi_threaded_cert_verifier.cc:72: //
I can appreciate your simplification by removing this - that said, I find it
significantly more difficult to reason about the flow of objects in the new
class. It'd be nice if you provide some documentation - ASCII art or otherwise -
about how the objects and their lifetimes relate to eachother.

https://codereview.chromium.org/1115903002/diff/40001/net/cert/multi_threaded...
File net/cert/multi_threaded_cert_verifier.cc (right):

https://codereview.chromium.org/1115903002/diff/40001/net/cert/multi_threaded...
net/cert/multi_threaded_cert_verifier.cc:137: public
base::LinkNode<CertVerifierRequest> {
It seems like you should be listing concrete base classes first, before
interfaces (or at least, that's how I'm historically used to seeing it)

base::LinkNode comes w/ concrete behaviours, whereas CertVerifier::Request is
just an opaque interface.

https://codereview.chromium.org/1115903002/diff/40001/net/cert/multi_threaded...
net/cert/multi_threaded_cert_verifier.cc:182: scoped_refptr<X509Certificate>
cert,
Because you're not using base::Passed(), you're forcing additional ref-count
churn. Make these scoped_refptr<>'s const-ref, or make your bind call use
base::Passed()

My own recommendation is const-ref.

https://codereview.chromium.org/1115903002/diff/40001/net/cert/multi_threaded...
net/cert/multi_threaded_cert_verifier.cc:186: scoped_refptr<CRLSet> crl_set,
Here as well.

https://codereview.chromium.org/1115903002/diff/40001/net/cert/multi_threaded...
net/cert/multi_threaded_cert_verifier.cc:232: const
scoped_refptr<X509Certificate>& cert,
Why is it necessary to pass |cert| again, when you took it in the constructor?
This seems to be difficult-to-dangerous to reason about the correctness and
lifetime of it.

https://codereview.chromium.org/1115903002/diff/40001/net/cert/multi_threaded...
net/cert/multi_threaded_cert_verifier.cc:244: base::Bind(DoVerifyOnWorkerThread,
verify_proc, cert, hostname,
Consistency: Why don't you take the address of this function, compared to the
rest of the base::Bind() calls [yes, it's legal in some compilers not to, MSVC
historically has whinged about this, but either way, consistency]

https://codereview.chromium.org/1115903002/diff/40001/net/cert/multi_threaded...
net/cert/multi_threaded_cert_verifier.cc:248: weak_ptr_factory_.GetWeakPtr(),
base::Passed(&result)),
SECURITY BUG: You're not guaranteed in the order of releasing. That is, line 246
may execute AFTER line 248 has caused result to be .Pass()'d into base::Bind(),
thus causing "&result->error" to be equivalent to an offset off a nullptr.

https://codereview.chromium.org/1115903002/diff/40001/net/cert/multi_threaded...
net/cert/multi_threaded_cert_verifier.cc:334: bool is_first_job_ = false;
I can't find anything that confirms this "does the right thing" when you
explicitly declare a constructor intializer list (line 216)

Certainly, our style guide is somewhat silent on this, and I don't terribly want
to have to dig into C++ spec to confirm it's the 'right' thing.

https://codereview.chromium.org/1115903002/diff/40001/net/cert/multi_threaded...
net/cert/multi_threaded_cert_verifier.cc:338:
CertVerifierRequest::~CertVerifierRequest() {
Please move this to line 178 so that it's logically grouped with the class.
Since you did everything else inline in that class, it's unclear why this dtor
isn't also inline.

https://codereview.chromium.org/1115903002/diff/40001/net/cert/multi_threaded...
net/cert/multi_threaded_cert_verifier.cc:517: return *it;
Why are you not just using set::find? Or, if you're going to use lower_bound,
use set::lower_bound, for which the per-container algorithms MAY be more
optimized than the generic interator classes?

auto it = inflight_.find(key);

https://codereview.chromium.org/1115903002/diff/40001/net/cert/multi_threaded...
File net/cert/multi_threaded_cert_verifier.h (right):

https://codereview.chromium.org/1115903002/diff/40001/net/cert/multi_threaded...
net/cert/multi_threaded_cert_verifier.h:131: using JobSet =
std::set<CertVerifierJob*>;
I'm not terribly keen on this change. The original implementation stored the
Jobs ordered by their RequestParams, so it was a quick lookup. Your new
implementation sorts the set by the raw pointer values, which then forces a full
scan of the set when trying to find the matching object. This seems
algorithmically suboptimal.

https://codereview.chromium.org/1115903002/diff/40001/net/cert/multi_threaded...
net/cert/multi_threaded_cert_verifier.h:136: void SaveResultToCache(const
RequestParams& key, const CachedResult& result);
Document

https://codereview.chromium.org/1115903002/diff/40001/net/cert/multi_threaded...
net/cert/multi_threaded_cert_verifier.h:142: scoped_ptr<CertVerifierJob>
RemoveJob(CertVerifierJob* job);
Document

[eroman, 2015-05-06 22:38:35]

In regards to destruction, that should not be the case.

In both the new and old approach, deleting the CertVerifier will implicitly
cancel all the requests.

The exception is if you delete the CertVerifier from within a callback. In that
case requests attached to the current Job are not cancelled and will be called
even following destruction. This behavior is also unchanged from before, however
I left a TODO comment about it because doesn't seem consistent to me.

[Ryan Sleevi, 2015-05-06 22:58:03]

On 2015/05/06 22:38:35, eroman wrote:
> In regards to destruction, that should not be the case.
> 
> In both the new and old approach, deleting the CertVerifier will implicitly
> cancel all the requests.
> 
> The exception is if you delete the CertVerifier from within a callback. In
that
> case requests attached to the current Job are not cancelled and will be called
> even following destruction. This behavior is also unchanged from before,
however
> I left a TODO comment about it because doesn't seem consistent to me.

Thanks for pushing back - for my own sake, I'm documenting why that's the case

First, assume that CertVerifier::Request objects are still alive (both the old
and the new)

~MultiThreadedCertVerifier calls STLDeleteElements(&inflight_), which is filled
with CertVerifierJobs. Because the CertVerifier::Request's are attached, the
Jobs hold many request.
~CertVerifierJob goes through each attached request and calls
CertVerifierRequest::OnJobCancelled()
CertVerifierRequest::OnJobCancelled resets job_ and sets callback_ to nullptr.

~CertVerifierJob also resets the WeakPtrFactory, which ensures that
OnJobCompleted won't be called, and thus callbacks wont be posted.

[eroman, 2015-05-08 23:44:16]

https://codereview.chromium.org/1115903002/diff/40001/net/cert/multi_threaded...
File net/cert/multi_threaded_cert_verifier.cc (left):

https://codereview.chromium.org/1115903002/diff/40001/net/cert/multi_threaded...
net/cert/multi_threaded_cert_verifier.cc:72: //
On 2015/05/06 22:31:47, Ryan Sleevi wrote:
> I can appreciate your simplification by removing this - that said, I find it
> significantly more difficult to reason about the flow of objects in the new
> class. It'd be nice if you provide some documentation - ASCII art or otherwise
-
> about how the objects and their lifetimes relate to eachother.

It appears I went full Blink on you here!

Fixed. Added a comment block explaining how it works, including the cancellation
cases.

I opted not to use ASCII art because I couldn't really understand the earlier
diagram.

https://codereview.chromium.org/1115903002/diff/40001/net/cert/multi_threaded...
File net/cert/multi_threaded_cert_verifier.cc (right):

https://codereview.chromium.org/1115903002/diff/40001/net/cert/multi_threaded...
net/cert/multi_threaded_cert_verifier.cc:137: public
base::LinkNode<CertVerifierRequest> {
On 2015/05/06 22:31:47, Ryan Sleevi wrote:
> It seems like you should be listing concrete base classes first, before
> interfaces (or at least, that's how I'm historically used to seeing it)
> 
> base::LinkNode comes w/ concrete behaviours, whereas CertVerifier::Request is
> just an opaque interface.

Done.

https://codereview.chromium.org/1115903002/diff/40001/net/cert/multi_threaded...
net/cert/multi_threaded_cert_verifier.cc:182: scoped_refptr<X509Certificate>
cert,
On 2015/05/06 22:31:47, Ryan Sleevi wrote:
> Because you're not using base::Passed(), you're forcing additional ref-count
> churn. Make these scoped_refptr<>'s const-ref, or make your bind call use
> base::Passed()
> 
> My own recommendation is const-ref.

Done.

https://codereview.chromium.org/1115903002/diff/40001/net/cert/multi_threaded...
net/cert/multi_threaded_cert_verifier.cc:186: scoped_refptr<CRLSet> crl_set,
On 2015/05/06 22:31:47, Ryan Sleevi wrote:
> Here as well.

Done.

https://codereview.chromium.org/1115903002/diff/40001/net/cert/multi_threaded...
net/cert/multi_threaded_cert_verifier.cc:244: base::Bind(DoVerifyOnWorkerThread,
verify_proc, cert, hostname,
On 2015/05/06 22:31:47, Ryan Sleevi wrote:
> Consistency: Why don't you take the address of this function, compared to the
> rest of the base::Bind() calls [yes, it's legal in some compilers not to, MSVC
> historically has whinged about this, but either way, consistency]

Done.

https://codereview.chromium.org/1115903002/diff/40001/net/cert/multi_threaded...
net/cert/multi_threaded_cert_verifier.cc:248: weak_ptr_factory_.GetWeakPtr(),
base::Passed(&result)),
On 2015/05/06 22:31:47, Ryan Sleevi wrote:
> SECURITY BUG: You're not guaranteed in the order of releasing. That is, line
246
> may execute AFTER line 248 has caused result to be .Pass()'d into
base::Bind(),
> thus causing "&result->error" to be equivalent to an offset off a nullptr.

Ugh you are right.
Some days I really hate C++ and its undefined behaviors :(

Fixed

https://codereview.chromium.org/1115903002/diff/40001/net/cert/multi_threaded...
net/cert/multi_threaded_cert_verifier.cc:334: bool is_first_job_ = false;
On 2015/05/06 22:31:47, Ryan Sleevi wrote:
> I can't find anything that confirms this "does the right thing" when you
> explicitly declare a constructor intializer list (line 216)
> 
> Certainly, our style guide is somewhat silent on this, and I don't terribly
want
> to have to dig into C++ spec to confirm it's the 'right' thing.

I have seen this pattern used extensively in mojo code.
Not sure if that is the best argument for its use, but there, I said it.

Done -- moved it to initializer list.

https://codereview.chromium.org/1115903002/diff/40001/net/cert/multi_threaded...
net/cert/multi_threaded_cert_verifier.cc:338:
CertVerifierRequest::~CertVerifierRequest() {
On 2015/05/06 22:31:47, Ryan Sleevi wrote:
> Please move this to line 178 so that it's logically grouped with the class.
> Since you did everything else inline in that class, it's unclear why this dtor
> isn't also inline.

Done.

I had it here for legacy reasons -- I wasn't originally using LinkedList, so
cancellation needed to call into CertVerifierJob to detach the request (and
CertVerifierJob isn't defined until here).

https://codereview.chromium.org/1115903002/diff/40001/net/cert/multi_threaded...
net/cert/multi_threaded_cert_verifier.cc:517: return *it;
On 2015/05/06 22:31:47, Ryan Sleevi wrote:
> Why are you not just using set::find?

set::find() expects a CertVerifierJob*, however in this case the search is being
done by RequestParams (to de-duplicate).

I could synthesize a CertVerifierJob for that purpose, but it is a bit gross, so
opted to provide a more direct comparator instead.

> Or, if you're going to use lower_bound,
> use set::lower_bound, for which the per-container algorithms MAY be more
> optimized than the generic interator classes?

Same issue as above. set::lower_bound() expects a value-type

> 
> auto it = inflight_.find(key);

Done

https://codereview.chromium.org/1115903002/diff/40001/net/cert/multi_threaded...
File net/cert/multi_threaded_cert_verifier.h (right):

https://codereview.chromium.org/1115903002/diff/40001/net/cert/multi_threaded...
net/cert/multi_threaded_cert_verifier.h:131: using JobSet =
std::set<CertVerifierJob*>;
On 2015/05/06 22:31:48, Ryan Sleevi wrote:
> I'm not terribly keen on this change. The original implementation stored the
> Jobs ordered by their RequestParams, so it was a quick lookup. Your new
> implementation sorts the set by the raw pointer values, which then forces a
full
> scan of the set when trying to find the matching object. This seems
> algorithmically suboptimal.

Wow, how embarassing :(
Thanks for spotting this!

There is supposed to be a custom comparator for this set (otherwise the other
assumptions with using lower_bound() really don't make sense).

Fixed, and added a unit test that covers this.

https://codereview.chromium.org/1115903002/diff/40001/net/cert/multi_threaded...
net/cert/multi_threaded_cert_verifier.h:136: void SaveResultToCache(const
RequestParams& key, const CachedResult& result);
On 2015/05/06 22:31:48, Ryan Sleevi wrote:
> Document

Done.

https://codereview.chromium.org/1115903002/diff/40001/net/cert/multi_threaded...
net/cert/multi_threaded_cert_verifier.h:142: scoped_ptr<CertVerifierJob>
RemoveJob(CertVerifierJob* job);
On 2015/05/06 22:31:48, Ryan Sleevi wrote:
> Document

Done.

[Ryan Sleevi, 2015-05-09 00:30:19]

lgtm

https://codereview.chromium.org/1115903002/diff/80001/net/cert/multi_threaded...
File net/cert/multi_threaded_cert_verifier.cc (right):

https://codereview.chromium.org/1115903002/diff/80001/net/cert/multi_threaded...
net/cert/multi_threaded_cert_verifier.cc:63: //     If the request was in flight
(attached to a job), then it is detached.
s/in flight/in-flight/

[eroman, 2015-05-09 01:40:33]

https://codereview.chromium.org/1115903002/diff/80001/net/cert/multi_threaded...
File net/cert/multi_threaded_cert_verifier.cc (right):

https://codereview.chromium.org/1115903002/diff/80001/net/cert/multi_threaded...
net/cert/multi_threaded_cert_verifier.cc:63: //     If the request was in flight
(attached to a job), then it is detached.
On 2015/05/09 00:30:18, Ryan Sleevi wrote:
> s/in flight/in-flight/

Done.

[eroman, 2015-05-11 19:43:42]

The CQ bit was checked by eroman@chromium.org

[eroman, 2015-05-11 19:43:43]

The patchset sent to the CQ was uploaded after l-g-t-m from rsleevi@chromium.org
Link to the patchset: https://codereview.chromium.org/1115903002/#ps120001
(title: "rebase onto master")

[commit-bot: I haz the power, 2015-05-11 19:44:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1115903002/120001

[commit-bot: I haz the power, 2015-05-11 19:52:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-11 19:52:16]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[eroman, 2015-05-11 19:56:34]

eroman@chromium.org changed reviewers:
+ pneubeck@chromium.org, sergeyu@chromium.org, zea@chromium.org

[eroman, 2015-05-11 19:56:34]

TRBring owners for the files outside of net:

pneubeck@chromium.org --> chrome/browser/chromeos/policy/*
zea@chromium.org --> google_apis/gcm/*
sergeyu@chromium.org --> remoting/protocol/*

[eroman, 2015-05-11 20:00:46]

The CQ bit was checked by eroman@chromium.org

[eroman, 2015-05-11 20:00:46]

The patchset sent to the CQ was uploaded after l-g-t-m from rsleevi@chromium.org
Link to the patchset: https://codereview.chromium.org/1115903002/#ps140001
(title: "rebase again")

[commit-bot: I haz the power, 2015-05-11 20:02:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1115903002/140001

[commit-bot: I haz the power, 2015-05-11 20:08:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-11 20:08:34]

Try jobs failed on following builders:
  linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[eroman, 2015-05-11 20:42:40]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2015-05-11 20:43:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1115903002/140001

[commit-bot: I haz the power, 2015-05-11 21:23:54]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-05-11 21:24:46]

Patchset 8 (id:??) landed as
https://crrev.com/7f9236a3b9a7c0f7eb1d231c65027d0c4c535f3c
Cr-Commit-Position: refs/heads/master@{#329234}

[pneubeck (no reviews), 2015-05-12 09:20:16]

chrome/browser/chromeos/policy/*
lgtm with a comment at the CertVerifier::Verify documentation.

https://codereview.chromium.org/1115903002/diff/140001/net/cert/cert_verifier.h
File net/cert/cert_verifier.h (right):

https://codereview.chromium.org/1115903002/diff/140001/net/cert/cert_verifier...
net/cert/cert_verifier.h:102: // |*out_req| will be filled with a pointer to the
asynchronous request.
It seems that it is not guaranteed that out_req will be not null when Verify()
returns.
This descriptions sounds like it however.
Maybe you should state that out_req will only be filled if IO_PENDING is
returned, otherwise it's reset to nullptr.

[eroman, 2015-05-12 14:22:11]

https://codereview.chromium.org/1115903002/diff/140001/net/cert/cert_verifier.h
File net/cert/cert_verifier.h (right):

https://codereview.chromium.org/1115903002/diff/140001/net/cert/cert_verifier...
net/cert/cert_verifier.h:102: // |*out_req| will be filled with a pointer to the
asynchronous request.
On 2015/05/12 09:20:16, pneubeck wrote:
> It seems that it is not guaranteed that out_req will be not null when Verify()
> returns.
> This descriptions sounds like it however.
> Maybe you should state that out_req will only be filled if IO_PENDING is
> returned, otherwise it's reset to nullptr.

Thanks Phillip! I will address that in a follow-up change as this has already
landed.

[Nico, 2015-05-12 18:05:00]

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

[Nico, 2015-05-12 18:05:01]

A whole bunch of policy tests started failing on the 32-bit clang/win debug bot
(and only there):
http://build.chromium.org/p/chromium.fyi/builders/CrWinClang%28dbg%29%20teste...

This is the only CL touching "policy" files in the first bad build – could this
be related?

[eroman, 2015-05-12 18:21:02]

Weird I don't think so, let me look a bit more closely at those failure

[pneubeck (no reviews), 2015-05-13 10:13:07]

https://codereview.chromium.org/1115903002/diff/140001/net/cert/cert_verifier.h
File net/cert/cert_verifier.h (right):

https://codereview.chromium.org/1115903002/diff/140001/net/cert/cert_verifier...
net/cert/cert_verifier.h:26: class Request {
another thing that I stumbled over:

it's not documented nor disallowed to copy this request. I'd suggest doing the
latter by using the DISALLOW_COPY macro.

[pneubeck (no reviews), 2015-05-13 12:01:20]

https://codereview.chromium.org/1115903002/diff/140001/net/cert/cert_verifier.h
File net/cert/cert_verifier.h (right):

https://codereview.chromium.org/1115903002/diff/140001/net/cert/cert_verifier...
net/cert/cert_verifier.h:26: class Request {
On 2015/05/13 10:13:07, pneubeck wrote:
> another thing that I stumbled over:
> 
> it's not documented nor disallowed to copy this request. I'd suggest doing the
> latter by using the DISALLOW_COPY macro.

and, since the copy is not allowed anymore, I find one use case particularly
painful:

If I maintain a list of pending requests, I want to remove any request from that
list once it finished.
However, since the Request is not available before the Verify() call, it cannot
be bound to the callback.
Before, I could resolve that with a trick, namely store the RequestHandle (ie.
void*) in at a heap allocated memory address and pass that to the callback.

So, i think i have two possible solutions:
- I continue heap allocating a Request* and fill it with the Request's address
after the Verify call

- I maintain my own Handle per request, e.g. a counter and a map<Handle,
Request>. This is very cumbersome...

- You could change the Verify callback to obtain a Request*, which would be the
cleanest solution, IMO.

[eroman, 2015-05-14 23:28:09]

Philip, I do not understand your last comment. Can you point me to the code in
question? Is this checked in code, or something you are working on?

Let's continue that discussion either on the existing bug thread, or email

[eroman, 2015-05-15 02:52:31]

Created this bug to follow up on the feedback:
https://code.google.com/p/chromium/issues/detail?id=488325

And with that, I will stop spamming this codereview thread :)