Move the NSS functions out of CertDatabase into a new NSSCertDatabase class.

Move the NSS functions out of CertDatabase into a new NSSCertDatabase class.

BUG=chromium-os:33872
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=155720

[Joao da Silva, 2012-09-04 17:00:37]

@rsleevi: please have a look. Early comments on cert_database.* and
cert_database_nss.* welcome!

Notice that the CertDatabase observes but doesn't unregister from
NSSCertDatabase. Let's keep discussing this over email.

[wtc, 2012-09-04 23:32:49]

Review comments on patch set 3:

I only reviewed two header files:
cert_database.h and the new cert_database_nss.h.

http://codereview.chromium.org/10916094/diff/5004/net/base/cert_database.h
File net/base/cert_database.h (left):

http://codereview.chromium.org/10916094/diff/5004/net/base/cert_database.h#ol...
net/base/cert_database.h:42: // Will be called when a new user certificate is
added.

A "user certificate" is a certificate whose private key is
also present. So "UserCertAdded" is a narrower event than
"CertAdded".

I just wanted to confirm that you meant to broaden OnUserCertAdded
to OnCertAdded.

http://codereview.chromium.org/10916094/diff/5004/net/base/cert_database.h
File net/base/cert_database.h (right):

http://codereview.chromium.org/10916094/diff/5004/net/base/cert_database.h#ne...
net/base/cert_database.h:15: // certificate store.

Nit: we probably should update this comment to reflect the
new, more restricted role of this class...

http://codereview.chromium.org/10916094/diff/5004/net/base/cert_database.h#ne...
net/base/cert_database.h:21: class NET_EXPORT CertDatabase {

Should this class be renamed UserCertDatabase?

http://codereview.chromium.org/10916094/diff/5004/net/base/cert_database.h#ne...
net/base/cert_database.h:44: virtual void OnCertTrustChanged(const
X509Certificate* cert) {}

The OnCertTrustChanged callback also seems NSS-specific...
It seems that we should remove OnCertTrustChanged from the
CertDatabase::Observer interface.

http://codereview.chromium.org/10916094/diff/5004/net/base/cert_database_nss.h
File net/base/cert_database_nss.h (right):

http://codereview.chromium.org/10916094/diff/5004/net/base/cert_database_nss....
net/base/cert_database_nss.h:5: #ifndef NET_BASE_CERT_DATABASE_NSS_H_

This file should be named nss_cert_database.h to match the
class name NSSCertDatabase.

http://codereview.chromium.org/10916094/diff/5004/net/base/cert_database_nss....
net/base/cert_database_nss.h:67: // Initializes the NSS internal structures, if
they aren't initialized yet.

Nit: just say "Initializes NSS, if it isn't initialized yet."

http://codereview.chromium.org/10916094/diff/5004/net/base/cert_database_nss....
net/base/cert_database_nss.h:160: static void
AddObserver(CertDatabase::Observer* observer);

It seems bad to reuse CertDatabase::Observer here.

[Ryan Sleevi, 2012-09-05 01:19:17]

http://codereview.chromium.org/10916094/diff/5004/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/certificate_pattern.cc (right):

http://codereview.chromium.org/10916094/diff/5004/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/certificate_pattern.cc:97: : cert_db_(cert_db) {}
nit: This change seems unnecessary. Did I miss something?

http://codereview.chromium.org/10916094/diff/5004/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/onc_network_parser.cc (right):

http://codereview.chromium.org/10916094/diff/5004/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/onc_network_parser.cc:958:
net::NSSCertDatabase::TRUST_DEFAULT;
nit: The indents are now screwed up here. Suggest re-aligning as appropriate.

http://codereview.chromium.org/10916094/diff/5004/net/base/cert_database.h
File net/base/cert_database.h (left):

http://codereview.chromium.org/10916094/diff/5004/net/base/cert_database.h#ol...
net/base/cert_database.h:42: // Will be called when a new user certificate is
added.
On 2012/09/04 23:32:49, wtc wrote:
> 
> A "user certificate" is a certificate whose private key is
> also present. So "UserCertAdded" is a narrower event than
> "CertAdded".
> 
> I just wanted to confirm that you meant to broaden OnUserCertAdded
> to OnCertAdded.

Since as we've discovered, adding certs can affect trust decisions, I think
broadening it is correct.

If this was UserCertDatabase, then I suspect we'd still want a holistic
"CertDatabase" to encompass (user or CA) certificates.

http://codereview.chromium.org/10916094/diff/5004/net/base/cert_database.h
File net/base/cert_database.h (right):

http://codereview.chromium.org/10916094/diff/5004/net/base/cert_database.h#ne...
net/base/cert_database.h:44: virtual void OnCertTrustChanged(const
X509Certificate* cert) {}
On 2012/09/04 23:32:49, wtc wrote:
> 
> The OnCertTrustChanged callback also seems NSS-specific...
> It seems that we should remove OnCertTrustChanged from the
> CertDatabase::Observer interface.

Actually, this one is - surprisingly - portable - we just haven't implemented it
yet.

With SecKeychain, we get it via keychain events, and with Windows, we get it via
ObjectWatchers on an HEVENT obtained via CertControlStore on the HCERTSTORE for
the ROOT

http://codereview.chromium.org/10916094/diff/5004/net/base/cert_database_nss.cc
File net/base/cert_database_nss.cc (right):

http://codereview.chromium.org/10916094/diff/5004/net/base/cert_database_nss....
net/base/cert_database_nss.cc:109: : certificate(cert), net_error(err) {}
nit: Because you had to wrap the constructor, each value for the initializer
list should be on a separate line.

http://codereview.chromium.org/10916094/diff/5004/net/base/cert_database_nss.h
File net/base/cert_database_nss.h (right):

http://codereview.chromium.org/10916094/diff/5004/net/base/cert_database_nss....
net/base/cert_database_nss.h:67: // Initializes the NSS internal structures, if
they aren't initialized yet.
On 2012/09/04 23:32:49, wtc wrote:
> 
> Nit: just say "Initializes NSS, if it isn't initialized yet."

It's also not clear to me what the threading requirements are for ChromeOS. I
seem to recall that this code may need to be called on the main/UI thread. I'm
assuming that's why this was made a public method and not lazily initialized
when needed - correct?

http://codereview.chromium.org/10916094/diff/5004/net/base/cert_database_nss....
net/base/cert_database_nss.h:160: static void
AddObserver(CertDatabase::Observer* observer);
On 2012/09/04 23:32:49, wtc wrote:
> 
> It seems bad to reuse CertDatabase::Observer here.

It's weird that this class mixes static and non-static methods, considering it
has no member variables.

It would seem like all of these are conceptually "static" methods. It would seem
like a better separation would be to either stop pretending, and call it a real
Singleton, and then expose these as non-static member methods.

We can thread the NSSCertDatabase through calls (or, as today, they can just get
the Singleton instance), but at least it's updating code to "observer *A*
database" rather than "observe *THE* database" (eg: disconnecting them from
needing to know about the Singleton-ness)

