Avoid loading an empty token when decrypt failed

components/signin/core/browser/webdata/token_service_table.cc

Index: components/signin/core/browser/webdata/token_service_table.cc
diff --git a/components/signin/core/browser/webdata/token_service_table.cc b/components/signin/core/browser/webdata/token_service_table.cc
index d1ccb08da294425c9bd1aa0e34cd5f46088ef0bb..f8f2be502006e53691113bdf4daac568d2fd6851 100644
--- a/components/signin/core/browser/webdata/token_service_table.cc
+++ b/components/signin/core/browser/webdata/token_service_table.cc
@@ -8,6 +8,7 @@
 #include <string>
 
 #include "base/logging.h"
+#include "base/metrics/histogram_macros.h"
 #include "components/os_crypt/os_crypt.h"
 #include "components/webdata/common/web_database.h"
 #include "sql/statement.h"
@@ -21,6 +22,14 @@ WebDatabaseTable::TypeKey GetKey() {
   return reinterpret_cast<void*>(&table_key);
 }
 
+// Entries in the |Signin.TokenTable.ReadTokenFromDB| histogram.
+enum ReadOneTokenEnum {
+  READ_ONE_TOKEN_SUCCESS,
+  READ_ONE_TOKEN_DB_SUCCESS_DECRYPT_FAILED,
+  READ_ONE_TOKEN_DB_FAILED_BAD_ENTRY,
+  READ_ONE_TOKEN_MAX_VALUE
+};
+
 }  // namespace
 
 TokenServiceTable* TokenServiceTable::FromWebDatabase(WebDatabase* db) {
@@ -93,8 +102,13 @@ bool TokenServiceTable::GetAllTokens(
   sql::Statement s(db_->GetUniqueStatement(
       "SELECT service, encrypted_token FROM token_service"));
 
-  if (!s.is_valid())
+  UMA_HISTOGRAM_BOOLEAN("Signin.TokenTable.GetAllTokensSqlStatement",
+                        s.is_valid());
+
+  if (!s.is_valid()) {
+    LOG(ERROR) << "Failed to load tokens (invalid SQL statement).";
     return false;
+  }
 
   while (s.Step()) {
     std::string encrypted_token;
@@ -104,10 +118,24 @@ bool TokenServiceTable::GetAllTokens(
     bool entry_ok = !service.empty() &&
                     s.ColumnBlobAsString(1, &encrypted_token);
     if (entry_ok) {
-      OSCrypt::DecryptString(encrypted_token, &decrypted_token);
-      (*tokens)[service] = decrypted_token;
+      if (OSCrypt::DecryptString(encrypted_token, &decrypted_token)) {
+        (*tokens)[service] = decrypted_token;
+        UMA_HISTOGRAM_ENUMERATION("Signin.TokenTable.ReadTokenFromDB",
+                                  READ_ONE_TOKEN_SUCCESS,
+                                  READ_ONE_TOKEN_MAX_VALUE);
+      } else {
+        // Chrome relies on native APIs to encrypt and decrypt the tokens which
+        // may fail (see http://crbug.com/686485).
+        LOG(ERROR) << "Failed to decrypt token for service " << service;
+        UMA_HISTOGRAM_ENUMERATION("Signin.TokenTable.ReadTokenFromDB",
+                                  READ_ONE_TOKEN_DB_SUCCESS_DECRYPT_FAILED,
+                                  READ_ONE_TOKEN_MAX_VALUE);
+      }
     } else {
-      NOTREACHED();
+      LOG(ERROR) << "Bad token entry for service " << service;
+      UMA_HISTOGRAM_ENUMERATION("Signin.TokenTable.ReadTokenFromDB",
+                                READ_ONE_TOKEN_DB_FAILED_BAD_ENTRY,
+                                READ_ONE_TOKEN_MAX_VALUE);

[Alexei Svitkine (slow), 2017/02/06 15:35:51]

Please refactor the code so that this macro only appears once for this
histogram.

Either make a helper function or use a local var here and log it below.

Each macro expands to a bunch of code, so extra macros for the same histogram
cause unnecessary bloat.

[msarda, 2017/02/06 16:59:27]

Done.

       return false;
     }
   }