Add support for walking and modifying the CachingCertVerifier

Add support for walking and modifying the CachingCertVerifier

In anticipation of experimenting with persisting the certificate
verification cache across process evictions on mobile, add the
ability to iterate through the cache's contents (allowing it to
be serialized) and to restore the contents later.

Because restoration is expected to be disjoint from object
creation, rather than requiring the deserialized entries be
presented at construction, allow them to be provided after the
cache already has results, provided that no other results are
evicted.

BUG=590875
Committed: https://crrev.com/b25d90c36cbf578cd8459f1e0acfd26930d0e139
Cr-Commit-Position: refs/heads/master@{#400100}

[Ryan Sleevi, 2016-05-20 08:39:28]

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

[Ryan Sleevi, 2016-05-20 08:39:29]

Raman: This offers a skeletal outline that you can then build upon up in the
Android code.

I haven't written tests yet because I've got a flight to catch in a few hours,
but hopefully this gives a good framework about how to hide the implementation
details appropriately, which was a big concern with your approach. Nothing a
little void* magic can't hide :)

[ramant (doing other things), 2016-05-20 17:56:47]

On 2016/05/20 08:39:29, Ryan Sleevi wrote:
> Raman: This offers a skeletal outline that you can then build upon up in the
> Android code.
> 
> I haven't written tests yet because I've got a flight to catch in a few hours,
> but hopefully this gives a good framework about how to hide the implementation
> details appropriately, which was a big concern with your approach. Nothing a
> little void* magic can't hide :)

Hi Ryan,
  Will build persistence and other code on top of this code. Hope you have a
safe flight.

-raman

[ramant (doing other things), 2016-06-10 21:34:17]

Description was changed from

==========
Add support for walking and modifying the CachingCertVerifier

In anticipation of experimenting with persisting the certificate
verification cache across process evictions on mobile, add the
ability to iterate through the cache's contents (allowing it to
be serialized) and to restore the contents later.

Because restoration is expected to be disjoint from object
creation, rather than requiring the deserialized entries be
presented at construction, allow them to be provided after the
cache already has results, provided that no other results are
evicted.

BUG=590875
==========

to

==========
Add support for walking and modifying the CachingCertVerifier

In anticipation of experimenting with persisting the certificate
verification cache across process evictions on mobile, add the
ability to iterate through the cache's contents (allowing it to
be serialized) and to restore the contents later.

Because restoration is expected to be disjoint from object
creation, rather than requiring the deserialized entries be
presented at construction, allow them to be provided after the
cache already has results, provided that no other results are
evicted.

BUG=590875
==========

[ramant (doing other things), 2016-06-10 21:35:24]

Hi Ryan,
  Not sure if you wanted me to wait for unittests or not. Changes in this CL:
LGTM

[Ryan Sleevi, 2016-06-13 13:04:35]

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

[Ryan Sleevi, 2016-06-13 13:04:41]

Adding Eric for review since he's been (helpfully) very critical on the other
CLs, which have all landed now.

[eroman, 2016-06-13 22:49:35]

Do you have unit tests for this? (Per your initial comment).

From an interface perspective have you considered these alternatives:

  (a) Making the internal cache a protected member, and requiring a separate
serialize-to-disk implementation through inheritance (which can poke at the
cache member directly). 

  (b) Rather than exposing iteration, read all the entries to a std::vector.

  (c) Rather than exposing iteration, have a visitor pattern for cache entries.


My reservation with the Iterator is it adds a public interface that is easy to
mis-use.

https://codereview.chromium.org/1999733002/diff/1/net/cert/caching_cert_verif...
File net/cert/caching_cert_verifier.h (right):

https://codereview.chromium.org/1999733002/diff/1/net/cert/caching_cert_verif...
net/cert/caching_cert_verifier.h:29: explicit Iterator(const
CachingCertVerifier& verifier);
I would have said this is clearer as a pointer rather than reference type given
how this is coupled to the lifetime of |verifier|.... But looks like the
underlying cache iterator using this same interface.

Can you  add a comment explaining that |verifier| needs to outlive |this| ?
(Technically a bit stronger requirement, but easier to explain).

https://codereview.chromium.org/1999733002/diff/1/net/cert/caching_cert_verif...
net/cert/caching_cert_verifier.h:32: bool HasNext() const;
Iteration currently exposes expired entries right?