[Joao da Silva, 2012-09-05 19:37:26]

Thanks for the comments! Please have another look.

http://codereview.chromium.org/10916094/diff/5004/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/certificate_pattern.cc (right):

http://codereview.chromium.org/10916094/diff/5004/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/certificate_pattern.cc:97: : cert_db_(cert_db) {}
On 2012/09/05 01:19:17, Ryan Sleevi wrote:
> nit: This change seems unnecessary. Did I miss something?

It's unnecessary and has been reverted. An intermediate version had an
nss_cert_db here.

http://codereview.chromium.org/10916094/diff/5004/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/onc_network_parser.cc (right):

http://codereview.chromium.org/10916094/diff/5004/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/onc_network_parser.cc:958:
net::NSSCertDatabase::TRUST_DEFAULT;
On 2012/09/05 01:19:17, Ryan Sleevi wrote:
> nit: The indents are now screwed up here. Suggest re-aligning as appropriate.

Done.

http://codereview.chromium.org/10916094/diff/5004/net/base/cert_database.h
File net/base/cert_database.h (left):

http://codereview.chromium.org/10916094/diff/5004/net/base/cert_database.h#ol...
net/base/cert_database.h:42: // Will be called when a new user certificate is
added.
On 2012/09/05 01:19:17, Ryan Sleevi wrote:
> On 2012/09/04 23:32:49, wtc wrote:
> > 
> > A "user certificate" is a certificate whose private key is
> > also present. So "UserCertAdded" is a narrower event than
> > "CertAdded".
> > 
> > I just wanted to confirm that you meant to broaden OnUserCertAdded
> > to OnCertAdded.
> 
> Since as we've discovered, adding certs can affect trust decisions, I think
> broadening it is correct.

