Allow browser tests to use a MockCertVerifier

Allow browser tests to use a MockCertVerifier

Introduces a ProfileIOData::SetCertVerifierForTesting() hook, called by
the CertVerifierBrowserTest class. CertVerifierBrowserTest tests can use
mock_cert_verifier() to set mock cert verify results.

BUG=

Committed: https://crrev.com/b1ee16776c38e71dfecf783273bd535bea9b16e8
Cr-Commit-Position: refs/heads/master@{#342038}

[estark, 2015-07-09 17:27:20]

estark@chromium.org changed reviewers:
+ mmenke@chromium.org

[estark, 2015-07-09 17:27:21]

mmenke: as discussed over email (with Ryan, cc'ed), this is an attempt to let
browser tests set a CertVerifierFactory to use a MockCertVerifier.

Could you please take a look? It's not a complete solution yet (doesn't work on
ChromeOS), but I wanted to get some feedback at this point because it seems kind
of weird somehow and I'm not sure if I'm doing it right. (I had some specific
questions that I left inline, but also interested in if the general approach
seems like the right direction.)

https://codereview.chromium.org/1227943002/diff/20001/chrome/browser/profiles...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/1227943002/diff/20001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:1093: if (policy_cert_verifier_) {
I'm not quite sure how to deal with policy_cert_verifier_ for ChromeOS. My only
thought so far is that we could have another global
SetPolicyCertServiceFactory() function that CertVerifierBrowserTests set to a
mock factory that always returns nullptrs when asked to create a
PolicyCertVerifier. So we would end up not having a policy_cert_verifier_ in
CertVerifierBrowserTests and using the default CertVerifier instead.

https://codereview.chromium.org/1227943002/diff/20001/chrome/test/base/cert_v...
File chrome/test/base/cert_verifier_browser_test.cc (right):

https://codereview.chromium.org/1227943002/diff/20001/chrome/test/base/cert_v...
chrome/test/base/cert_verifier_browser_test.cc:20: return mock_cert_verifier_;
This is a little weird, because if gets called twice, both callers might take
ownership of the same MockCertVerifier. (In practice, I think it only gets
called once, so it's fine, but still seems weird.)

https://codereview.chromium.org/1227943002/diff/20001/net/cert/cert_verify_re...
File net/cert/cert_verify_result.h (right):

https://codereview.chromium.org/1227943002/diff/20001/net/cert/cert_verify_re...
net/cert/cert_verify_result.h:14: #include "net/cert/x509_certificate.h"
I can't for the life of me figure out why this is needed. :( Without it,
CertVerifierBrowserTests fail to compile:

In file included from ../../chrome/test/base/cert_verifier_browser_test.cc:5:
In file included from ../../chrome/test/base/cert_verifier_browser_test.h:8:
In file included from ../../chrome/test/base/in_process_browser_test.h:10:
../../base/memory/ref_counted.h:398:6: error: member access into incomplete type
'net::X509Certificate'
  ptr->AddRef();
     ^
../../base/memory/ref_counted.h:316:7: note: in instantiation of member function
'scoped_refptr<net::X509Certificate>::AddRef' requested here
      AddRef(p);
      ^
../../base/memory/ref_counted.h:325:18: note: in instantiation of member
function 'scoped_refptr<net::X509Certificate>::operator=' requested here
    return *this = r.ptr_;
                 ^
../../net/cert/cert_verify_result.h:20:18: note: in instantiation of member
function 'scoped_refptr<net::X509Certificate>::operator=' requested here
class NET_EXPORT CertVerifyResult {
                 ^
../../net/cert/cert_verifier.h:20:7: note: forward declaration of
'net::X509Certificate'
class X509Certificate;

[mmenke, 2015-07-09 18:35:20]

I'll start looking at this tomorrow (Or possibly later today).

On 2015/07/09 17:27:21, estark wrote:
> mmenke: as discussed over email (with Ryan, cc'ed), this is an attempt to let
> browser tests set a CertVerifierFactory to use a MockCertVerifier.
> 
> Could you please take a look? It's not a complete solution yet (doesn't work
on
> ChromeOS), but I wanted to get some feedback at this point because it seems
kind
> of weird somehow and I'm not sure if I'm doing it right. (I had some specific
> questions that I left inline, but also interested in if the general approach
> seems like the right direction.)
> 
>
https://codereview.chromium.org/1227943002/diff/20001/chrome/browser/profiles...
> File chrome/browser/profiles/profile_io_data.cc (right):
> 
>
https://codereview.chromium.org/1227943002/diff/20001/chrome/browser/profiles...
> chrome/browser/profiles/profile_io_data.cc:1093: if (policy_cert_verifier_) {
> I'm not quite sure how to deal with policy_cert_verifier_ for ChromeOS. My
only
> thought so far is that we could have another global
> SetPolicyCertServiceFactory() function that CertVerifierBrowserTests set to a
> mock factory that always returns nullptrs when asked to create a
> PolicyCertVerifier. So we would end up not having a policy_cert_verifier_ in
> CertVerifierBrowserTests and using the default CertVerifier instead.
> 
>
https://codereview.chromium.org/1227943002/diff/20001/chrome/test/base/cert_v...
> File chrome/test/base/cert_verifier_browser_test.cc (right):
> 
>
https://codereview.chromium.org/1227943002/diff/20001/chrome/test/base/cert_v...
> chrome/test/base/cert_verifier_browser_test.cc:20: return mock_cert_verifier_;
> This is a little weird, because if gets called twice, both callers might take
> ownership of the same MockCertVerifier. (In practice, I think it only gets
> called once, so it's fine, but still seems weird.)
> 
>
https://codereview.chromium.org/1227943002/diff/20001/net/cert/cert_verify_re...
> File net/cert/cert_verify_result.h (right):
> 
>
https://codereview.chromium.org/1227943002/diff/20001/net/cert/cert_verify_re...
> net/cert/cert_verify_result.h:14: #include "net/cert/x509_certificate.h"
> I can't for the life of me figure out why this is needed. :( Without it,
> CertVerifierBrowserTests fail to compile:
> 
> In file included from ../../chrome/test/base/cert_verifier_browser_test.cc:5:
> In file included from ../../chrome/test/base/cert_verifier_browser_test.h:8:
> In file included from ../../chrome/test/base/in_process_browser_test.h:10:
> ../../base/memory/ref_counted.h:398:6: error: member access into incomplete
type
> 'net::X509Certificate'
>   ptr->AddRef();
>      ^
> ../../base/memory/ref_counted.h:316:7: note: in instantiation of member
function
> 'scoped_refptr<net::X509Certificate>::AddRef' requested here
>       AddRef(p);
>       ^
> ../../base/memory/ref_counted.h:325:18: note: in instantiation of member
> function 'scoped_refptr<net::X509Certificate>::operator=' requested here
>     return *this = r.ptr_;
>                  ^
> ../../net/cert/cert_verify_result.h:20:18: note: in instantiation of member
> function 'scoped_refptr<net::X509Certificate>::operator=' requested here
> class NET_EXPORT CertVerifyResult {
>                  ^
> ../../net/cert/cert_verifier.h:20:7: note: forward declaration of
> 'net::X509Certificate'
> class X509Certificate;

[Ryan Sleevi, 2015-07-09 19:35:27]

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

[Ryan Sleevi, 2015-07-09 19:35:28]

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

https://codereview.chromium.org/1227943002/diff/20001/net/cert/cert_verifier....
net/cert/cert_verifier.h:134: static CertVerifier*
CreateWithVerifyProc(CertVerifyProc* cert_verify_proc);
Why the need to expose this interface?

I really didn't want CertVerifyProc to be part of the public API people are
fiddling with.

https://codereview.chromium.org/1227943002/diff/20001/net/cert/cert_verifier....
net/cert/cert_verifier.h:139: class NET_EXPORT CertVerifierFactory {
This is a design I intentionally wanted to avoid as much as possible. Even the
CertVerifier::CreateDefault() is an anti-pattern that I left in because I
couldn't get to all the callsites reasonably to sort this out.

That is, I really don't want us to go the HostResolverProc path, and the factory
is also somewhat squicky.

Is there a thread with the design motivations? Have I just forgotten all
context?

[Ryan Sleevi, 2015-07-09 19:40:27]

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

https://codereview.chromium.org/1227943002/diff/20001/net/cert/cert_verifier....
net/cert/cert_verifier.h:155: static CertVerifierFactory*
cert_verifier_factory_;
There's (generally) no need for such private statics - you can leave them in the
unname namespace of the .cc

https://codereview.chromium.org/1227943002/diff/20001/net/cert/cert_verify_re...
File net/cert/cert_verify_result.h (right):

https://codereview.chromium.org/1227943002/diff/20001/net/cert/cert_verify_re...
net/cert/cert_verify_result.h:14: #include "net/cert/x509_certificate.h"
On 2015/07/09 17:27:21, estark wrote:
> I can't for the life of me figure out why this is needed. :( Without it,
> CertVerifierBrowserTests fail to compile:
> 
> In file included from ../../chrome/test/base/cert_verifier_browser_test.cc:5:
> In file included from ../../chrome/test/base/cert_verifier_browser_test.h:8:
> In file included from ../../chrome/test/base/in_process_browser_test.h:10:
> ../../base/memory/ref_counted.h:398:6: error: member access into incomplete
type
> 'net::X509Certificate'
>   ptr->AddRef();
>      ^
> ../../base/memory/ref_counted.h:316:7: note: in instantiation of member
function
> 'scoped_refptr<net::X509Certificate>::AddRef' requested here
>       AddRef(p);
>       ^
> ../../base/memory/ref_counted.h:325:18: note: in instantiation of member
> function 'scoped_refptr<net::X509Certificate>::operator=' requested here
>     return *this = r.ptr_;
>                  ^
> ../../net/cert/cert_verify_result.h:20:18: note: in instantiation of member
> function 'scoped_refptr<net::X509Certificate>::operator=' requested here
> class NET_EXPORT CertVerifyResult {
>                  ^
> ../../net/cert/cert_verifier.h:20:7: note: forward declaration of
> 'net::X509Certificate'
> class X509Certificate;

Because you're invoking a compiler-default copy-ctor, which triggers
AddRef/Release on line 37, which needs to be a complete type.

Was this error when you had additional code / tests using the CFBT? I'd expect
it when you return the CertVerifyResult in a .cc that hadn't included the
x509_certificate.h

[estark, 2015-07-09 20:11:50]

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

https://codereview.chromium.org/1227943002/diff/20001/net/cert/cert_verifier....
net/cert/cert_verifier.h:139: class NET_EXPORT CertVerifierFactory {
On 2015/07/09 19:35:27, Ryan Sleevi (slow through 7-15 wrote:
> This is a design I intentionally wanted to avoid as much as possible. Even the
> CertVerifier::CreateDefault() is an anti-pattern that I left in because I
> couldn't get to all the callsites reasonably to sort this out.
> 
> That is, I really don't want us to go the HostResolverProc path, and the
factory
> is also somewhat squicky.
> 
> Is there a thread with the design motivations? Have I just forgotten all
> context?

Email thread with you me and Matt titled "MockCertVerifier". Maybe I
misunderstood what you were suggesting?

[estark, 2015-07-09 20:14:24]

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

https://codereview.chromium.org/1227943002/diff/20001/net/cert/cert_verifier....
net/cert/cert_verifier.h:134: static CertVerifier*
CreateWithVerifyProc(CertVerifyProc* cert_verify_proc);
On 2015/07/09 19:35:28, Ryan Sleevi (slow through 7-15 wrote:
> Why the need to expose this interface?
> 
> I really didn't want CertVerifyProc to be part of the public API people are
> fiddling with.

So I'm trying to use this instead of places where people are constructing new
MultithreadedCertVerifiers themselves (namely
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/io_...,
which already knows about CertVerifyProcs because it's creating one to pass to
the MultithreadedCertVerifier constructor).

[Ryan Sleevi, 2015-07-10 12:52:26]

On 2015/07/09 20:11:50, estark wrote:
> Email thread with you me and Matt titled "MockCertVerifier". Maybe I
> misunderstood what you were suggesting?

Yeah, mostly "CertVerifyProc" is a (quasi)implementation detail of
MultiThreadedCertVerifier, with the goal being that you should have to know
about either if you "just" want a CertVerifier.

Consider, for example, the world we're moving to in which cert verification is
done w/in Chrome itself (as part of the BoringSSL work) on some platforms. On
those platforms, they'll never see a MultiThreadedCertVerifier, because there
won't be a blocking API call that we'll have to worry about - it'll just be a
series of asynchronous continuations using TaskRunners.

The implication of this being something like

class CertVerifierFactory {
 public:
  virtual scoped_ptr<CertVerifier> CreateCertVerifier() = 0;

 protected:
  CertVerifierFactory() {}
  virtual ~CertVerifierFactory() {}
};

// Something something about threading (IO thread only or not)
// Something something |cert_verified_factory| to nullptr to reset to default
void SetCertVerifierFactoryForTesting(CertVerifierFactory*
cert_verifier_factory);


And then in the C++

namespace {
CertVerifierFactory* g_test_factory = nullptr;
}

void SetCertVerifierFactoryForTesting(CertVerifierFactory*
cert_verifier_factory) {
  // Something something locking?
  g_test_factory = cert_verifier_factory;
}

scoped_ptr<CertVerifier> CertVerifier::CreateDefault() {
  if (g_test_factory)
    return g_test_factory->CreateCertVerifier();
#if defined(FOO)
  return new Blah;
#else
  return new Baz;
#endif
}

[estark, 2015-07-17 00:39:46]

On 2015/07/10 12:52:26, Ryan Sleevi (slow through 7-15 wrote:
> On 2015/07/09 20:11:50, estark wrote:
> > Email thread with you me and Matt titled "MockCertVerifier". Maybe I
> > misunderstood what you were suggesting?
> 
> Yeah, mostly "CertVerifyProc" is a (quasi)implementation detail of
> MultiThreadedCertVerifier, with the goal being that you should have to know
> about either if you "just" want a CertVerifier.
> 
> Consider, for example, the world we're moving to in which cert verification is
> done w/in Chrome itself (as part of the BoringSSL work) on some platforms. On
> those platforms, they'll never see a MultiThreadedCertVerifier, because there
> won't be a blocking API call that we'll have to worry about - it'll just be a
> series of asynchronous continuations using TaskRunners.
> 
> The implication of this being something like
> 
> class CertVerifierFactory {
>  public:
>   virtual scoped_ptr<CertVerifier> CreateCertVerifier() = 0;
> 
>  protected:
>   CertVerifierFactory() {}
>   virtual ~CertVerifierFactory() {}
> };
> 
> // Something something about threading (IO thread only or not)
> // Something something |cert_verified_factory| to nullptr to reset to default
> void SetCertVerifierFactoryForTesting(CertVerifierFactory*
> cert_verifier_factory);
> 
> 
> And then in the C++
> 
> namespace {
> CertVerifierFactory* g_test_factory = nullptr;
> }
> 
> void SetCertVerifierFactoryForTesting(CertVerifierFactory*
> cert_verifier_factory) {
>   // Something something locking?
>   g_test_factory = cert_verifier_factory;
> }
> 
> scoped_ptr<CertVerifier> CertVerifier::CreateDefault() {
>   if (g_test_factory)
>     return g_test_factory->CreateCertVerifier();
> #if defined(FOO)
>   return new Blah;
> #else
>   return new Baz;
> #endif
> }

Thanks, Ryan. PTAL at patch set #4 and let me know if it's more in line with
what you were thinking.

(I'm still trying to figure out what to do for ChromeOS, which creates a new
CertVerifier for each profile using a CertVerifyProcChromeOS.)

[estark, 2015-07-17 00:44:48]

On 2015/07/17 00:39:46, estark wrote:
> On 2015/07/10 12:52:26, Ryan Sleevi (slow through 7-15 wrote:
> > On 2015/07/09 20:11:50, estark wrote:
> > > Email thread with you me and Matt titled "MockCertVerifier". Maybe I
> > > misunderstood what you were suggesting?
> > 
> > Yeah, mostly "CertVerifyProc" is a (quasi)implementation detail of
> > MultiThreadedCertVerifier, with the goal being that you should have to know
> > about either if you "just" want a CertVerifier.
> > 
> > Consider, for example, the world we're moving to in which cert verification
is
> > done w/in Chrome itself (as part of the BoringSSL work) on some platforms.
On
> > those platforms, they'll never see a MultiThreadedCertVerifier, because
there
> > won't be a blocking API call that we'll have to worry about - it'll just be
a
> > series of asynchronous continuations using TaskRunners.
> > 
> > The implication of this being something like
> > 
> > class CertVerifierFactory {
> >  public:
> >   virtual scoped_ptr<CertVerifier> CreateCertVerifier() = 0;
> > 
> >  protected:
> >   CertVerifierFactory() {}
> >   virtual ~CertVerifierFactory() {}
> > };
> > 
> > // Something something about threading (IO thread only or not)
> > // Something something |cert_verified_factory| to nullptr to reset to
default
> > void SetCertVerifierFactoryForTesting(CertVerifierFactory*
> > cert_verifier_factory);
> > 
> > 
> > And then in the C++
> > 
> > namespace {
> > CertVerifierFactory* g_test_factory = nullptr;
> > }
> > 
> > void SetCertVerifierFactoryForTesting(CertVerifierFactory*
> > cert_verifier_factory) {
> >   // Something something locking?
> >   g_test_factory = cert_verifier_factory;
> > }
> > 
> > scoped_ptr<CertVerifier> CertVerifier::CreateDefault() {
> >   if (g_test_factory)
> >     return g_test_factory->CreateCertVerifier();
> > #if defined(FOO)
> >   return new Blah;
> > #else
> >   return new Baz;
> > #endif
> > }
> 
> Thanks, Ryan. PTAL at patch set #4 and let me know if it's more in line with
> what you were thinking.

Actually, sorry, make that patch set #5

> 
> (I'm still trying to figure out what to do for ChromeOS, which creates a new
> CertVerifier for each profile using a CertVerifyProcChromeOS.)

[estark, 2015-07-17 00:49:17]

On 2015/07/17 00:44:48, estark wrote:
> On 2015/07/17 00:39:46, estark wrote:
> > On 2015/07/10 12:52:26, Ryan Sleevi (slow through 7-15 wrote:
> > > On 2015/07/09 20:11:50, estark wrote:
> > > > Email thread with you me and Matt titled "MockCertVerifier". Maybe I
> > > > misunderstood what you were suggesting?
> > > 
> > > Yeah, mostly "CertVerifyProc" is a (quasi)implementation detail of
> > > MultiThreadedCertVerifier, with the goal being that you should have to
know
> > > about either if you "just" want a CertVerifier.
> > > 
> > > Consider, for example, the world we're moving to in which cert
verification
> is
> > > done w/in Chrome itself (as part of the BoringSSL work) on some platforms.
> On
> > > those platforms, they'll never see a MultiThreadedCertVerifier, because
> there
> > > won't be a blocking API call that we'll have to worry about - it'll just
be
> a
> > > series of asynchronous continuations using TaskRunners.
> > > 
> > > The implication of this being something like
> > > 
> > > class CertVerifierFactory {
> > >  public:
> > >   virtual scoped_ptr<CertVerifier> CreateCertVerifier() = 0;
> > > 
> > >  protected:
> > >   CertVerifierFactory() {}
> > >   virtual ~CertVerifierFactory() {}
> > > };
> > > 
> > > // Something something about threading (IO thread only or not)
> > > // Something something |cert_verified_factory| to nullptr to reset to
> default
> > > void SetCertVerifierFactoryForTesting(CertVerifierFactory*
> > > cert_verifier_factory);
> > > 
> > > 
> > > And then in the C++
> > > 
> > > namespace {
> > > CertVerifierFactory* g_test_factory = nullptr;
> > > }
> > > 
> > > void SetCertVerifierFactoryForTesting(CertVerifierFactory*
> > > cert_verifier_factory) {
> > >   // Something something locking?
> > >   g_test_factory = cert_verifier_factory;
> > > }
> > > 
> > > scoped_ptr<CertVerifier> CertVerifier::CreateDefault() {
> > >   if (g_test_factory)
> > >     return g_test_factory->CreateCertVerifier();
> > > #if defined(FOO)
> > >   return new Blah;
> > > #else
> > >   return new Baz;
> > > #endif
> > > }
> > 
> > Thanks, Ryan. PTAL at patch set #4 and let me know if it's more in line with
> > what you were thinking.
> 
> Actually, sorry, make that patch set #5
> 
> > 
> > (I'm still trying to figure out what to do for ChromeOS, which creates a new
> > CertVerifier for each profile using a CertVerifyProcChromeOS.)

And ugh, one more apology, I just realized that I accidentally included a rebase
in patch set #3... :( I'm sorry! I'm happy to go back and delete the last few
patch sets and fix it up if it would make it easier to review.

[estark, 2015-08-03 21:33:53]

Friendly ping to rsleevi, though I think you might already have this on your
radar.

Also, I wanted to record for posterity the troubles that I'm having getting this
to work for ChromeOS. As I understand it, on the UI thread, ProfileIOData tries
to create a PolicyCertVerifier for ChromeOS, and keeps around a raw pointer to
it, which it uses to initialize the PolicyCertVerifier on the IO thread.

On the IO thread, ProfileIOData uses the aforementioned PolicyCertVerifier as
the |cert_verifier_| (which is scoped to ProfileIOData's lifetime), but if the
PolicyCertVerifier wasn't created for whatever reason, then it creates a new
MultiThreadedCertVerifier using a chromeos::CertVerifyProcChromeOS.

So far I've only been able to come up with tragically clunky ideas for dealing
with this, such as:

1. Make CertVerifierBrowserTests not run on ChromeOS. Boo.

2. Add another global... that is, make a factory-based
chromeos::CreateDefaultCertVerifierForProfile() with a corresponding
chromeos::SetCertVerifierFactoryForTesting() that looks something like this:

  scoped_ptr<CertVerifier> CreateDefaultCertVerifierForProfile(ProfileParams*
profile_params, scoped_ptr<PolicyCertVerifier> policy_cert_verifier_for_profile)
{
    if (g_factory_for_testing)
      return g_factory_for_testing->CreateCertVerifier();
    if (policy_cert_verifier_for_profile)
      return policy_cert_verifier_for_profile.Pass();
    return make_scoped_ptr(new MultiThreadedCertVerifier(new
chromeos::CertVerifyProcChromeOS(...)));
  }

And then in ProfileIOData, inside `#if defined(OS_CHROMEOS)`:

// on the UI thread
policy_cert_verifier_ = policy::PolicyCertServiceFactory::CreateForProfile(..);
// on the IO thread
if (policy_cert_verifier_)
  policy_cert_verifier_->InitializeOnIOThread(...);
cert_verifier_ = chromeos::CreateDefaultCertVerifier(profile_params_,
policy_cert_verifier_.Pass());

[Related: I assume that the PolicyCertVerifier must be created on the UI thread
because the code right now jumps through some hoops to do so, but I can't
actually find anywhere that documents this assumption.]

[estark, 2015-08-04 00:58:04]

Discussed with Ryan out-of-band; PTAL at the latest patch set, which sets the
factory via a static ProfileIOData::SetCertVerifierFactoryForTesting() method,
which lets things work better for ChromeOS.

The CertVerifierBrowserTest::mock_cert_verifier() interface doesn't seem quite
right to me, since it implies that only one MockCertVerifier will be created per
browser test (and I don't think we can guarantee that's the case?). One idea
I've been tossing around is for CertVerifierBrowserTest to have a cert verifier
factory that creates a MockCertVerifier each time it's asked but keeps a raw
pointer to each one, and exposes AddDefaultCertVerifyResult(),
AddResultForCert(), etc. methods that apply to *all* MockCertVerifiers that have
been created. But this doesn't seem quite right to me either, because once the
cert verifier factory hands over ownership of the MockCertVerifier, who knows if
it'll be alive the next time AddResultForCert(), etc. is called.

[Ryan Sleevi, 2015-08-04 08:28:36]

On 2015/08/04 00:58:04, estark wrote:
> Discussed with Ryan out-of-band; PTAL at the latest patch set, which sets the
> factory via a static ProfileIOData::SetCertVerifierFactoryForTesting() method,
> which lets things work better for ChromeOS.
> 
> The CertVerifierBrowserTest::mock_cert_verifier() interface doesn't seem quite
> right to me, since it implies that only one MockCertVerifier will be created
per
> browser test (and I don't think we can guarantee that's the case?). One idea
> I've been tossing around is for CertVerifierBrowserTest to have a cert
verifier
> factory that creates a MockCertVerifier each time it's asked but keeps a raw
> pointer to each one, and exposes AddDefaultCertVerifyResult(),
> AddResultForCert(), etc. methods that apply to *all* MockCertVerifiers that
have
> been created. But this doesn't seem quite right to me either, because once the
> cert verifier factory hands over ownership of the MockCertVerifier, who knows
if
> it'll be alive the next time AddResultForCert(), etc. is called.

Correct, I think you're close, and it's just about making a decision.

1) Don't go the factory route. Simply "SetCertVerifierForTesting", which does
not take ownership at all (caller maintains control), and causes all
ProfileIOData to use the same CertVerifier instead when initializing.

2) Use a Create method. It should return a scoped_ptr<CertVerifier> to make
explicit that an ownership transfer is occurring. Each of the CertVerifiers
returned by your factory, which we'll call "TestCertVerifier", all point back to
the factory's CertVerifier, and tests interact with it via the factory.

That would look something like
class MyFactory : public ... {
 public:
  MyFactory() : mock_cert_verifier_(new SharedMockVerifier) {};

  scoped_ptr<CertVerifier> CreateCertVerifier() {
    return scoped_ptr<CertVerifier>(new MyCertVerifier(mock_verifier_));
  }

  MockCertVerifier* mock_cert_verifier() { return mock_cert_verifier_->data; }

 private:
  using SharedMockVerifier = RefCountedData<MockCertVerifier>;

  // Creates a CertVerifier that always defers to a shared MockCertVerifier
  class MyCertVerifier : public CertVerifier {
   public:
    explicit MyCertVerifier(scoped_refptr<SharedMockVerifier>
mock_cert_verifier) : mock_cert_verifier_(mock_cert_verifier.Pass()) {}
    ~MyCertVerifier() override {}

    int Verify(...) override { return mock_cert_verifier_->data.Verify(...); }

   private:
    scoped_refptr<SharedMockVerifier> mock_cert_verifier_;
  };

  scoped_refptr<SharedMockVerifier> mock_cert_verifier_;
};


The reason for all the overhead/boilerplate is that way you can change the
Factory out, affecting new profiles, without having to have the caller worry
about ownership for all the existing profiles that may still be valid. It has
the strong guarantee that ownership of the CertVerifier is always given fully to
the Profile and that all data members it needs are kept alive, but that they all
live on the IO thread.

Personally, I'm tempted to suggest #1, provided your base BrowserTest can ensure
the CertVerifier is created before any profiles and that you have a hook to
defer cleaning up the CertVerifier you created *after* all Profiles are shut
down. I think BrowserTests offer that guarantee, and if so, #1 is far simpler.
But #2 is viable, mod better naming schemes, documentation, and splitting up the
header for the impl a lil bit to make it more readable :)

[Ryan Sleevi, 2015-08-04 08:28:44]

https://codereview.chromium.org/1227943002/diff/120001/chrome/browser/profile...
File chrome/browser/profiles/profile_io_data.h (right):

https://codereview.chromium.org/1227943002/diff/120001/chrome/browser/profile...
chrome/browser/profiles/profile_io_data.h:563: mutable
scoped_ptr<net::CertVerifier> cert_verifier_;
Since policy_cert_verifier_ points to this (line 557), should this perhaps be
before (line 556), so that the non-owning pointer is destroyed before the owning
pointer?

https://codereview.chromium.org/1227943002/diff/120001/net/cert/cert_verifier.cc
File net/cert/cert_verifier.cc (right):

https://codereview.chromium.org/1227943002/diff/120001/net/cert/cert_verifier...
net/cert/cert_verifier.cc:38: CertVerifierFactory::CertVerifierFactory() {}
Don't need any of these now, do you? It's sufficient to just let the
//chrome/browser code handle this

[estark, 2015-08-04 17:44:38]

Ok, I like option #1, since it's simple and gets the job done, and I think
InProcessBrowserTest does provide the lifetime requirements we need
(BrowserTestBase::SetUpInProcessBrowserTestFixture() gets called before
BrowserMain()). So I went with that in the latest patchset.

https://codereview.chromium.org/1227943002/diff/120001/chrome/browser/profile...
File chrome/browser/profiles/profile_io_data.h (right):

https://codereview.chromium.org/1227943002/diff/120001/chrome/browser/profile...
chrome/browser/profiles/profile_io_data.h:563: mutable
scoped_ptr<net::CertVerifier> cert_verifier_;
On 2015/08/04 08:28:44, Ryan Sleevi wrote:
> Since policy_cert_verifier_ points to this (line 557), should this perhaps be
> before (line 556), so that the non-owning pointer is destroyed before the
owning
> pointer?

Done.

https://codereview.chromium.org/1227943002/diff/120001/net/cert/cert_verifier.cc
File net/cert/cert_verifier.cc (right):

https://codereview.chromium.org/1227943002/diff/120001/net/cert/cert_verifier...
net/cert/cert_verifier.cc:38: CertVerifierFactory::CertVerifierFactory() {}
On 2015/08/04 08:28:44, Ryan Sleevi wrote:
> Don't need any of these now, do you? It's sufficient to just let the
> //chrome/browser code handle this

Yep, done.

[Ryan Sleevi, 2015-08-04 18:24:34]

LGTM, but could mmenke spot-check to make sure I didn't miss anything obvious :)

https://codereview.chromium.org/1227943002/diff/160001/chrome/browser/profile...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/1227943002/diff/160001/chrome/browser/profile...
chrome/browser/profiles/profile_io_data.cc:166: }
This function isn't necessary, is it? I mean, it's really just doing "return
g_cert_verifier_for_testing", since the only way line 163 fails if it it's
already nullptr. I'd just update line 1111 to point to this variable directly.

https://codereview.chromium.org/1227943002/diff/160001/chrome/browser/profile...
chrome/browser/profiles/profile_io_data.cc:1124: if (!cert_verifier) {
Rather than both sides of the #ifdef checking for cert_verifier (line 1124 you
do !cert_verifier, but line 1136 you do cert_verifier - thus making it hard to
match if/else pairs), why not lift the conditional up

line 1111
if (g_cert_verifier_for_testing) {
  main_request_context_->set_cert_verifier(g_cert_verifier_for_testing);
} else {
#if defined(OS_CHROMEOS)
  ...
#else
  ...
#endif
}

https://codereview.chromium.org/1227943002/diff/160001/chrome/browser/ssl/ssl...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

https://codereview.chromium.org/1227943002/diff/160001/chrome/browser/ssl/ssl...
chrome/browser/ssl/ssl_browser_tests.cc:2293:
mock_cert_verifier()->set_default_result(net::ERR_CERT_DATE_INVALID);
nit: I'm slightly paranoid of date-based error codes, since those can happen if
we forget to refresh certs ;)

Maybe some other 'obviously wrong' code that depends only on the cert? like a
name constraints violation?

https://codereview.chromium.org/1227943002/diff/160001/chrome/test/base/cert_...
File chrome/test/base/cert_verifier_browser_test.cc (right):

https://codereview.chromium.org/1227943002/diff/160001/chrome/test/base/cert_...
chrome/test/base/cert_verifier_browser_test.cc:19: void
CertVerifierBrowserTest::SetUpInProcessBrowserTestFixture() {
Be sure to reset this global state in TearDownInProcessBrowserTestFixture

[mmenke, 2015-08-04 19:01:23]

This looks reasonable to me (Well...to the extent that adding more code to
ProfileIOData is ever reasonable...), modulo Ryan's comments on ProfileIOData

https://codereview.chromium.org/1227943002/diff/160001/chrome/test/base/cert_...
File chrome/test/base/cert_verifier_browser_test.cc (right):

https://codereview.chromium.org/1227943002/diff/160001/chrome/test/base/cert_...
chrome/test/base/cert_verifier_browser_test.cc:8: #include
"content/public/browser/browser_thread.h"
Not used

https://codereview.chromium.org/1227943002/diff/160001/chrome/test/base/cert_...
chrome/test/base/cert_verifier_browser_test.cc:9: #include
"net/cert/cert_verifier.h"
I believe this can be removed, though it's debateable, since you're passing a
MockCertVerifier as a CertVerifier

https://codereview.chromium.org/1227943002/diff/160001/chrome/test/base/cert_...
chrome/test/base/cert_verifier_browser_test.cc:11: #include
"net/cert/x509_certificate.h"
Not used

https://codereview.chromium.org/1227943002/diff/160001/chrome/test/base/cert_...
File chrome/test/base/cert_verifier_browser_test.h (right):

https://codereview.chromium.org/1227943002/diff/160001/chrome/test/base/cert_...
chrome/test/base/cert_verifier_browser_test.h:9: #include
"net/cert/cert_verifier.h"
I don't think this is needed.

https://codereview.chromium.org/1227943002/diff/160001/chrome/test/base/cert_...
chrome/test/base/cert_verifier_browser_test.h:33:
scoped_ptr<net::MockCertVerifier> mock_cert_verifier_;
include base/memory/scoped_ptr

[estark, 2015-08-04 19:32:20]

https://codereview.chromium.org/1227943002/diff/160001/chrome/browser/profile...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/1227943002/diff/160001/chrome/browser/profile...
chrome/browser/profiles/profile_io_data.cc:166: }
On 2015/08/04 18:24:34, Ryan Sleevi wrote:
> This function isn't necessary, is it? I mean, it's really just doing "return
> g_cert_verifier_for_testing", since the only way line 163 fails if it it's
> already nullptr. I'd just update line 1111 to point to this variable directly.

Done.

https://codereview.chromium.org/1227943002/diff/160001/chrome/browser/profile...
chrome/browser/profiles/profile_io_data.cc:1124: if (!cert_verifier) {
On 2015/08/04 18:24:34, Ryan Sleevi wrote:
> Rather than both sides of the #ifdef checking for cert_verifier (line 1124 you
> do !cert_verifier, but line 1136 you do cert_verifier - thus making it hard to
> match if/else pairs), why not lift the conditional up
> 
> line 1111
> if (g_cert_verifier_for_testing) {
>   main_request_context_->set_cert_verifier(g_cert_verifier_for_testing);
> } else {
> #if defined(OS_CHROMEOS)
>   ...
> #else
>   ...
> #endif
> }

Done.

https://codereview.chromium.org/1227943002/diff/160001/chrome/browser/ssl/ssl...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

https://codereview.chromium.org/1227943002/diff/160001/chrome/browser/ssl/ssl...
chrome/browser/ssl/ssl_browser_tests.cc:2293:
mock_cert_verifier()->set_default_result(net::ERR_CERT_DATE_INVALID);
On 2015/08/04 18:24:34, Ryan Sleevi wrote:
> nit: I'm slightly paranoid of date-based error codes, since those can happen
if
> we forget to refresh certs ;)
> 
> Maybe some other 'obviously wrong' code that depends only on the cert? like a
> name constraints violation?

Done.

https://codereview.chromium.org/1227943002/diff/160001/chrome/test/base/cert_...
File chrome/test/base/cert_verifier_browser_test.cc (right):

https://codereview.chromium.org/1227943002/diff/160001/chrome/test/base/cert_...
chrome/test/base/cert_verifier_browser_test.cc:8: #include
"content/public/browser/browser_thread.h"
On 2015/08/04 19:01:23, mmenke wrote:
> Not used

Done.

https://codereview.chromium.org/1227943002/diff/160001/chrome/test/base/cert_...
chrome/test/base/cert_verifier_browser_test.cc:9: #include
"net/cert/cert_verifier.h"
On 2015/08/04 19:01:23, mmenke wrote:
> I believe this can be removed, though it's debateable, since you're passing a
> MockCertVerifier as a CertVerifier

Done.

https://codereview.chromium.org/1227943002/diff/160001/chrome/test/base/cert_...
chrome/test/base/cert_verifier_browser_test.cc:11: #include
"net/cert/x509_certificate.h"
On 2015/08/04 19:01:23, mmenke wrote:
> Not used

Done.

https://codereview.chromium.org/1227943002/diff/160001/chrome/test/base/cert_...
chrome/test/base/cert_verifier_browser_test.cc:19: void
CertVerifierBrowserTest::SetUpInProcessBrowserTestFixture() {
On 2015/08/04 18:24:34, Ryan Sleevi wrote:
> Be sure to reset this global state in TearDownInProcessBrowserTestFixture

Done.

https://codereview.chromium.org/1227943002/diff/160001/chrome/test/base/cert_...
File chrome/test/base/cert_verifier_browser_test.h (right):

https://codereview.chromium.org/1227943002/diff/160001/chrome/test/base/cert_...
chrome/test/base/cert_verifier_browser_test.h:9: #include
"net/cert/cert_verifier.h"
On 2015/08/04 19:01:23, mmenke wrote:
> I don't think this is needed.

Done.

https://codereview.chromium.org/1227943002/diff/160001/chrome/test/base/cert_...
chrome/test/base/cert_verifier_browser_test.h:33:
scoped_ptr<net::MockCertVerifier> mock_cert_verifier_;
On 2015/08/04 19:01:23, mmenke wrote:
> include base/memory/scoped_ptr

Done.

[estark, 2015-08-04 19:37:24]

estark@chromium.org changed reviewers:
+ sky@chromium.org

[estark, 2015-08-04 19:37:25]

sky@: could you please review the files in //chrome/test/base? Thanks!

[sky, 2015-08-04 20:06:04]

https://codereview.chromium.org/1227943002/diff/200001/chrome/test/base/cert_...
File chrome/test/base/cert_verifier_browser_test.h (right):

https://codereview.chromium.org/1227943002/diff/200001/chrome/test/base/cert_...
chrome/test/base/cert_verifier_browser_test.h:5: #ifndef
CHROME_TEST_BASE_CERT_VERIFIER_BROWSER_TEST_H_
Can this test live in chrome/browser/ssl? Or some more focused directory rather
than chrome/test/base?

[estark, 2015-08-04 20:52:08]

https://codereview.chromium.org/1227943002/diff/200001/chrome/test/base/cert_...
File chrome/test/base/cert_verifier_browser_test.h (right):

https://codereview.chromium.org/1227943002/diff/200001/chrome/test/base/cert_...
chrome/test/base/cert_verifier_browser_test.h:5: #ifndef
CHROME_TEST_BASE_CERT_VERIFIER_BROWSER_TEST_H_
On 2015/08/04 20:06:04, sky wrote:
> Can this test live in chrome/browser/ssl? Or some more focused directory
rather
> than chrome/test/base?

Good point, I'm not sure why I put it here originally; //c/b/ssl makes more
sense.

After moving it, you're no longer needed for OWNERS review, so I'm going to
remove you from the reviewers list, but please feel free to review if you'd like
to. Thanks!

[estark, 2015-08-04 20:52:15]

estark@chromium.org changed reviewers:
- sky@chromium.org

[mmenke, 2015-08-04 21:06:23]

https://codereview.chromium.org/1227943002/diff/220001/chrome/browser/profile...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/1227943002/diff/220001/chrome/browser/profile...
chrome/browser/profiles/profile_io_data.cc:1110: username_hash_ =
profile_params_->username_hash;
Should all of this be gated on whether g_cert_verifier_for_testing is true or
not?  username_hash_, at least, is exposed publicly by the ProfileIOData, as is
use_system_key_slot_ for that matter.

[estark, 2015-08-04 21:14:23]

https://codereview.chromium.org/1227943002/diff/220001/chrome/browser/profile...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/1227943002/diff/220001/chrome/browser/profile...
chrome/browser/profiles/profile_io_data.cc:1110: username_hash_ =
profile_params_->username_hash;
On 2015/08/04 21:06:22, mmenke wrote:
> Should all of this be gated on whether g_cert_verifier_for_testing is true or
> not?  username_hash_, at least, is exposed publicly by the ProfileIOData, as
is
> use_system_key_slot_ for that matter.

Ah, I missed that. I guess the choice then is two #ifdef pairs or a conditional
on either side of the #ifdef. I went with the latter, but using the same
conditional on both sides to hopefully address Ryan's earlier comment about the
confusing-ness of having `if (cert_verifier)` on one side and `if
(!cert_verifier)` on the other.

[mmenke, 2015-08-05 20:59:13]

https://codereview.chromium.org/1227943002/diff/240001/chrome/browser/profile...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/1227943002/diff/240001/chrome/browser/profile...
chrome/browser/profiles/profile_io_data.cc:1110:
EnableNSSSystemKeySlotForResourceContext(resource_context_.get());
I think two OS_CHROMEOS blocks is a little prettier, one inside the else block,
one outside it.  It's a bit less code, but more importantly, I think it's a bit
cleaner, though I'm all for combining #if blocks when we can do so cleanly.

[estark, 2015-08-05 21:09:53]

https://codereview.chromium.org/1227943002/diff/240001/chrome/browser/profile...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/1227943002/diff/240001/chrome/browser/profile...
chrome/browser/profiles/profile_io_data.cc:1110:
EnableNSSSystemKeySlotForResourceContext(resource_context_.get());
On 2015/08/05 20:59:13, mmenke wrote:
> I think two OS_CHROMEOS blocks is a little prettier, one inside the else
block,
> one outside it.  It's a bit less code, but more importantly, I think it's a
bit
> cleaner, though I'm all for combining #if blocks when we can do so cleanly.

Ok, done.

[mmenke, 2015-08-05 21:11:51]

On 2015/08/05 21:09:53, estark wrote:
>
https://codereview.chromium.org/1227943002/diff/240001/chrome/browser/profile...
> File chrome/browser/profiles/profile_io_data.cc (right):
> 
>
https://codereview.chromium.org/1227943002/diff/240001/chrome/browser/profile...
> chrome/browser/profiles/profile_io_data.cc:1110:
> EnableNSSSystemKeySlotForResourceContext(resource_context_.get());
> On 2015/08/05 20:59:13, mmenke wrote:
> > I think two OS_CHROMEOS blocks is a little prettier, one inside the else
> block,
> > one outside it.  It's a bit less code, but more importantly, I think it's a
> bit
> > cleaner, though I'm all for combining #if blocks when we can do so cleanly.
> 
> Ok, done.

Thanks!  LGTM

[estark, 2015-08-05 21:16:29]

The CQ bit was checked by estark@chromium.org

[estark, 2015-08-05 21:16:30]

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/1227943002/#ps260001
(title: "use two OS_CHROMEOS blocks")

[commit-bot: I haz the power, 2015-08-05 21:16:48]

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

[commit-bot: I haz the power, 2015-08-05 21:37:10]

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

[commit-bot: I haz the power, 2015-08-05 21:37:11]

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_...)

[estark, 2015-08-05 21:44:19]

The CQ bit was checked by estark@chromium.org

[estark, 2015-08-05 21:44:19]

The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org,
rsleevi@chromium.org
Link to the patchset: https://codereview.chromium.org/1227943002/#ps280001
(title: "put the files in the right .gypi")

[commit-bot: I haz the power, 2015-08-05 21:46:29]

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

[commit-bot: I haz the power, 2015-08-05 22:52:12]

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

[commit-bot: I haz the power, 2015-08-05 22:52:13]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[estark, 2015-08-06 01:08:32]

The CQ bit was checked by estark@chromium.org

[commit-bot: I haz the power, 2015-08-06 01:08:58]

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

[commit-bot: I haz the power, 2015-08-06 01:15:31]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2015-08-06 01:16:12]

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