Should a compaction be done before iterating, or is the expectation on the
client to not bother serializing expired entries?

https://codereview.chromium.org/1999733002/diff/1/net/cert/caching_cert_verif...
net/cert/caching_cert_verifier.h:43: };
Disallow copy and assign.

https://codereview.chromium.org/1999733002/diff/1/net/cert/caching_cert_verif...
net/cert/caching_cert_verifier.h:65: // Opportunistically attempt to add |error|
and |verify_result| as the
style nit: use descriptive rather than imperative comment. i.e. "attempt" -->
"attempts"

https://codereview.chromium.org/1999733002/diff/1/net/cert/caching_cert_verif...
net/cert/caching_cert_verifier.h:70: // exists).
nit: Mention how how this relates to the return value.

[Ryan Sleevi, 2016-06-13 23:00:15]

On 2016/06/13 22:49:35, eroman wrote:
> Do you have unit tests for this? (Per your initial comment).

Will do.

> From an interface perspective have you considered these alternatives:
> 
>   (a) Making the internal cache a protected member, and requiring a separate
> serialize-to-disk implementation through inheritance (which can poke at the
> cache member directly). 

I didn't like this because it exposes too much of the implementation detail; for
example, it allows fresher cached entries to be overwritten (a bug in Raman's
early CL)

>   (b) Rather than exposing iteration, read all the entries to a std::vector.

That seems to add overhead for copying, and I'm unclear the value.

>   (c) Rather than exposing iteration, have a visitor pattern for cache
entries.

I'm curious how you see this design pattern resulting in any different code. The
iterator is effectively a visitor in which the caller drives. A visitor pattern
where you have to visit every instance is inefficient.

> My reservation with the Iterator is it adds a public interface that is easy to
> mis-use.

My choice of Iterator was that it was the most difficult interface to misuse.
I'm curious what risks you see, or how you see the others address?

[eroman, 2016-06-13 23:39:40]

> My choice of Iterator was that it was the most difficult interface to misuse.
> I'm curious what risks you see, or how you see the others address?

Lifetime of the iterator and mutation.Having the Iterator outlive the current
stack frame for instance is likely incorrect -- the cache could be cleared
through observer interface, or other mutations could happen.

From past experiences, I have learned that the moment you expose iteration as an
interface, random code may start depending on it and might even depend on the
iteration order in surprising ways. Not really concerned about that here, but
since you were asking why I don't generally like these patterns these are some
of the thoughts in my head.

> I'm curious how you see this design pattern resulting in any different code.
The
> iterator is effectively a visitor in which the caller drives. A visitor
pattern
> where you have to visit every instance is inefficient.

The visitor pattern internalizes the lifetime of the cache iterator, which
avoids a certain class of mis-uses (i.e. resuming iteration after the cache has
been deleted, or after the stack has unwound and another mutation occurred to
invalidate iterators).

If you are comfortable with exposing an iterator that is fine. I wasn't
requiring you to change, but wondering about what designs you considered before
settling on this.

I'll give another review pass after the tests are added.

[Ryan Sleevi, 2016-06-13 23:41:52]

On 2016/06/13 23:39:40, eroman wrote:
> The visitor pattern internalizes the lifetime of the cache iterator, which
> avoids a certain class of mis-uses (i.e. resuming iteration after the cache
has
> been deleted, or after the stack has unwound and another mutation occurred to
> invalidate iterators).

Fair enough - I can totally buy that argument. I'll adopt a Visitor pattern
here, which should not negatively impact Raman's code building on this.

[Ryan Sleevi, 2016-06-15 22:54:08]

Eric: Please take another look based on the feedback
Raman: Heads up that the interface substantially changed for your dependent CL

[ramant (doing other things), 2016-06-15 22:59:03]

On 2016/06/15 22:54:08, Ryan Sleevi wrote:
> Eric: Please take another look based on the feedback
> Raman: Heads up that the interface substantially changed for your dependent CL

Thanks much Ryan. Will make changes to my CL using the latest version from this
CL.

[eroman, 2016-06-16 00:25:04]

thanks for making those changes!

LGTM

https://codereview.chromium.org/1999733002/diff/1/net/cert/caching_cert_verif...
File net/cert/caching_cert_verifier.h (right):

https://codereview.chromium.org/1999733002/diff/1/net/cert/caching_cert_verif...
net/cert/caching_cert_verifier.h:70: // exists).
On 2016/06/13 22:49:35, eroman wrote:
> nit: Mention how how this relates to the return value.

