[GCM] Adding retry logic to registration

* in certain cases, we want to repeat the registration if the first attempt fails, e.g. malformed response, or credentials not being propagated yet.
* also adding response parsing into a relevant status.

BUG=284553

Review URL: https://codereview.chromium.org/140433013

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@249904 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 233 insertions, 23 deletions

diff --git a/google_apis/gcm/engine/registration_request.cc b/google_apis/gcm/engine/registration_request.cc
index dcfdb97..81c5817 100644
--- a/google_apis/gcm/engine/registration_request.cc
+++ b/google_apis/gcm/engine/registration_request.cc
@@ -38,7 +38,12 @@ const char kUserSerialNumberKey[] = "device_user_id";
 const size_t kMaxSenders = 100;
 
 // Response constants.
+const char kErrorPrefix[] = "Error=";
 const char kTokenPrefix[] = "token=";
+const char kDeviceRegistrationError[] = "PHONE_REGISTRATION_ERROR";
+const char kAuthenticationFailed[] = "AUTHENTICATION_FAILED";
+const char kInvalidSender[] = "INVALID_SENDER";
+const char kInvalidParameters[] = "INVALID_PARAMETERS";
 
 void BuildFormEncoding(const std::string& key,
                        const std::string& value,
@@ -48,6 +53,27 @@ void BuildFormEncoding(const std::string& key,
   out->append(key + "=" + net::EscapeUrlEncodedData(value, true));
 }
 
+// Gets correct status from the error message.
+RegistrationRequest::Status GetStatusFromError(const std::string& error) {
+  if (error == kDeviceRegistrationError)
+    return RegistrationRequest::DEVICE_REGISTRATION_ERROR;
+  if (error == kAuthenticationFailed)
+    return RegistrationRequest::AUTHENTICATION_FAILED;
+  if (error == kInvalidSender)
+    return RegistrationRequest::INVALID_SENDER;
+  if (error == kInvalidParameters)
+    return RegistrationRequest::INVALID_PARAMETERS;
+  return RegistrationRequest::UNKNOWN_ERROR;
+}
+
+// Indicates whether a retry attempt should be made based on the status of the
+// last request.
+bool ShouldRetryWithStatus(RegistrationRequest::Status status) {
+  return status == RegistrationRequest::UNKNOWN_ERROR ||
+         status == RegistrationRequest::AUTHENTICATION_FAILED ||
+         status == RegistrationRequest::DEVICE_REGISTRATION_ERROR;
+}
+
 }  // namespace
 
 RegistrationRequest::RequestInfo::RequestInfo(
@@ -64,17 +90,22 @@ RegistrationRequest::RequestInfo::RequestInfo(
       user_serial_number(user_serial_number),
       app_id(app_id),
       cert(cert),
-      sender_ids(sender_ids) {}
+      sender_ids(sender_ids) {
+}
 
 RegistrationRequest::RequestInfo::~RequestInfo() {}
 
 RegistrationRequest::RegistrationRequest(
     const RequestInfo& request_info,
+    const net::BackoffEntry::Policy& backoff_policy,
     const RegistrationCallback& callback,
     scoped_refptr<net::URLRequestContextGetter> request_context_getter)
     : callback_(callback),
       request_info_(request_info),
-      request_context_getter_(request_context_getter) {}
+      backoff_entry_(&backoff_policy),
+      request_context_getter_(request_context_getter),
+      weak_ptr_factory_(this) {
+}
 
 RegistrationRequest::~RegistrationRequest() {}
 
@@ -125,35 +156,70 @@ void RegistrationRequest::Start() {
                       &body);
   }
 
+  DVLOG(1) << "Performing registration for: " << request_info_.app_id;
   DVLOG(1) << "Registration request: " << body;
   url_fetcher_->SetUploadData(kRegistrationRequestContentType, body);
-
-  DVLOG(1) << "Performing registration for: " << request_info_.app_id;
   url_fetcher_->Start();
 }
 