That's why I thought the naming wasn't accurate anymore and renamed it. Trust
decisions have to be recomputed if the policy store changes too.

> 
> If this was UserCertDatabase, then I suspect we'd still want a holistic
> "CertDatabase" to encompass (user or CA) certificates.

http://codereview.chromium.org/10916094/diff/5004/net/base/cert_database.h
File net/base/cert_database.h (right):

http://codereview.chromium.org/10916094/diff/5004/net/base/cert_database.h#ne...
net/base/cert_database.h:15: // certificate store.
On 2012/09/04 23:32:49, wtc wrote:
> 
> Nit: we probably should update this comment to reflect the
> new, more restricted role of this class...

Done.

http://codereview.chromium.org/10916094/diff/5004/net/base/cert_database.h#ne...
net/base/cert_database.h:21: class NET_EXPORT CertDatabase {
On 2012/09/04 23:32:49, wtc wrote:
> 
> Should this class be renamed UserCertDatabase?

Observers of cert added/removed/trust-changed should react to those events both
when they are triggered from the user store or from the policy store; in either
case, trust chains have to be recomputed. So I'd keep the name; wdyt?

http://codereview.chromium.org/10916094/diff/5004/net/base/cert_database.h#ne...
net/base/cert_database.h:44: virtual void OnCertTrustChanged(const
X509Certificate* cert) {}
On 2012/09/05 01:19:17, Ryan Sleevi wrote:
> On 2012/09/04 23:32:49, wtc wrote:
> > 
> > The OnCertTrustChanged callback also seems NSS-specific...
> > It seems that we should remove OnCertTrustChanged from the
> > CertDatabase::Observer interface.

Some of the observers just flush their caches whenever one of these events
happens (e.g. DefaultClientSocketFactory, ClientSocketPoolManagerImpl,
SpdySessionPool). So they'd have to observe both stores to get these events;
I've kept it here since we agreed to make CertDatabase a "frontend" so that
client code doesn't have to care about the specific impl. Do you agree?

> 
> Actually, this one is - surprisingly - portable - we just haven't implemented
it
> yet.
> 
> With SecKeychain, we get it via keychain events, and with Windows, we get it
via
> ObjectWatchers on an HEVENT obtained via CertControlStore on the HCERTSTORE
for
> the ROOT

http://codereview.chromium.org/10916094/diff/5004/net/base/cert_database_nss.cc
File net/base/cert_database_nss.cc (right):

http://codereview.chromium.org/10916094/diff/5004/net/base/cert_database_nss....
net/base/cert_database_nss.cc:109: : certificate(cert), net_error(err) {}
On 2012/09/05 01:19:17, Ryan Sleevi wrote:
> nit: Because you had to wrap the constructor, each value for the initializer
> list should be on a separate line.

Done.

http://codereview.chromium.org/10916094/diff/5004/net/base/cert_database_nss.h
File net/base/cert_database_nss.h (right):

http://codereview.chromium.org/10916094/diff/5004/net/base/cert_database_nss....
net/base/cert_database_nss.h:5: #ifndef NET_BASE_CERT_DATABASE_NSS_H_
On 2012/09/04 23:32:49, wtc wrote:
> 
> This file should be named nss_cert_database.h to match the
> class name NSSCertDatabase.

Done.

http://codereview.chromium.org/10916094/diff/5004/net/base/cert_database_nss....
net/base/cert_database_nss.h:67: // Initializes the NSS internal structures, if
they aren't initialized yet.
On 2012/09/04 23:32:49, wtc wrote:
> 
> Nit: just say "Initializes NSS, if it isn't initialized yet."

Done.

http://codereview.chromium.org/10916094/diff/5004/net/base/cert_database_nss....
net/base/cert_database_nss.h:67: // Initializes the NSS internal structures, if
they aren't initialized yet.
On 2012/09/05 01:19:17, Ryan Sleevi wrote:
> On 2012/09/04 23:32:49, wtc wrote:
> > 
> > Nit: just say "Initializes NSS, if it isn't initialized yet."
> 
> It's also not clear to me what the threading requirements are for ChromeOS. I
> seem to recall that this code may need to be called on the main/UI thread. I'm
> assuming that's why this was made a public method and not lazily initialized
> when needed - correct?

Some places just create a local CertDatabase and expect it to "work", but that
requires that NSS is initialized. Having a static Init() here allows the
CertDatabase ctor to call this (this was previously in the body of
CertDatabase::CertDatabase() for the NSS impl). It's still lazily initialized
the first time that a CertDatabase is used.

http://codereview.chromium.org/10916094/diff/5004/net/base/cert_database_nss....
net/base/cert_database_nss.h:160: static void
AddObserver(CertDatabase::Observer* observer);
On 2012/09/04 23:32:49, wtc wrote:
> 
> It seems bad to reuse CertDatabase::Observer here.

Added NSSCertDatabase::Observer.

http://codereview.chromium.org/10916094/diff/5004/net/base/cert_database_nss....
net/base/cert_database_nss.h:160: static void
AddObserver(CertDatabase::Observer* observer);
On 2012/09/05 01:19:17, Ryan Sleevi wrote:
> On 2012/09/04 23:32:49, wtc wrote:
> > 
> > It seems bad to reuse CertDatabase::Observer here.
> 
> It's weird that this class mixes static and non-static methods, considering it
> has no member variables.
> 
> It would seem like all of these are conceptually "static" methods. It would
seem
> like a better separation would be to either stop pretending, and call it a
real
> Singleton, and then expose these as non-static member methods.
> 
> We can thread the NSSCertDatabase through calls (or, as today, they can just
get
> the Singleton instance), but at least it's updating code to "observer *A*
> database" rather than "observe *THE* database" (eg: disconnecting them from
> needing to know about the Singleton-ness)

