Fix crash in GCM if multiple errors reported

In subtle cases like state CONNECTING being entered via
MCSClient::ResetStateAndBuildLoginRequest(), it is possible that the
error callback gets triggered twice.

With OnceCallback() being adopted recently in
https://chromium-review.googlesource.com/c/chromium/src/+/1939891, this
leads to a crash, and surfaced by flakiness in sync integration tests.

Bug: 1039598
Change-Id: Iad23dfe81e07f11939a6640fb792a7ae1386c397
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1991434
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Auto-Submit: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Dmitry Titov <dimich@chromium.org>
Cr-Commit-Position: refs/heads/master@{#729799}

google_apis/gcm/engine/mcs_client.cc

@@ -187,14 +187,14 @@ MCSClient::~MCSClient() {
 }
 
 void MCSClient::Initialize(
-    ErrorCallback error_callback,
+    const ErrorCallback& error_callback,
     const OnMessageReceivedCallback& message_received_callback,
     const OnMessageSentCallback& message_sent_callback,
     std::unique_ptr<GCMStore::LoadResult> load_result) {
   DCHECK_EQ(state_, UNINITIALIZED);
 
   state_ = LOADED;
-  mcs_error_callback_ = std::move(error_callback);
+  mcs_error_callback_ = error_callback;
   message_received_callback_ = message_received_callback;
   message_sent_callback_ = message_sent_callback;
 
@@ -238,7 +238,7 @@ void MCSClient::Initialize(
     if (!base::StringToUint64(iter->first, &timestamp)) {
       LOG(ERROR) << "Invalid restored message.";
       // TODO(fgorski): Error: data unreadable
-      std::move(mcs_error_callback_).Run();
+      mcs_error_callback_.Run();
       return;
     }
 
@@ -708,7 +708,7 @@ void MCSClient::HandlePacketFromWire(
         LOG(ERROR) << "Failed to log in to GCM, resetting connection.";
         connection_factory_->SignalConnectionReset(
             ConnectionFactory::LOGIN_FAILURE);
-        std::move(mcs_error_callback_).Run();
+        mcs_error_callback_.Run();
         return;
       }
 

google_apis/gcm/engine/mcs_client.h

@@ -84,10 +84,12 @@ class GCM_EXPORT MCSClient {
     SEND_STATUS_COUNT
   };
 
-  // Callback for MCSClient's error conditions.
+  // Callback for MCSClient's error conditions. A repeating callback is used
+  // because occasionally multiple errors are reported, see crbug.com/1039598
+  // for more context.
   // TODO(fgorski): Keeping it as a callback with intention to add meaningful
   // error information.
-  using ErrorCallback = base::OnceCallback<void()>;
+  using ErrorCallback = base::RepeatingClosure;
   // Callback when a message is received.
   using OnMessageReceivedCallback =
       base::RepeatingCallback<void(const MCSMessage& message)>;
@@ -116,7 +118,7 @@ class GCM_EXPORT MCSClient {
   // |success == true|.
   // If an error loading the GCM store is encountered,
   // |initialization_callback| will be invoked with |success == false|.
-  void Initialize(ErrorCallback initialization_callback,
+  void Initialize(const ErrorCallback& initialization_callback,
                   const OnMessageReceivedCallback& message_received_callback,
                   const OnMessageSentCallback& message_sent_callback,
                   std::unique_ptr<GCMStore::LoadResult> load_result);

google_apis/gcm/tools/mcs_probe.cc

@@ -319,7 +319,7 @@ void MCSProbe::LoadCallback(std::unique_ptr<GCMStore::LoadResult> load_result) {
     DVLOG(1) << "Loaded MCS id " << android_id_;
   }
   mcs_client_->Initialize(
-      base::BindOnce(&MCSProbe::ErrorCallback, base::Unretained(this)),
+      base::BindRepeating(&MCSProbe::ErrorCallback, base::Unretained(this)),
       base::BindRepeating(&MessageReceivedCallback),
       base::BindRepeating(&MessageSentCallback), std::move(load_result));