Make NSSCertDatabase::ListCerts work async on a worker thread.

Make NSSCertDatabase::ListCerts async and use the worker pool for actual certificate listing.

BUG=340460
TBR=sky@chromium.org (for rename in chrome/browser/certificate_manager_model.cc)

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=249334

[tbarzic, 2014-01-24 04:32:37]

I still have to add some comments, but can you see if this looks ok?

(diff against https://codereview.chromium.org/135193007/)

[mattm, 2014-01-28 00:51:48]

Not sure about moving the filtering into the parent class.. maybe an alternate
approach would be to have the static list certs method be protected, then the
NSSCertDatabaseChromeOS could have its own static list certs method which calls
the parent one and then does the filtering?
Ryan, thoughts?

[Ryan Sleevi, 2014-01-28 02:11:22]

This CL does not match its description.

It says it's about making ListCerts async, but it introduces all sorts of
filtering logic. Why the disconnect?

https://codereview.chromium.org/144423007/diff/200003/net/cert/nss_cert_datab...
File net/cert/nss_cert_database.h (right):

https://codereview.chromium.org/144423007/diff/200003/net/cert/nss_cert_datab...
net/cert/nss_cert_database.h:224: const scoped_refptr<base::TaskRunner>&
task_runner);
Why is this not private w/ friend?

[Ryan Sleevi, 2014-01-28 02:18:26]

On 2014/01/28 00:51:48, mattm wrote:
> Not sure about moving the filtering into the parent class.. maybe an alternate
> approach would be to have the static list certs method be protected, then the
> NSSCertDatabaseChromeOS could have its own static list certs method which
calls
> the parent one and then does the filtering?
> Ryan, thoughts?

Yeah, I definitely don't like filtering in the parent class. Defeats the
abstraction that was trying to be preserved by splitting them.

I also don't know why a static list certs method is needed - you can always
reference the base classes implementation by qualifying the class name, can you
not?

[tbarzic, 2014-01-28 02:22:15]

On 2014/01/28 02:11:22, Ryan Sleevi wrote:
> This CL does not match its description.
> 
> It says it's about making ListCerts async, but it introduces all sorts of
> filtering logic. Why the disconnect?
> 
>
https://codereview.chromium.org/144423007/diff/200003/net/cert/nss_cert_datab...
> File net/cert/nss_cert_database.h (right):
> 
>
https://codereview.chromium.org/144423007/diff/200003/net/cert/nss_cert_datab...
> net/cert/nss_cert_database.h:224: const scoped_refptr<base::TaskRunner>&
> task_runner);
> Why is this not private w/ friend?

I'll add more details to the bug description, but I planned to check that the
approach here is OK before I do more cleanup on the CL.
The cl doesn't actually introduce filtering logic, it only moves it from
NSSCertDatabaseChromeOS to NSSCertDatabase.

[tbarzic, 2014-01-28 02:31:26]

On 2014/01/28 02:18:26, Ryan Sleevi wrote:
> On 2014/01/28 00:51:48, mattm wrote:
> > Not sure about moving the filtering into the parent class.. maybe an
alternate
> > approach would be to have the static list certs method be protected, then
the
> > NSSCertDatabaseChromeOS could have its own static list certs method which
> calls
> > the parent one and then does the filtering?
> > Ryan, thoughts?
> 
> Yeah, I definitely don't like filtering in the parent class. Defeats the
> abstraction that was trying to be preserved by splitting them.
>

OK, I'll change it so the filtering stays contained in the ChromeOS
implementation.

> I also don't know why a static list certs method is needed - you can always
> reference the base classes implementation by qualifying the class name, can
you
> not?

If we do ListCerts on the worker thread, it could get called after
NSSCertDatabaseChromeOS is deleted (which happens on profile destruction).

[Ryan Sleevi, 2014-01-29 02:24:20]

https://codereview.chromium.org/144423007/diff/530001/net/cert/nss_cert_datab...
File net/cert/nss_cert_database.cc (right):

https://codereview.chromium.org/144423007/diff/530001/net/cert/nss_cert_datab...
net/cert/nss_cert_database.cc:360: CHECK(certs);
unnecessary check - certs->clear() will error if certs is NULL.

https://codereview.chromium.org/144423007/diff/530001/net/cert/nss_cert_datab...
File net/cert/nss_cert_database.h (right):