It was initially in this style to follow the pattern in CertDatabase. I've
converted both to not use any static methods.

Observers now look a bit strange but I think this is unavoidable given that
these classes are singletons:

SomeClass::SomeClass() {
  CertDatabase cert_db;
  cert_db.AddObserver(this);
  // Seems like the object being observed just went away!
}

@Ryan: would you rather have GetInstance() calls in [NSS]CertDatabase?

[Ryan Sleevi, 2012-09-05 21:44:17]

http://codereview.chromium.org/10916094/diff/5004/net/base/cert_database_nss.h
File net/base/cert_database_nss.h (right):

http://codereview.chromium.org/10916094/diff/5004/net/base/cert_database_nss....
net/base/cert_database_nss.h:160: static void
AddObserver(CertDatabase::Observer* observer);
On 2012/09/05 19:37:27, Joao da Silva wrote:
> On 2012/09/05 01:19:17, Ryan Sleevi wrote:
> > On 2012/09/04 23:32:49, wtc wrote:
> > > 
> > > It seems bad to reuse CertDatabase::Observer here.
> > 
> > It's weird that this class mixes static and non-static methods, considering
it
> > has no member variables.
> > 
> > It would seem like all of these are conceptually "static" methods. It would
> seem
> > like a better separation would be to either stop pretending, and call it a
> real
> > Singleton, and then expose these as non-static member methods.
> > 
> > We can thread the NSSCertDatabase through calls (or, as today, they can just
> get
> > the Singleton instance), but at least it's updating code to "observer *A*
> > database" rather than "observe *THE* database" (eg: disconnecting them from
> > needing to know about the Singleton-ness)
> 
> It was initially in this style to follow the pattern in CertDatabase. I've
> converted both to not use any static methods.
> 
> Observers now look a bit strange but I think this is unavoidable given that
> these classes are singletons:
> 
> SomeClass::SomeClass() {
>   CertDatabase cert_db;
>   cert_db.AddObserver(this);
>   // Seems like the object being observed just went away!
> }
> 
> @Ryan: would you rather have GetInstance() calls in [NSS]CertDatabase?

Sorry it wasn't clear:

Because these are effectively Singletons (no member variables, requires an
explicit init, singletons under the hood), I think it would be better

SomeClass::SomeClass() {
  CertDatabase::GetInstance()->AddObserver(this);
}

I think the pattern of
{
  CertDatabase db;
  db.AddObserver(this);
}

Creates confusion about the lifetime of the "observing", even more than the
CertDatabase::AddObserver(this) did.

