Call crypto::InitializeTPMToken on the IO thread (Take 2)

Call crypto::InitializeTPMToken on the IO thread (Take 2)

This is identical to https://chromiumcodereview.appspot.com/15649018
with the following addition:

Call DeviceSettingsService::EnsureReload once certs are loaded


BUG=244455
R=mnissler@chromium.org, nkostylev@chromium.org, rsleevi@chromium.org, xiyuan@chromium.org

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

[stevenjb, 2013-07-24 20:58:36]

pneubeck@chromium.org, ryan.sleevi@gmail.com: These changes are identical to
https://chromiumcodereview.appspot.com/15649018/ + changes to
device_settings_service.cc

xiyuan@ for certificate_manager_browsertest.cc owner

mnissler@chromium, nkostylev@chromium.org, please take a look. In particular, is
there any code that might behave badly if ownership status changes later in the
initialization process? The code is already asynchronous, so I would expect it
to be robust...

[xiyuan, 2013-07-24 21:06:36]

certificate_manager_browsertest.cc LGTM

[Ryan Sleevi, 2013-07-24 21:30:51]

LGTM from an NSS view - but not sure what service(s) may make any implicit
assumptions about TPM load state.

[Mattias Nissler (ping if slow), 2013-07-25 08:56:24]

LGTM with a little grief (feel free to ignore or file a bug).

Regarding your question about code that might be affected by this, I can't give
you good answer unfortunately. It's basically "everything depending on ownership
may break", and unfortunately I've seen quite a few places in the past that
didn't properly handle asynchronous owner detection. I think about:flags was one
offender (which I believe has been fixed), owner settings in chrome://settings
are another thing to check. And there certainly are more that I'm not aware of
:-/

https://chromiumcodereview.appspot.com/20130002/diff/9001/chrome/browser/chro...
File chrome/browser/chromeos/settings/device_settings_service.cc (right):

https://chromiumcodereview.appspot.com/20130002/diff/9001/chrome/browser/chro...
chrome/browser/chromeos/settings/device_settings_service.cc:79: if
(NetworkHandler::IsInitialized())
I'm a bit uncomfortable about the fact that we now depend on network stuff,
where all we need is the NSSDB.

[pneubeck (no reviews), 2013-07-25 09:08:42]

https://chromiumcodereview.appspot.com/20130002/diff/9001/chrome/browser/chro...
File chrome/browser/chromeos/settings/device_settings_service.cc (right):

https://chromiumcodereview.appspot.com/20130002/diff/9001/chrome/browser/chro...
chrome/browser/chromeos/settings/device_settings_service.cc:79: if
(NetworkHandler::IsInitialized())
On 2013/07/25 08:56:24, Mattias Nissler wrote:
> I'm a bit uncomfortable about the fact that we now depend on network stuff,
> where all we need is the NSSDB.

CertLoader itself doesn't depend on any of the other Network classes.
To cleanup these dependencies, CertLoader could be separated from
NetworkHandler.

https://chromiumcodereview.appspot.com/20130002/diff/9001/chromeos/network/ce...
File chromeos/network/cert_loader.cc (right):

https://chromiumcodereview.appspot.com/20130002/diff/9001/chromeos/network/ce...
chromeos/network/cert_loader.cc:122: // This is the entry point to the TPM token
initialization process,
This comment should be moved to the first if, as certificates_requested_ is the
more reliable test for this claim ("...at most once...").
tpm_token_state_ is updated asynchronously and can't prevent re-requests.

This DCHECK verifies that we start in the right state, which is OK.

[stevenjb, 2013-07-25 17:02:22]

https://chromiumcodereview.appspot.com/20130002/diff/9001/chrome/browser/chro...
File chrome/browser/chromeos/settings/device_settings_service.cc (right):

https://chromiumcodereview.appspot.com/20130002/diff/9001/chrome/browser/chro...
chrome/browser/chromeos/settings/device_settings_service.cc:79: if
(NetworkHandler::IsInitialized())
On 2013/07/25 09:08:43, pneubeck wrote:
> On 2013/07/25 08:56:24, Mattias Nissler wrote:
> > I'm a bit uncomfortable about the fact that we now depend on network stuff,
> > where all we need is the NSSDB.
> 
> CertLoader itself doesn't depend on any of the other Network classes.
> To cleanup these dependencies, CertLoader could be separated from
> NetworkHandler.

OK, I'll make CertLoader its own singleton. I agree it's a bit confusing as it
currently is.

Done.

https://chromiumcodereview.appspot.com/20130002/diff/9001/chromeos/network/ce...
File chromeos/network/cert_loader.cc (right):

https://chromiumcodereview.appspot.com/20130002/diff/9001/chromeos/network/ce...
chromeos/network/cert_loader.cc:122: // This is the entry point to the TPM token
initialization process,
On 2013/07/25 09:08:43, pneubeck wrote:
> This comment should be moved to the first if, as certificates_requested_ is
the
> more reliable test for this claim ("...at most once...").
> tpm_token_state_ is updated asynchronously and can't prevent re-requests.
> 
> This DCHECK verifies that we start in the right state, which is OK.

Done.

[Mattias Nissler (ping if slow), 2013-07-25 18:14:26]

LGTM

[commit-bot: I haz the power, 2013-07-25 19:38:44]

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

[Nikita (slow), 2013-07-25 21:46:05]

On 2013/07/25 08:56:24, Mattias Nissler wrote:
> LGTM with a little grief (feel free to ignore or file a bug).
> 
> Regarding your question about code that might be affected by this, I can't
give
> you good answer unfortunately. It's basically "everything depending on
ownership
> may break", and unfortunately I've seen quite a few places in the past that
> didn't properly handle asynchronous owner detection. I think about:flags was
one
> offender (which I believe has been fixed), owner settings in chrome://settings
> are another thing to check. And there certainly are more that I'm not aware of
> :-/

I think we'll just need to fix those places separately.

lgtm

[commit-bot: I haz the power, 2013-07-26 02:23:09]

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

[commit-bot: I haz the power, 2013-07-26 13:30:34]

Failed to apply patch for chromeos/chromeos.gyp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chromeos/chromeos.gyp
  Hunk #2 FAILED at 211.
  1 out of 2 hunks FAILED -- saving rejects to file chromeos/chromeos.gyp.rej

Patch:       chromeos/chromeos.gyp
Index: chromeos/chromeos.gyp
diff --git a/chromeos/chromeos.gyp b/chromeos/chromeos.gyp
index
807f1d092be66f745c83388390794ee6a29f85e1..262e9766687fb67b90fbf8e92a1b1765ab8b1c04
100644
--- a/chromeos/chromeos.gyp
+++ b/chromeos/chromeos.gyp
@@ -46,6 +46,8 @@
         'attestation/attestation_constants.h',
         'attestation/attestation_flow.cc',
         'attestation/attestation_flow.h',
+        'cert_loader.cc',
+        'cert_loader.h',
         'chromeos_constants.cc',
         'chromeos_constants.h',
         'chromeos_export.h',
@@ -209,8 +211,6 @@
         'ime/xkeyboard.h',
         'login/login_state.cc',
         'login/login_state.h',
-        'network/cert_loader.cc',
-        'network/cert_loader.h',
         'network/certificate_handler.cc',
         'network/certificate_handler.h',
         'network/certificate_pattern.cc',

[stevenjb, 2013-07-26 17:48:58]

Committed patchset #5 manually as r213951 (presubmit successful).