[GCM] RegistrationRequest is retrying when sender is invlalid

It turns out that some of the Error="*" responses come as HTTP_OK, and some not. Given that check for HTTP_OK was done before response was parsed, we were missing some of the error conditions, including INVALID_SENDER and retrying assuming UNKNOWN_ERROR.

The problem was already partially mitigated by failing after sever attempts, but with this fix we will actually start seeing the INVALID_SENDER response.

Semantics of the HTTP_NOT_OK result was updated to distinguish it from UNKNOWN_ERROR in the following way:
* if we have HTTP_OK but cannot parse token or error, we return UNKNOWN_ERROR,
* if we don't have HTTP_OK an cannot parse error, we return HTTP_NOT_OK.

Also updating the string comparisons in GetStatusFromError to use find instead of ==, to make sure that if there is more content in response, we can parse the error.

BUG=345408

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

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

2 files changed, 44 insertions, 19 deletions

diff --git a/google_apis/gcm/engine/registration_request.cc b/google_apis/gcm/engine/registration_request.cc
index a6fd036..3acd591 100644
--- a/google_apis/gcm/engine/registration_request.cc
+++ b/google_apis/gcm/engine/registration_request.cc
@@ -54,13 +54,15 @@ void BuildFormEncoding(const std::string& key,
 
 // Gets correct status from the error message.
 RegistrationRequest::Status GetStatusFromError(const std::string& error) {
-  if (error == kDeviceRegistrationError)
+  // TODO(fgorski): Improve error parsing in case there is nore then just an
+  // Error=ERROR_STRING in response.
+  if (error.find(kDeviceRegistrationError) != std::string::npos)
     return RegistrationRequest::DEVICE_REGISTRATION_ERROR;
-  if (error == kAuthenticationFailed)
+  if (error.find(kAuthenticationFailed) != std::string::npos)
     return RegistrationRequest::AUTHENTICATION_FAILED;
-  if (error == kInvalidSender)
+  if (error.find(kInvalidSender) != std::string::npos)
     return RegistrationRequest::INVALID_SENDER;
-  if (error == kInvalidParameters)
+  if (error.find(kInvalidParameters) != std::string::npos)
     return RegistrationRequest::INVALID_PARAMETERS;
   return RegistrationRequest::UNKNOWN_ERROR;
 }
@@ -189,29 +191,39 @@ RegistrationRequest::Status RegistrationRequest::ParseResponse(
     LOG(ERROR) << "URL fetching failed.";
     return URL_FETCHING_FAILED;
   }
-  if (source->GetResponseCode() != net::HTTP_OK) {
-    LOG(ERROR) << "URL fetching HTTP response code is not OK. It is "
-               << source->GetResponseCode();
-    return HTTP_NOT_OK;
-  }
+
   std::string response;
   if (!source->GetResponseAsString(&response)) {
     LOG(ERROR) << "Failed to parse registration response as a string.";
     return RESPONSE_PARSING_FAILED;
   }
 
-  DVLOG(1) << "Parsing registration response: " << response;
-  size_t token_pos = response.find(kTokenPrefix);
-  if (token_pos != std::string::npos) {
-    *token = response.substr(token_pos + arraysize(kTokenPrefix) - 1);
-    return SUCCESS;
+  if (source->GetResponseCode() == net::HTTP_OK) {
+    size_t token_pos = response.find(kTokenPrefix);
+    if (token_pos != std::string::npos) {
+      *token = response.substr(token_pos + arraysize(kTokenPrefix) - 1);
+      return SUCCESS;
+    }
   }
 
+  // If we are able to parse a meaningful known error, let's do so. Some errors
+  // will have HTTP_BAD_REQUEST, some will have HTTP_OK response code.
   size_t error_pos = response.find(kErrorPrefix);
-  if (error_pos == std::string::npos)
-    return UNKNOWN_ERROR;
-  std::string error = response.substr(error_pos + arraysize(kErrorPrefix) - 1);
-  return GetStatusFromError(error);
+  if (error_pos != std::string::npos) {
+    std::string error = response.substr(
+        error_pos + arraysize(kErrorPrefix) - 1);
+    return GetStatusFromError(error);
+  }
+
+  // If we cannot tell what the error is, but at least we know response code was
+  // not OK.
+  if (source->GetResponseCode() != net::HTTP_OK) {
+    DLOG(ERROR) << "URL fetching HTTP response code is not OK. It is "
+                << source->GetResponseCode();
+    return HTTP_NOT_OK;
+  }
+
+  return UNKNOWN_ERROR;
 }
 
 void RegistrationRequest::OnURLFetchComplete(const net::URLFetcher* source) {
diff --git a/google_apis/gcm/engine/registration_request_unittest.cc b/google_apis/gcm/engine/registration_request_unittest.cc
index 4764d13..f08c4f4 100644
--- a/google_apis/gcm/engine/registration_request_unittest.cc
+++ b/google_apis/gcm/engine/registration_request_unittest.cc
@@ -293,7 +293,8 @@ TEST_F(RegistrationRequestTest, ResponseAuthenticationError) {
   CreateRequest("sender1,sender2");
   request_->Start();
 
-  SetResponseStatusAndString(net::HTTP_OK, "Error=AUTHENTICATION_FAILED");
+  SetResponseStatusAndString(net::HTTP_UNAUTHORIZED,
+                             "Error=AUTHENTICATION_FAILED");
   CompleteFetch();
 
   EXPECT_FALSE(callback_called_);
@@ -331,6 +332,18 @@ TEST_F(RegistrationRequestTest, ResponseInvalidSender) {
   EXPECT_EQ(std::string(), registration_id_);
 }
 
+TEST_F(RegistrationRequestTest, ResponseInvalidSenderBadRequest) {
+  CreateRequest("sender1");
+  request_->Start();
+
+  SetResponseStatusAndString(net::HTTP_BAD_REQUEST, "Error=INVALID_SENDER");
+  CompleteFetch();
+
+  EXPECT_TRUE(callback_called_);
+  EXPECT_EQ(RegistrationRequest::INVALID_SENDER, status_);
+  EXPECT_EQ(std::string(), registration_id_);
+}
+
 TEST_F(RegistrationRequestTest, RequestNotSuccessful) {
   CreateRequest("sender1,sender2");
   request_->Start();