Remove TPMTokenInfoDelegate

Remove TPMTokenInfoDelegate to make TPM initialization code path simple

Move Cryptohome D-Bus method calls to chromeos::CertLibrary

BUG=125848
TEST=can login

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=137646

[stevenjb, 2012-05-16 16:51:19]

cert_library changes lgtm

[Ryan Sleevi, 2012-05-16 17:02:16]

crypto/ LGTM.

A few style nits below, plus one (pre-existing) issue that can be addressed in
follow-up.

http://codereview.chromium.org/10332191/diff/2001/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/cert_library.cc (right):

http://codereview.chromium.org/10332191/diff/2001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/cert_library.cc:159:
crypto::EnableTPMTokenForNSS();
See comments in crypto/

http://codereview.chromium.org/10332191/diff/2001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/cert_library.cc:468: << __FUNCTION__ << " should be
called on UI thread.";
nit: These strings will not get optimized out in a release build, but will not
show up in crashdumps either.

If a user runs into them, they'll either be able to get a backtrace (if using
ChromiumOS) or it'll just crash.

Just wanted to make sure it was intentional to include the extra messaging -
normally the CHECK itself (which includes the file & line, IIRC) is sufficient.

Same elsewhere in this file as well.

http://codereview.chromium.org/10332191/diff/2001/crypto/nss_util.cc
File crypto/nss_util.cc (right):

http://codereview.chromium.org/10332191/diff/2001/crypto/nss_util.cc#newcode256
crypto/nss_util.cc:256: }
nit: Remove the { } for the one-line conditionals (same with 259-261)

http://codereview.chromium.org/10332191/diff/2001/crypto/nss_util.cc#newcode381
crypto/nss_util.cc:381: if (!tpm_token_enabled_for_nss_) {
Note: This is still thread-racey.

EnableTPMTokenForNSS is called on the UI or DBus thread by CertLibrary, but this
is called on the IO thread (and the UI thread)

[hashimoto, 2012-05-17 07:35:07]

http://codereview.chromium.org/10332191/diff/2001/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/cert_library.cc (right):

http://codereview.chromium.org/10332191/diff/2001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/cert_library.cc:468: << __FUNCTION__ << " should be
called on UI thread.";
On 2012/05/16 17:02:16, Ryan Sleevi wrote:
> nit: These strings will not get optimized out in a release build, but will not
> show up in crashdumps either.
> 
> If a user runs into them, they'll either be able to get a backtrace (if using
> ChromiumOS) or it'll just crash.
> 
> Just wanted to make sure it was intentional to include the extra messaging -
> normally the CHECK itself (which includes the file & line, IIRC) is
sufficient.
> 
> Same elsewhere in this file as well.

Agree, CHECK itself is sufficient, removed the strings.

http://codereview.chromium.org/10332191/diff/2001/crypto/nss_util.cc
File crypto/nss_util.cc (right):

http://codereview.chromium.org/10332191/diff/2001/crypto/nss_util.cc#newcode256
crypto/nss_util.cc:256: }
On 2012/05/16 17:02:16, Ryan Sleevi wrote:
> nit: Remove the { } for the one-line conditionals (same with 259-261)

Done.

http://codereview.chromium.org/10332191/diff/2001/crypto/nss_util.cc#newcode381
crypto/nss_util.cc:381: if (!tpm_token_enabled_for_nss_) {
On 2012/05/16 17:02:16, Ryan Sleevi wrote:
> Note: This is still thread-racey.
> 
> EnableTPMTokenForNSS is called on the UI or DBus thread by CertLibrary, but
this
Now EnableTPMTokenForNSS is called only on UI thread.

> is called on the IO thread (and the UI thread)

How would you like to solve this problem?
Adding lock is not feasible because Wait() in UI thread is not allowed after
http://crrev.com/134114.
Are you planning to make this method called only from IO (or UI) thread?

[commit-bot: I haz the power, 2012-05-17 08:38:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hashimoto@chromium.org/10332191/11002

[commit-bot: I haz the power, 2012-05-17 10:04:20]

Change committed as 137646

[Ryan Sleevi, 2012-05-17 17:09:45]

http://codereview.chromium.org/10332191/diff/2001/crypto/nss_util.cc
File crypto/nss_util.cc (right):

http://codereview.chromium.org/10332191/diff/2001/crypto/nss_util.cc#newcode381
crypto/nss_util.cc:381: if (!tpm_token_enabled_for_nss_) {
On 2012/05/17 07:35:07, hashimoto wrote:
> On 2012/05/16 17:02:16, Ryan Sleevi wrote:
> > Note: This is still thread-racey.
> > 
> > EnableTPMTokenForNSS is called on the UI or DBus thread by CertLibrary, but
> this
> Now EnableTPMTokenForNSS is called only on UI thread.
> 
> > is called on the IO thread (and the UI thread)
> 
> How would you like to solve this problem?
> Adding lock is not feasible because Wait() in UI thread is not allowed after
> http://crrev.com/134114.
> Are you planning to make this method called only from IO (or UI) thread?

I would prefer it only be called on the I/O thread, which is currently where all
crypto/NSS functions are delegated. Then, if we need to move to another thread
(as being discussed for CrOS), it's clear what the semantics will be.

You can use PostTaskAndReplyWithResult to post to the IO thread to query if it's
available, then get the result passed as an input to your callback function on
the origin thread (UI thread)