GCM Engine: Split up reg/unreg UNKNOWN_ERROR to improve metrics

When the GCM server handles registration/unregistration requests, it can
return several specific error names that Chrome's GCM Engine was lumping
together as UNKNOWN_ERROR. This patch splits them out into distinct enum
values, so we'll be able to compare their occurrence rates using UMA and
get more precise bug reports from users reading chrome://gcm-internals.

UNKNOWN_ERROR should no longer occur... until GCM next add new error
types anyway.

The patch also removes some code duplication between
GCMUnregistrationRequestHandler and InstanceIDDeleteTokenRequestHandler,
but it stops short of merging RegistrationRequest and
UnregistrationRequest since this patch may need to be merged to m55.

There should be minimal functional changes, except that we will no
longer auto-retry QUOTA_EXCEEDED and TOO_MANY_REGISTRATIONS registration
errors, which were unlikely to succeed when retried anyway.

BUG=623062

Review-Url: https://codereview.chromium.org/2434243002
Cr-Commit-Position: refs/heads/master@{#429254}

components/gcm_driver/gcm_stats_recorder_impl.cc

@@ -106,6 +106,14 @@ std::string GetRegistrationStatusString(
       return "NO_RESPONSE_BODY";
     case gcm::RegistrationRequest::REACHED_MAX_RETRIES:
       return "REACHED_MAX_RETRIES";
+    case gcm::RegistrationRequest::RESPONSE_PARSING_FAILED:
+      return "RESPONSE_PARSING_FAILED";
+    case gcm::RegistrationRequest::INTERNAL_SERVER_ERROR:
+      return "INTERNAL_SERVER_ERROR";
+    case gcm::RegistrationRequest::QUOTA_EXCEEDED:
+      return "QUOTA_EXCEEDED";
+    case gcm::RegistrationRequest::TOO_MANY_REGISTRATIONS:
+      return "TOO_MANY_REGISTRATIONS";
     case gcm::RegistrationRequest::STATUS_COUNT:
       NOTREACHED();
       break;
@@ -140,6 +148,8 @@ std::string GetUnregistrationStatusString(
       return "UNKNOWN_ERROR";
     case gcm::UnregistrationRequest::REACHED_MAX_RETRIES:
       return "REACHED_MAX_RETRIES";
+    case gcm::UnregistrationRequest::DEVICE_REGISTRATION_ERROR:
+      return "DEVICE_REGISTRATION_ERROR";
     case gcm::UnregistrationRequest::UNREGISTRATION_STATUS_COUNT:
       NOTREACHED();
       break;

google_apis/gcm/engine/gcm_unregistration_request_handler.cc

@@ -21,8 +21,6 @@ const char kUnregistrationCallerValue[] = "false";
 
 // Response constants.
 const char kDeletedPrefix[] = "deleted=";
-const char kErrorPrefix[] = "Error=";
-const char kInvalidParameters[] = "INVALID_PARAMETERS";
 
 }  // namespace
 
@@ -38,13 +36,7 @@ void GCMUnregistrationRequestHandler::BuildRequestBody(std::string* body){
 }
 
 UnregistrationRequest::Status GCMUnregistrationRequestHandler::ParseResponse(
-    const net::URLFetcher* source) {
-  std::string response;
-  if (!source->GetResponseAsString(&response)) {
-    DVLOG(1) << "Failed to get response body.";
-    return UnregistrationRequest::NO_RESPONSE_BODY;
-  }
-
+    const std::string& response) {
   DVLOG(1) << "Parsing unregistration response.";
   if (response.find(kDeletedPrefix) != std::string::npos) {
     std::string deleted_app_id = response.substr(
@@ -54,14 +46,6 @@ UnregistrationRequest::Status GCMUnregistrationRequestHandler::ParseResponse(
         UnregistrationRequest::INCORRECT_APP_ID;
   }
 
-  if (response.find(kErrorPrefix) != std::string::npos) {
-    std::string error = response.substr(
-        response.find(kErrorPrefix) + arraysize(kErrorPrefix) - 1);
-   return error == kInvalidParameters ?
-        UnregistrationRequest::INVALID_PARAMETERS :
-        UnregistrationRequest::UNKNOWN_ERROR;
-  }
-
   DVLOG(1) << "Not able to parse a meaningful output from response body."
            << response;
   return UnregistrationRequest::RESPONSE_PARSING_FAILED;

google_apis/gcm/engine/gcm_unregistration_request_handler.h

@@ -21,7 +21,7 @@ class GCM_EXPORT GCMUnregistrationRequestHandler :
   // UnregistrationRequest::CustomRequestHandler overrides:
   void BuildRequestBody(std::string* body) override;
   UnregistrationRequest::Status ParseResponse(
-      const net::URLFetcher* source) override;
+      const std::string& response) override;
   void ReportUMAs(UnregistrationRequest::Status status,
                   int retry_count,
                   base::TimeDelta complete_time) override;

google_apis/gcm/engine/instance_id_delete_token_request_handler.cc

@@ -24,8 +24,6 @@ const char kExtraScopeKey[] = "X-scope";
 
 // Response constants.
 const char kTokenPrefix[] = "token=";
-const char kErrorPrefix[] = "Error=";
-const char kInvalidParameters[] = "INVALID_PARAMETERS";
 
 }  // namespace
 
@@ -55,21 +53,7 @@ void InstanceIDDeleteTokenRequestHandler::BuildRequestBody(std::string* body){
 
 UnregistrationRequest::Status
 InstanceIDDeleteTokenRequestHandler::ParseResponse(
-    const net::URLFetcher* source) {
-  std::string response;
-  if (!source->GetResponseAsString(&response)) {
-    DVLOG(1) << "Failed to get response body.";
-    return UnregistrationRequest::NO_RESPONSE_BODY;
-  }
-
-  if (response.find(kErrorPrefix) != std::string::npos) {
-    std::string error = response.substr(
-        response.find(kErrorPrefix) + arraysize(kErrorPrefix) - 1);
-    return error == kInvalidParameters ?
-        UnregistrationRequest::INVALID_PARAMETERS :
-        UnregistrationRequest::UNKNOWN_ERROR;
-  }
-
+    const std::string& response) {
   if (response.find(kTokenPrefix) == std::string::npos)
     return UnregistrationRequest::RESPONSE_PARSING_FAILED;
 

google_apis/gcm/engine/instance_id_delete_token_request_handler.h

@@ -26,7 +26,7 @@ class GCM_EXPORT InstanceIDDeleteTokenRequestHandler :
    // UnregistrationRequest overrides:
   void BuildRequestBody(std::string* body) override;
   UnregistrationRequest::Status ParseResponse(
-      const net::URLFetcher* source) override;
+      const std::string& response) override;
   void ReportUMAs(UnregistrationRequest::Status status,
                   int retry_count,
                   base::TimeDelta complete_time) override;

google_apis/gcm/engine/registration_request.cc

@@ -45,11 +45,12 @@ const char kDeviceRegistrationError[] = "PHONE_REGISTRATION_ERROR";
 const char kAuthenticationFailed[] = "AUTHENTICATION_FAILED";
 const char kInvalidSender[] = "INVALID_SENDER";
 const char kInvalidParameters[] = "INVALID_PARAMETERS";
+const char kInternalServerError[] = "InternalServerError";
+const char kQuotaExceeded[] = "QUOTA_EXCEEDED";
+const char kTooManyRegistrations[] = "TOO_MANY_REGISTRATIONS";
 
 // Gets correct status from the error message.
 RegistrationRequest::Status GetStatusFromError(const std::string& error) {
-  // 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.find(kAuthenticationFailed) != std::string::npos)
@@ -58,6 +59,13 @@ RegistrationRequest::Status GetStatusFromError(const std::string& error) {
     return RegistrationRequest::INVALID_SENDER;
   if (error.find(kInvalidParameters) != std::string::npos)
     return RegistrationRequest::INVALID_PARAMETERS;
+  if (error.find(kInternalServerError) != std::string::npos)
+    return RegistrationRequest::INTERNAL_SERVER_ERROR;
+  if (error.find(kQuotaExceeded) != std::string::npos)
+    return RegistrationRequest::QUOTA_EXCEEDED;
+  if (error.find(kTooManyRegistrations) != std::string::npos)
+    return RegistrationRequest::TOO_MANY_REGISTRATIONS;
+  // Should not be reached, unless the server adds new error types.
   return RegistrationRequest::UNKNOWN_ERROR;
 }
 
@@ -70,10 +78,14 @@ bool ShouldRetryWithStatus(RegistrationRequest::Status status) {
     case RegistrationRequest::URL_FETCHING_FAILED:
     case RegistrationRequest::HTTP_NOT_OK:
     case RegistrationRequest::NO_RESPONSE_BODY:
+    case RegistrationRequest::RESPONSE_PARSING_FAILED:
+    case RegistrationRequest::INTERNAL_SERVER_ERROR:
       return true;
     case RegistrationRequest::SUCCESS:
     case RegistrationRequest::INVALID_PARAMETERS:
     case RegistrationRequest::INVALID_SENDER:
+    case RegistrationRequest::QUOTA_EXCEEDED:
+    case RegistrationRequest::TOO_MANY_REGISTRATIONS:
     case RegistrationRequest::REACHED_MAX_RETRIES:
       return false;
     case RegistrationRequest::STATUS_COUNT:
@@ -199,42 +211,41 @@ void RegistrationRequest::RetryWithBackoff() {
 RegistrationRequest::Status RegistrationRequest::ParseResponse(
     const net::URLFetcher* source, std::string* token) {
   if (!source->GetStatus().is_success()) {
-    LOG(ERROR) << "URL fetching failed.";
+    DVLOG(1) << "Registration URL fetching failed.";
     return URL_FETCHING_FAILED;
   }
 
   std::string response;
   if (!source->GetResponseAsString(&response)) {
-    LOG(ERROR) << "Failed to parse registration response as a string.";
+    DVLOG(1) << "Failed to get registration response body.";
     return NO_RESPONSE_BODY;
   }
 
-  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.
+  // If we are able to parse a meaningful known error, let's do so. Note that
+  // some errors will have HTTP_OK response code!
   size_t error_pos = response.find(kErrorPrefix);
   if (error_pos != std::string::npos) {
     std::string error = response.substr(
         error_pos + arraysize(kErrorPrefix) - 1);
+    DVLOG(1) << "Registration response error message: " << error;
     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();
+    DVLOG(1) << "Registration HTTP response code not OK: "
+             << source->GetResponseCode();
     return HTTP_NOT_OK;
   }
 
-  return UNKNOWN_ERROR;
+  size_t token_pos = response.find(kTokenPrefix);
+  if (token_pos != std::string::npos) {
+    *token = response.substr(token_pos + arraysize(kTokenPrefix) - 1);
+    return SUCCESS;
+  }
+
+  return RESPONSE_PARSING_FAILED;
 }
 
 void RegistrationRequest::OnURLFetchComplete(const net::URLFetcher* source) {

google_apis/gcm/engine/registration_request.h

@@ -51,6 +51,10 @@ class GCM_EXPORT RegistrationRequest : public net::URLFetcherDelegate {
     HTTP_NOT_OK,                // HTTP status was not OK.
     NO_RESPONSE_BODY,           // No response body.
     REACHED_MAX_RETRIES,        // Reached maximum number of retries.
+    RESPONSE_PARSING_FAILED,    // Registration response parsing failed.
+    INTERNAL_SERVER_ERROR,      // Internal server error during request.
+    QUOTA_EXCEEDED,             // Registration quota exceeded.
+    TOO_MANY_REGISTRATIONS,     // Max registrations per device exceeded.
     // NOTE: always keep this entry at the end. Add new status types only
     // immediately above this line. Make sure to update the corresponding
     // histogram enum accordingly.

google_apis/gcm/engine/registration_request_unittest.cc

@@ -193,6 +193,24 @@ TEST_F(GCMRegistrationRequestTest, ResponseParsing) {
   EXPECT_EQ("2501", registration_id_);
 }
 
+TEST_F(GCMRegistrationRequestTest, ResponseParsingFailed) {
+  CreateRequest("sender1,sender2");
+  request_->Start();
+
+  SetResponse(net::HTTP_OK, "tok");  // Simulate truncated message.
+  CompleteFetch();
+
+  EXPECT_FALSE(callback_called_);
+
+  // Ensuring a retry happened and succeeds.
+  SetResponse(net::HTTP_OK, "token=2501");
+  CompleteFetch();
+
+  EXPECT_TRUE(callback_called_);
+  EXPECT_EQ(RegistrationRequest::SUCCESS, status_);
+  EXPECT_EQ("2501", registration_id_);
+}
+
 TEST_F(GCMRegistrationRequestTest, ResponseHttpStatusNotOK) {
   CreateRequest("sender1,sender2");
   request_->Start();
@@ -270,6 +288,24 @@ TEST_F(GCMRegistrationRequestTest, ResponseAuthenticationError) {
   EXPECT_EQ("2501", registration_id_);
 }
 
+TEST_F(GCMRegistrationRequestTest, ResponseInternalServerError) {
+  CreateRequest("sender1,sender2");
+  request_->Start();
+
+  SetResponse(net::HTTP_INTERNAL_SERVER_ERROR, "Error=InternalServerError");
+  CompleteFetch();
+
+  EXPECT_FALSE(callback_called_);
+
+  // Ensuring a retry happened and succeeds.
+  SetResponse(net::HTTP_OK, "token=2501");
+  CompleteFetch();
+
+  EXPECT_TRUE(callback_called_);
+  EXPECT_EQ(RegistrationRequest::SUCCESS, status_);
+  EXPECT_EQ("2501", registration_id_);
+}
+
 TEST_F(GCMRegistrationRequestTest, ResponseInvalidParameters) {
   CreateRequest("sender1,sender2");
   request_->Start();
@@ -306,6 +342,30 @@ TEST_F(GCMRegistrationRequestTest, ResponseInvalidSenderBadRequest) {
   EXPECT_EQ(std::string(), registration_id_);
 }
 
+TEST_F(GCMRegistrationRequestTest, ResponseQuotaExceeded) {
+  CreateRequest("sender1");
+  request_->Start();
+
+  SetResponse(net::HTTP_SERVICE_UNAVAILABLE, "Error=QUOTA_EXCEEDED");
+  CompleteFetch();
+
+  EXPECT_TRUE(callback_called_);
+  EXPECT_EQ(RegistrationRequest::QUOTA_EXCEEDED, status_);
+  EXPECT_EQ(std::string(), registration_id_);
+}
+
+TEST_F(GCMRegistrationRequestTest, ResponseTooManyRegistrations) {
+  CreateRequest("sender1");
+  request_->Start();
+
+  SetResponse(net::HTTP_OK, "Error=TOO_MANY_REGISTRATIONS");
+  CompleteFetch();
+
+  EXPECT_TRUE(callback_called_);
+  EXPECT_EQ(RegistrationRequest::TOO_MANY_REGISTRATIONS, status_);
+  EXPECT_EQ(std::string(), registration_id_);
+}
+
 TEST_F(GCMRegistrationRequestTest, RequestNotSuccessful) {
   CreateRequest("sender1,sender2");
   request_->Start();

google_apis/gcm/engine/unregistration_request.cc

@@ -36,6 +36,24 @@ const char kDeleteValue[] = "true";
 const char kDeviceIdKey[] = "device";
 const char kLoginHeader[] = "AidLogin";
 
+// Response constants.
+const char kErrorPrefix[] = "Error=";
+const char kInvalidParameters[] = "INVALID_PARAMETERS";
+const char kInternalServerError[] = "InternalServerError";
+const char kDeviceRegistrationError[] = "PHONE_REGISTRATION_ERROR";
+
+// Gets correct status from the error message.
+UnregistrationRequest::Status GetStatusFromError(const std::string& error) {
+  if (error.find(kInvalidParameters) != std::string::npos)
+    return UnregistrationRequest::INVALID_PARAMETERS;
+  if (error.find(kInternalServerError) != std::string::npos)
+    return UnregistrationRequest::INTERNAL_SERVER_ERROR;
+  if (error.find(kDeviceRegistrationError) != std::string::npos)
+    return UnregistrationRequest::DEVICE_REGISTRATION_ERROR;
+  // Should not be reached, unless the server adds new error types.
+  return UnregistrationRequest::UNKNOWN_ERROR;
+}
+
 // Determines whether to retry based on the status of the last request.
 bool ShouldRetryWithStatus(UnregistrationRequest::Status status) {
   switch (status) {
@@ -49,6 +67,7 @@ bool ShouldRetryWithStatus(UnregistrationRequest::Status status) {
       return true;
     case UnregistrationRequest::SUCCESS:
     case UnregistrationRequest::INVALID_PARAMETERS:
+    case UnregistrationRequest::DEVICE_REGISTRATION_ERROR:
     case UnregistrationRequest::UNKNOWN_ERROR:
     case UnregistrationRequest::REACHED_MAX_RETRIES:
       return false;
@@ -159,14 +178,29 @@ void UnregistrationRequest::BuildRequestBody(std::string* body) {
 UnregistrationRequest::Status UnregistrationRequest::ParseResponse(
     const net::URLFetcher* source) {
   if (!source->GetStatus().is_success()) {
-    DVLOG(1) << "Fetcher failed";
+    DVLOG(1) << "Unregistration URL fetching failed.";
     return URL_FETCHING_FAILED;
   }
 
+  std::string response;
+  if (!source->GetResponseAsString(&response)) {
+    DVLOG(1) << "Failed to get unregistration response body.";
+    return NO_RESPONSE_BODY;
+  }
+
+  // If we are able to parse a meaningful known error, let's do so. Note that
+  // some errors will have HTTP_OK response code!
+  if (response.find(kErrorPrefix) != std::string::npos) {
+    std::string error = response.substr(response.find(kErrorPrefix) +
+                                        arraysize(kErrorPrefix) - 1);
+    DVLOG(1) << "Unregistration response error message: " << error;
+    return GetStatusFromError(error);
+  }
+
   net::HttpStatusCode response_status = static_cast<net::HttpStatusCode>(
       source->GetResponseCode());
   if (response_status != net::HTTP_OK) {
-    DVLOG(1) << "HTTP Status code is not OK, but: " << response_status;
+    DVLOG(1) << "Unregistration HTTP response code not OK: " << response_status;
     if (response_status == net::HTTP_SERVICE_UNAVAILABLE)
       return SERVICE_UNAVAILABLE;
     if (response_status == net::HTTP_INTERNAL_SERVER_ERROR)
@@ -175,7 +209,7 @@ UnregistrationRequest::Status UnregistrationRequest::ParseResponse(
   }
 
   DCHECK(custom_request_handler_.get());
-  return custom_request_handler_->ParseResponse(source);
+  return custom_request_handler_->ParseResponse(response);
 }
 
 void UnregistrationRequest::RetryWithBackoff() {

google_apis/gcm/engine/unregistration_request.h

@@ -40,8 +40,7 @@ class GCM_EXPORT UnregistrationRequest : public net::URLFetcherDelegate {
     URL_FETCHING_FAILED,      // URL fetching failed.
     NO_RESPONSE_BODY,         // No response body.
     RESPONSE_PARSING_FAILED,  // Failed to parse a meaningful output from
-                              // response
-                              // body.
+                              // response body.
     INCORRECT_APP_ID,         // App ID returned by the fetcher does not match
                               // request.
     INVALID_PARAMETERS,       // Request parameters were invalid.
@@ -50,6 +49,7 @@ class GCM_EXPORT UnregistrationRequest : public net::URLFetcherDelegate {
     HTTP_NOT_OK,              // HTTP response code was not OK.
     UNKNOWN_ERROR,            // Unknown error.
     REACHED_MAX_RETRIES,      // Reached maximum number of retries.
+    DEVICE_REGISTRATION_ERROR,// Chrome is not properly registered.
     // NOTE: Always keep this entry at the end. Add new status types only
     // immediately above this line. Make sure to update the corresponding
     // histogram enum accordingly.
@@ -96,7 +96,7 @@ class GCM_EXPORT UnregistrationRequest : public net::URLFetcherDelegate {
 
     // Parses the HTTP response. It is called after
     // UnregistrationRequest::ParseResponse to proceed the parsing.
-    virtual Status ParseResponse(const net::URLFetcher* source) = 0;
+    virtual Status ParseResponse(const std::string& response) = 0;
 
     // Reports various UMAs, including status, retry count and completion time.
     virtual void ReportUMAs(Status status,

google_apis/gcm/engine/unregistration_request_unittest.cc

@@ -197,6 +197,17 @@ TEST_F(GCMUnregistrationRequestTest, InvalidParametersError) {
   EXPECT_EQ(UnregistrationRequest::INVALID_PARAMETERS, status_);
 }
 
+TEST_F(GCMUnregistrationRequestTest, DeviceRegistrationError) {
+  CreateRequest();
+  request_->Start();
+
+  SetResponse(net::HTTP_OK, "Error=PHONE_REGISTRATION_ERROR");
+  CompleteFetch();
+
+  EXPECT_TRUE(callback_called_);
+  EXPECT_EQ(UnregistrationRequest::DEVICE_REGISTRATION_ERROR, status_);
+}
+
 TEST_F(GCMUnregistrationRequestTest, UnkwnownError) {
   CreateRequest();
   request_->Start();

tools/metrics/histograms/histograms.xml

@@ -18570,7 +18570,12 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries.
 <histogram name="GCM.RegistrationRequestStatus"
     enum="GCMRegistrationRequestStatus">
   <owner>juyik@chromium.org</owner>
-  <summary>Status code of the outcome of a GCM registration request.</summary>
+  <summary>
+    Status code of the outcome of a GCM registration request. The Unknown error
+    case was split up in M56 (merged mid-beta to M55) to separate out the
+    Response parsing failed, Internal server error, Quota exceeded, and Device
+    has too many registrations cases.
+  </summary>
 </histogram>
 
 <histogram name="GCM.RegistrationRetryCount">
@@ -86847,6 +86852,10 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries.
   <int value="7" label="HTTP not OK"/>
   <int value="8" label="No response body"/>
   <int value="9" label="Reached maximum number of retries"/>
+  <int value="10" label="Response parsing failed"/>
+  <int value="11" label="Internal server error"/>
+  <int value="12" label="Quota exceeded"/>
+  <int value="13" label="Device has too many registrations"/>
 </enum>
 
 <enum name="GCMResetStoreError" type="int">
@@ -86876,6 +86885,7 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries.
   <int value="8" label="HTTP reponse code not OK"/>
   <int value="9" label="Unknown error"/>
   <int value="10" label="Reached maximum number of retries"/>
+  <int value="11" label="Device registration error"/>
 </enum>
 
 <enum name="GCMUpstreamMessageStatus" type="int">