ping? (My request was to comment that returns true if an entry was added)

https://codereview.chromium.org/1999733002/diff/60001/net/cert/caching_cert_v...
File net/cert/caching_cert_verifier.cc (right):

https://codereview.chromium.org/1999733002/diff/60001/net/cert/caching_cert_v...
net/cert/caching_cert_verifier.cc:210: Iter it(cache_);
optional: Move the definition of "it" into the for loop. Does that work with
ctor?

https://codereview.chromium.org/1999733002/diff/60001/net/cert/caching_cert_v...
File net/cert/caching_cert_verifier.h (right):

https://codereview.chromium.org/1999733002/diff/60001/net/cert/caching_cert_v...
net/cert/caching_cert_verifier.h:39: // Visitor class to allow read-only
inspection of the verification cache.
optional: Either here or in VisitEntries() mention that the CachingCertVerifier
should not be mutated, as it may invalidate iteration.

https://codereview.chromium.org/1999733002/diff/60001/net/cert/caching_cert_v...
net/cert/caching_cert_verifier.h:42: virtual ~CacheVisitor() {}
optional: Move to an out-of-line dtor in .cc file, and use "= default"

https://codereview.chromium.org/1999733002/diff/60001/net/cert/caching_cert_v...
net/cert/caching_cert_verifier.h:47: virtual bool VisitEntry(const
RequestParams& params,
optional: Name this VisitCertificateCacheEntry(). The more specific name may be
convenient if you want to smash it into an existing class.

Or alternately instead of a visitor class use a callback -- this might be
convenient for callers wanting to writing an inline visitor through lambda, or
base::Bind()ing stuff.

At any rate, I am happy with the visitor class as written.

https://codereview.chromium.org/1999733002/diff/60001/net/cert/caching_cert_v...
net/cert/caching_cert_verifier.h:91: void VisitEntries(CacheVisitor* visitor);
Can this method be "const" ?

https://codereview.chromium.org/1999733002/diff/60001/net/cert/caching_cert_v...
net/cert/caching_cert_verifier.h:94: friend class CacheVisitor;
Is this needed?

[Ryan Sleevi, 2016-06-16 00:44:34]

https://codereview.chromium.org/1999733002/diff/60001/net/cert/caching_cert_v...
File net/cert/caching_cert_verifier.h (right):

https://codereview.chromium.org/1999733002/diff/60001/net/cert/caching_cert_v...
net/cert/caching_cert_verifier.h:47: virtual bool VisitEntry(const
RequestParams& params,
On 2016/06/16 00:25:04, eroman wrote:
> optional: Name this VisitCertificateCacheEntry(). The more specific name may
be
> convenient if you want to smash it into an existing class.

I'm pretty anti-smashing of pure interfaces :)

> Or alternately instead of a visitor class use a callback -- this might be
> convenient for callers wanting to writing an inline visitor through lambda, or
> base::Bind()ing stuff.

They wouldn't be able to mix a lambda with a callback at present, and
callbacks-called-multiple-times are an anti-pattern (e.g. if you're curring an
owned pointer, everything gets wrong).

> At any rate, I am happy with the visitor class as written.

SG, opted for no change.

https://codereview.chromium.org/1999733002/diff/60001/net/cert/caching_cert_v...
net/cert/caching_cert_verifier.h:91: void VisitEntries(CacheVisitor* visitor);
On 2016/06/16 00:25:04, eroman wrote:
> Can this method be "const" ?

I saw no reason to impose that contract on implementations, since from the POV
of this interface, the const-ness is irrelevant, but imposing it would be
limiting (e.g. if you wanted to build up a serialized version in a member field)

https://codereview.chromium.org/1999733002/diff/60001/net/cert/caching_cert_v...
net/cert/caching_cert_verifier.h:94: friend class CacheVisitor;
On 2016/06/16 00:25:04, eroman wrote:
> Is this needed?

Nope

[Ryan Sleevi, 2016-06-16 00:45:05]

The CQ bit was checked by rsleevi@chromium.org

[Ryan Sleevi, 2016-06-16 00:45:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
rtenneti@chromium.org, eroman@chromium.org
Link to the patchset: https://codereview.chromium.org/1999733002/#ps80001
(title: "Review remarks")

[commit-bot: I haz the power, 2016-06-16 00:45:36]

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

[eroman, 2016-06-16 00:54:51]

https://codereview.chromium.org/1999733002/diff/60001/net/cert/caching_cert_v...
File net/cert/caching_cert_verifier.h (right):

https://codereview.chromium.org/1999733002/diff/60001/net/cert/caching_cert_v...
net/cert/caching_cert_verifier.h:91: void VisitEntries(CacheVisitor* visitor);
On 2016/06/16 00:44:34, Ryan Sleevi wrote:
> On 2016/06/16 00:25:04, eroman wrote:
> > Can this method be "const" ?
> 
> I saw no reason to impose that contract on implementations, since from the POV
> of this interface, the const-ness is irrelevant, but imposing it would be
> limiting (e.g. if you wanted to build up a serialized version in a member
field)

I think you misunderstood. What I am asking for is:

  void VisitEntries(CacheVisitor* visitor) const;

[Ryan Sleevi, 2016-06-16 01:39:53]

The CQ bit was unchecked by rsleevi@chromium.org

[Ryan Sleevi, 2016-06-16 01:40:07]

On 2016/06/16 00:54:51, eroman wrote:
> I think you misunderstood. What I am asking for is:
> 
>   void VisitEntries(CacheVisitor* visitor) const;

Oh. Yeah. Easy fix.

[Ryan Sleevi, 2016-06-16 01:42:34]

The CQ bit was checked by rsleevi@chromium.org

[Ryan Sleevi, 2016-06-16 01:42:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
rtenneti@chromium.org, eroman@chromium.org
Link to the patchset: https://codereview.chromium.org/1999733002/#ps100001
(title: "Constify")

[commit-bot: I haz the power, 2016-06-16 01:43:01]

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

[commit-bot: I haz the power, 2016-06-16 02:44:23]

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

[commit-bot: I haz the power, 2016-06-16 02:44:24]

Try jobs failed on following builders:
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[Ryan Sleevi, 2016-06-16 06:30:10]

The CQ bit was checked by rsleevi@chromium.org

[commit-bot: I haz the power, 2016-06-16 06:30:22]

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

[commit-bot: I haz the power, 2016-06-16 07:12:11]

Description was changed from

==========
Add support for walking and modifying the CachingCertVerifier

In anticipation of experimenting with persisting the certificate
verification cache across process evictions on mobile, add the
ability to iterate through the cache's contents (allowing it to
be serialized) and to restore the contents later.

Because restoration is expected to be disjoint from object
creation, rather than requiring the deserialized entries be
presented at construction, allow them to be provided after the
cache already has results, provided that no other results are
evicted.

BUG=590875
==========

to

==========
Add support for walking and modifying the CachingCertVerifier

In anticipation of experimenting with persisting the certificate
verification cache across process evictions on mobile, add the
ability to iterate through the cache's contents (allowing it to
be serialized) and to restore the contents later.

Because restoration is expected to be disjoint from object
creation, rather than requiring the deserialized entries be
presented at construction, allow them to be provided after the
cache already has results, provided that no other results are
evicted.

BUG=590875
==========

[commit-bot: I haz the power, 2016-06-16 07:12:12]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-06-16 07:12:20]

CQ bit was unchecked

[commit-bot: I haz the power, 2016-06-16 07:13:53]

Description was changed from

==========
Add support for walking and modifying the CachingCertVerifier

In anticipation of experimenting with persisting the certificate
verification cache across process evictions on mobile, add the
ability to iterate through the cache's contents (allowing it to
be serialized) and to restore the contents later.

Because restoration is expected to be disjoint from object
creation, rather than requiring the deserialized entries be
presented at construction, allow them to be provided after the
cache already has results, provided that no other results are
evicted.

BUG=590875
==========

to

==========
Add support for walking and modifying the CachingCertVerifier

In anticipation of experimenting with persisting the certificate
verification cache across process evictions on mobile, add the
ability to iterate through the cache's contents (allowing it to
be serialized) and to restore the contents later.

Because restoration is expected to be disjoint from object
creation, rather than requiring the deserialized entries be
presented at construction, allow them to be provided after the
cache already has results, provided that no other results are
evicted.

BUG=590875

Committed: https://crrev.com/b25d90c36cbf578cd8459f1e0acfd26930d0e139
Cr-Commit-Position: refs/heads/master@{#400100}
==========

[commit-bot: I haz the power, 2016-06-16 07:13:54]

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