Replace c/b/nss_context by a KeyedService.

Replace c/b/nss_context by a KeyedService.

Replaces nss_context.h by the new KeyedServices CertDatabaseService. Unifies the maintenance of NSSCertDatabase on Linux and ChromeOS (cf. nss_context_{linux,chromeos}.cc).

The new service takes care of triggering the lower level NSS database and TPM initialization, which was previously in profile_io_data.cc which was clearly exposing too many details there.

The new classes are:
 CertDatabaseService: the UI part of the Service, provides access to the IO part.
 CertDatabaseServiceIOPart (with implementations for Linux and ChromeOS): the IO part of the Service. Provides access to the backing NSSCertDatabase through callback. Ensures that the database is returned after all tokens/slots are fully initialized.
 CertDatabaseServiceFactory: creating the service instances per Profile.

More detailed list of changes:
1) Add CertDatabase classes, implement a KeyedService. Code lives in a new component cert_database.

2) Implement per-user NSS initialization in CertDatabaseServiceIOPart with a state machine and remove it from UserSessionManager and ProfileImpl.

3) CertDatabaseServiceIOPartChromeOS has to wait for the system token being initialized. This was not well implemented before: it was possible to get() the system token from NSSCertDatabase, before it was set().

4) Remove nss_context_*

5) Migrate all usages of nss_context to CertDatabaseService

Still missing unit tests for the new classes.

BUG=413219

[pneubeck (no reviews), 2014-07-24 22:03:56]

Hi Ryan,
this is the first draft for the keyed service. I still have to move the nss
initialization code from profile_io_data.cc to CertDatabaseServiceIOPart.

It would be really awesome if you could take a look at the high-level structure.
It's still WIP, so it's not really worth to spend time in details (e.g.
base::Unretained... *ehem*).

However, this is depending on the two other CLs that are still pending :-/

[pneubeck (no reviews), 2014-07-24 22:05:07]

+Toni, maybe you could also take an early look at this, since you wrote the
initialization from UserManager (now UserSessionManager).

Thanks!

[pneubeck (no reviews), 2014-07-25 08:07:47]

On 2014/07/24 22:05:07, pneubeck wrote:
> +Toni, maybe you could also take an early look at this, since you wrote the
> initialization from UserManager (now UserSessionManager).
> 
> Thanks!

great... didn't upload half of the new files :-/

[pneubeck (no reviews), 2014-09-02 16:08:34]

pneubeck@chromium.org changed reviewers:
+ mattm@chromium.org

[pneubeck (no reviews), 2014-09-02 16:14:08]

I think this progressed far enough that the high-level design can be reviewed.

Please take a look and don't bother about nits. Tomorrow, I will look into the
Linux implementation.

[pneubeck (no reviews), 2014-09-02 16:14:22]

Thanks!

[pneubeck (no reviews), 2014-09-03 15:31:49]

Patchset #3 (id:240001) has been deleted

[pneubeck (no reviews), 2014-09-03 15:35:23]

Added the (trivial) Linux implementation.

[mattm, 2014-09-05 07:35:29]

Haven't fully digested this, but some initial comments.

https://codereview.chromium.org/419013003/diff/260001/chrome/browser/chromeos...
File
chrome/browser/chromeos/policy/user_network_configuration_updater_factory.cc
(right):

https://codereview.chromium.org/419013003/diff/260001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_network_configuration_updater_factory.cc:100:
base::Unretained(updater)));
Is Unretained safe here?  Are ownership/lifetime issues fair game or part of the
details you were saving for later? (Just wondering how hard I should think about
this)

https://codereview.chromium.org/419013003/diff/260001/chrome/browser/net/nss_...
File chrome/browser/net/nss_context.h (right):

https://codereview.chromium.org/419013003/diff/260001/chrome/browser/net/nss_...
chrome/browser/net/nss_context.h:49: #endif  //
CHROME_BROWSER_NET_NSS_CONTEXT_H_
whole file should be deleted?

https://codereview.chromium.org/419013003/diff/260001/chrome/browser/profiles...
File chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc
(right):

https://codereview.chromium.org/419013003/diff/260001/chrome/browser/profiles...
chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc:223:
CertDatabaseServiceFactory::GetInstance();
do you need to do this in linux too?

https://codereview.chromium.org/419013003/diff/260001/chrome/browser/profiles...
File chrome/browser/profiles/profile_io_data.h (right):

https://codereview.chromium.org/419013003/diff/260001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.h:221: std::string username_hash() const
{
I think this can be removed too

https://codereview.chromium.org/419013003/diff/260001/components/cert_databas...
File components/cert_database.gypi (right):

https://codereview.chromium.org/419013003/diff/260001/components/cert_databas...
components/cert_database.gypi:20:
'cert_database/linux/cert_database_service_io_part_linux.cc',
not in CL

https://codereview.chromium.org/419013003/diff/260001/components/cert_databas...
components/cert_database.gypi:26:
'cert_database/public/linux/cert_database_service_io_part_linux.h',
these 2 not in CL

[pneubeck (no reviews), 2014-09-05 08:04:56]

https://codereview.chromium.org/419013003/diff/260001/chrome/browser/chromeos...
File
chrome/browser/chromeos/policy/user_network_configuration_updater_factory.cc
(right):

https://codereview.chromium.org/419013003/diff/260001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_network_configuration_updater_factory.cc:100:
base::Unretained(updater)));
On 2014/09/05 07:35:28, mattm wrote:
> Is Unretained safe here?  Are ownership/lifetime issues fair game or part of
the
> details you were saving for later? (Just wondering how hard I should think
about
> this)

Yes, this one isn't thought through yet.
I'll add a HUGE comment here.

https://codereview.chromium.org/419013003/diff/260001/chrome/browser/net/nss_...
File chrome/browser/net/nss_context.h (right):

https://codereview.chromium.org/419013003/diff/260001/chrome/browser/net/nss_...
chrome/browser/net/nss_context.h:49: #endif  //
CHROME_BROWSER_NET_NSS_CONTEXT_H_
On 2014/09/05 07:35:29, mattm wrote:
> whole file should be deleted?

Done.

https://codereview.chromium.org/419013003/diff/260001/chrome/browser/profiles...
File chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc
(right):

https://codereview.chromium.org/419013003/diff/260001/chrome/browser/profiles...
chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc:223:
CertDatabaseServiceFactory::GetInstance();
On 2014/09/05 07:35:29, mattm wrote:
> do you need to do this in linux too?

Yes, should be there for USE_NSS

https://codereview.chromium.org/419013003/diff/260001/chrome/browser/profiles...
File chrome/browser/profiles/profile_io_data.h (right):

https://codereview.chromium.org/419013003/diff/260001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.h:221: std::string username_hash() const
{
On 2014/09/05 07:35:29, mattm wrote:
> I think this can be removed too

Yes, I've removed it now from ProfileIOData.
But sadly we can't remove it from ProfileParams yet, because we still use it in
ProfileIOData::Init:
 ..public_slot.. =
crypto::GetPublicSlotForChromeOSUser(profile_params_->username_hash)
  ... new chromeos::CertVerifyProcChromeOS(public_slot.Pass()))

The new ServiceIOPart can't be used in this case without further adaptions,
because CertVerifyProcChromeOS expects the public_slot to be available at
construction time.

https://codereview.chromium.org/419013003/diff/260001/components/cert_databas...
File components/cert_database.gypi (right):

https://codereview.chromium.org/419013003/diff/260001/components/cert_databas...
components/cert_database.gypi:20:
'cert_database/linux/cert_database_service_io_part_linux.cc',
On 2014/09/05 07:35:29, mattm wrote:
> not in CL

Done.

https://codereview.chromium.org/419013003/diff/260001/components/cert_databas...
components/cert_database.gypi:26:
'cert_database/public/linux/cert_database_service_io_part_linux.h',
On 2014/09/05 07:35:29, mattm wrote:
> these 2 not in CL

Grmpf... added.

[pneubeck (no reviews), 2014-09-05 08:56:26]

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

[pneubeck (no reviews), 2014-09-05 08:56:27]

+David for looking at the high-level design as well.

[pneubeck (no reviews), 2014-09-09 16:58:31]

If there are no high-level concerns until tomorrow, I'd continue preparing this
CL for a full detailed review.

[pneubeck (no reviews), 2014-09-11 13:47:02]

Patchset #7 (id:340001) has been deleted

[pneubeck (no reviews), 2014-09-11 16:25:04]

@Matt, I think in previous patchsets it was not a good idea to move the
ClientCertStoreChromeOS from //net to //chromeos.
Instead, I added now a NSSProfileFilterChromeOSFactory to inject the
NSSProfileFilter instead of the CertDatabaseServiceIOPart.

