Make InstanceIDBridge fully async to fix strict mode violations

components/gcm_driver/instance_id/instance_id_android.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/gcm_driver/instance_id/instance_id_android.h"
 
 #include <stdint.h>
 
+#include <memory>
+
 #include "base/android/context_utils.h"
 #include "base/android/jni_android.h"
 #include "base/android/jni_array.h"
 #include "base/android/jni_string.h"
 #include "base/bind.h"
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/thread_task_runner_handle.h"
 #include "base/time/time.h"
@@ skipping 49 matching lines @@
 InstanceIDAndroid::~InstanceIDAndroid() {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   JNIEnv* env = AttachCurrentThread();
   Java_InstanceIDBridge_destroy(env, java_ref_.obj());
 }
 
 void InstanceIDAndroid::GetID(const GetIDCallback& callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
+  int32_t request_id = get_id_callbacks_.Add(new GetIDCallback(callback));
+
   JNIEnv* env = AttachCurrentThread();
-  std::string id = ConvertJavaStringToUTF8(
-      Java_InstanceIDBridge_getId(env, java_ref_.obj()));
-  base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE,
-                                                base::Bind(callback, id));
+  Java_InstanceIDBridge_getId(env, java_ref_.obj(), request_id);
 }
 
 void InstanceIDAndroid::GetCreationTime(
     const GetCreationTimeCallback& callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
+  int32_t request_id =
+      get_creation_time_callbacks_.Add(new GetCreationTimeCallback(callback));
+
   JNIEnv* env = AttachCurrentThread();
-  int64_t creation_time_unix_ms =
-      Java_InstanceIDBridge_getCreationTime(env, java_ref_.obj());
-  base::Time creation_time;
-  // If the InstanceID's getId, getToken and deleteToken methods have never been
-  // called, or deleteInstanceID has cleared it since, creation time will be 0.
-  if (creation_time_unix_ms) {
-    creation_time = base::Time::UnixEpoch() +
-                    base::TimeDelta::FromMilliseconds(creation_time_unix_ms);
-  }
-  base::ThreadTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE, base::Bind(callback, creation_time));
+  Java_InstanceIDBridge_getCreationTime(env, java_ref_.obj(), request_id);
 }
 
 void InstanceIDAndroid::GetToken(
     const std::string& authorized_entity,
     const std::string& scope,
     const std::map<std::string, std::string>& options,
     const GetTokenCallback& callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   int32_t request_id = get_token_callbacks_.Add(new GetTokenCallback(callback));
@@ skipping 29 matching lines @@
 
 void InstanceIDAndroid::DeleteID(const DeleteIDCallback& callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   int32_t request_id = delete_id_callbacks_.Add(new DeleteIDCallback(callback));
 
   JNIEnv* env = AttachCurrentThread();
   Java_InstanceIDBridge_deleteInstanceID(env, java_ref_.obj(), request_id);
 }
 
+void InstanceIDAndroid::DidGetID(
+    JNIEnv* env,
+    const base::android::JavaParamRef<jobject>& obj,
+    jint request_id,
+    const base::android::JavaParamRef<jstring>& jid) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  GetIDCallback* callback = get_id_callbacks_.Lookup(request_id);
+  DCHECK(callback);
+  callback->Run(ConvertJavaStringToUTF8(jid));
+  get_id_callbacks_.Remove(request_id);
+}
+
+void InstanceIDAndroid::DidGetCreationTime(
+    JNIEnv* env,
+    const base::android::JavaParamRef<jobject>& obj,
+    jint request_id,
+    jlong creation_time_unix_ms) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  base::Time creation_time;
+  // If the InstanceID's getId, getToken and deleteToken methods have never been
+  // called, or deleteInstanceID has cleared it since, creation time will be 0.
+  if (creation_time_unix_ms) {
+    creation_time = base::Time::UnixEpoch() +
+                    base::TimeDelta::FromMilliseconds(creation_time_unix_ms);
+  }
+
+  GetCreationTimeCallback* callback =
+      get_creation_time_callbacks_.Lookup(request_id);
+  DCHECK(callback);
+  callback->Run(creation_time);
+  get_creation_time_callbacks_.Remove(request_id);
+}
+
 void InstanceIDAndroid::DidGetToken(
     JNIEnv* env,
     const base::android::JavaParamRef<jobject>& obj,
     jint request_id,
     const base::android::JavaParamRef<jstring>& jtoken) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   GetTokenCallback* callback = get_token_callbacks_.Lookup(request_id);
   DCHECK(callback);
   std::string token = ConvertJavaStringToUTF8(jtoken);
@@ skipping 15 matching lines @@
   delete_token_callbacks_.Remove(request_id);
 }
 
 void InstanceIDAndroid::DidDeleteID(
     JNIEnv* env,
     const base::android::JavaParamRef<jobject>& obj,
     jint request_id,
     jboolean success) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
-  DeleteIDCallback* callback = delete_id_callbacks_.Lookup(request_id);
+  // Must remove the callback from the IDMap before running it, since running it
+  // usually deletes this and hence the IDMap (but isn't guaranteed to either).
+  DeleteTokenCallback* callback = delete_id_callbacks_.Lookup(request_id);

[Peter Beverloo, 2016/04/26 15:12:23]

Where does this happen? The three calls to InstanceID::DeleteID() all look fine
to me. 

The InstanceID instances are owned by the InstanceIDDriver, so this would only
occur when someone deletes their ID and then, in the callback handler, calls
InstanceIDDriver::RemoveInstanceID. The only candidate is
ExtensionGCMAppHandler, which does this through a PostTask().

In general, this problem is not unique to DidDeleteID, but would apply to any of
the asynchronous methods in this implementation.

Instead, could we add a class-level comment that the callbacks should *not*
synchronously remove the instances and leave it at that?

[johnme, 2016/04/26 17:18:27]

The pattern set by the only existing user of IIDs (ExtensionGCMAppHandler), is
that you call InstanceIDDriver::RemoveInstanceID after InstanceID::DeleteID
completes (this is a bit of a silly pattern, but refactoring it is a task for
another day). Hence I do the same in
https://codereview.chromium.org/1851423003/diff/80001/chrome/browser/push_mes...,
though I happened to omit the PostTask, and that seems an easy mistake to make
(even with a class comment).

So I'd rather be defensive here. An alternative would be to run the DeleteID
callback using PostTask, if you think that's better.

I'm less concerned about the other methods, since there's no strong reason to
call RemoveInstanceID from their callbacks, though I'll add a class comment. I
also toned down the comments here from usually/likely to maybe.

   DCHECK(callback);
-  callback->Run(success ? InstanceID::SUCCESS : InstanceID::UNKNOWN_ERROR);
+  DeleteIDCallback callback_copy = *callback;
   delete_id_callbacks_.Remove(request_id);
+  // Now |callback| has been deleted.
+  callback_copy.Run(success ? InstanceID::SUCCESS : InstanceID::UNKNOWN_ERROR);
+  // Now |this| will likely have been deleted.
 }
 
 }  // namespace instance_id