Make InstanceIDBridge fully async to fix strict mode violations

components/gcm_driver/instance_id/instance_id_android.cc

Index: components/gcm_driver/instance_id/instance_id_android.cc
diff --git a/components/gcm_driver/instance_id/instance_id_android.cc b/components/gcm_driver/instance_id/instance_id_android.cc
index 5d8de6dc6cbe695e9a444ea603797a498e60a3dd..82230dbcc8fb8c3d54f8f34dd04d7500c882330a 100644
--- a/components/gcm_driver/instance_id/instance_id_android.cc
+++ b/components/gcm_driver/instance_id/instance_id_android.cc
@@ -6,6 +6,8 @@
 
 #include <stdint.h>
 
+#include <memory>
+
 #include "base/android/context_utils.h"
 #include "base/android/jni_android.h"
 #include "base/android/jni_array.h"
@@ -75,29 +77,21 @@ InstanceIDAndroid::~InstanceIDAndroid() {
 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(
@@ -147,6 +141,41 @@ void InstanceIDAndroid::DeleteID(const DeleteIDCallback& callback) {
   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,
@@ -182,10 +211,15 @@ void InstanceIDAndroid::DidDeleteID(
     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