chromeos: Load chaps module and lookup TPM slots on the worker pool.

chromeos: Load chaps module and lookup TPM slots on the worker pool.

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

[mattm, 2014-02-26 06:17:18]

notes:
Didn't increment refcount of the chaps module when posting tasks since that uses
SECMODModule->refLock, which could block. And isn't necessary since
NSSInitSingleton is leaky.

The references to the tpm slots are also leaked, so they would not cause
SECMOD_SlotDestroyModule to get called.



I also poked around a bit looking for other issues. These may not be news.


The global NSS moduleLock could still cause some blocking, though it looks like
it generally isn't held very long.

Possibly interesting place that was using moduleLock I found:
SSLClientSocketNSS::Core::~Core can be run on the IO thread, and in the case of
a connection using client certs ssl3_ClientAuthTokenPresent ends up calling
SECMOD_FindModuleByID which takes a readlock on moduleLock. It looks like Core
could easily be changed to destruct on the NSS thread. I could make a CL for
that if you think it's worthwhile.

SSLAddCertHandler does some things on IO thread that take a readlock on
moduleLock, and then does PK11_ImportCert on the IO thread. It also uses
AutoNSSWriteLock which at least according to the bug referenced in the comment
in crypto/nss_util.h shouldn't be necessary any more.


Obvious things:
InitializeNSSForChromeOSUser which calls SECMOD_OpenUserDB from IO thread.

And of course the certificate manager does a bunch of stuff on UI thread that it
shouldn't.
onc_certificate_importer might also be, not clear just from looking at the code.

[Ryan Sleevi, 2014-02-27 04:22:32]

LGTM, mod nit. Thanks for taking this on (and explaining it in person)

https://codereview.chromium.org/181053002/diff/40001/crypto/nss_util.cc
File crypto/nss_util.cc (right):

https://codereview.chromium.org/181053002/diff/40001/crypto/nss_util.cc#newco...
crypto/nss_util.cc:441: static PK11SlotInfo* GetTPMSlotForId(SECMODModule*
chaps_module,
Should you rename this method to clearly indicate it should only be called on
the WorkerThread? (Because of the SECMOD_UpdateSlotList)

[mattm, 2014-02-27 05:32:37]

made one extra cleanup if you want to look at that too

https://codereview.chromium.org/181053002/diff/40001/crypto/nss_util.cc
File crypto/nss_util.cc (right):

https://codereview.chromium.org/181053002/diff/40001/crypto/nss_util.cc#newco...
crypto/nss_util.cc:441: static PK11SlotInfo* GetTPMSlotForId(SECMODModule*
chaps_module,
On 2014/02/27 04:22:32, Ryan Sleevi wrote:
> Should you rename this method to clearly indicate it should only be called on
> the WorkerThread? (Because of the SECMOD_UpdateSlotList)

Done.

https://codereview.chromium.org/181053002/diff/60001/crypto/nss_util.cc
File crypto/nss_util.cc (right):

https://codereview.chromium.org/181053002/diff/60001/crypto/nss_util.cc#newco...
crypto/nss_util.cc:494:
base::Bind(&NSSInitSingleton::InitializeTPMTokenOnWorkerThread,
Also, I realized that the function previously called
GetTPMSlotForIdOnWorkerThread is somewhat redundant,
InitializeTPMTokenOnWorkerThread can be used for both the InitializeTPMToken and
InitializeTPMForChromeOSUser calls. So I changed that too. Does that seem
reasonable?

[Ryan Sleevi, 2014-02-27 05:40:25]

lgtm

[commit-bot: I haz the power, 2014-02-27 05:42:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mattm@chromium.org/181053002/60001

[commit-bot: I haz the power, 2014-02-27 06:05:06]

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

[commit-bot: I haz the power, 2014-02-27 06:05:06]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[mattm, 2014-02-27 06:32:51]

+stevenjb for chromeos/

[stevenjb, 2014-02-27 18:38:12]

chromeos/tpm_token_loader.cc lgtm

[mattm, 2014-02-27 19:04:00]

The CQ bit was checked by mattm@chromium.org

[commit-bot: I haz the power, 2014-02-27 19:06:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mattm@chromium.org/181053002/60001

[commit-bot: I haz the power, 2014-02-27 20:51:01]

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

[commit-bot: I haz the power, 2014-02-27 20:51:02]

Try jobs failed on following builders: linux_chromium_rel

[mattm, 2014-02-27 20:57:05]

The CQ bit was checked by mattm@chromium.org

[commit-bot: I haz the power, 2014-02-27 20:58:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mattm@chromium.org/181053002/60001

[commit-bot: I haz the power, 2014-02-27 22:27:27]

Change committed as 253942