That will also make the unit_test of the ClientCertStoreChromeOS simpler, as the
store only depends on the Filter and not on the service or NSSCertDatabase
anymore.

Does that look ok to you?

[pneubeck (no reviews), 2014-09-12 09:41:13]

Patchset #7 (id:360001) has been deleted

[pneubeck (no reviews), 2014-09-12 09:46:17]

Patchset #8 (id:400001) has been deleted

[pneubeck (no reviews), 2014-09-12 16:15:37]

Patchset #9 (id:440001) has been deleted

[pneubeck (no reviews), 2014-09-12 16:20:24]

https://codereview.chromium.org/419013003/diff/260001/chrome/browser/chromeos...
File
chrome/browser/chromeos/policy/user_network_configuration_updater_factory.cc
(right):

https://codereview.chromium.org/419013003/diff/260001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_network_configuration_updater_factory.cc:100:
base::Unretained(updater)));
This is fixed now.

[pneubeck (no reviews), 2014-09-12 16:53:23]

Patchset #10 (id:480001) has been deleted

[pneubeck (no reviews), 2014-09-12 17:40:12]

Patchset #9 (id:460001) has been deleted

[pneubeck (no reviews), 2014-09-18 15:18:55]

Patchset #10 (id:520001) has been deleted

[pneubeck (no reviews), 2014-09-19 12:05:54]

Patchset #11 (id:560001) has been deleted

[pneubeck (no reviews), 2014-09-19 13:38:41]

Patchset #11 (id:580001) has been deleted

[pneubeck (no reviews), 2014-09-19 13:38:59]

Patchset #11 (id:600001) has been deleted

[pneubeck (no reviews), 2014-10-13 17:03:12]

Patchset #12 (id:640001) has been deleted

[pneubeck (no reviews), 2014-10-14 15:44:10]

@Ryan, Matt
This should be good for review now. Not sure how you want to split review work.
You can skip at least the files
  *network_configuration_updater*
  *platform_keys*

The only major missing part is unit tests in components/cert_database .
One possible solution is to change how it accesses crypto/nss_util , so that the
calls can be mocked out. Or we use/extend the test capabilities in nss_util . If
you have a preference, please let me know.

Sorry, that it's still such a large CL. I already tried to separate changes into
preparing CLs so that this one is smaller / easier to review.

Thanks!

[mattm, 2014-10-18 00:45:16]

https://codereview.chromium.org/419013003/diff/1330001/chrome/browser/chromeo...
File chrome/browser/chromeos/net/cert_profile_filter.cc (right):

https://codereview.chromium.org/419013003/diff/1330001/chrome/browser/chromeo...
chrome/browser/chromeos/net/cert_profile_filter.cc:32: return
scoped_ptr<net::NSSProfileFilterChromeOS>();
return nullptr

https://codereview.chromium.org/419013003/diff/1330001/chrome/browser/chromeo...
chrome/browser/chromeos/net/cert_profile_filter.cc:61: new
ProfileFilterFactory(cert_db_io));
I think you can use make_scoped_ptr for this case now.

https://codereview.chromium.org/419013003/diff/1330001/chrome/browser/chromeo...
File chrome/browser/chromeos/net/cert_profile_filter.h (right):

