Use a map to track pending requests sent to Java. Use key as callback_id.

Previously:
Use the address of the URLCheckCallbackMeta object as a callback_id.
This id was is to track the pending requests in Java. When a response
is received, reinterpret the callback ID as an address to the
URLCheckCallbackMeta object and then call Run on the callback object.
This caused a race condition if the request timed out in Java and the
object address got re-used. See: crbug/889972#c44

Now:
Maintain a map of pending requests sent to Java in a map
<callback_id (int), URLCheckCallbackMeta>. When a response is received,
look up the map on IO thread to get the callback object to respond to.
The callback_id is based off an incrementing counter,
not derived from the address of the callback object.

Bug: 893110,889972
Change-Id: I503379ecb1e3f52fc31c272386b1d2f1b065586c
Reviewed-on: https://chromium-review.googlesource.com/c/1359308
Commit-Queue: Varun Khaneja <vakh@chromium.org>
Reviewed-by: Daniel Rubery <drubery@chromium.org>
Reviewed-by: Nate Fischer <ntfschr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#613396}

components/safe_browsing/android/safe_browsing_api_handler_bridge.cc

@@ -32,6 +32,7 @@ using content::BrowserThread;
 namespace safe_browsing {
 
 namespace {
+
 void RunCallbackOnIOThread(
     std::unique_ptr<SafeBrowsingApiHandler::URLCheckCallbackMeta> callback,
     SBThreatType threat_type,
@@ -80,6 +81,24 @@ ScopedJavaLocalRef<jintArray> SBThreatTypeSetToJavaArray(
   return ToJavaIntArray(env, int_threat_types, threat_types.size());
 }
 
+// The map that holds the callback_id used to reference each pending request
+// sent to Java, and the corresponding callback to call on receiving the
+// response.
+typedef std::unordered_map<
+    jlong,
+    std::unique_ptr<SafeBrowsingApiHandler::URLCheckCallbackMeta>>
+    PendingCallbacksMap;
+
+static PendingCallbacksMap* GetPendingCallbacksMapOnIOThread() {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
+  // Holds the list of callback objects that we are currently waiting to hear
+  // the result of from GmsCore.
+  // The key is a unique count-up integer.
+  static PendingCallbacksMap pending_callbacks;
+  return &pending_callbacks;
+}
+
 }  // namespace
 
 // Java->Native call, to check whether the feature to use local blacklists is
@@ -88,82 +107,103 @@ jboolean JNI_SafeBrowsingApiBridge_AreLocalBlacklistsEnabled(JNIEnv* env) {
   return base::FeatureList::IsEnabled(kUseLocalBlacklistsV2);
 }
 
-// Java->Native call, invoked when a check is done.
+// Respond to the URL reputation request by looking up the callback information
+// stored in |pending_callbacks|.
 //   |callback_id| is an int form of pointer to a URLCheckCallbackMeta
 //                 that will be called and then deleted here.
 //   |result_status| is one of those from SafeBrowsingApiHandler.java
 //   |metadata| is a JSON string classifying the threat if there is one.
-//
-//   Careful note: this can be called on multiple threads, so make sure there is
-//   nothing thread unsafe happening here.
-void JNI_SafeBrowsingApiBridge_OnUrlCheckDone(
-    JNIEnv* env,
-    jlong callback_id,
-    jint result_status,
-    const JavaParamRef<jstring>& metadata,
-    jlong check_delta) {
-  DCHECK(callback_id);
-  UMA_HISTOGRAM_COUNTS_10M("SB2.RemoteCall.CheckDelta", check_delta);
-
-  const std::string metadata_str =
-      (metadata ? ConvertJavaStringToUTF8(env, metadata) : "");
-
-  TRACE_EVENT1("safe_browsing", "SafeBrowsingApiHandlerBridge::OnUrlCheckDone",
-               "metadata", metadata_str);
+void OnUrlCheckDoneOnIOThread(jlong callback_id,
+                              jint result_status,
+                              const std::string metadata) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  DVLOG(1) << __FUNCTION__ << ": check: " << callback_id
+           << " status: " << result_status << " metadata: [" << metadata << "]";
 
-  DVLOG(1) << "OnURLCheckDone invoked for check " << callback_id
-           << " with status=" << result_status << " and metadata=["
-           << metadata_str << "]";
+  PendingCallbacksMap* pending_callbacks = GetPendingCallbacksMapOnIOThread();
+  bool found = base::ContainsKey(*pending_callbacks, callback_id);
+  DCHECK(found) << "Not found in pending_callbacks: " << callback_id;
+  if (!found)
+    return;
 
-  // Convert java long long int to c++ pointer, take ownership.
-  std::unique_ptr<SafeBrowsingApiHandler::URLCheckCallbackMeta> callback(
-      reinterpret_cast<SafeBrowsingApiHandlerBridge::URLCheckCallbackMeta*>(
-          callback_id));
+  std::unique_ptr<SafeBrowsingApiHandler::URLCheckCallbackMeta> callback =
+      std::move((*pending_callbacks)[callback_id]);
+  CHECK(callback);  // Remove after fixing crbug.com/889972
+  pending_callbacks->erase(callback_id);
 
   if (result_status != RESULT_STATUS_SUCCESS) {
     if (result_status == RESULT_STATUS_TIMEOUT) {
-      CHECK(callback);              // Remove after fixing crbug.com/889972
       CHECK(!callback->is_null());  // Remove after fixing crbug.com/889972
 
       ReportUmaResult(UMA_STATUS_TIMEOUT);
-      VLOG(1) << "Safe browsing API call timed-out";
+      DVLOG(1) << "Safe browsing API call timed-out";
     } else {
-      CHECK(callback);              // Remove after fixing crbug.com/889972
       CHECK(!callback->is_null());  // Remove after fixing crbug.com/889972
 
       DCHECK_EQ(result_status, RESULT_STATUS_INTERNAL_ERROR);
       ReportUmaResult(UMA_STATUS_INTERNAL_ERROR);
     }
-    RunCallbackOnIOThread(std::move(callback), SB_THREAT_TYPE_SAFE,
-                          ThreatMetadata());
+    std::move(*callback).Run(SB_THREAT_TYPE_SAFE, ThreatMetadata());
     return;
   }
 
   // Shortcut for safe, so we don't have to parse JSON.
-  if (metadata_str == "{}") {
-    CHECK(callback);              // Remove after fixing crbug.com/889972
+  if (metadata == "{}") {
     CHECK(!callback->is_null());  // Remove after fixing crbug.com/889972
 
     ReportUmaResult(UMA_STATUS_SAFE);
-    RunCallbackOnIOThread(std::move(callback), SB_THREAT_TYPE_SAFE,
-                          ThreatMetadata());
+    std::move(*callback).Run(SB_THREAT_TYPE_SAFE, ThreatMetadata());
   } else {
-    CHECK(callback);              // Remove after fixing crbug.com/889972
     CHECK(!callback->is_null());  // Remove after fixing crbug.com/889972
 
     // Unsafe, assuming we can parse the JSON.
     SBThreatType worst_threat;
     ThreatMetadata threat_metadata;
     ReportUmaResult(
-        ParseJsonFromGMSCore(metadata_str, &worst_threat, &threat_metadata));
+        ParseJsonFromGMSCore(metadata, &worst_threat, &threat_metadata));
     if (worst_threat != SB_THREAT_TYPE_SAFE) {
       DVLOG(1) << "Check " << callback_id << " was a MATCH";
     }
 
-    RunCallbackOnIOThread(std::move(callback), worst_threat, threat_metadata);
+    std::move(*callback).Run(worst_threat, threat_metadata);
   }
 }
 
+// Java->Native call, invoked when a check is done.
+//   |callback_id| is a key into the |pending_callbacks_| map, whose value is a
+//                 URLCheckCallbackMeta that will be called and then deleted on
+//                 the IO thread.
+//   |result_status| is one of those from SafeBrowsingApiHandler.java
+//   |metadata| is a JSON string classifying the threat if there is one.
+//   |check_delta| is the number of microseconds it took to look up the URL
+//                 reputation from GmsCore.
+//
+//   Careful note: this can be called on multiple threads, so make sure there is
+//   nothing thread unsafe happening here.
+void JNI_SafeBrowsingApiBridge_OnUrlCheckDone(
+    JNIEnv* env,
+    jlong callback_id,
+    jint result_status,
+    const JavaParamRef<jstring>& metadata,
+    jlong check_delta) {
+  UMA_HISTOGRAM_COUNTS_10M("SB2.RemoteCall.CheckDelta", check_delta);
+
+  const std::string metadata_str =
+      (metadata ? ConvertJavaStringToUTF8(env, metadata) : "");
+
+  TRACE_EVENT1("safe_browsing", "SafeBrowsingApiHandlerBridge::OnUrlCheckDone",
+               "metadata", metadata_str);
+
+  DVLOG(1) << "OnURLCheckDone invoked for check " << callback_id
+           << " with status=" << result_status << " and metadata=["
+           << metadata_str << "]";
+
+  base::PostTaskWithTraits(
+      FROM_HERE, {BrowserThread::IO},
+      base::BindOnce(&OnUrlCheckDoneOnIOThread, callback_id, result_status,
+                     metadata_str));
+}
+
 //
 // SafeBrowsingApiHandlerBridge
 //
@@ -187,6 +227,7 @@ void SafeBrowsingApiHandlerBridge::StartURLCheck(
     std::unique_ptr<SafeBrowsingApiHandler::URLCheckCallbackMeta> callback,
     const GURL& url,
     const SBThreatTypeSet& threat_types) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
   if (!CheckApiIsSupported()) {
     // Mark all requests as safe. Only users who have an old, broken GMSCore or
     // have sideloaded Chrome w/o PlayStore should land here.
@@ -196,10 +237,9 @@ void SafeBrowsingApiHandlerBridge::StartURLCheck(
     return;
   }
 
-  // Save the address on the heap so we can pass it through JNI. The unique ptr
-  // releases ownership, we will re-own this callback when the response is
-  // received in JNI_SafeBrowsingApiBridge_OnUrlCheckDone.
-  intptr_t callback_id = reinterpret_cast<intptr_t>(callback.release());
+  jlong callback_id = next_callback_id_++;
+  GetPendingCallbacksMapOnIOThread()->insert(
+      {callback_id, std::move(callback)});
   DVLOG(1) << "Starting check " << callback_id << " for URL " << url;
 
   DCHECK(!threat_types.empty());

components/safe_browsing/android/safe_browsing_api_handler_bridge.h

@@ -19,6 +19,7 @@
 #include "url/gurl.h"
 
 namespace safe_browsing {
+
 class SafeBrowsingApiHandlerBridge : public SafeBrowsingApiHandler {
  public:
   SafeBrowsingApiHandlerBridge();
@@ -40,6 +41,10 @@ class SafeBrowsingApiHandlerBridge : public SafeBrowsingApiHandler {
   // True if we've once tried to create the above object.
   bool checked_api_support_;
 
+  // Used as a key to identify unique requests sent to Java to get Safe Browsing
+  // reputation from GmsCore.
+  jlong next_callback_id_ = 0;
+
   DISALLOW_COPY_AND_ASSIGN(SafeBrowsingApiHandlerBridge);
 };