https://codereview.chromium.org/144423007/diff/530001/net/cert/nss_cert_datab...
net/cert/nss_cert_database.h:33: #endif
Ok, good example of where ForTest() was cleaner :)

Feel free to push back more in the future

https://codereview.chromium.org/144423007/diff/530001/net/cert/nss_cert_datab...
net/cert/nss_cert_database.h:109: // instance of all certificates).
Is this deprecated? It would seem it should be.

https://codereview.chromium.org/144423007/diff/530001/net/cert/nss_cert_datab...
net/cert/nss_cert_database.h:115: ListCertsCallback;
Move typedef to line 104 (
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...
)

Design: Why does this API receive a scoped_ptr<CertificateList>? Why not just
have it be const CertificateList&?

The motivation (perhaps misguided) of having ListCertsSync was to avoid a
reallocation of a vector, but that's unavoidable here with the async ListCerts,
so the API doesn't make sense there.

https://codereview.chromium.org/144423007/diff/530001/net/cert/nss_cert_datab...
File net/cert/nss_cert_database_chromeos.h (right):

https://codereview.chromium.org/144423007/diff/530001/net/cert/nss_cert_datab...
net/cert/nss_cert_database_chromeos.h:42: const
scoped_refptr<NSSProfileFilterChromeOS> profile_filter,
style: Don't pass by value - "const scoped_refptr<...>&"

design: There's no need to expose private static methods as part of the class
interface, it can be part of an unnamed namespace within the .cc

https://codereview.chromium.org/144423007/diff/530001/net/cert/nss_profile_fi...
File net/cert/nss_profile_filter_chromeos.h (right):

https://codereview.chromium.org/144423007/diff/530001/net/cert/nss_profile_fi...
net/cert/nss_profile_filter_chromeos.h:18: : public
base::RefCountedThreadSafe<NSSProfileFilterChromeOS> {
NACK on making this ref-counted as part of the interface.

Just making it copy constructable so that you can create a copy for the worker,
and use PK11_ReferenceSlot to initialize the crypto::ScopedPK11Slots.

[tbarzic, 2014-01-29 20:50:52]

https://codereview.chromium.org/144423007/diff/530001/net/cert/nss_cert_datab...
File net/cert/nss_cert_database.cc (right):

https://codereview.chromium.org/144423007/diff/530001/net/cert/nss_cert_datab...
net/cert/nss_cert_database.cc:360: CHECK(certs);
On 2014/01/29 02:24:20, Ryan Sleevi wrote:
> unnecessary check - certs->clear() will error if certs is NULL.

Done.

https://codereview.chromium.org/144423007/diff/530001/net/cert/nss_cert_datab...
File net/cert/nss_cert_database.h (right):

https://codereview.chromium.org/144423007/diff/530001/net/cert/nss_cert_datab...
net/cert/nss_cert_database.h:33: #endif
On 2014/01/29 02:24:20, Ryan Sleevi wrote:
> Ok, good example of where ForTest() was cleaner :)
> 
> Feel free to push back more in the future

removed :)

https://codereview.chromium.org/144423007/diff/530001/net/cert/nss_cert_datab...
net/cert/nss_cert_database.h:109: // instance of all certificates).
On 2014/01/29 02:24:20, Ryan Sleevi wrote:
> Is this deprecated? It would seem it should be.

yep

https://codereview.chromium.org/144423007/diff/530001/net/cert/nss_cert_datab...
net/cert/nss_cert_database.h:115: ListCertsCallback;
On 2014/01/29 02:24:20, Ryan Sleevi wrote:
> Move typedef to line 104 (
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...
> )
> 
> Design: Why does this API receive a scoped_ptr<CertificateList>? Why not just
> have it be const CertificateList&?
> 
> The motivation (perhaps misguided) of having ListCertsSync was to avoid a
> reallocation of a vector, but that's unavoidable here with the async
ListCerts,
> so the API doesn't make sense there.

The motivation was to avoid copying the vector while passing it between threads
(by passing around a pointer), and not having to wrap the callback passed to
|ListCerts| with a method that would call it with a reference instead of a
pointer.

Though, having a wrapper around |callback| would be useful for handling case
where the databasae is gone by the time callback triggers, which could happen
for the NSSCertDatabaseChromeOS.

btw. what do you think is the right thing to do in the case the callback is
scheduled after the database is deleted?