https://codereview.chromium.org/419013003/diff/1330001/chrome/browser/chromeo...
chrome/browser/chromeos/net/cert_profile_filter.h:27:
scoped_ptr<net::NSSProfileFilterChromeOSFactory> CreateProfileFilterFactory(
name should be a bit more specific

https://codereview.chromium.org/419013003/diff/1330001/chrome/browser/net/cer...
File chrome/browser/net/cert_database_service_factory.cc (right):

https://codereview.chromium.org/419013003/diff/1330001/chrome/browser/net/cer...
chrome/browser/net/cert_database_service_factory.cc:31: namespace cert_database
{
Is it normal to use the same namespace in chrome/ and in the components/ dir?

https://codereview.chromium.org/419013003/diff/1330001/chrome/browser/net/cer...
chrome/browser/net/cert_database_service_factory.cc:36: void
WrapReadyCallbackForUI(
Maybe "RunReadyCallbackOnIOThread" ?

https://codereview.chromium.org/419013003/diff/1330001/chrome/browser/net/cer...
chrome/browser/net/cert_database_service_factory.cc:73: // create the
CertDatabaseService on demand.
Is this comment in the right place?

https://codereview.chromium.org/419013003/diff/1330001/chrome/browser/net/cer...
chrome/browser/net/cert_database_service_factory.cc:83: #if defined(OS_CHROMEOS)
Can this be changed to use platform specific files for
CertDatabaseServiceFactory::BuildServiceInstanceFor instead of ifdefs?

https://codereview.chromium.org/419013003/diff/1330001/chrome/browser/net/cer...
chrome/browser/net/cert_database_service_factory.cc:84: Profile* profile =
static_cast<Profile*>(context);
Profile::FromBrowserContext(context)

https://codereview.chromium.org/419013003/diff/1330001/chrome/browser/net/cer...
chrome/browser/net/cert_database_service_factory.cc:128:
service->SetIOPart(io_part.PassAs<CertDatabaseServiceIOPart>());
I think you can use Pass instead of PassAs now

https://codereview.chromium.org/419013003/diff/1330001/chrome/browser/ui/cryp...
File chrome/browser/ui/crypto_module_delegate_nss.h (right):

https://codereview.chromium.org/419013003/diff/1330001/chrome/browser/ui/cryp...
chrome/browser/ui/crypto_module_delegate_nss.h:39: static void
CreateForResourceContext(
update name

https://codereview.chromium.org/419013003/diff/1330001/components/cert_databa...
File components/cert_database/chromeos/cert_database_service_io_part_chromeos.cc
(right):

https://codereview.chromium.org/419013003/diff/1330001/components/cert_databa...
components/cert_database/chromeos/cert_database_service_io_part_chromeos.cc:112:
void RunNextStep(const State& next_state) {
hm, not entirely sure about the state machine, especially the way the system
token ready is handled sort of weirdly by the state machine.

https://codereview.chromium.org/419013003/diff/1330001/components/cert_databa...
components/cert_database/chromeos/cert_database_service_io_part_chromeos.cc:216:
void GetPrivateSlotDONE(crypto::ScopedPK11Slot private_slot) {
Name style. Maybe GotPrivateSlot or DidGetPrivateSlot. (elsewhere too)

https://codereview.chromium.org/419013003/diff/1330001/components/cert_databa...
components/cert_database/chromeos/cert_database_service_io_part_chromeos.cc:268:
io_part_->DidCreateNSSCertDatabase(db.PassAs<net::NSSCertDatabase>());
can use Pass() now

https://codereview.chromium.org/419013003/diff/1330001/net/ssl/client_cert_st...
File net/ssl/client_cert_store_chromeos.h (right):

https://codereview.chromium.org/419013003/diff/1330001/net/ssl/client_cert_st...
net/ssl/client_cert_store_chromeos.h:53: #endif  //
NET_SSL_CLIENT_CERT_STORE_CHROMEOS_H_
Weird, I wonder why reitveld lists client_cert_store_chromeos.h and
client_cert_store_chromeos.cc as deleted in the issue summary.

[pneubeck (no reviews), 2014-10-21 09:22:09]

https://codereview.chromium.org/419013003/diff/1330001/chrome/browser/chromeo...
File chrome/browser/chromeos/net/cert_profile_filter.cc (right):

https://codereview.chromium.org/419013003/diff/1330001/chrome/browser/chromeo...
chrome/browser/chromeos/net/cert_profile_filter.cc:32: return
scoped_ptr<net::NSSProfileFilterChromeOS>();
On 2014/10/18 00:45:15, mattm wrote:
> return nullptr

Done.

https://codereview.chromium.org/419013003/diff/1330001/chrome/browser/chromeo...
chrome/browser/chromeos/net/cert_profile_filter.cc:61: new
ProfileFilterFactory(cert_db_io));
On 2014/10/18 00:45:15, mattm wrote:
> I think you can use make_scoped_ptr for this case now.

Done.

https://codereview.chromium.org/419013003/diff/1330001/chrome/browser/chromeo...
File chrome/browser/chromeos/net/cert_profile_filter.h (right):

https://codereview.chromium.org/419013003/diff/1330001/chrome/browser/chromeo...
chrome/browser/chromeos/net/cert_profile_filter.h:27:
scoped_ptr<net::NSSProfileFilterChromeOSFactory> CreateProfileFilterFactory(
On 2014/10/18 00:45:15, mattm wrote:
> name should be a bit more specific

Done.

https://codereview.chromium.org/419013003/diff/1330001/chrome/browser/net/cer...
File chrome/browser/net/cert_database_service_factory.cc (right):

https://codereview.chromium.org/419013003/diff/1330001/chrome/browser/net/cer...
chrome/browser/net/cert_database_service_factory.cc:31: namespace cert_database
{
On 2014/10/18 00:45:15, mattm wrote:
> Is it normal to use the same namespace in chrome/ and in the components/ dir?

Not sure, I find examples for both, even for the same component.
E.g.
  chrome/browser/history/history_service.h
  chrome/browser/history/history_database.h

Bookmarks also reuses the namespace
  chrome/browser/ui/cocoa/bookmarks/bookmark_bar_constants.h

If we had a chrome namespace, is would prefer chrome::cert_database.
Since we don't, I think the current choice is reasonable.

https://codereview.chromium.org/419013003/diff/1330001/chrome/browser/net/cer...
chrome/browser/net/cert_database_service_factory.cc:36: void
WrapReadyCallbackForUI(
On 2014/10/18 00:45:16, mattm wrote:
> Maybe "RunReadyCallbackOnIOThread" ?

Done.

https://codereview.chromium.org/419013003/diff/1330001/chrome/browser/net/cer...
chrome/browser/net/cert_database_service_factory.cc:73: // create the
CertDatabaseService on demand.
On 2014/10/18 00:45:16, mattm wrote:
> Is this comment in the right place?

Done.

https://codereview.chromium.org/419013003/diff/1330001/chrome/browser/net/cer...
chrome/browser/net/cert_database_service_factory.cc:84: Profile* profile =
static_cast<Profile*>(context);
On 2014/10/18 00:45:15, mattm wrote:
> Profile::FromBrowserContext(context)

Done.

https://codereview.chromium.org/419013003/diff/1330001/chrome/browser/net/cer...
chrome/browser/net/cert_database_service_factory.cc:128:
service->SetIOPart(io_part.PassAs<CertDatabaseServiceIOPart>());
On 2014/10/18 00:45:16, mattm wrote:
> I think you can use Pass instead of PassAs now

Awesome! Thanks for pointing that out.

https://codereview.chromium.org/419013003/diff/1330001/chrome/browser/ui/cryp...
File chrome/browser/ui/crypto_module_delegate_nss.h (right):

https://codereview.chromium.org/419013003/diff/1330001/chrome/browser/ui/cryp...
chrome/browser/ui/crypto_module_delegate_nss.h:39: static void
CreateForResourceContext(
On 2014/10/18 00:45:16, mattm wrote:
> update name

Done.

https://codereview.chromium.org/419013003/diff/1330001/components/cert_databa...
File components/cert_database/chromeos/cert_database_service_io_part_chromeos.cc
(right):

https://codereview.chromium.org/419013003/diff/1330001/components/cert_databa...
components/cert_database/chromeos/cert_database_service_io_part_chromeos.cc:112:
void RunNextStep(const State& next_state) {
On 2014/10/18 00:45:16, mattm wrote:
> hm, not entirely sure about the state machine, especially the way the system
> token ready is handled sort of weirdly by the state machine.

What specific do you mean?
The fact that it's waiting for a call from the outside? That's like a
notification, which should be fine.

The TPMTokenLoader will still have to be updated (independent of / after this
change), as it was meant at some time for the user's token and now is
responsible for the device token.
Alternatively, if nss_util already takes care of global state of TPM
initialization, then we could probably get rid of TPMTokenLoader altogether?
But I'd prefer postponing that to a follow-up CL.

https://codereview.chromium.org/419013003/diff/1330001/components/cert_databa...
components/cert_database/chromeos/cert_database_service_io_part_chromeos.cc:216:
void GetPrivateSlotDONE(crypto::ScopedPK11Slot private_slot) {
On 2014/10/18 00:45:16, mattm wrote:
> Name style. Maybe GotPrivateSlot or DidGetPrivateSlot. (elsewhere too)

Done.

https://codereview.chromium.org/419013003/diff/1330001/components/cert_databa...
components/cert_database/chromeos/cert_database_service_io_part_chromeos.cc:268:
io_part_->DidCreateNSSCertDatabase(db.PassAs<net::NSSCertDatabase>());
On 2014/10/18 00:45:16, mattm wrote:
> can use Pass() now

Done.

https://codereview.chromium.org/419013003/diff/1330001/net/ssl/client_cert_st...
File net/ssl/client_cert_store_chromeos.h (right):

https://codereview.chromium.org/419013003/diff/1330001/net/ssl/client_cert_st...
net/ssl/client_cert_store_chromeos.h:53: #endif  //
NET_SSL_CLIENT_CERT_STORE_CHROMEOS_H_
On 2014/10/18 00:45:16, mattm wrote:
> Weird, I wonder why reitveld lists client_cert_store_chromeos.h and
> client_cert_store_chromeos.cc as deleted in the issue summary.

I moved it in a previous patchset. No idea why it doesn't pick it up...

[tbarzic, 2014-10-22 20:38:20]

https://codereview.chromium.org/419013003/diff/1370001/chrome/browser/net/cer...
File chrome/browser/net/cert_database_service_factory_chromeos.cc (right):

https://codereview.chromium.org/419013003/diff/1370001/chrome/browser/net/cer...
chrome/browser/net/cert_database_service_factory_chromeos.cc:51: // domain as
the device is registered to.
why do we allow system key slot only in this case (unrelated to this cl, I'm
just curious)

https://codereview.chromium.org/419013003/diff/1370001/chrome/browser/ui/cryp...
File chrome/browser/ui/crypto_module_delegate_nss.cc (right):

https://codereview.chromium.org/419013003/diff/1370001/chrome/browser/ui/cryp...
chrome/browser/ui/crypto_module_delegate_nss.cc:18: void
CreateWithSlot(chrome::CryptoModulePasswordReason reason,
nit: I'd rename this. Maybe to CreateWithPrivateSlot/CreateWithCertDatabase

https://codereview.chromium.org/419013003/diff/1370001/components/cert_databa...
File components/cert_database/chromeos/cert_database_service_io_part_chromeos.cc
(right):

https://codereview.chromium.org/419013003/diff/1370001/components/cert_databa...
components/cert_database/chromeos/cert_database_service_io_part_chromeos.cc:69:
enum State {
You could add a NOT_STARTED state (to distinguish between states before and just
after Run is called)

https://codereview.chromium.org/419013003/diff/1370001/components/cert_databa...
components/cert_database/chromeos/cert_database_service_io_part_chromeos.cc:197:
if (call_status == chromeos::DBUS_METHOD_CALL_FAILURE) {
I wonder if we should handle method call failure by retrying it, similar to what
we do in TPMTokenLoader for system slot (and also add a call to
Pkcs11IsTpmTokenReady).
I'm not completely sure whether it's safe to assume the TPM is ready for the
user at this point.

Though, it would probably be better to do this in a follow up (since this
matches the current behaviour).

https://codereview.chromium.org/419013003/diff/1370001/components/cert_databa...
components/cert_database/chromeos/cert_database_service_io_part_chromeos.cc:226:
return;
DCHECK system_tpm_token_status_ == UNKNOWN

https://codereview.chromium.org/419013003/diff/1370001/components/cert_databa...
components/cert_database/chromeos/cert_database_service_io_part_chromeos.cc:324:
&OnSystemTPMTokenReady, GetWeakPtr(), base::Unretained(this));
Since Internal already has weak_ptr_factory, maybe you could add AsWeakPtr() to
Internal and bind the callback directly to internal_->OnSystemTPMTokenReady.

https://codereview.chromium.org/419013003/diff/1370001/components/cert_databa...
components/cert_database/chromeos/cert_database_service_io_part_chromeos.cc:335:

This is not safe if the method's called more than once (io_part->internal_ may
have been reset in DidCreateNSSCertDatabase). Can you either add a comment about
this to GetSystemTPMTokenReadyCallback, or handle io_part->internal_.get() ==
NULL.

https://codereview.chromium.org/419013003/diff/1370001/components/cert_databa...
File components/cert_database/public/cert_database_service.h (right):

https://codereview.chromium.org/419013003/diff/1370001/components/cert_databa...
components/cert_database/public/cert_database_service.h:47: // This service is
owning the IOPart. However, it lives on the IO thread thus
Maybe mention that the io_part delete task is posted to IO thread from dtor

https://codereview.chromium.org/419013003/diff/1370001/components/cert_databa...
File components/cert_database/public/cert_database_service_io_part.h (right):

https://codereview.chromium.org/419013003/diff/1370001/components/cert_databa...
components/cert_database/public/cert_database_service_io_part.h:38: virtual void
DidCreateNSSCertDatabase(scoped_ptr<net::NSSCertDatabase> db);
I don't think this really has to be virtual, you can just use another name for
CertDatabaseServiceIOPartChromeOS::DidCreateNSSCertDatabase
(linux implementation does not override this method, while ChromeOS
implementation ends up calling the base implementation)

[pneubeck (no reviews), 2014-10-24 08:17:05]

Patchset #17 (id:1410001) has been deleted

[pneubeck (no reviews), 2014-10-24 12:51:37]

https://codereview.chromium.org/419013003/diff/1370001/chrome/browser/net/cer...
File chrome/browser/net/cert_database_service_factory_chromeos.cc (right):

https://codereview.chromium.org/419013003/diff/1370001/chrome/browser/net/cer...
chrome/browser/net/cert_database_service_factory_chromeos.cc:51: // domain as
the device is registered to.
On 2014/10/22 20:38:19, tbarzic wrote:
> why do we allow system key slot only in this case (unrelated to this cl, I'm
> just curious)

clarified the comment, so that other readers do not have to wonder either.

https://codereview.chromium.org/419013003/diff/1370001/chrome/browser/ui/cryp...
File chrome/browser/ui/crypto_module_delegate_nss.cc (right):

https://codereview.chromium.org/419013003/diff/1370001/chrome/browser/ui/cryp...
chrome/browser/ui/crypto_module_delegate_nss.cc:18: void
CreateWithSlot(chrome::CryptoModulePasswordReason reason,
On 2014/10/22 20:38:19, tbarzic wrote:
> nit: I'd rename this. Maybe to CreateWithPrivateSlot/CreateWithCertDatabase

used NSSCertDB to distinguish it from the public function CreateForCertDatabase.

https://codereview.chromium.org/419013003/diff/1370001/components/cert_databa...
File components/cert_database/chromeos/cert_database_service_io_part_chromeos.cc
(right):

https://codereview.chromium.org/419013003/diff/1370001/components/cert_databa...
components/cert_database/chromeos/cert_database_service_io_part_chromeos.cc:69:
enum State {
On 2014/10/22 20:38:19, tbarzic wrote:
> You could add a NOT_STARTED state (to distinguish between states before and
just
> after Run is called)

Done.

https://codereview.chromium.org/419013003/diff/1370001/components/cert_databa...
components/cert_database/chromeos/cert_database_service_io_part_chromeos.cc:197:
if (call_status == chromeos::DBUS_METHOD_CALL_FAILURE) {
On 2014/10/22 20:38:19, tbarzic wrote:
> I wonder if we should handle method call failure by retrying it, similar to
what
> we do in TPMTokenLoader for system slot (and also add a call to
> Pkcs11IsTpmTokenReady).
> I'm not completely sure whether it's safe to assume the TPM is ready for the
> user at this point.
> 
> Though, it would probably be better to do this in a follow up (since this
> matches the current behaviour).

openend a bug and added a todo.

https://codereview.chromium.org/419013003/diff/1370001/components/cert_databa...
components/cert_database/chromeos/cert_database_service_io_part_chromeos.cc:226:
return;
On 2014/10/22 20:38:19, tbarzic wrote:
> DCHECK system_tpm_token_status_ == UNKNOWN

Done.

https://codereview.chromium.org/419013003/diff/1370001/components/cert_databa...
components/cert_database/chromeos/cert_database_service_io_part_chromeos.cc:324:
&OnSystemTPMTokenReady, GetWeakPtr(), base::Unretained(this));
On 2014/10/22 20:38:19, tbarzic wrote:
> Since Internal already has weak_ptr_factory, maybe you could add AsWeakPtr()
to
> Internal and bind the callback directly to internal_->OnSystemTPMTokenReady.

Done. Also clarified the exact usage of the GetCallback function and the
callback itself.
Actually not totally trivial :-/

https://codereview.chromium.org/419013003/diff/1370001/components/cert_databa...
components/cert_database/chromeos/cert_database_service_io_part_chromeos.cc:335:

On 2014/10/22 20:38:19, tbarzic wrote:
> This is not safe if the method's called more than once (io_part->internal_ may
> have been reset in DidCreateNSSCertDatabase). Can you either add a comment
about
> this to GetSystemTPMTokenReadyCallback, or handle io_part->internal_.get() ==
> NULL.

Done.

https://codereview.chromium.org/419013003/diff/1370001/components/cert_databa...
File components/cert_database/public/cert_database_service.h (right):

https://codereview.chromium.org/419013003/diff/1370001/components/cert_databa...
components/cert_database/public/cert_database_service.h:47: // This service is
owning the IOPart. However, it lives on the IO thread thus
On 2014/10/22 20:38:19, tbarzic wrote:
> Maybe mention that the io_part delete task is posted to IO thread from dtor

Done.

https://codereview.chromium.org/419013003/diff/1370001/components/cert_databa...
File components/cert_database/public/cert_database_service_io_part.h (right):

https://codereview.chromium.org/419013003/diff/1370001/components/cert_databa...
components/cert_database/public/cert_database_service_io_part.h:38: virtual void
DidCreateNSSCertDatabase(scoped_ptr<net::NSSCertDatabase> db);
On 2014/10/22 20:38:19, tbarzic wrote:
> I don't think this really has to be virtual, you can just use another name for
> CertDatabaseServiceIOPartChromeOS::DidCreateNSSCertDatabase
> (linux implementation does not override this method, while ChromeOS
> implementation ends up calling the base implementation)

Yes, that's better.

[pneubeck (no reviews), 2014-10-24 12:51:58]

Patchset #17 (id:1430001) has been deleted

[tbarzic, 2014-10-27 20:16:51]

lgtm

https://codereview.chromium.org/419013003/diff/1450001/components/cert_databa...
File components/cert_database/chromeos/cert_database_service_io_part_chromeos.cc
(right):

https://codereview.chromium.org/419013003/diff/1450001/components/cert_databa...
components/cert_database/chromeos/cert_database_service_io_part_chromeos.cc:165:
void FinishWithState(const State& state) {
suggestion: Rename to SetFinalState (I'd expect a method named FinishWithState
to do more work, e.g. to call io_part_->DidCreateNSSCertDatabase)

[pneubeck (no reviews), 2014-10-29 15:15:53]

Patchset #18 (id:1460001) has been deleted

[pneubeck (no reviews), 2014-10-29 15:19:35]

pneubeck@chromium.org changed reviewers:
- rsleevi@chromium.org

[pneubeck (no reviews), 2014-10-29 15:54:52]

pneubeck@chromium.org changed reviewers:
+ blundell@chromium.org, mmenke@chromium.org, nkostylev@chromium.org,
rsleevi@chromium.org, stevenjb@chromium.org, thestig@chromium.org
- davidben@chromium.org

[pneubeck (no reviews), 2014-10-29 15:54:53]

Please owner review as listed below.
Thanks!

rsleevi@chromium.org:
  chrome/browser/ssl/

stevenjb@chromium.org:
  chromeos/

nkostylev@chromium.org:
  chrome/browser/chromeos/{login,net,settings}
  chrome/browser/ui/webui/net_internals/net_internals_ui.cc

mmenke@chromium.org:
  chrome/browser/profiles/

blundell@chromium.org:
  components/
  Not for the actual implementation but for the added dependencies and code
layout.

thestig@chromium.org:
  chrome/browser/DEPS
  chrome/browser/certificate_manager_model*
  chrome/browser/ui/crypto_module_delegate_nss.cc
  The latter do not have specific owners, Matt and Toni are already the best
reviewers, AFAIK.

[blundell, 2014-10-29 16:06:29]

This CL contains more code than I would be comfortable reviewing, but I'll leave
that up to the reviewers handling the content of the CL.

As far as the component layout, you should collapse all of the code into
//components/cert_database:
(1) core/ is used only in layered components, which this is not.
(2) The platform-specific subdirectories are not necessary, as the "_linux" and
"_chromeos" file suffixes convey the same information.
(3) public/ is useful only if you want to split the set of headers into "headers
that embedders can include" and "headers that embedders can't include". Here,
all your headers are in public/.

[stevenjb, 2014-10-29 18:43:51]

I didn't look too closely at the cert_database code itself, but the rest looks
pretty good with some nits. I'll try to look at the cert_database code tomorrow,
but if other more qualified folks review that part, the rest lgtm.

https://codereview.chromium.org/419013003/diff/1480001/chrome/browser/certifi...
File chrome/browser/certificate_manager_model.h (right):

https://codereview.chromium.org/419013003/diff/1480001/chrome/browser/certifi...
chrome/browser/certificate_manager_model.h:20: }
// namespace cert_database (for consistency with content below)

https://codereview.chromium.org/419013003/diff/1480001/chrome/browser/chromeo...
File chrome/browser/chromeos/platform_keys/platform_keys_nss.cc (right):

https://codereview.chromium.org/419013003/diff/1480001/chrome/browser/chromeo...
chrome/browser/chromeos/platform_keys/platform_keys_nss.cc:116:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
nit: DCHECK(state) since we pass state to other methods.

https://codereview.chromium.org/419013003/diff/1480001/chromeos/network/onc/o...
File chromeos/network/onc/onc_certificate_importer_impl.cc (right):

https://codereview.chromium.org/419013003/diff/1480001/chromeos/network/onc/o...
chromeos/network/onc/onc_certificate_importer_impl.cc:45: void
GetNSSCertDatabase(
This name is a bit confusing, maybe something like 'CallWithCertDatabase'?

https://codereview.chromium.org/419013003/diff/1480001/components/cert_databa...
File components/cert_database/core/cert_database_service.cc (right):

https://codereview.chromium.org/419013003/diff/1480001/components/cert_databa...
components/cert_database/core/cert_database_service.cc:27: void
GetNSSCertDBOnIOThread(
nit: Rename?

https://codereview.chromium.org/419013003/diff/1480001/components/cert_databa...
components/cert_database/core/cert_database_service.cc:57: const
GetCertDBCallback& callback) {
nit: Same comment as before, I find this name confusing, something like
CallWithCertDatabase() might be more clear.

[pneubeck (no reviews), 2014-10-29 21:13:06]

On 2014/10/29 18:43:51, stevenjb wrote:
> I didn't look too closely at the cert_database code itself, but the rest looks
> pretty good with some nits. I'll try to look at the cert_database code
tomorrow,
> but if other more qualified folks review that part, the rest lgtm.
Thanks for reviewing! I'd rely on Matt and Toni who looked at cert_database in
detail already.

[mmenke, 2014-10-29 21:47:44]

On 2014/10/29 21:13:06, pneubeck wrote:
> On 2014/10/29 18:43:51, stevenjb wrote:
> > I didn't look too closely at the cert_database code itself, but the rest
looks
> > pretty good with some nits. I'll try to look at the cert_database code
> tomorrow,
> > but if other more qualified folks review that part, the rest lgtm.
> Thanks for reviewing! I'd rely on Matt and Toni who looked at cert_database in
> detail already.

I'm going to hold off on signing off on this CL until mattm or ryan have done so
- I don't know the modified profile IO data at all, so will just defer to them.

[Lei Zhang, 2014-10-29 21:51:40]

I defer my portion to rsleevi. Happy to rubber stamp when needed.

https://codereview.chromium.org/419013003/diff/1480001/chrome/browser/certifi...
File chrome/browser/certificate_manager_model.h (right):

https://codereview.chromium.org/419013003/diff/1480001/chrome/browser/certifi...
chrome/browser/certificate_manager_model.h:20: }
On 2014/10/29 18:43:51, stevenjb wrote:
> // namespace cert_database (for consistency with content below)

IMO there's no need when then the namespace block is so small. I'd chang e
content to match this.

[mattm, 2014-10-30 03:37:48]

generally lgtm

https://codereview.chromium.org/419013003/diff/1330001/components/cert_databa...
File components/cert_database/chromeos/cert_database_service_io_part_chromeos.cc
(right):

https://codereview.chromium.org/419013003/diff/1330001/components/cert_databa...
components/cert_database/chromeos/cert_database_service_io_part_chromeos.cc:112:
void RunNextStep(const State& next_state) {
On 2014/10/21 09:22:09, pneubeck wrote:
> On 2014/10/18 00:45:16, mattm wrote:
> > hm, not entirely sure about the state machine, especially the way the system
> > token ready is handled sort of weirdly by the state machine.
> 
> What specific do you mean?
> The fact that it's waiting for a call from the outside? That's like a
> notification, which should be fine.
> 
> The TPMTokenLoader will still have to be updated (independent of / after this
> change), as it was meant at some time for the user's token and now is
> responsible for the device token.
> Alternatively, if nss_util already takes care of global state of TPM
> initialization, then we could probably get rid of TPMTokenLoader altogether?
> But I'd prefer postponing that to a follow-up CL.

I dunno, just feels a little weird to me. But I don't do a lot of state machines
like this, so maybe just that. Feel free to ignore if no one else complains.

https://codereview.chromium.org/419013003/diff/1480001/components/cert_databa...
File components/cert_database/core/cert_database_service.cc (right):

https://codereview.chromium.org/419013003/diff/1480001/components/cert_databa...
components/cert_database/core/cert_database_service.cc:57: const
GetCertDBCallback& callback) {
On 2014/10/29 18:43:51, stevenjb wrote:
> nit: Same comment as before, I find this name confusing, something like
> CallWithCertDatabase() might be more clear.

May just be me, but "Foo" posting to "FooOnSomeThread" seems like a pretty
common pattern, doesn't seem confusing to me.

https://codereview.chromium.org/419013003/diff/1480001/components/cert_databa...
File
components/cert_database/public/chromeos/cert_database_service_io_part_chromeos.h
(right):

https://codereview.chromium.org/419013003/diff/1480001/components/cert_databa...
components/cert_database/public/chromeos/cert_database_service_io_part_chromeos.h:42:
// be called after Init() on the IO thread.
confusing wording, maybe
"the callback must be called after Init(), and must be called on the IO thread."

[pneubeck (no reviews), 2014-10-30 08:26:58]

pneubeck@chromium.org changed reviewers:
- nkostylev@chromium.org

[pneubeck (no reviews), 2014-10-30 08:32:31]

pneubeck@chromium.org changed reviewers:
+ joaodasilva@chromium.org

[pneubeck (no reviews), 2014-10-30 08:32:32]

@Joao, ptal at chrome/browser/chromeos, in particular *policy* . Thanks.

[Joao da Silva, 2014-10-30 09:48:02]

https://codereview.chromium.org/419013003/diff/1480001/chrome/browser/DEPS
File chrome/browser/DEPS (right):

https://codereview.chromium.org/419013003/diff/1480001/chrome/browser/DEPS#ne...
chrome/browser/DEPS:20: "+components/cert_database/public",
nit: order

https://codereview.chromium.org/419013003/diff/1480001/chrome/browser/certifi...
File chrome/browser/certificate_manager_model.cc (right):

https://codereview.chromium.org/419013003/diff/1480001/chrome/browser/certifi...
chrome/browser/certificate_manager_model.cc:67: cert_db_service->GetIOPart(),
cert_database_service_factory_chromeos.cc returns NULL for GetForBrowserContext
sometimes.

https://codereview.chromium.org/419013003/diff/1480001/chrome/browser/chromeo...
File chrome/browser/chromeos/net/client_cert_filter_chromeos.h (right):

https://codereview.chromium.org/419013003/diff/1480001/chrome/browser/chromeo...
chrome/browser/chromeos/net/client_cert_filter_chromeos.h:38:
ClientCertFilterChromeOS(const base::WeakPtr<
explicit

https://codereview.chromium.org/419013003/diff/1480001/chrome/browser/chromeo...
File
chrome/browser/chromeos/policy/user_network_configuration_updater_factory.cc
(right):

https://codereview.chromium.org/419013003/diff/1480001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/user_network_configuration_updater_factory.cc:95:
cert_service->GetIOPart()));
cert_service may be NULL

https://codereview.chromium.org/419013003/diff/1480001/chrome/browser/extensi...
File
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_apitest_nss.cc
(right):

https://codereview.chromium.org/419013003/diff/1480001/chrome/browser/extensi...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_apitest_nss.cc:236:
->GetNSSCertDatabase(
The service might be NULL, but I think this works for the test

https://codereview.chromium.org/419013003/diff/1480001/chrome/browser/net/cer...
File chrome/browser/net/cert_database_service_factory.cc (right):

https://codereview.chromium.org/419013003/diff/1480001/chrome/browser/net/cer...
chrome/browser/net/cert_database_service_factory.cc:37: return
chrome::GetBrowserContextRedirectedInIncognito(context);
Is this a good idea? Should the CertDatabaseService be shared among the normal
and the OTR profile?

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

https://codereview.chromium.org/419013003/diff/1480001/chrome/browser/ssl/ssl...
chrome/browser/ssl/ssl_browser_tests.cc:748: ->GetNSSCertDatabase(
May be NULL?

https://codereview.chromium.org/419013003/diff/1480001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/net_internals/net_internals_ui.cc (right):

https://codereview.chromium.org/419013003/diff/1480001/chrome/browser/ui/webu...
chrome/browser/ui/webui/net_internals/net_internals_ui.cc:1447:
cert_service->GetIOPart());
May be NULL

https://codereview.chromium.org/419013003/diff/1480001/components/cert_databa...
File components/cert_database.gypi (right):

https://codereview.chromium.org/419013003/diff/1480001/components/cert_databa...
components/cert_database.gypi:13: '../crypto/crypto.gyp:crypto',
nit: order

https://codereview.chromium.org/419013003/diff/1480001/components/cert_databa...
components/cert_database.gypi:20:
'cert_database/linux/cert_database_service_io_part_linux.cc',
nit: order

https://codereview.chromium.org/419013003/diff/1480001/components/cert_databa...
File components/cert_database/DEPS (right):

https://codereview.chromium.org/419013003/diff/1480001/components/cert_databa...
components/cert_database/DEPS:6: "+crypto",
nit: order

https://codereview.chromium.org/419013003/diff/1480001/components/cert_databa...
File components/cert_database/chromeos/DEPS (right):

https://codereview.chromium.org/419013003/diff/1480001/components/cert_databa...
components/cert_database/chromeos/DEPS:4: 
nit: remove empty newline

https://codereview.chromium.org/419013003/diff/1480001/components/cert_databa...
File components/cert_database/chromeos/cert_database_service_io_part_chromeos.cc
(right):

https://codereview.chromium.org/419013003/diff/1480001/components/cert_databa...
components/cert_database/chromeos/cert_database_service_io_part_chromeos.cc:63:
explicit Internal(
No need for explicit

https://codereview.chromium.org/419013003/diff/1480001/components/cert_databa...
components/cert_database/chromeos/cert_database_service_io_part_chromeos.cc:88:
thread_checker_.CalledOnValidThread();
DCHECK

https://codereview.chromium.org/419013003/diff/1480001/components/cert_databa...
File components/cert_database/core/cert_database_service_io_part.cc (right):

https://codereview.chromium.org/419013003/diff/1480001/components/cert_databa...
components/cert_database/core/cert_database_service_io_part.cc:23:
thread_checker_.CalledOnValidThread();
DCHECK?

https://codereview.chromium.org/419013003/diff/1480001/components/cert_databa...
File components/cert_database/public/cert_database_service.h (right):

https://codereview.chromium.org/419013003/diff/1480001/components/cert_databa...
components/cert_database/public/cert_database_service.h:8: #include <string>
used?

https://codereview.chromium.org/419013003/diff/1480001/components/cert_databa...
components/cert_database/public/cert_database_service.h:28: const
scoped_refptr<base::SequencedTaskRunner>& io_task_runner);
explicit

https://codereview.chromium.org/419013003/diff/1480001/components/cert_databa...
components/cert_database/public/cert_database_service.h:40:
base::WeakPtr<CertDatabaseServiceIOPart> GetIOPart();
Please document on which thread this is meant to be called; IO only? Or should
be called from UI but the pointer is valid only on IO?

https://codereview.chromium.org/419013003/diff/1480001/components/cert_databa...
File components/cert_database/public/cert_database_service_io_part.h (right):

https://codereview.chromium.org/419013003/diff/1480001/components/cert_databa...
components/cert_database/public/cert_database_service_io_part.h:46:
base::ThreadChecker thread_checker_;
Inherit from NonThreadSafe instead

https://codereview.chromium.org/419013003/diff/1480001/components/cert_databa...
File components/cert_database/public/linux/cert_database_service_io_part_linux.h
(right):

https://codereview.chromium.org/419013003/diff/1480001/components/cert_databa...
components/cert_database/public/linux/cert_database_service_io_part_linux.h:8:
#include <string>
used?

https://codereview.chromium.org/419013003/diff/1480001/components/cert_databa...
components/cert_database/public/linux/cert_database_service_io_part_linux.h:10:
#include "base/files/file_path.h"
used?

https://codereview.chromium.org/419013003/diff/1480001/components/cert_databa...
components/cert_database/public/linux/cert_database_service_io_part_linux.h:12:
#include "base/memory/ref_counted.h"
used?

https://codereview.chromium.org/419013003/diff/1480001/components/cert_databa...
components/cert_database/public/linux/cert_database_service_io_part_linux.h:13:
#include "base/sequenced_task_runner.h"
used?

[mmenke, 2014-11-04 20:02:19]

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

https://codereview.chromium.org/419013003/diff/1500001/chrome/browser/profile...
chrome/browser/profiles/profile_io_data.cc:831: // If |cert_db_io_| is not
available, we're shutting down already. Return the
nit:  Don't use "we" in comments.

https://codereview.chromium.org/419013003/diff/1500001/components/cert_databa...
File components/cert_database/core/cert_database_service.cc (right):

https://codereview.chromium.org/419013003/diff/1500001/components/cert_databa...
components/cert_database/core/cert_database_service.cc:71:
ignore_result(io_part.release());
Who owns the io_part_ after this?  It's not at all clear.  If you're releasing
ownership, something else should be taking it.

https://codereview.chromium.org/419013003/diff/1500001/components/cert_databa...
File components/cert_database/public/cert_database_service_io_part.h (right):

https://codereview.chromium.org/419013003/diff/1500001/components/cert_databa...
components/cert_database/public/cert_database_service_io_part.h:37:
base::WeakPtr<CertDatabaseServiceIOPart> GetWeakPtr();
It's generally considered a bad idea to publicly vend weak ptr - that's why
WeakPtrFactory is preferred over SupportsWeakPtr.  Is this all needed to allow
the CertDatabaseServiceIOPart to be destroyed before the ProfileIOData or
something?

[pneubeck (no reviews), 2014-11-05 13:18:44]

Patchset #20 (id:1520001) has been deleted

[pneubeck (no reviews), 2014-11-05 14:53:38]

https://codereview.chromium.org/419013003/diff/1480001/chrome/browser/DEPS
File chrome/browser/DEPS (right):

https://codereview.chromium.org/419013003/diff/1480001/chrome/browser/DEPS#ne...
chrome/browser/DEPS:20: "+components/cert_database/public",
On 2014/10/30 09:48:00, Joao da Silva wrote:
> nit: order

Done.

https://codereview.chromium.org/419013003/diff/1480001/chrome/browser/certifi...
File chrome/browser/certificate_manager_model.cc (right):

https://codereview.chromium.org/419013003/diff/1480001/chrome/browser/certifi...
chrome/browser/certificate_manager_model.cc:67: cert_db_service->GetIOPart(),
On 2014/10/30 09:48:00, Joao da Silva wrote:
> cert_database_service_factory_chromeos.cc returns NULL for
GetForBrowserContext
> sometimes.

Done.

https://codereview.chromium.org/419013003/diff/1480001/chrome/browser/certifi...
File chrome/browser/certificate_manager_model.h (right):

https://codereview.chromium.org/419013003/diff/1480001/chrome/browser/certifi...
chrome/browser/certificate_manager_model.h:20: }
On 2014/10/29 21:51:40, Lei Zhang wrote:
> On 2014/10/29 18:43:51, stevenjb wrote:
> > // namespace cert_database (for consistency with content below)
> 
> IMO there's no need when then the namespace block is so small. I'd chang e
> content to match this.

I think it's more common to drop the namespace comment for short forward
declarations.

Done.

https://codereview.chromium.org/419013003/diff/1480001/chrome/browser/chromeo...
File chrome/browser/chromeos/net/client_cert_filter_chromeos.h (right):

https://codereview.chromium.org/419013003/diff/1480001/chrome/browser/chromeo...
chrome/browser/chromeos/net/client_cert_filter_chromeos.h:38:
ClientCertFilterChromeOS(const base::WeakPtr<
On 2014/10/30 09:48:00, Joao da Silva wrote:
> explicit

Done.

https://codereview.chromium.org/419013003/diff/1480001/chrome/browser/chromeo...
File chrome/browser/chromeos/platform_keys/platform_keys_nss.cc (right):

https://codereview.chromium.org/419013003/diff/1480001/chrome/browser/chromeo...
chrome/browser/chromeos/platform_keys/platform_keys_nss.cc:116:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
On 2014/10/29 18:43:51, stevenjb wrote:
> nit: DCHECK(state) since we pass state to other methods.

Done.

https://codereview.chromium.org/419013003/diff/1480001/chrome/browser/chromeo...
File
chrome/browser/chromeos/policy/user_network_configuration_updater_factory.cc
(right):

https://codereview.chromium.org/419013003/diff/1480001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/user_network_configuration_updater_factory.cc:95:
cert_service->GetIOPart()));
On 2014/10/30 09:48:01, Joao da Silva wrote:
> cert_service may be NULL

Done.

https://codereview.chromium.org/419013003/diff/1480001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/user_network_configuration_updater_factory.cc:95:
cert_service->GetIOPart()));
On 2014/10/30 09:48:01, Joao da Silva wrote:
> cert_service may be NULL

Done.

https://codereview.chromium.org/419013003/diff/1480001/chrome/browser/extensi...
File
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_apitest_nss.cc
(right):

https://codereview.chromium.org/419013003/diff/1480001/chrome/browser/extensi...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_apitest_nss.cc:236:
->GetNSSCertDatabase(
On 2014/10/30 09:48:01, Joao da Silva wrote:
> The service might be NULL, but I think this works for the test

Done.

https://codereview.chromium.org/419013003/diff/1480001/chrome/browser/net/cer...
File chrome/browser/net/cert_database_service_factory.cc (right):

https://codereview.chromium.org/419013003/diff/1480001/chrome/browser/net/cer...
chrome/browser/net/cert_database_service_factory.cc:37: return
chrome::GetBrowserContextRedirectedInIncognito(context);
On 2014/10/30 09:48:01, Joao da Silva wrote:
> Is this a good idea? Should the CertDatabaseService be shared among the normal
> and the OTR profile?

My understanding is that although we created a separate instance of
NSSCertDatabase for OTR profile, it effectively shared all data with the normal
profile namely the public and the private slot.
I also didn't find any special handling/initialization of that NSSCertDatabase
instance.

@Ryan and Matt, do we need any special handling for OTR?
If my description above is correct and we still want to change it then we should
do that in a follow up.

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

https://codereview.chromium.org/419013003/diff/1480001/chrome/browser/ssl/ssl...
chrome/browser/ssl/ssl_browser_tests.cc:748: ->GetNSSCertDatabase(
On 2014/10/30 09:48:01, Joao da Silva wrote:
> May be NULL?

Done.

https://codereview.chromium.org/419013003/diff/1480001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/net_internals/net_internals_ui.cc (right):

https://codereview.chromium.org/419013003/diff/1480001/chrome/browser/ui/webu...
chrome/browser/ui/webui/net_internals/net_internals_ui.cc:1447:
cert_service->GetIOPart());
On 2014/10/30 09:48:01, Joao da Silva wrote:
> May be NULL

Done.

https://codereview.chromium.org/419013003/diff/1480001/chromeos/network/onc/o...
File chromeos/network/onc/onc_certificate_importer_impl.cc (right):

https://codereview.chromium.org/419013003/diff/1480001/chromeos/network/onc/o...
chromeos/network/onc/onc_certificate_importer_impl.cc:45: void
GetNSSCertDatabase(
On 2014/10/29 18:43:51, stevenjb wrote:
> This name is a bit confusing, maybe something like 'CallWithCertDatabase'?

Added a comment to clarify. I think the naming scheme is rather common for
asynchronous results.
Done.

https://codereview.chromium.org/419013003/diff/1480001/components/cert_databa...
File components/cert_database.gypi (right):

https://codereview.chromium.org/419013003/diff/1480001/components/cert_databa...
components/cert_database.gypi:13: '../crypto/crypto.gyp:crypto',
On 2014/10/30 09:48:01, Joao da Silva wrote:
> nit: order

Done.

https://codereview.chromium.org/419013003/diff/1480001/components/cert_databa...
components/cert_database.gypi:20:
'cert_database/linux/cert_database_service_io_part_linux.cc',
On 2014/10/30 09:48:01, Joao da Silva wrote:
> nit: order

Done.

https://codereview.chromium.org/419013003/diff/1480001/components/cert_databa...
File components/cert_database/DEPS (right):

https://codereview.chromium.org/419013003/diff/1480001/components/cert_databa...
components/cert_database/DEPS:6: "+crypto",
On 2014/10/30 09:48:01, Joao da Silva wrote:
> nit: order

Done.

https://codereview.chromium.org/419013003/diff/1480001/components/cert_databa...
File components/cert_database/chromeos/DEPS (right):

https://codereview.chromium.org/419013003/diff/1480001/components/cert_databa...
components/cert_database/chromeos/DEPS:4: 
On 2014/10/30 09:48:01, Joao da Silva wrote:
> nit: remove empty newline

Done.

https://codereview.chromium.org/419013003/diff/1480001/components/cert_databa...
File components/cert_database/chromeos/cert_database_service_io_part_chromeos.cc
(right):

https://codereview.chromium.org/419013003/diff/1480001/components/cert_databa...
components/cert_database/chromeos/cert_database_service_io_part_chromeos.cc:63:
explicit Internal(
On 2014/10/30 09:48:01, Joao da Silva wrote:
> No need for explicit

Done.

https://codereview.chromium.org/419013003/diff/1480001/components/cert_databa...
components/cert_database/chromeos/cert_database_service_io_part_chromeos.cc:88:
thread_checker_.CalledOnValidThread();
On 2014/10/30 09:48:01, Joao da Silva wrote:
> DCHECK

why? This check can never fail because of the Detach in the line before.

https://codereview.chromium.org/419013003/diff/1480001/components/cert_databa...
File components/cert_database/core/cert_database_service.cc (right):

https://codereview.chromium.org/419013003/diff/1480001/components/cert_databa...
components/cert_database/core/cert_database_service.cc:27: void
GetNSSCertDBOnIOThread(
On 2014/10/29 18:43:51, stevenjb wrote:
> nit: Rename?

Done.

https://codereview.chromium.org/419013003/diff/1480001/components/cert_databa...
components/cert_database/core/cert_database_service.cc:57: const
GetCertDBCallback& callback) {
On 2014/10/29 18:43:51, stevenjb wrote:
> nit: Same comment as before, I find this name confusing, something like
> CallWithCertDatabase() might be more clear.

I had the impression that it is a common pattern for asynchronous getter
methods.
https://code.google.com/p/chromium/codesearch#search/&q=get.*%5C(.*callback%2...

https://codereview.chromium.org/419013003/diff/1480001/components/cert_databa...
File components/cert_database/core/cert_database_service_io_part.cc (right):

https://codereview.chromium.org/419013003/diff/1480001/components/cert_databa...
components/cert_database/core/cert_database_service_io_part.cc:23:
thread_checker_.CalledOnValidThread();
On 2014/10/30 09:48:01, Joao da Silva wrote:
> DCHECK?

this call always succeeds because of the detach in the line before.

https://codereview.chromium.org/419013003/diff/1480001/components/cert_databa...
File components/cert_database/public/cert_database_service.h (right):

https://codereview.chromium.org/419013003/diff/1480001/components/cert_databa...
components/cert_database/public/cert_database_service.h:8: #include <string>
On 2014/10/30 09:48:01, Joao da Silva wrote:
> used?

Done.

https://codereview.chromium.org/419013003/diff/1480001/components/cert_databa...
components/cert_database/public/cert_database_service.h:28: const
scoped_refptr<base::SequencedTaskRunner>& io_task_runner);
On 2014/10/30 09:48:01, Joao da Silva wrote:
> explicit

Done.

https://codereview.chromium.org/419013003/diff/1480001/components/cert_databa...
components/cert_database/public/cert_database_service.h:40:
base::WeakPtr<CertDatabaseServiceIOPart> GetIOPart();
On 2014/10/30 09:48:01, Joao da Silva wrote:
> Please document on which thread this is meant to be called; IO only? Or should
> be called from UI but the pointer is valid only on IO?

Done.

https://codereview.chromium.org/419013003/diff/1480001/components/cert_databa...
File components/cert_database/public/cert_database_service_io_part.h (right):

https://codereview.chromium.org/419013003/diff/1480001/components/cert_databa...
components/cert_database/public/cert_database_service_io_part.h:46:
base::ThreadChecker thread_checker_;
On 2014/10/30 09:48:01, Joao da Silva wrote:
> Inherit from NonThreadSafe instead

made the member private, in which case the member is the preferred way,
according to ThreadChecker's documentation.

https://codereview.chromium.org/419013003/diff/1480001/components/cert_databa...
File
components/cert_database/public/chromeos/cert_database_service_io_part_chromeos.h
(right):

https://codereview.chromium.org/419013003/diff/1480001/components/cert_databa...
components/cert_database/public/chromeos/cert_database_service_io_part_chromeos.h:42:
// be called after Init() on the IO thread.
On 2014/10/30 03:37:48, mattm wrote:
> confusing wording, maybe
> "the callback must be called after Init(), and must be called on the IO
thread."

Done.

https://codereview.chromium.org/419013003/diff/1480001/components/cert_databa...
File components/cert_database/public/linux/cert_database_service_io_part_linux.h
(right):

https://codereview.chromium.org/419013003/diff/1480001/components/cert_databa...
components/cert_database/public/linux/cert_database_service_io_part_linux.h:8:
#include <string>
On 2014/10/30 09:48:01, Joao da Silva wrote:
> used?

Done.

https://codereview.chromium.org/419013003/diff/1480001/components/cert_databa...
components/cert_database/public/linux/cert_database_service_io_part_linux.h:10:
#include "base/files/file_path.h"
On 2014/10/30 09:48:01, Joao da Silva wrote:
> used?

Done.

https://codereview.chromium.org/419013003/diff/1480001/components/cert_databa...
components/cert_database/public/linux/cert_database_service_io_part_linux.h:12:
#include "base/memory/ref_counted.h"
On 2014/10/30 09:48:01, Joao da Silva wrote:
> used?

Done.

https://codereview.chromium.org/419013003/diff/1480001/components/cert_databa...
components/cert_database/public/linux/cert_database_service_io_part_linux.h:13:
#include "base/sequenced_task_runner.h"
On 2014/10/30 09:48:02, Joao da Silva wrote:
> used?

Done.

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

https://codereview.chromium.org/419013003/diff/1500001/chrome/browser/profile...
chrome/browser/profiles/profile_io_data.cc:831: // If |cert_db_io_| is not
available, we're shutting down already. Return the
On 2014/11/04 20:02:18, mmenke wrote:
> nit:  Don't use "we" in comments.

Done.

https://codereview.chromium.org/419013003/diff/1500001/components/cert_databa...
File components/cert_database/core/cert_database_service.cc (right):

https://codereview.chromium.org/419013003/diff/1500001/components/cert_databa...
components/cert_database/core/cert_database_service.cc:71:
ignore_result(io_part.release());
On 2014/11/04 20:02:18, mmenke wrote:
> Who owns the io_part_ after this?  It's not at all clear.  If you're releasing
> ownership, something else should be taking it.

Added a comment to clarify (there was one in the header already).
Is there a preferred way of declaring/handling the ownership of the io part of a
KeyedService?
Adding it to profile_io_data is contradicting the idea of KeyedServices, namely
to not initialize/own everything explicitly in profile.

https://codereview.chromium.org/419013003/diff/1500001/components/cert_databa...
File components/cert_database/public/cert_database_service_io_part.h (right):

https://codereview.chromium.org/419013003/diff/1500001/components/cert_databa...
components/cert_database/public/cert_database_service_io_part.h:37:
base::WeakPtr<CertDatabaseServiceIOPart> GetWeakPtr();
On 2014/11/04 20:02:18, mmenke wrote:
> It's generally considered a bad idea to publicly vend weak ptr - that's why
> WeakPtrFactory is preferred over SupportsWeakPtr.  Is this all needed to allow
> the CertDatabaseServiceIOPart to be destroyed before the ProfileIOData or
> something?

Do you know a concrete threat by using WeakPtr in this case?

In the current form, the Service will post in its destructor a task to delete
the IOPart.
Thus, there is no obvious relation to the lifetime of ProfileIOData.

Users of the Service obtain a ptr to the IOPart. They switch (potentially
several loops later) to the IO thread, and can use the IOPart.
If the IOPart is not leaky in the IO thread, the obvious thing seems to be to
use a WeakPtr.

The only sane way (that is without making many global assumptions) that I see to
avoid the WeakPtr is inversion of control. Pass a
Callback<void(CertDatabaseServiceIOPart*)> to Service::GetIOPart.
That however makes it impossible to pass multiple ptrs (say IO parts of
different services) to the IO thread.

I'm sure that I miss some alternatives and would be happy implement it in a
better way.

[Joao da Silva, 2014-11-05 15:07:51]

policy/ lgtm

https://codereview.chromium.org/419013003/diff/1480001/components/cert_databa...
File components/cert_database/chromeos/cert_database_service_io_part_chromeos.cc
(right):

https://codereview.chromium.org/419013003/diff/1480001/components/cert_databa...
components/cert_database/chromeos/cert_database_service_io_part_chromeos.cc:88:
thread_checker_.CalledOnValidThread();
On 2014/11/05 14:53:37, pneubeck wrote:
> On 2014/10/30 09:48:01, Joao da Silva wrote:
> > DCHECK
> 
> why? This check can never fail because of the Detach in the line before.

Ah, the call to CalledOnValidThread() is just to attach to the current thread...
that wasn't very clear.

https://codereview.chromium.org/419013003/diff/1540001/chrome/browser/certifi...
File chrome/browser/certificate_manager_model.cc (right):

https://codereview.chromium.org/419013003/diff/1540001/chrome/browser/certifi...
chrome/browser/certificate_manager_model.cc:268: get_db_callback.Run(cert_db);
else?

Should this call get_db_callback.Run(nullptr) or not?

[mmenke, 2014-11-05 16:19:22]

https://codereview.chromium.org/419013003/diff/1500001/components/cert_databa...
File components/cert_database/public/cert_database_service_io_part.h (right):

https://codereview.chromium.org/419013003/diff/1500001/components/cert_databa...
components/cert_database/public/cert_database_service_io_part.h:37:
base::WeakPtr<CertDatabaseServiceIOPart> GetWeakPtr();
On 2014/11/05 14:53:38, pneubeck wrote:
> On 2014/11/04 20:02:18, mmenke wrote:
> > It's generally considered a bad idea to publicly vend weak ptr - that's why
> > WeakPtrFactory is preferred over SupportsWeakPtr.  Is this all needed to
allow
> > the CertDatabaseServiceIOPart to be destroyed before the ProfileIOData or
> > something?
> 
> Do you know a concrete threat by using WeakPtr in this case?
> 
> In the current form, the Service will post in its destructor a task to delete
> the IOPart.
> Thus, there is no obvious relation to the lifetime of ProfileIOData.
> 
> Users of the Service obtain a ptr to the IOPart. They switch (potentially
> several loops later) to the IO thread, and can use the IOPart.
> If the IOPart is not leaky in the IO thread, the obvious thing seems to be to
> use a WeakPtr.
> 
> The only sane way (that is without making many global assumptions) that I see
to
> avoid the WeakPtr is inversion of control. Pass a
> Callback<void(CertDatabaseServiceIOPart*)> to Service::GetIOPart.
> That however makes it impossible to pass multiple ptrs (say IO parts of
> different services) to the IO thread.
> 
> I'm sure that I miss some alternatives and would be happy implement it in a
> better way.

That's a problem - destruction order is not known or enforced, and the ownership
graph is unclear, as it what does and does not have to worry about the IO part
of the service being destroyed out from under it.  Here's Will Chan's rant on
the topic: 
https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/Oy3uuiTSwDk...

I'd rather see the ProfileIOData own the IO part, and it and the UI part talk to
each other via weak ptrs, if that's needed.  Then only one class has to worry
about the relevant cleanup logic, and what's safe to expose.  In general, should
limit the scope of WeakPtrs.

The nice thing about ProfileKeyedService is a profile doesn't have to know about
the service, but the ProfileIOData already has to know about this class anyways,
so I don't think we really get much from not making the ProfileIOData own it. 
I'm open to other ideas.

[Ryan Sleevi, 2014-11-05 22:09:10]

Sorry for the delays on circling through, but I'm really appreciative for Matt
taking the close look on this.

I've taken a quick scan, and I have to agree with Matt, the WeakPtr usage is
sort of the anti-pattern. I haven't reviewed closely enough to give a
suggestion, but it sounds like there are some ideas here. Passing a WeakPtr /
exposing WeakPtr's is a definite anti-pattern that I can't support for new code.

[mmenke, 2014-11-05 22:13:41]

On 2014/11/05 22:09:10, Ryan Sleevi wrote:
> Sorry for the delays on circling through, but I'm really appreciative for Matt
> taking the close look on this.
> 
> I've taken a quick scan, and I have to agree with Matt, the WeakPtr usage is
> sort of the anti-pattern. I haven't reviewed closely enough to give a
> suggestion, but it sounds like there are some ideas here. Passing a WeakPtr /
> exposing WeakPtr's is a definite anti-pattern that I can't support for new
code.

Disclaimer:  I have not taken a close look at this, I just saw the WeakPtr
usage, and suggested a simple, though not great, workaround, without digging
into whether it's the best way to go.

[mmenke, 2015-02-27 20:41:57]

mmenke@chromium.org changed reviewers:
- mmenke@chromium.org

[Joao da Silva, 2015-03-03 10:55:48]

What's the status of this CL?