Added a timeout for platform verification key generation.

chrome/browser/chromeos/attestation/platform_verification_flow.h

Index: chrome/browser/chromeos/attestation/platform_verification_flow.h
diff --git a/chrome/browser/chromeos/attestation/platform_verification_flow.h b/chrome/browser/chromeos/attestation/platform_verification_flow.h
index 64f36274649b2ccc83f0daca72071a6823745d3e..57768b1fea9c7c905fcd83bd76f66e8ada7325c3 100644
--- a/chrome/browser/chromeos/attestation/platform_verification_flow.h
+++ b/chrome/browser/chromeos/attestation/platform_verification_flow.h
@@ -9,8 +9,8 @@
 
 #include "base/basictypes.h"
 #include "base/callback.h"
+#include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
-#include "base/memory/weak_ptr.h"
 #include "url/gurl.h"
 
 class PrefService;
@@ -39,11 +39,23 @@ class AttestationFlow;
 
 // This class allows platform verification for the content protection use case.
 // All methods must only be called on the UI thread.  Example:
-//   PlatformVerificationFlow verifier;
+//   scoped_refptr<PlatformVerificationFlow> verifier =
+//       new PlatformVerificationFlow();
 //   PlatformVerificationFlow::Callback callback = base::Bind(&MyCallback);