In an "ideal" world, at "some later point in the future", this would be supplied
as a dependency for everyone who needs it, as part of the general Chromium goal
of "Don't use Singletons"

SomeClass::SomeClass(CertDatabase* cert_database)
    : cert_database_(cert_database) {
  cert_database_->AddObserver(this);
}

http://codereview.chromium.org/10916094/diff/7005/net/base/cert_database.cc
File net/base/cert_database.cc (right):

http://codereview.chromium.org/10916094/diff/7005/net/base/cert_database.cc#n...
net/base/cert_database.cc:22: class CertDatabaseNotifier : public
NSSCertDatabase::Observer {
nit: Add a comment about why this inheritance (that it's forwarding along
notifications)

http://codereview.chromium.org/10916094/diff/7005/net/base/cert_database.cc#n...
net/base/cert_database.cc:22: class CertDatabaseNotifier : public
NSSCertDatabase::Observer {
nit: I'm generally not a fan of conditional inheritance like this, since it
forces the implementation details to be 'leaked' through the interface.

It's a nit because this is a private class within an implementation file, so the
leaking is localized to this file. However, it seems possible to fully
accomodate this with encapsulation, rather than inheritance, and that might make
it easier to understand the relationship of these interfaces

class CertDatabaseNotifier {
 public:
  ...

 private:
#if defined(USE_NSS)
  class NSSObserver : public NSSCertDatabase::Observer {
   public:
    explicit NSSObserver(CertDatabaseNotifier* notifier)
        : notifier_(notifier) {
      NSSCertDatabase::GetInstance()->AddNotifier(this);
    }

    // NSSCertDatabase::Observer implementation
    virtual void OnCertAdded(...) OVERRIDE {
      notifier->NotifyObserversOfCertAdded(...);
    }

    ...
  };

  NSSObserver observer_;
#endif
};

This uses the "bridge class" concept to glue these two together, rather than
directly inheriting.

Like I said, it's not major, and does involve a little bit more wire-work to get
things structured, but I think it is more in-line with Chromium convention and
patterns.

http://codereview.chromium.org/10916094/diff/7005/net/base/cert_database.cc#n...
net/base/cert_database.cc:41: #if defined(USE_NSS)
nit:
// NSSCertDatabase::Observer implementation

to clarify why these methods are #if defined(USE_NSS)

http://codereview.chromium.org/10916094/diff/7005/net/base/nss_cert_database.h
File net/base/nss_cert_database.h (right):

http://codereview.chromium.org/10916094/diff/7005/net/base/nss_cert_database....
net/base/nss_cert_database.h:91: static void EnsureInit();
nit: In case it wasn't clear, I think EnsureInit can/should be implicit on
Singleton use. AFAICT, this shouldn't break anyone - should it?

[Joao da Silva, 2012-09-06 15:11:41]

Please have another look. Thanks!

http://codereview.chromium.org/10916094/diff/5004/net/base/cert_database_nss.h
File net/base/cert_database_nss.h (right):

http://codereview.chromium.org/10916094/diff/5004/net/base/cert_database_nss....
net/base/cert_database_nss.h:160: static void
AddObserver(CertDatabase::Observer* observer);
On 2012/09/05 21:44:17, Ryan Sleevi wrote:
> On 2012/09/05 19:37:27, Joao da Silva wrote:
> > On 2012/09/05 01:19:17, Ryan Sleevi wrote:
> > > On 2012/09/04 23:32:49, wtc wrote:
> > > > 
> > > > It seems bad to reuse CertDatabase::Observer here.
> > > 
> > > It's weird that this class mixes static and non-static methods,
considering
> it
> > > has no member variables.
> > > 
> > > It would seem like all of these are conceptually "static" methods. It
would
> > seem
> > > like a better separation would be to either stop pretending, and call it a
> > real
> > > Singleton, and then expose these as non-static member methods.
> > > 
> > > We can thread the NSSCertDatabase through calls (or, as today, they can
just
> > get
> > > the Singleton instance), but at least it's updating code to "observer *A*
> > > database" rather than "observe *THE* database" (eg: disconnecting them
from
> > > needing to know about the Singleton-ness)
> > 
> > It was initially in this style to follow the pattern in CertDatabase. I've
> > converted both to not use any static methods.
> > 
> > Observers now look a bit strange but I think this is unavoidable given that
> > these classes are singletons:
> > 
> > SomeClass::SomeClass() {
> >   CertDatabase cert_db;
> >   cert_db.AddObserver(this);
> >   // Seems like the object being observed just went away!
> > }
> > 
> > @Ryan: would you rather have GetInstance() calls in [NSS]CertDatabase?
> 
> Sorry it wasn't clear:
> 
> Because these are effectively Singletons (no member variables, requires an
> explicit init, singletons under the hood), I think it would be better
> 
> SomeClass::SomeClass() {
>   CertDatabase::GetInstance()->AddObserver(this);
> }
> 
> I think the pattern of
> {
>   CertDatabase db;
>   db.AddObserver(this);
> }
> 
> Creates confusion about the lifetime of the "observing", even more than the
> CertDatabase::AddObserver(this) did.
> 
> In an "ideal" world, at "some later point in the future", this would be
supplied
> as a dependency for everyone who needs it, as part of the general Chromium
goal
> of "Don't use Singletons"
> 
> SomeClass::SomeClass(CertDatabase* cert_database)
>     : cert_database_(cert_database) {
>   cert_database_->AddObserver(this);
> }

Done! Went the full way and made CertDatabase and NSSCertDatabase singletons
with GetInstance() getters.

http://codereview.chromium.org/10916094/diff/7005/net/base/cert_database.cc
File net/base/cert_database.cc (right):

http://codereview.chromium.org/10916094/diff/7005/net/base/cert_database.cc#n...
net/base/cert_database.cc:22: class CertDatabaseNotifier : public
NSSCertDatabase::Observer {
On 2012/09/05 21:44:17, Ryan Sleevi wrote:
> nit: Add a comment about why this inheritance (that it's forwarding along
> notifications)

Done.

http://codereview.chromium.org/10916094/diff/7005/net/base/cert_database.cc#n...
net/base/cert_database.cc:22: class CertDatabaseNotifier : public
NSSCertDatabase::Observer {
On 2012/09/05 21:44:17, Ryan Sleevi wrote:
> nit: I'm generally not a fan of conditional inheritance like this, since it
> forces the implementation details to be 'leaked' through the interface.
> 
> It's a nit because this is a private class within an implementation file, so
the
> leaking is localized to this file. However, it seems possible to fully
> accomodate this with encapsulation, rather than inheritance, and that might
make
> it easier to understand the relationship of these interfaces
> 
> class CertDatabaseNotifier {
>  public:
>   ...
> 
>  private:
> #if defined(USE_NSS)
>   class NSSObserver : public NSSCertDatabase::Observer {
>    public:
>     explicit NSSObserver(CertDatabaseNotifier* notifier)
>         : notifier_(notifier) {
>       NSSCertDatabase::GetInstance()->AddNotifier(this);
>     }
> 
>     // NSSCertDatabase::Observer implementation
>     virtual void OnCertAdded(...) OVERRIDE {
>       notifier->NotifyObserversOfCertAdded(...);
>     }
> 
>     ...
>   };
> 
>   NSSObserver observer_;
> #endif
> };
> 
> This uses the "bridge class" concept to glue these two together, rather than
> directly inheriting.
> 
> Like I said, it's not major, and does involve a little bit more wire-work to
get
> things structured, but I think it is more in-line with Chromium convention and
> patterns.

Done.

http://codereview.chromium.org/10916094/diff/7005/net/base/cert_database.cc#n...
net/base/cert_database.cc:41: #if defined(USE_NSS)
On 2012/09/05 21:44:17, Ryan Sleevi wrote:
> nit:
> // NSSCertDatabase::Observer implementation
> 
> to clarify why these methods are #if defined(USE_NSS)

Done (though the #ifdef is gone)

http://codereview.chromium.org/10916094/diff/7005/net/base/nss_cert_database.h
File net/base/nss_cert_database.h (right):

http://codereview.chromium.org/10916094/diff/7005/net/base/nss_cert_database....
net/base/nss_cert_database.h:91: static void EnsureInit();
On 2012/09/05 21:44:17, Ryan Sleevi wrote:
> nit: In case it wasn't clear, I think EnsureInit can/should be implicit on
> Singleton use. AFAICT, this shouldn't break anyone - should it?

This was the case before (the CertDatabase ctor from the _nss version calls
this), so it shouldn't break anyone. This is now called from the ctor of
NSSCertDatabase, which in turn is called from the ctor of CertDatabase too.

[Ryan Sleevi, 2012-09-06 17:49:16]

Mechanical changes LGTM, but there's two points needing resolution before
committing and one nit. Because they're trivial, I don't think there's a need
for re-review, hence the LGTM, but please make sure to address before
committing.

http://codereview.chromium.org/10916094/diff/5083/net/base/cert_database.cc
File net/base/cert_database.cc (right):

http://codereview.chromium.org/10916094/diff/5083/net/base/cert_database.cc#n...
net/base/cert_database.cc:25: ~Notifier() {
nit: virtual (clang should have warned about this?)

http://codereview.chromium.org/10916094/diff/5083/net/base/cert_database_nss.cc
File net/base/cert_database_nss.cc (left):

http://codereview.chromium.org/10916094/diff/5083/net/base/cert_database_nss....
net/base/cert_database_nss.cc:34: CertDatabase::CertDatabase() {
BUG: Seems like you still need to implement the following functions on
CertDatabase (eg: NOT in NSSCertDatabase)

CertDatabase::CheckUserCert()
CertDatabase::AddUserCert()

They should be in a file named cert_database_nss.cc, not nss_cert_database

http://codereview.chromium.org/10916094/diff/5083/net/net.gyp
File net/net.gyp (right):

http://codereview.chromium.org/10916094/diff/5083/net/net.gyp#newcode1444
net/net.gyp:1444: }, {  # else: OS is not in the above list
Seems this comment is wrong. Looks like it was duplicated from line 981, but
unlike that block, we don't have OS-specific switches inside the use_glib==1
conditional.

Probably fine to remove the comment, or to rephrase it to highlight that
use_glib is being used for posix && !mac

[Joao da Silva, 2012-09-07 08:58:51]

Thanks for the review!

@wtc: are these changes OK for you too?

@jhawkins: please do an owner review for chrome/

@satorux: please do an owner review for chrome/browser/chromeos/

http://codereview.chromium.org/10916094/diff/5083/net/base/cert_database.cc
File net/base/cert_database.cc (right):

http://codereview.chromium.org/10916094/diff/5083/net/base/cert_database.cc#n...
net/base/cert_database.cc:25: ~Notifier() {
On 2012/09/06 17:49:16, Ryan Sleevi wrote:
> nit: virtual (clang should have warned about this?)

Done.

http://codereview.chromium.org/10916094/diff/5083/net/base/cert_database_nss.cc
File net/base/cert_database_nss.cc (left):

http://codereview.chromium.org/10916094/diff/5083/net/base/cert_database_nss....
net/base/cert_database_nss.cc:34: CertDatabase::CertDatabase() {
On 2012/09/06 17:49:16, Ryan Sleevi wrote:
> BUG: Seems like you still need to implement the following functions on
> CertDatabase (eg: NOT in NSSCertDatabase)
> 
> CertDatabase::CheckUserCert()
> CertDatabase::AddUserCert()
> 
> They should be in a file named cert_database_nss.cc, not nss_cert_database

Moved to the cert_database_nss.cc file.

http://codereview.chromium.org/10916094/diff/5083/net/net.gyp
File net/net.gyp (right):

http://codereview.chromium.org/10916094/diff/5083/net/net.gyp#newcode1444
net/net.gyp:1444: }, {  # else: OS is not in the above list
On 2012/09/06 17:49:16, Ryan Sleevi wrote:
> Seems this comment is wrong. Looks like it was duplicated from line 981, but
> unlike that block, we don't have OS-specific switches inside the use_glib==1
> conditional.
> 
> Probably fine to remove the comment, or to rephrase it to highlight that
> use_glib is being used for posix && !mac

Updated the comment.

[satorux1, 2012-09-07 17:03:16]

chrome/browser/chromeos lgtm

[James Hawkins, 2012-09-09 18:57:24]

chrome/ LGTM

[commit-bot: I haz the power, 2012-09-09 20:05:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/10916094/31

[commit-bot: I haz the power, 2012-09-10 11:51:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/10916094/7064

[commit-bot: I haz the power, 2012-09-10 14:11:09]

Change committed as 155720

[wtc, 2012-09-10 22:11:00]

Review comments on patch set 14:

joaodasilva,rsleevi: Sorry about the late review. The CL
looks good to me except for the OnUserCertAdded =>
OnCertAdded issue that I brought up before. I think the fact
that it is a *user* cert that was added is useful info to
several observers and we should preserve that.

http://codereview.chromium.org/10916094/diff/7064/net/base/cert_database.cc
File net/base/cert_database.cc (right):

http://codereview.chromium.org/10916094/diff/7064/net/base/cert_database.cc#n...
net/base/cert_database.cc:19: class CertDatabase::Notifier : public
NSSCertDatabase::Observer {

Ideally the USE_NSS code in this file should be moved to
net/base/cert_database_nss.cc.  I guess it is difficult to
do that?

http://codereview.chromium.org/10916094/diff/7064/net/base/cert_database.h
File net/base/cert_database.h (right):

http://codereview.chromium.org/10916094/diff/7064/net/base/cert_database.h#ne...
net/base/cert_database.h:39: virtual void OnCertAdded(const X509Certificate*
cert) {}

IMPORTANT:

A problem with broadening OnUserCertAdded to OnCertAdded is
that an observer that is only interested in user certs (such
as net::SSLClientAuthCache) could potentially be notified
when an unrelated server cert or CA cert is added. I don't
think the current code does this, but the definition of the
Observer::OnCertAdded() method allows this.

My preference is to avoid unnecessary notifications. I hope
it is clear why unnecessary notifications are undesirable.

I analyzed this CL and verified that the current code calls
NotifyObserversOfCertAdded() only when user certs are added,
and all the observers that implement Observer::OnCertAdded
are only interested in user certs. So it is not too late to
change "CertAdded" back to "UserCertAdded" (and "CertRemoved"
back to "UserCertRemoved").

http://codereview.chromium.org/10916094/diff/7064/net/base/cert_database.h#ne...
net/base/cert_database.h:91: scoped_ptr<Notifier> notifier_;

I guess this member can't be simply
  Notifier notifier_;
because we want to hide the definition of the Notifier class?

[Ryan Sleevi, 2012-09-10 22:23:19]

http://codereview.chromium.org/10916094/diff/7064/net/base/cert_database.h
File net/base/cert_database.h (right):

http://codereview.chromium.org/10916094/diff/7064/net/base/cert_database.h#ne...
net/base/cert_database.h:39: virtual void OnCertAdded(const X509Certificate*
cert) {}
On 2012/09/10 22:11:01, wtc wrote:
> 
> IMPORTANT:
> 
> A problem with broadening OnUserCertAdded to OnCertAdded is
> that an observer that is only interested in user certs (such
> as net::SSLClientAuthCache) could potentially be notified
> when an unrelated server cert or CA cert is added. I don't
> think the current code does this, but the definition of the
> Observer::OnCertAdded() method allows this.

I believe this is a good thing and the correct behaviour.

Because we determine what client cert(s) are acceptable based on the available
CAs, adding or removing a CA may affect what certificates are acceptable or not.

For example, imagine a client cert chain:
EE(A) -> B -> C

Now imagine the server, whose CertificateRequest message only lists C (for size
optimization)

Now imagine that the client only has A installed

Without B, we'd only choose the NULL cert, because no certs match C. After the
user is prompted to and installs B, they can now use A.

> 
> My preference is to avoid unnecessary notifications. I hope
> it is clear why unnecessary notifications are undesirable.
> 
> I analyzed this CL and verified that the current code calls
> NotifyObserversOfCertAdded() only when user certs are added,
> and all the observers that implement Observer::OnCertAdded
> are only interested in user certs. So it is not too late to
> change "CertAdded" back to "UserCertAdded" (and "CertRemoved"
> back to "UserCertRemoved").

I agree, we should avoid unnecessary notifications, but I think we're already at
that point.

The current "OnUserCertAdded" code really doesn't and shouldn't be watching for
individual certs being added - they should be watching to see if a (previously
selected) client cert has now changed its options.

That is, there's no need to remove the entire SSL session cache when a cert is
added/removed, we only need to remove the cached entries for which there was a
CertificateRequest that we replied to (either NULL or with a cert) that we now
have a new option to respond to.

That said, I think your proposal to restore the original names is fine until we
get to that point.