+void RegistrationRequest::RetryWithBackoff(bool update_backoff) {
+  if (update_backoff) {
+    url_fetcher_.reset();
+    backoff_entry_.InformOfRequest(false);
+  }
+
+  if (backoff_entry_.ShouldRejectRequest()) {
+    DVLOG(1) << "Delaying GCM registration of app: "
+             << request_info_.app_id << ", for "
+             << backoff_entry_.GetTimeUntilRelease().InMilliseconds()
+             << " milliseconds.";
+    base::MessageLoop::current()->PostDelayedTask(
+        FROM_HERE,
+        base::Bind(&RegistrationRequest::RetryWithBackoff,
+                   weak_ptr_factory_.GetWeakPtr(),
+                   false),
+        backoff_entry_.GetTimeUntilRelease());
+    return;
+  }
+
+  Start();
+}
+
 void RegistrationRequest::OnURLFetchComplete(const net::URLFetcher* source) {
   std::string response;
   if (!source->GetStatus().is_success() ||
       source->GetResponseCode() != net::HTTP_OK ||
       !source->GetResponseAsString(&response)) {
-    // TODO(fgoski): Introduce retry logic.
-    // TODO(jianli): Handle "Error=INVALID_SENDER".
     LOG(ERROR) << "Failed to get registration response: "
                << source->GetStatus().is_success() << " "
                << source->GetResponseCode();
-    callback_.Run("");
+    RetryWithBackoff(true);
     return;
   }
 
   DVLOG(1) << "Parsing registration response: " << response;
   size_t token_pos = response.find(kTokenPrefix);
-  std::string token;
-  if (token_pos != std::string::npos)
-    token = response.substr(token_pos + strlen(kTokenPrefix));
-  else
-    LOG(ERROR) << "Failed to extract token.";
-  callback_.Run(token);
+  if (token_pos != std::string::npos) {
+    std::string token =
+        response.substr(token_pos + arraysize(kTokenPrefix) - 1);
+    callback_.Run(SUCCESS, token);
+    return;
+  }
+
+  size_t error_pos = response.find(kErrorPrefix);
+  Status status = UNKNOWN_ERROR;
+  if (error_pos != std::string::npos) {
+    std::string error =
+        response.substr(error_pos + arraysize(kErrorPrefix) - 1);
+    status = GetStatusFromError(error);
+  }
+
+  if (ShouldRetryWithStatus(status)) {
+    RetryWithBackoff(true);
+    return;
+  }
+
+  callback_.Run(status, std::string());
 }
 
 }  // namespace gcm
diff --git a/google_apis/gcm/engine/registration_request.h b/google_apis/gcm/engine/registration_request.h
index 47cce76..6360cf7 100644
--- a/google_apis/gcm/engine/registration_request.h
+++ b/google_apis/gcm/engine/registration_request.h
@@ -12,7 +12,9 @@
 #include "base/callback.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
 #include "google_apis/gcm/base/gcm_export.h"