https://codereview.chromium.org/144423007/diff/530001/net/cert/nss_cert_datab...
File net/cert/nss_cert_database_chromeos.h (right):

https://codereview.chromium.org/144423007/diff/530001/net/cert/nss_cert_datab...
net/cert/nss_cert_database_chromeos.h:42: const
scoped_refptr<NSSProfileFilterChromeOS> profile_filter,
On 2014/01/29 02:24:20, Ryan Sleevi wrote:
> style: Don't pass by value - "const scoped_refptr<...>&"
> 
> design: There's no need to expose private static methods as part of the class
> interface, it can be part of an unnamed namespace within the .cc

The reason this is part of the class is to be able to use protected
NSSCertDatabase::ListCertsImpl.

https://codereview.chromium.org/144423007/diff/530001/net/cert/nss_profile_fi...
File net/cert/nss_profile_filter_chromeos.h (right):

https://codereview.chromium.org/144423007/diff/530001/net/cert/nss_profile_fi...
net/cert/nss_profile_filter_chromeos.h:18: : public
base::RefCountedThreadSafe<NSSProfileFilterChromeOS> {
On 2014/01/29 02:24:20, Ryan Sleevi wrote:
> NACK on making this ref-counted as part of the interface.
> 
> Just making it copy constructable so that you can create a copy for the
worker,
> and use PK11_ReferenceSlot to initialize the crypto::ScopedPK11Slots.

I prefer having a CreateCopy method instead, but if you don't like the idea,
I'll go with the copy constructor

[Ryan Sleevi, 2014-01-30 04:16:30]

API works for me, mod comment below and nits.

LGTM

https://codereview.chromium.org/144423007/diff/530001/net/cert/nss_cert_datab...
File net/cert/nss_cert_database.h (right):

https://codereview.chromium.org/144423007/diff/530001/net/cert/nss_cert_datab...
net/cert/nss_cert_database.h:115: ListCertsCallback;
On 2014/01/29 20:50:52, tbarzic wrote:
> On 2014/01/29 02:24:20, Ryan Sleevi wrote:
> > Move typedef to line 104 (
> >
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...
> > )
> > 
> > Design: Why does this API receive a scoped_ptr<CertificateList>? Why not
just
> > have it be const CertificateList&?
> > 
> > The motivation (perhaps misguided) of having ListCertsSync was to avoid a
> > reallocation of a vector, but that's unavoidable here with the async
> ListCerts,
> > so the API doesn't make sense there.
> 
> The motivation was to avoid copying the vector while passing it between
threads
> (by passing around a pointer), and not having to wrap the callback passed to
> |ListCerts| with a method that would call it with a reference instead of a
> pointer.
> 
> Though, having a wrapper around |callback| would be useful for handling case
> where the databasae is gone by the time callback triggers, which could happen
> for the NSSCertDatabaseChromeOS.
> 
> btw. what do you think is the right thing to do in the case the callback is
> scheduled after the database is deleted?

"Don't allow that"?

Otherwise, use a WeakPtrFactory as an internal implementation detail, although
that creates thread affinity issues.

You're not calling back into this class though, nor its members, so I'm not sure
what the problem would be.

https://codereview.chromium.org/144423007/diff/530001/net/cert/nss_profile_fi...
File net/cert/nss_profile_filter_chromeos.h (right):

https://codereview.chromium.org/144423007/diff/530001/net/cert/nss_profile_fi...
net/cert/nss_profile_filter_chromeos.h:18: : public
base::RefCountedThreadSafe<NSSProfileFilterChromeOS> {
On 2014/01/29 20:50:52, tbarzic wrote:
> On 2014/01/29 02:24:20, Ryan Sleevi wrote:
> > NACK on making this ref-counted as part of the interface.
> > 
> > Just making it copy constructable so that you can create a copy for the
> worker,
> > and use PK11_ReferenceSlot to initialize the crypto::ScopedPK11Slots.
> 
> I prefer having a CreateCopy method instead, but if you don't like the idea,
> I'll go with the copy constructor

I'm not a terrible fan of design patterns that force heap allocation, especially
since the ProfileFilterChromeOS is really just a struct holder with some helper
methods. Let's just go copy ctor?

https://codereview.chromium.org/144423007/diff/610001/net/cert/nss_cert_datab...
File net/cert/nss_cert_database.cc (right):

https://codereview.chromium.org/144423007/diff/610001/net/cert/nss_cert_datab...
net/cert/nss_cert_database.cc:48: 
unnecessary newline

https://codereview.chromium.org/144423007/diff/610001/net/cert/nss_cert_datab...
net/cert/nss_cert_database.cc:90: base::Unretained(raw_certs)),
base::Unretained(certs.get()) ?

https://codereview.chromium.org/144423007/diff/610001/net/cert/nss_cert_datab...
File net/cert/nss_cert_database.h (right):

https://codereview.chromium.org/144423007/diff/610001/net/cert/nss_cert_datab...
net/cert/nss_cert_database.h:104: // DEPRECATED: Use |ListCerts| instead.
File a bug to track this work & reference here

https://codereview.chromium.org/144423007/diff/610001/net/cert/nss_cert_datab...
File net/cert/nss_cert_database_chromeos.cc (right):

https://codereview.chromium.org/144423007/diff/610001/net/cert/nss_cert_datab...
net/cert/nss_cert_database_chromeos.cc:79: certs->erase(std::remove_if(
IWYU: include <algorithm> if using remove_if

https://codereview.chromium.org/144423007/diff/610001/net/cert/nss_cert_datab...
File net/cert/nss_cert_database_chromeos.h (right):

https://codereview.chromium.org/144423007/diff/610001/net/cert/nss_cert_datab...
net/cert/nss_cert_database_chromeos.h:41: static void
ListCertsImpl(scoped_ptr<NSSProfileFilterChromeOS> profile_filter,
Just pass as const NSSProfileFilterChromeOS&

base::BindState will allocate storage for the bound parameters, which is the
same effect as the scoped_ptr<>, but avoids an *additional* allocation (of a
holder for *this* scoped_ptr)

[mattm, 2014-01-30 04:34:36]

https://codereview.chromium.org/144423007/diff/610001/chromeos/cert_loader.h
File chromeos/cert_loader.h (right):

https://codereview.chromium.org/144423007/diff/610001/chromeos/cert_loader.h#...
chromeos/cert_loader.h:64: // expexts it to stay alive at least until the
shutdown of the main thread.
expects

https://codereview.chromium.org/144423007/diff/610001/chromeos/cert_loader.h#...
chromeos/cert_loader.h:64: // expexts it to stay alive at least until the
shutdown of the main thread.
This doesn't seem true, or maybe just confusingly worded? The database is owned
by the ResourceContext, I would expect that to get destroyed while the IOThread
is still alive, let alone the main thread.

https://codereview.chromium.org/144423007/diff/610001/net/cert/nss_cert_datab...
File net/cert/nss_cert_database.h (right):

https://codereview.chromium.org/144423007/diff/610001/net/cert/nss_cert_datab...
net/cert/nss_cert_database.h:232: // Static so it may safelly be used on the
worker thread.
spelling nit: safely

https://codereview.chromium.org/144423007/diff/610001/net/cert/nss_cert_datab...
net/cert/nss_cert_database.h:235: // Gets task runner that should be use for
slow tasks like certificate
nit: s/use/used/

https://codereview.chromium.org/144423007/diff/610001/net/cert/nss_cert_datab...
File net/cert/nss_cert_database_chromeos.h (right):

https://codereview.chromium.org/144423007/diff/610001/net/cert/nss_cert_datab...
net/cert/nss_cert_database_chromeos.h:39: // is additionally filterred by
|profile_filter|.
filtered

https://codereview.chromium.org/144423007/diff/610001/net/cert/nss_cert_datab...
net/cert/nss_cert_database_chromeos.h:40: // Static so it may safelly be used on
the worker thread.
safely

[tbarzic, 2014-02-03 23:42:04]

https://codereview.chromium.org/144423007/diff/530001/net/cert/nss_cert_datab...
File net/cert/nss_cert_database.h (right):

https://codereview.chromium.org/144423007/diff/530001/net/cert/nss_cert_datab...
net/cert/nss_cert_database.h:115: ListCertsCallback;
On 2014/01/30 04:16:30, Ryan Sleevi wrote:
> On 2014/01/29 20:50:52, tbarzic wrote:
> > On 2014/01/29 02:24:20, Ryan Sleevi wrote:
> > > Move typedef to line 104 (
> > >
> >
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...
> > > )
> > > 
> > > Design: Why does this API receive a scoped_ptr<CertificateList>? Why not
> just
> > > have it be const CertificateList&?
> > > 
> > > The motivation (perhaps misguided) of having ListCertsSync was to avoid a
> > > reallocation of a vector, but that's unavoidable here with the async
> > ListCerts,
> > > so the API doesn't make sense there.
> > 
> > The motivation was to avoid copying the vector while passing it between
> threads
> > (by passing around a pointer), and not having to wrap the callback passed to
> > |ListCerts| with a method that would call it with a reference instead of a
> > pointer.
> > 
> > Though, having a wrapper around |callback| would be useful for handling case
> > where the databasae is gone by the time callback triggers, which could
happen
> > for the NSSCertDatabaseChromeOS.
> > 
> > btw. what do you think is the right thing to do in the case the callback is
> > scheduled after the database is deleted?
> 
> "Don't allow that"?
> 
> Otherwise, use a WeakPtrFactory as an internal implementation detail, although
> that creates thread affinity issues.
> 
> You're not calling back into this class though, nor its members, so I'm not
sure
> what the problem would be.

ah, sorry for a bit vague question. There's no problem in NSSCertDatabase per
se. I was more concerned about caller assuming that the database is still around
when the callback runs.. something like:

database->ListCerts(base::Bind(&OnCertList, database));

void OnCertList(database, cert_list) {
  // do something with database
}

What I meant to ask is whether you'd prefer not to invoke the callback if the
database is gone instead of relying on ListCert users to do the right thing. The
thing I don't like about that approach is exactly what you mentioned: stricter
threading constraints due to use of weak ptrs (i.e. ListCert could not be called
from the UI thread).

https://codereview.chromium.org/144423007/diff/610001/chromeos/cert_loader.h
File chromeos/cert_loader.h (right):

https://codereview.chromium.org/144423007/diff/610001/chromeos/cert_loader.h#...
chromeos/cert_loader.h:64: // expexts it to stay alive at least until the
shutdown of the main thread.
On 2014/01/30 04:34:37, mattm wrote:
> This doesn't seem true, or maybe just confusingly worded? The database is
owned
> by the ResourceContext, I would expect that to get destroyed while the
IOThread
> is still alive, let alone the main thread.

Not worded precisely enough. It should be:
"it expects it to stay alive at least until the shutdown _starts_ on the main
thread."(i.e. before we start tearing down profiles)

https://codereview.chromium.org/144423007/diff/610001/chromeos/cert_loader.h#...
chromeos/cert_loader.h:64: // expexts it to stay alive at least until the
shutdown of the main thread.
On 2014/01/30 04:34:37, mattm wrote:
> expects

Done.

https://codereview.chromium.org/144423007/diff/610001/net/cert/nss_cert_datab...
File net/cert/nss_cert_database.cc (right):

https://codereview.chromium.org/144423007/diff/610001/net/cert/nss_cert_datab...
net/cert/nss_cert_database.cc:48: 
On 2014/01/30 04:16:30, Ryan Sleevi wrote:
> unnecessary newline

Done.

https://codereview.chromium.org/144423007/diff/610001/net/cert/nss_cert_datab...
net/cert/nss_cert_database.cc:90: base::Unretained(raw_certs)),
On 2014/01/30 04:16:30, Ryan Sleevi wrote:
> base::Unretained(certs.get()) ?

that would return NULL due to base::Passed bellow

https://codereview.chromium.org/144423007/diff/610001/net/cert/nss_cert_datab...
File net/cert/nss_cert_database.h (right):

https://codereview.chromium.org/144423007/diff/610001/net/cert/nss_cert_datab...
net/cert/nss_cert_database.h:104: // DEPRECATED: Use |ListCerts| instead.
On 2014/01/30 04:16:30, Ryan Sleevi wrote:
> File a bug to track this work & reference here

Done.

https://codereview.chromium.org/144423007/diff/610001/net/cert/nss_cert_datab...
net/cert/nss_cert_database.h:232: // Static so it may safelly be used on the
worker thread.
On 2014/01/30 04:34:37, mattm wrote:
> spelling nit: safely

Done.

https://codereview.chromium.org/144423007/diff/610001/net/cert/nss_cert_datab...
net/cert/nss_cert_database.h:235: // Gets task runner that should be use for
slow tasks like certificate
On 2014/01/30 04:34:37, mattm wrote:
> nit: s/use/used/

Done.

https://codereview.chromium.org/144423007/diff/610001/net/cert/nss_cert_datab...
File net/cert/nss_cert_database_chromeos.cc (right):

https://codereview.chromium.org/144423007/diff/610001/net/cert/nss_cert_datab...
net/cert/nss_cert_database_chromeos.cc:79: certs->erase(std::remove_if(
On 2014/01/30 04:16:30, Ryan Sleevi wrote:
> IWYU: include <algorithm> if using remove_if

Done.

https://codereview.chromium.org/144423007/diff/610001/net/cert/nss_cert_datab...
File net/cert/nss_cert_database_chromeos.h (right):

https://codereview.chromium.org/144423007/diff/610001/net/cert/nss_cert_datab...
net/cert/nss_cert_database_chromeos.h:39: // is additionally filterred by
|profile_filter|.
On 2014/01/30 04:34:37, mattm wrote:
> filtered

Done.

https://codereview.chromium.org/144423007/diff/610001/net/cert/nss_cert_datab...
net/cert/nss_cert_database_chromeos.h:40: // Static so it may safelly be used on
the worker thread.
On 2014/01/30 04:34:37, mattm wrote:
> safely

Done.

https://codereview.chromium.org/144423007/diff/610001/net/cert/nss_cert_datab...
net/cert/nss_cert_database_chromeos.h:41: static void
ListCertsImpl(scoped_ptr<NSSProfileFilterChromeOS> profile_filter,
On 2014/01/30 04:16:30, Ryan Sleevi wrote:
> Just pass as const NSSProfileFilterChromeOS&
> 
> base::BindState will allocate storage for the bound parameters, which is the
> same effect as the scoped_ptr<>, but avoids an *additional* allocation (of a
> holder for *this* scoped_ptr)

Done.

[mattm, 2014-02-04 02:28:38]

lgtm

[Ryan Sleevi, 2014-02-04 02:50:19]

lgtm

https://codereview.chromium.org/144423007/diff/530001/net/cert/nss_cert_datab...
File net/cert/nss_cert_database.h (right):

https://codereview.chromium.org/144423007/diff/530001/net/cert/nss_cert_datab...
net/cert/nss_cert_database.h:115: ListCertsCallback;
On 2014/02/03 23:42:04, tbarzic wrote:
> On 2014/01/30 04:16:30, Ryan Sleevi wrote:
> > On 2014/01/29 20:50:52, tbarzic wrote:
> > > On 2014/01/29 02:24:20, Ryan Sleevi wrote:
> > > > Move typedef to line 104 (
> > > >
> > >
> >
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...
> > > > )
> > > > 
> > > > Design: Why does this API receive a scoped_ptr<CertificateList>? Why not
> > just
> > > > have it be const CertificateList&?
> > > > 
> > > > The motivation (perhaps misguided) of having ListCertsSync was to avoid
a
> > > > reallocation of a vector, but that's unavoidable here with the async
> > > ListCerts,
> > > > so the API doesn't make sense there.
> > > 
> > > The motivation was to avoid copying the vector while passing it between
> > threads
> > > (by passing around a pointer), and not having to wrap the callback passed
to
> > > |ListCerts| with a method that would call it with a reference instead of a
> > > pointer.
> > > 
> > > Though, having a wrapper around |callback| would be useful for handling
case
> > > where the databasae is gone by the time callback triggers, which could
> happen
> > > for the NSSCertDatabaseChromeOS.
> > > 
> > > btw. what do you think is the right thing to do in the case the callback
is
> > > scheduled after the database is deleted?
> > 
> > "Don't allow that"?
> > 
> > Otherwise, use a WeakPtrFactory as an internal implementation detail,
although
> > that creates thread affinity issues.
> > 
> > You're not calling back into this class though, nor its members, so I'm not
> sure
> > what the problem would be.
> 
> ah, sorry for a bit vague question. There's no problem in NSSCertDatabase per
> se. I was more concerned about caller assuming that the database is still
around
> when the callback runs.. something like:
> 
> database->ListCerts(base::Bind(&OnCertList, database));
> 
> void OnCertList(database, cert_list) {
>   // do something with database
> }
> 
> What I meant to ask is whether you'd prefer not to invoke the callback if the
> database is gone instead of relying on ListCert users to do the right thing.
The
> thing I don't like about that approach is exactly what you mentioned: stricter
> threading constraints due to use of weak ptrs (i.e. ListCert could not be
called
> from the UI thread).

Yeah, that's "caller's responsibility, so don't do that if it's dangerous"

[tbarzic, 2014-02-04 18:56:05]

+Steven for chromeos bits

[stevenjb, 2014-02-04 22:28:41]

https://codereview.chromium.org/144423007/diff/670001/chrome/browser/certific...
File chrome/browser/certificate_manager_model.cc (right):

https://codereview.chromium.org/144423007/diff/670001/chrome/browser/certific...
chrome/browser/certificate_manager_model.cc:93: DVLOG(1) << "refresh listing
certs...";
TODO(): make this async

https://codereview.chromium.org/144423007/diff/670001/chromeos/cert_loader.cc
File chromeos/cert_loader.cc (right):

https://codereview.chromium.org/144423007/diff/670001/chromeos/cert_loader.cc...
chromeos/cert_loader.cc:149: weak_factory_.GetWeakPtr()));
nit: align

https://codereview.chromium.org/144423007/diff/670001/chromeos/cert_loader.cc...
chromeos/cert_loader.cc:159: cert_list_.swap(*cert_list);
cert_list.release() to make the ownership transfer clear. Better yet would be to
make cert_list_ a scoped_ptr<> and Pass() cert_list (but that looks like it
would be a bigger change, so fine not to do that here).

https://codereview.chromium.org/144423007/diff/670001/chromeos/network/onc/on...
File chromeos/network/onc/onc_certificate_importer_impl.cc (right):

https://codereview.chromium.org/144423007/diff/670001/chromeos/network/onc/on...
chromeos/network/onc/onc_certificate_importer_impl.cc:81:
net::NSSCertDatabase::GetInstance()->ListCertsSync(&all_certs);
TODO(): make this async

https://codereview.chromium.org/144423007/diff/670001/net/cert/nss_cert_datab...
File net/cert/nss_cert_database.cc (right):

https://codereview.chromium.org/144423007/diff/670001/net/cert/nss_cert_datab...
net/cert/nss_cert_database.cc:84: CertificateList* raw_certs = certs.get();
nit: No need for local here, I think it would be more clear to have the
certs.get() near the base::Passed(&certs) in PostTaskAndReply.

https://codereview.chromium.org/144423007/diff/670001/net/cert/nss_cert_datab...
File net/cert/nss_cert_database.h (right):

https://codereview.chromium.org/144423007/diff/670001/net/cert/nss_cert_datab...
net/cert/nss_cert_database.h:233: static void ListCertsImpl(CertificateList*
certs);
Does this need to be exposed? It seems like it can and should be file local.

https://codereview.chromium.org/144423007/diff/670001/net/cert/nss_cert_datab...
File net/cert/nss_cert_database_chromeos.cc (right):

https://codereview.chromium.org/144423007/diff/670001/net/cert/nss_cert_datab...
net/cert/nss_cert_database_chromeos.cc:38: CertificateList* raw_certs =
certs.get();
nit: unnecessary.

https://codereview.chromium.org/144423007/diff/670001/net/cert/nss_cert_datab...
File net/cert/nss_cert_database_chromeos.h (right):

https://codereview.chromium.org/144423007/diff/670001/net/cert/nss_cert_datab...
net/cert/nss_cert_database_chromeos.h:42: CertificateList* certs);
File local?

[tbarzic, 2014-02-04 23:12:37]

https://codereview.chromium.org/144423007/diff/670001/chrome/browser/certific...
File chrome/browser/certificate_manager_model.cc (right):

https://codereview.chromium.org/144423007/diff/670001/chrome/browser/certific...
chrome/browser/certificate_manager_model.cc:93: DVLOG(1) << "refresh listing
certs...";
On 2014/02/04 22:28:42, stevenjb wrote:
> TODO(): make this async

Done.

https://codereview.chromium.org/144423007/diff/670001/chromeos/cert_loader.cc
File chromeos/cert_loader.cc (right):

https://codereview.chromium.org/144423007/diff/670001/chromeos/cert_loader.cc...
chromeos/cert_loader.cc:149: weak_factory_.GetWeakPtr()));
On 2014/02/04 22:28:42, stevenjb wrote:
> nit: align

Done.

https://codereview.chromium.org/144423007/diff/670001/chromeos/cert_loader.cc...
chromeos/cert_loader.cc:159: cert_list_.swap(*cert_list);
On 2014/02/04 22:28:42, stevenjb wrote:
> cert_list.release() to make the ownership transfer clear. Better yet would be
to
> make cert_list_ a scoped_ptr<> and Pass() cert_list (but that looks like it
> would be a bigger change, so fine not to do that here).
> 

It's not that bigger.. Done.

https://codereview.chromium.org/144423007/diff/670001/chromeos/network/onc/on...
File chromeos/network/onc/onc_certificate_importer_impl.cc (right):

https://codereview.chromium.org/144423007/diff/670001/chromeos/network/onc/on...
chromeos/network/onc/onc_certificate_importer_impl.cc:81:
net::NSSCertDatabase::GetInstance()->ListCertsSync(&all_certs);
On 2014/02/04 22:28:42, stevenjb wrote:
> TODO(): make this async

Done.

https://codereview.chromium.org/144423007/diff/670001/net/cert/nss_cert_datab...
File net/cert/nss_cert_database.cc (right):

https://codereview.chromium.org/144423007/diff/670001/net/cert/nss_cert_datab...
net/cert/nss_cert_database.cc:84: CertificateList* raw_certs = certs.get();
On 2014/02/04 22:28:42, stevenjb wrote:
> nit: No need for local here, I think it would be more clear to have the
> certs.get() near the base::Passed(&certs) in PostTaskAndReply.
> 

base::Passed(&certs) would get called before certs.get() (so certs would be
NULLed out).

https://codereview.chromium.org/144423007/diff/670001/net/cert/nss_cert_datab...
File net/cert/nss_cert_database.h (right):

https://codereview.chromium.org/144423007/diff/670001/net/cert/nss_cert_datab...
net/cert/nss_cert_database.h:233: static void ListCertsImpl(CertificateList*
certs);
On 2014/02/04 22:28:42, stevenjb wrote:
> Does this need to be exposed? It seems like it can and should be file local.

Yep, It's used by NSSCertDatabaseChromeOS.

https://codereview.chromium.org/144423007/diff/670001/net/cert/nss_cert_datab...
File net/cert/nss_cert_database_chromeos.h (right):

https://codereview.chromium.org/144423007/diff/670001/net/cert/nss_cert_datab...
net/cert/nss_cert_database_chromeos.h:42: CertificateList* certs);
On 2014/02/04 22:28:42, stevenjb wrote:
> File local?