-//   verifier.ChallengePlatformKey(my_web_contents, "my_id", "some_challenge",
-//                                 callback);
-class PlatformVerificationFlow {
+//   verifier->ChallengePlatformKey(my_web_contents, "my_id", "some_challenge",
+//                                  callback);
+//
+// This class is RefCountedThreadSafe because it may need to outlive its caller.
+// The attestation flow that needs to happen to establish a certified platform
+// key may take minutes on some hardware.  This class will timeout after a much
+// shorter time so the caller can proceed without platform verification but it
+// is important that the pending operation be allowed to finish.  If the
+// attestation flow is aborted at any stage, it will need to start over.  If we
+// use weak pointers, the attestation flow will stop when the next callback is
+// run.  So we need the instance to stay alive until the platform key is fully
+// certified so the next time ChallegePlatformKey() is invoked it will be quick.
+class PlatformVerificationFlow
+    : public base::RefCountedThreadSafe<PlatformVerificationFlow> {
  public:
   enum Result {
     SUCCESS,                // The operation succeeded.
@@ -53,6 +65,7 @@ class PlatformVerificationFlow {
                             // - It is not running a verified OS image.
     USER_REJECTED,          // The user explicitly rejected the operation.
     POLICY_REJECTED,        // The operation is not allowed by policy/settings.
+    TIMEOUT,                // The operation timed out.
   };
 
   enum ConsentType {
@@ -111,8 +124,6 @@ class PlatformVerificationFlow {
                            UserManager* user_manager,
                            Delegate* delegate);
 
-  virtual ~PlatformVerificationFlow();
-
   // Invokes an asynchronous operation to challenge a platform key.  Any user
   // interaction will be associated with |web_contents|.  The |service_id| is an
   // arbitrary value but it should uniquely identify the origin of the request
@@ -138,52 +149,80 @@ class PlatformVerificationFlow {
     testing_url_ = testing_url;
   }
 
+  void set_timeout_delay(int timeout_delay) {
+    timeout_delay_ = timeout_delay;
+  }
+
  private:
+  // Tracks timeout status for an asynchronous operation.
+  struct TimeoutStatus : public base::RefCountedThreadSafe<TimeoutStatus> {

[DaleCurtis, 2013/10/30 18:06:00]

Why all this instead of base::DelayTimer from base/timer/timer.h? It's self
invalidating so you don't need any additional RefCounted or WeakPtr business all
over.

[Darren Krahn, 2013/10/30 19:14:30]

AFAICT, DelayTimer offers no real benefit over PostDelayedTask which I'm using. 
This ref-counted status exists because the class is designed to support multiple
ChallengePlatformKey calls in flight simultaneously so I want status tracked
with the call, not global to the class instance.  When either of the callbacks
(timer/actual) return, they need to be aware of whether the other callback has
already run, so they both hold a ref to the status.

Maybe I'm missing something?

[DaleCurtis, 2013/10/30 20:11:52]

Ah I forgot there can be multiple calls outstanding.  I still think it'd be
cleaner in terms of less code and better tested (timer already has extensive
tests) than manually managing this TimeoutStatus structure.  Up to you though.

Also, err, sorry, I meant OneShotTimer.  If you instead make ChallengeContext a
ref counted structure (which would save on copies anyways), you can stick the
OneShotTimer in there.  Then you can use Timer::IsRunning() to determine if it
has fired or not and Timer::Stop() to prevent it from firing.

[Darren Krahn, 2013/10/30 22:24:53]

Good suggestion -- updated to use base::Timer and base::Passed.

+    bool finished;
+    bool timed_out;
+
+    TimeoutStatus() : finished(false), timed_out(false) {}
+
+   private:
+    friend class base::RefCountedThreadSafe<TimeoutStatus>;
+    ~TimeoutStatus() {}
+  };
+
+  // Holds the arguments of a ChallengePlatformKey call.  This is convenient for
+  // use with base::Bind so we don't get too many arguments.
+  struct ChallengeContext {
+    content::WebContents* web_contents;
+    std::string service_id;
+    std::string challenge;
+    ChallengeCallback callback;
+  };
+
+  friend class base::RefCountedThreadSafe<PlatformVerificationFlow>;
+  ~PlatformVerificationFlow();
+
   // Checks whether we need to prompt the user for consent before proceeding and
-  // invokes the consent UI if so.  All parameters are the same as in
-  // ChallengePlatformKey except for the additional |attestation_enrolled| which
-  // specifies whether attestation has been enrolled for this device.
-  void CheckConsent(content::WebContents* web_contents,
-                    const std::string& service_id,
-                    const std::string& challenge,
-                    const ChallengeCallback& callback,
+  // invokes the consent UI if so.  The arguments to ChallengePlatformKey are
+  // in |context| and |attestation_enrolled| specifies whether attestation has
+  // been enrolled for this device.
+  void CheckConsent(const ChallengeContext& context,
                     bool attestation_enrolled);
 
-  // A callback called when the user has given their consent response.  All
-  // parameters are the same as in ChallengePlatformKey except for the
-  // additional |consent_type| and |consent_response| which indicate the consent
-  // type and user response, respectively.  If the response indicates that the
-  // operation should proceed, this method invokes a certificate request.
-  void OnConsentResponse(content::WebContents* web_contents,
-                         const std::string& service_id,
-                         const std::string& challenge,
-                         const ChallengeCallback& callback,
+  // A callback called when the user has given their consent response.  The
+  // arguments to ChallengePlatformKey are in |context|.  |consent_type| and
+  // |consent_response| indicate the consent type and user response,
+  // respectively.  If the response indicates that the operation should proceed,
+  // this method invokes a certificate request.
+  void OnConsentResponse(const ChallengeContext& context,
                          ConsentType consent_type,
                          ConsentResponse consent_response);
 
   // A callback called when an attestation certificate request operation
-  // completes.  |service_id|, |challenge|, and |callback| are the same as in
-  // ChallengePlatformKey.  |user_id| identifies the user for which the
-  // certificate was requested.  |operation_success| is true iff the certificate
-  // request operation succeeded.  |certificate| holds the certificate for the
-  // platform key on success.  If the certificate request was successful, this
-  // method invokes a request to sign the challenge.
-  void OnCertificateReady(const std::string& user_id,
-                          const std::string& service_id,
-                          const std::string& challenge,
-                          const ChallengeCallback& callback,
+  // completes.  The arguments to ChallengePlatformKey are in |context|.
+  // |user_id| identifies the user for which the certificate was requested.
+  // |operation_success| is true iff the certificate request operation
+  // succeeded.  |certificate| holds the certificate for the platform key on
+  // success.  If the certificate request was successful, this method invokes a
+  // request to sign the challenge.  If the operation timed out prior to this
+  // method being called, this method does nothing - notably, the callback is
+  // not invoked.
+  void OnCertificateReady(const ChallengeContext& context,
+                          const std::string& user_id,
+                          TimeoutStatus* timeout_status,
                           bool operation_success,
                           const std::string& certificate);
 
+  // A callback run after a constant delay to handle timeouts for lengthy
+  // certificate requests.  |context.callback| will be invoked with a TIMEOUT
+  // result if the certificate request has not already finished.
+  void OnCertificateTimeout(const ChallengeContext& context,
+                            TimeoutStatus* timeout_status);
+
   // A callback called when a challenge signing request has completed.  The
   // |certificate| is the platform certificate for the key which signed the
-  // |challenge|.  |callback| is the same as in ChallengePlatformKey.
+  // |challenge|.  The arguments to ChallengePlatformKey are in |context|.
   // |operation_success| is true iff the challenge signing operation was
   // successful.  If it was successful, |response_data| holds the challenge
-  // response and the method will invoke |callback|.
-  void OnChallengeReady(const std::string& certificate,
-                        const std::string& challenge,
-                        const ChallengeCallback& callback,
+  // response and the method will invoke |context.callback|.
+  void OnChallengeReady(const ChallengeContext& context,
+                        const std::string& certificate,
                         bool operation_success,
                         const std::string& response_data);
 
@@ -242,10 +281,7 @@ class PlatformVerificationFlow {
   scoped_ptr<Delegate> default_delegate_;
   PrefService* testing_prefs_;
   GURL testing_url_;
-
-  // Note: This should remain the last member so it'll be destroyed and
-  // invalidate the weak pointers before any other members are destroyed.
-  base::WeakPtrFactory<PlatformVerificationFlow> weak_factory_;
+  int timeout_delay_;

[DaleCurtis, 2013/10/30 20:11:52]

base::TimeDelta

[Darren Krahn, 2013/10/30 22:24:53]

Done.

 
   DISALLOW_COPY_AND_ASSIGN(PlatformVerificationFlow);
 };