+#include "net/base/backoff_entry.h"
 #include "net/url_request/url_fetcher_delegate.h"
 
 namespace net {
@@ -27,8 +29,18 @@ namespace gcm {
 // be authorized to address the application using it's assigned registration ID.
 class GCM_EXPORT RegistrationRequest : public net::URLFetcherDelegate {
  public:
+  enum Status {
+    SUCCESS,                    // Registration completed successfully.
+    INVALID_PARAMETERS,         // One of request paramteres was invalid.
+    INVALID_SENDER,             // One of the provided senders was invalid.
+    AUTHENTICATION_FAILED,      // Authentication failed.
+    DEVICE_REGISTRATION_ERROR,  // Chrome is not properly registered.
+    UNKNOWN_ERROR,              // Unknown error.
+  };
+
   // Callback completing the registration request.
-  typedef base::Callback<void(const std::string& registration_id)>
+  typedef base::Callback<void(Status status,
+                              const std::string& registration_id)>
       RegistrationCallback;
 
   // Details of the of the Registration Request. Only user's android ID and
@@ -62,6 +74,7 @@ class GCM_EXPORT RegistrationRequest : public net::URLFetcherDelegate {
 
   RegistrationRequest(
       const RequestInfo& request_info,
+      const net::BackoffEntry::Policy& backoff_policy,
       const RegistrationCallback& callback,
       scoped_refptr<net::URLRequestContextGetter> request_context_getter);
   virtual ~RegistrationRequest();
@@ -72,12 +85,19 @@ class GCM_EXPORT RegistrationRequest : public net::URLFetcherDelegate {
   virtual void OnURLFetchComplete(const net::URLFetcher* source) OVERRIDE;
 
  private:
+  // Schedules a retry attempt, informs the backoff of a previous request's
+  // failure, when |update_backoff| is true.
+  void RetryWithBackoff(bool update_backoff);
+
   RegistrationCallback callback_;
   RequestInfo request_info_;
 
+  net::BackoffEntry backoff_entry_;
   scoped_refptr<net::URLRequestContextGetter> request_context_getter_;
   scoped_ptr<net::URLFetcher> url_fetcher_;
 
+  base::WeakPtrFactory<RegistrationRequest> weak_ptr_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(RegistrationRequest);
 };
 
diff --git a/google_apis/gcm/engine/registration_request_unittest.cc b/google_apis/gcm/engine/registration_request_unittest.cc
index d3e06c8..ad4c400 100644
--- a/google_apis/gcm/engine/registration_request_unittest.cc
+++ b/google_apis/gcm/engine/registration_request_unittest.cc
@@ -20,11 +20,40 @@ const uint64 kAndroidId = 42UL;
 const char kCert[] = "0DEADBEEF420";
 const char kDeveloperId[] = "Project1";
 const char kLoginHeader[] = "AidLogin";
-const char kNoRegistrationIdReturned[] = "No Registration Id returned";
 const char kAppId[] = "TestAppId";
 const uint64 kSecurityToken = 77UL;
 const uint64 kUserAndroidId = 1111;
 const int64 kUserSerialNumber = 1;
+
+// Backoff policy for testing registration request.
+const net::BackoffEntry::Policy kDefaultBackoffPolicy = {
+  // Number of initial errors (in sequence) to ignore before applying
+  // exponential back-off rules.
+  // Explicitly set to 2 to skip the delay on the first retry, as we are not
+  // trying to test the backoff itself, but rather the fact that retry happens.
+  2,
+
+  // Initial delay for exponential back-off in ms.
+  15000,  // 15 seconds.
+
+  // Factor by which the waiting time will be multiplied.
+  2,
+
+  // Fuzzing percentage. ex: 10% will spread requests randomly
+  // between 90%-100% of the calculated time.
+  0.5,  // 50%.
+
+  // Maximum amount of time we are willing to delay our request in ms.
+  1000 * 60 * 5, // 5 minutes.
+
+  // Time to keep an entry from being discarded even when it
+  // has no significant state, -1 to never discard.
+  -1,
+
+  // Don't use initial delay unless the last request was an error.
+  false,
+};
+
 }  // namespace
 
 class RegistrationRequestTest : public testing::Test {
@@ -32,7 +61,8 @@ class RegistrationRequestTest : public testing::Test {
   RegistrationRequestTest();
   virtual ~RegistrationRequestTest();
 
-  void RegistrationCallback(const std::string& registration_id);
+  void RegistrationCallback(RegistrationRequest::Status status,
+                            const std::string& registration_id);
 
   void CreateRequest(const std::string& sender_ids);
   void SetResponseStatusAndString(net::HttpStatusCode status_code,
@@ -40,7 +70,9 @@ class RegistrationRequestTest : public testing::Test {
   void CompleteFetch();
 
  protected:
+  RegistrationRequest::Status status_;
   std::string registration_id_;
+  bool callback_called_;
   std::map<std::string, std::string> extras_;
   scoped_ptr<RegistrationRequest> request_;
   base::MessageLoop message_loop_;
@@ -49,14 +81,19 @@ class RegistrationRequestTest : public testing::Test {
 };
 
 RegistrationRequestTest::RegistrationRequestTest()
-    : url_request_context_getter_(new net::TestURLRequestContextGetter(
+    : status_(RegistrationRequest::SUCCESS),
+      callback_called_(false),
+      url_request_context_getter_(new net::TestURLRequestContextGetter(
           message_loop_.message_loop_proxy())) {}
 
 RegistrationRequestTest::~RegistrationRequestTest() {}
 
 void RegistrationRequestTest::RegistrationCallback(
+    RegistrationRequest::Status status,
     const std::string& registration_id) {
+  status_ = status;
   registration_id_ = registration_id;
+  callback_called_ = true;
 }
 
 void RegistrationRequestTest::CreateRequest(const std::string& sender_ids) {
@@ -73,6 +110,7 @@ void RegistrationRequestTest::CreateRequest(const std::string& sender_ids) {
                                        kAppId,
                                        kCert,
                                        senders),
+      kDefaultBackoffPolicy,
       base::Bind(&RegistrationRequestTest::RegistrationCallback,
                  base::Unretained(this)),
       url_request_context_getter_.get()));
@@ -88,7 +126,10 @@ void RegistrationRequestTest::SetResponseStatusAndString(
 }
 
 void RegistrationRequestTest::CompleteFetch() {
-  registration_id_ = kNoRegistrationIdReturned;
+  registration_id_.clear();
+  status_ = RegistrationRequest::SUCCESS;
+  callback_called_ = false;
+
   net::TestURLFetcher* fetcher = url_fetcher_factory_.GetFetcherByID(0);
   ASSERT_TRUE(fetcher);
   fetcher->delegate()->OnURLFetchComplete(fetcher);
@@ -171,6 +212,8 @@ TEST_F(RegistrationRequestTest, ResponseParsing) {
   SetResponseStatusAndString(net::HTTP_OK, "token=2501");
   CompleteFetch();
 
+  EXPECT_TRUE(callback_called_);
+  EXPECT_EQ(RegistrationRequest::SUCCESS, status_);
   EXPECT_EQ("2501", registration_id_);
 }
 
@@ -181,7 +224,14 @@ TEST_F(RegistrationRequestTest, ResponseHttpStatusNotOK) {
   SetResponseStatusAndString(net::HTTP_UNAUTHORIZED, "token=2501");
   CompleteFetch();
 
-  EXPECT_EQ("", registration_id_);
+  EXPECT_FALSE(callback_called_);
+
+  SetResponseStatusAndString(net::HTTP_OK, "token=2501");
+  CompleteFetch();
+
+  EXPECT_TRUE(callback_called_);
+  EXPECT_EQ(RegistrationRequest::SUCCESS, status_);
+  EXPECT_EQ("2501", registration_id_);
 }
 
 TEST_F(RegistrationRequestTest, ResponseMissingRegistrationId) {
@@ -191,15 +241,80 @@ TEST_F(RegistrationRequestTest, ResponseMissingRegistrationId) {
   SetResponseStatusAndString(net::HTTP_OK, "");
   CompleteFetch();
 
-  EXPECT_EQ("", registration_id_);
+  EXPECT_FALSE(callback_called_);
 
+  SetResponseStatusAndString(net::HTTP_OK, "some error in response");
+  CompleteFetch();
+
+  EXPECT_FALSE(callback_called_);
+
+  // Ensuring a retry happened and succeeds.
+  SetResponseStatusAndString(net::HTTP_OK, "token=2501");
+  CompleteFetch();
+
+  EXPECT_TRUE(callback_called_);
+  EXPECT_EQ(RegistrationRequest::SUCCESS, status_);
+  EXPECT_EQ("2501", registration_id_);
+}
+
+TEST_F(RegistrationRequestTest, ResponseDeviceRegistrationError) {
   CreateRequest("sender1,sender2");
   request_->Start();
 
-  SetResponseStatusAndString(net::HTTP_OK, "some error in response");
+  SetResponseStatusAndString(net::HTTP_OK, "Error=PHONE_REGISTRATION_ERROR");
+  CompleteFetch();
+
+  EXPECT_FALSE(callback_called_);
+
+  // Ensuring a retry happened and succeeds.
+  SetResponseStatusAndString(net::HTTP_OK, "token=2501");
+  CompleteFetch();
+
+  EXPECT_TRUE(callback_called_);
+  EXPECT_EQ(RegistrationRequest::SUCCESS, status_);
+  EXPECT_EQ("2501", registration_id_);
+}
+
+TEST_F(RegistrationRequestTest, ResponseAuthenticationError) {
+  CreateRequest("sender1,sender2");
+  request_->Start();
+
+  SetResponseStatusAndString(net::HTTP_OK, "Error=AUTHENTICATION_FAILED");
+  CompleteFetch();
+
+  EXPECT_FALSE(callback_called_);
+
+  // Ensuring a retry happened and succeeds.
+  SetResponseStatusAndString(net::HTTP_OK, "token=2501");
+  CompleteFetch();
+
+  EXPECT_TRUE(callback_called_);
+  EXPECT_EQ(RegistrationRequest::SUCCESS, status_);
+  EXPECT_EQ("2501", registration_id_);
+}
+
+TEST_F(RegistrationRequestTest, ResponseInvalidParameters) {
+  CreateRequest("sender1,sender2");
+  request_->Start();
+
+  SetResponseStatusAndString(net::HTTP_OK, "Error=INVALID_PARAMETERS");
+  CompleteFetch();
+
+  EXPECT_TRUE(callback_called_);
+  EXPECT_EQ(RegistrationRequest::INVALID_PARAMETERS, status_);
+  EXPECT_EQ(std::string(), registration_id_);
+}
+
+TEST_F(RegistrationRequestTest, ResponseInvalidSender) {
+  CreateRequest("sender1,sender2");
+  request_->Start();
+
+  SetResponseStatusAndString(net::HTTP_OK, "Error=INVALID_SENDER");
   CompleteFetch();
 
-  EXPECT_EQ("", registration_id_);
+  EXPECT_TRUE(callback_called_);
+  EXPECT_EQ(RegistrationRequest::INVALID_SENDER, status_);
+  EXPECT_EQ(std::string(), registration_id_);
 }
 
 }  // namespace gcm
diff --git a/google_apis/gcm/gcm_client_impl.cc b/google_apis/gcm/gcm_client_impl.cc
index abadb62..549569c 100644
--- a/google_apis/gcm/gcm_client_impl.cc
+++ b/google_apis/gcm/gcm_client_impl.cc
@@ -376,6 +376,7 @@ void GCMClientImpl::Register(const std::string& username,
 
   RegistrationRequest* registration_request =
       new RegistrationRequest(request_info,
+                              kDefaultBackoffPolicy,
                               base::Bind(&GCMClientImpl::OnRegisterCompleted,
                                          weak_ptr_factory_.GetWeakPtr(),
                                          registration_key),
@@ -386,6 +387,7 @@ void GCMClientImpl::Register(const std::string& username,
 
 void GCMClientImpl::OnRegisterCompleted(
     const PendingRegistrationKey& registration_key,
+    RegistrationRequest::Status status,
     const std::string& registration_id) {
   Delegate* delegate = user_list_->GetDelegateByUsername(
       registration_key.username);
@@ -398,6 +400,12 @@ void GCMClientImpl::OnRegisterCompleted(
 
   std::string app_id = registration_key.app_id;
 
+  if (status == RegistrationRequest::INVALID_SENDER) {
+    delegate->OnRegisterFinished(
+        app_id, std::string(), GCMClientImpl::INVALID_PARAMETER);
+    return;
+  }
+
   if (registration_id.empty()) {
     delegate->OnRegisterFinished(
         app_id, std::string(), GCMClient::SERVER_ERROR);
diff --git a/google_apis/gcm/gcm_client_impl.h b/google_apis/gcm/gcm_client_impl.h
index e20d86b..bcd111b 100644
--- a/google_apis/gcm/gcm_client_impl.h
+++ b/google_apis/gcm/gcm_client_impl.h
@@ -16,6 +16,7 @@
 #include "google_apis/gcm/base/mcs_message.h"
 #include "google_apis/gcm/engine/gcm_store.h"
 #include "google_apis/gcm/engine/mcs_client.h"
+#include "google_apis/gcm/engine/registration_request.h"
 #include "google_apis/gcm/gcm_client.h"
 #include "google_apis/gcm/protocol/android_checkin.pb.h"
 #include "net/base/net_log.h"
@@ -34,7 +35,6 @@ namespace gcm {
 class CheckinRequest;
 class ConnectionFactory;
 class GCMClientImplTest;
-class RegistrationRequest;
 class UserList;
 
 // Implements the GCM Client. It is used to coordinate MCS Client (communication
@@ -151,6 +151,7 @@ class GCM_EXPORT GCMClientImpl : public GCMClient {
 
   // Completes the registration request.
   void OnRegisterCompleted(const PendingRegistrationKey& registration_key,
+                           RegistrationRequest::Status status,
                            const std::string& registration_id);
 
   // Callback for setting a delegate on a |user_list_|. Informs that the