It uses protected NSSCertDatabase::ListCertsImpl.

[stevenjb, 2014-02-05 00:24:59]

lgtm

https://codereview.chromium.org/144423007/diff/670001/net/cert/nss_cert_datab...
File net/cert/nss_cert_database.cc (right):

https://codereview.chromium.org/144423007/diff/670001/net/cert/nss_cert_datab...
net/cert/nss_cert_database.cc:84: CertificateList* raw_certs = certs.get();
On 2014/02/04 23:12:38, tbarzic wrote:
> On 2014/02/04 22:28:42, stevenjb wrote:
> > nit: No need for local here, I think it would be more clear to have the
> > certs.get() near the base::Passed(&certs) in PostTaskAndReply.
> > 
> 
> base::Passed(&certs) would get called before certs.get() (so certs would be
> NULLed out).

Ah, right, sorry, nevermind :)

https://codereview.chromium.org/144423007/diff/670001/net/cert/nss_cert_datab...
File net/cert/nss_cert_database_chromeos.h (right):

https://codereview.chromium.org/144423007/diff/670001/net/cert/nss_cert_datab...
net/cert/nss_cert_database_chromeos.h:42: CertificateList* certs);
On 2014/02/04 23:12:38, tbarzic wrote:
> On 2014/02/04 22:28:42, stevenjb wrote:
> > File local?
> 
> It uses protected NSSCertDatabase::ListCertsImpl.

Ah, I see. A bit confusing, but fine I guess.

[tbarzic, 2014-02-06 01:50:13]

The CQ bit was checked by tbarzic@chromium.org

[commit-bot: I haz the power, 2014-02-06 01:54:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tbarzic@chromium.org/144423007/1110001

[commit-bot: I haz the power, 2014-02-06 10:24:21]

Change committed as 249334