Make nss/TPM integration in ChromeOS more robust.

Make nss/TPM integration in ChromeOS more robust.

Change-Id: I09e942098cb780f110c115f5097f57e8344170cb

BUG=chromium-os:26177
TEST=See issue. Ensure certificates still work with VPN and wifi.


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

[Greg Spencer (Chromium), 2012-02-14 01:44:04]

http://codereview.chromium.org/9389028/diff/2002/crypto/nss_util.cc
File crypto/nss_util.cc (right):

http://codereview.chromium.org/9389028/diff/2002/crypto/nss_util.cc#newcode386
crypto/nss_util.cc:386: if (IsTPMTokenReady()) {
So what if nobody ever calls EnsureTPMTokenReady before this?  Then the
tpm_slot_ won't ever get initialized and IsTPMTokenReady won't return true.  I
know we call EnsureTPMTokenReady right now in the browser startup code, so
that's probably sufficient to ensure that doesn't happen, but I guess I don't
see why this is better.  Is it because this might get called earlier than it
should be?

[stevenjb, 2012-02-14 02:13:37]

On 2012/02/14 01:44:04, Greg Spencer (Chromium) wrote:
> http://codereview.chromium.org/9389028/diff/2002/crypto/nss_util.cc
> File crypto/nss_util.cc (right):
> 
> http://codereview.chromium.org/9389028/diff/2002/crypto/nss_util.cc#newcode386
> crypto/nss_util.cc:386: if (IsTPMTokenReady()) {
> So what if nobody ever calls EnsureTPMTokenReady before this?  Then the
> tpm_slot_ won't ever get initialized and IsTPMTokenReady won't return true.  I
> know we call EnsureTPMTokenReady right now in the browser startup code, so
> that's probably sufficient to ensure that doesn't happen, but I guess I don't
> see why this is better.  Is it because this might get called earlier than it
> should be?

It's because this is currently getting called from a non UI thread (see
stacktrace in issue) which is triggering an assert. If we're not calling
EnsureTPMReady(), we need to call it explicitly somewhere that's guaranteed to
be on the UI thread.

[Greg Spencer (Chromium), 2012-02-14 16:49:08]

LGTM

[stevenjb, 2012-02-14 17:23:46]

After thinking about your comment, I double checked the logic and added an extra
check and a comment.

[commit-bot: I haz the power, 2012-02-14 21:57:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/9389028/9001

[commit-bot: I haz the power, 2012-02-14 21:57:30]

Presubmit check for 9389028-9001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for: crypto/nss_util.cc

[stevenjb, 2012-02-14 22:13:32]

+wtc for owner

[wtc, 2012-02-15 01:41:32]

Patch Set 3 LGTM.

Nit: The CL's commit message "Make nss/TPM integration in
ChromeOS more robust." is vague.  It would be nice to add
a concise summary of your description of the fix in
http://code.google.com/p/chromium-os/issues/detail?id=26177#c9

http://codereview.chromium.org/9389028/diff/9001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/user_manager.cc (right):

http://codereview.chromium.org/9389028/diff/9001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/user_manager.cc:243:
CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));

Should this CHECK be the first line of this function, or
do you intend to skip this CHECK if token_ready_ is true?
If it's intentional, a comment that explains why would be
nice.

[stevenjb, 2012-02-15 01:58:51]

http://codereview.chromium.org/9389028/diff/9001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/user_manager.cc (right):

http://codereview.chromium.org/9389028/diff/9001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/user_manager.cc:243:
CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
On 2012/02/15 01:41:32, wtc wrote:
> 
> Should this CHECK be the first line of this function, or
> do you intend to skip this CHECK if token_ready_ is true?
> If it's intentional, a comment that explains why would be
> nice.

Comment added.

[commit-bot: I haz the power, 2012-02-15 01:59:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/9389028/7003

[commit-bot: I haz the power, 2012-02-15 04:17:02]

Change committed as 122037