Wait for the attestation to be ready (TPM being prepared for attestation) before trying to enroll.

chromeos/attestation/attestation_flow.cc

Index: chromeos/attestation/attestation_flow.cc
diff --git a/chromeos/attestation/attestation_flow.cc b/chromeos/attestation/attestation_flow.cc
index c82cbe554008bf6f81cde6267570743de5e8c3b5..643dfee7e95006bf26067f7d58e80e23c30b6bd5 100644
--- a/chromeos/attestation/attestation_flow.cc
+++ b/chromeos/attestation/attestation_flow.cc
@@ -4,9 +4,13 @@
 
 #include "chromeos/attestation/attestation_flow.h"
 
+#include <algorithm>
 #include <utility>
 
 #include "base/bind.h"
+#include "base/memory/ptr_util.h"
+#include "base/time/tick_clock.h"
+#include "base/timer/timer.h"
 #include "chromeos/cryptohome/async_method_caller.h"
 #include "chromeos/cryptohome/cryptohome_parameters.h"
 #include "chromeos/dbus/cryptohome_client.h"
@@ -17,6 +21,9 @@ namespace attestation {
 
 namespace {
 
+// Delay before checking whether attestation has been prepared again.
+constexpr base::TimeDelta kRetryDelay = base::TimeDelta::FromSeconds(7);
+
 // Redirects to one of three callbacks based on a boolean value and dbus call
 // status.
 //
@@ -29,10 +36,11 @@ namespace {
 void DBusBoolRedirectCallback(const base::Closure& on_true,
                               const base::Closure& on_false,
                               const base::Closure& on_fail,
+                              const std::string& on_fail_message,
                               DBusMethodCallStatus status,
                               bool value) {
   if (status != DBUS_METHOD_CALL_SUCCESS) {
-    LOG(ERROR) << "Attestation: Failed to query enrollment state.";
+    LOG(ERROR) << "Attestation: Failed to " << on_fail_message << ".";
     if (!on_fail.is_null())
       on_fail.Run();
     return;
@@ -59,6 +67,18 @@ void DBusDataMethodCallback(
 
 }  // namespace
 
+struct AttestationFlow::RetryData {
+  RetryData(base::TimeDelta timeout, base::TickClock* tick_clock)
+      : timeout(timeout),
+        tick_clock(tick_clock),
+        retry_timer(tick_clock),
+        begin(tick_clock->NowTicks()) {}
+  base::TimeDelta timeout;
+  base::TickClock* tick_clock;
+  base::OneShotTimer retry_timer;
+  base::TimeTicks begin;
+};
+
 AttestationKeyType AttestationFlow::GetKeyTypeForProfile(
     AttestationCertificateProfile certificate_profile) {
   switch (certificate_profile) {
@@ -91,10 +111,12 @@ std::string AttestationFlow::GetKeyNameForProfile(
 
 AttestationFlow::AttestationFlow(cryptohome::AsyncMethodCaller* async_caller,
                                  CryptohomeClient* cryptohome_client,
-                                 std::unique_ptr<ServerProxy> server_proxy)
+                                 std::unique_ptr<ServerProxy> server_proxy,
+                                 base::TimeDelta preparedness_timeout)
     : async_caller_(async_caller),
       cryptohome_client_(cryptohome_client),
       server_proxy_(std::move(server_proxy)),
+      preparedness_timeout_(preparedness_timeout),
       weak_factory_(this) {}
 
 AttestationFlow::~AttestationFlow() {
@@ -112,15 +134,46 @@ void AttestationFlow::GetCertificate(
       &AttestationFlow::StartCertificateRequest, weak_factory_.GetWeakPtr(),
       certificate_profile, account_id, request_origin, force_new_key, callback);
   base::Closure on_enroll_failure = base::Bind(callback, false, "");
-  base::Closure do_enroll = base::Bind(&AttestationFlow::StartEnroll,
-                                       weak_factory_.GetWeakPtr(),
-                                       on_enroll_failure,
-                                       do_cert_request);
-  cryptohome_client_->TpmAttestationIsEnrolled(base::Bind(
-      &DBusBoolRedirectCallback,
-      do_cert_request,  // If enrolled, proceed with cert request.
-      do_enroll,        // If not enrolled, initiate enrollment.
-      on_enroll_failure));
+  base::Closure initiate_enroll =

[achuithb, 2016/12/05 19:53:59]

const.

on_enroll_failure too

+      base::Bind(&AttestationFlow::InitiateEnroll, weak_factory_.GetWeakPtr(),
+                 on_enroll_failure, do_cert_request);
+  cryptohome_client_->TpmAttestationIsEnrolled(
+      base::Bind(&DBusBoolRedirectCallback,
+                 do_cert_request,  // If enrolled, proceed with cert request.
+                 initiate_enroll,  // If not enrolled, initiate enrollment.
+                 on_enroll_failure, "check enrollment state"));
+}
+
+void AttestationFlow::SetTickClockForTest(base::TickClock* tick_clock) {
+  tick_clock_ = tick_clock;
+}
+
+void AttestationFlow::InitiateEnroll(const base::Closure& on_failure,
+                                     const base::Closure& next_task) {
+  TryInitiateEnroll(new RetryData(preparedness_timeout_, tick_clock_),

[achuithb, 2016/12/05 19:53:59]

YOu should pass ownership with a unique_ptr.

Should retry_data be a data member of this class instead?

+                    on_failure, next_task);
+}
+
+void AttestationFlow::TryInitiateEnroll(RetryData* retry_data,
+                                        const base::Closure& on_failure,
+                                        const base::Closure& next_task) {
+  base::Closure do_enroll = base::Bind(

[achuithb, 2016/12/05 19:53:59]

const here and below

+      &AttestationFlow::DoneRetrying, weak_factory_.GetWeakPtr(), retry_data,
+      base::Bind(&AttestationFlow::StartEnroll, weak_factory_.GetWeakPtr(),
+                 on_failure, next_task));
+  base::Closure retry_initiate_enroll = base::Bind(
+      &AttestationFlow::StillRetrying, weak_factory_.GetWeakPtr(), retry_data,
+      base::Bind(&AttestationFlow::DoneRetrying, weak_factory_.GetWeakPtr(),
+                 retry_data, on_failure),
+      base::Bind(&AttestationFlow::TryInitiateEnroll,
+                 weak_factory_.GetWeakPtr(), retry_data, on_failure,
+                 next_task));
+  base::Closure do_fail =
+      base::Bind(&AttestationFlow::DoneRetrying, weak_factory_.GetWeakPtr(),
+                 retry_data, on_failure);
+  cryptohome_client_->TpmAttestationIsPrepared(
+      base::Bind(&DBusBoolRedirectCallback, do_enroll, retry_initiate_enroll,
+                 do_fail, "check for attestation readiness"));
 }
 
 void AttestationFlow::StartEnroll(const base::Closure& on_failure,
@@ -222,7 +275,8 @@ void AttestationFlow::StartCertificateRequest(
     cryptohome_client_->TpmAttestationDoesKeyExist(
         key_type, cryptohome::Identification(account_id), key_name,
         base::Bind(&DBusBoolRedirectCallback, on_key_exists, on_key_not_exists,
-                   base::Bind(callback, false, "")));
+                   base::Bind(callback, false, ""),
+                   "check for existence of attestation key"));
   }
 }
 
@@ -277,6 +331,31 @@ void AttestationFlow::GetExistingCertificate(
       base::Bind(&DBusDataMethodCallback, callback));
 }
 
+void AttestationFlow::StillRetrying(RetryData* retry_data,
+                                    const base::Closure& on_giving_up,
+                                    const base::Closure& on_retrying) {
+  base::TimeTicks now = retry_data->tick_clock->NowTicks();

[achuithb, 2016/12/05 19:53:59]

const

+  base::TimeDelta elapsed = now - retry_data->begin;

[achuithb, 2016/12/05 19:53:59]

const

+  if (elapsed + kRetryDelay > retry_data->timeout) {
+    LOG(ERROR) << "Attestation: Not prepared."
+               << " Giving up on retrying after " << elapsed << ".";
+    if (!on_giving_up.is_null())
+      on_giving_up.Run();
+  } else {
+    LOG(WARNING) << "Attestation: Not prepared yet."

[achuithb, 2016/12/05 19:53:59]

Should this be a LOG(WARNING)? Maybe VLOG(1)?

+                 << " Retrying in " << kRetryDelay << ".";
+    retry_data->retry_timer.Start(FROM_HERE, kRetryDelay, on_retrying);
+  }
+}
+
+void AttestationFlow::DoneRetrying(RetryData* retry_data,

[achuithb, 2016/12/05 19:53:59]

We don's pass naked pointers around like this with implicit ownership

+                                   const base::Closure& continuation) {
+  retry_data->retry_timer.Stop();
+  delete retry_data;

[apronin1, 2016/12/05 19:25:50]

Not a big fan of deleting using the pointer passed from above. Especially
without the ability to set the variable to NULL in the calling layer. 

And not sure if it's ok with the styleguide.
go/cppguide#Ownership_and_Smart_Pointers

Can it be somehow converted to using unique_ptr, for example, and moving it
around as necessary?

+  if (!continuation.is_null())
+    continuation.Run();
+}
+
 ServerProxy::~ServerProxy() {}
 
 PrivacyCAType ServerProxy::GetType() {