[safe_browsing] Dispatch SafetyNet API request on a worker thread.

Recent data shows that the time it takes to dispatch the IPC to
Google Play Services is quite expensive.

Android canary data (so probably trends higher end), for the
subresource filter check (in navigation critical path):
50th percentile: 6ms
75th percentile: 10ms
95th percentile: 28ms
99th percentile: 100ms
Average: 11ms

Since this is directly in the critical path of navigation, it makes
sense not to hog the thread / deschedule it before the network request
goes out. It also looks like we hit some perf issues in other uses of
the API too. See the linked bug for some chrome://tracing traces.

Bug: 779914
Change-Id: Ibc44d9f199715c3fbf7a2d66604e82ee3ccfc5e4
Reviewed-on: https://chromium-review.googlesource.com/760550
Commit-Queue: Charlie Harrison <csharrison@chromium.org>
Reviewed-by: Varun Khaneja <vakh@chromium.org>
Reviewed-by: François Doray <fdoray@chromium.org>
Cr-Commit-Position: refs/heads/master@{#517389}

components/safe_browsing/android/java/src/org/chromium/components/safe_browsing/SafeBrowsingApiBridge.java

@@ -32,7 +32,6 @@ public final class SafeBrowsingApiBridge {
 
     /**
      * Create a SafeBrowsingApiHandler obj and initialize its client, if supported.
-     * Should be called on IO thread.
      *
      * @return the handler if it's usable, or null if the API is not supported.
      */
@@ -55,6 +54,9 @@ public final class SafeBrowsingApiBridge {
         return initSuccesssful ? handler : null;
     }
 
+    /**
+     * Starts a Safe Browsing check. Must be called on the same sequence as |create|.
+     */
     @CalledByNative
     private static void startUriLookup(
             SafeBrowsingApiHandler handler, long callbackId, String uri, int[] threatsOfInterest) {

components/safe_browsing/android/java/src/org/chromium/components/safe_browsing/SafeBrowsingApiHandler.java

@@ -34,7 +34,7 @@ public interface SafeBrowsingApiHandler {
 
     /**
      * Verifies that SafeBrowsingApiHandler can operate and initializes if feasible.
-     * Should be called on IO thread.
+     * Should be called on the same sequence as |startUriLookup|.
      *
      * @return the handler if it's usable, or null if the API is not supported.
      */
@@ -42,7 +42,7 @@ public interface SafeBrowsingApiHandler {
 
     /**
      * Start a URI-lookup to determine if it matches one of the specified threats.
-     * This is called on every URL resource Chrome loads, on the IO thread.
+     * This is called on every URL resource Chrome loads, on the same sequence as |init|.
      */
     public void startUriLookup(long callbackId, String uri, int[] threatsOfInterest);
 }

components/safe_browsing/android/safe_browsing_api_handler_bridge.cc

@@ -9,8 +9,14 @@
 
 #include "base/android/jni_array.h"
 #include "base/android/jni_string.h"
+#include "base/bind.h"
+#include "base/bind_helpers.h"
 #include "base/containers/flat_set.h"
+#include "base/feature_list.h"
 #include "base/metrics/histogram_macros.h"
+#include "base/task_scheduler/post_task.h"
+#include "base/task_scheduler/task_traits.h"
+#include "base/trace_event/trace_event.h"
 #include "components/safe_browsing/android/safe_browsing_api_handler_util.h"
 #include "components/safe_browsing/db/v4_protocol_manager_util.h"
 #include "content/public/browser/browser_thread.h"
@@ -26,6 +32,9 @@ using content::BrowserThread;
 
 namespace safe_browsing {
 
+const base::Feature kDispatchSafetyNetCheckOffThread{
+    "DispatchSafetyNetCheckOffThread", base::FEATURE_DISABLED_BY_DEFAULT};
+
 namespace {
 void RunCallbackOnIOThread(
     const SafeBrowsingApiHandler::URLCheckCallbackMeta& callback,
@@ -84,6 +93,7 @@ void OnUrlCheckDone(JNIEnv* env,
                     const JavaParamRef<jstring>& metadata) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK(callback_id);
+  TRACE_EVENT0("safe_browsing", "SafeBrowsingApiHandlerBridge::OnUrlCheckDone");
 
   const std::string metadata_str =
       (metadata ? ConvertJavaStringToUTF8(env, metadata) : "");
@@ -130,27 +140,78 @@ void OnUrlCheckDone(JNIEnv* env,
 //
 // SafeBrowsingApiHandlerBridge
 //
-SafeBrowsingApiHandlerBridge::SafeBrowsingApiHandlerBridge()
-    : checked_api_support_(false) {}
+SafeBrowsingApiHandlerBridge::SafeBrowsingApiHandlerBridge() {}
+
+SafeBrowsingApiHandlerBridge::~SafeBrowsingApiHandlerBridge() {
+  if (api_task_runner_)
+    api_task_runner_->DeleteSoon(FROM_HERE, core_.release());
+}
 
-SafeBrowsingApiHandlerBridge::~SafeBrowsingApiHandlerBridge() {}
+void SafeBrowsingApiHandlerBridge::Initialize() {
+  DCHECK(!core_);
+  core_ = std::make_unique<Core>();
+  if (base::FeatureList::IsEnabled(kDispatchSafetyNetCheckOffThread)) {
+    api_task_runner_ = base::CreateSequencedTaskRunnerWithTraits(
+        {base::MayBlock(), base::TaskPriority::USER_VISIBLE});
+  }
+}
 
-bool SafeBrowsingApiHandlerBridge::CheckApiIsSupported() {
+void SafeBrowsingApiHandlerBridge::StartURLCheck(
+    const SafeBrowsingApiHandler::URLCheckCallbackMeta& callback,
+    const GURL& url,
+    const SBThreatTypeSet& threat_types) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  // Initialize on the first URL check, when the feature list API is ready to be
+  // used.
+  if (!core_)
+    Initialize();
+
+  // Note: it turns out in practice that dispatching the IPC to Google Play
+  // Services can be quite expensive in terms of wall time, often due to thread
+  // descheduling. Since this task runs in an extremely performance critical
+  // place (it blocks navigation and subresource requests), dispatch it on a
+  // worker thread. In high percentiles it seems like the dispatching can take
+  // >100ms, so use base::MayBlock even though we aren't technically doing
+  // blocking IO.
+  if (!api_task_runner_) {
+    core_->StartURLCheck(callback, url, threat_types);
+    return;
+  }
+  // Unretained is safe because the task to delete |core_| will be sequenced
+  // after any task posted here.
+  api_task_runner_->PostTask(
+      FROM_HERE,
+      base::BindOnce(&SafeBrowsingApiHandlerBridge::Core::StartURLCheck,
+                     base::Unretained(core_.get()), callback, url,
+                     threat_types));
+}
+
+SafeBrowsingApiHandlerBridge::Core::Core() {
+  // The sequence checker is constructed on a different sequence from where it
+  // is used.
+  DETACH_FROM_SEQUENCE(sequence_checker_);
+}
+
+SafeBrowsingApiHandlerBridge::Core::~Core() = default;
+
+bool SafeBrowsingApiHandlerBridge::Core::CheckApiIsSupported() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   if (!checked_api_support_) {
     DVLOG(1) << "Checking API support.";
-    j_api_handler_ = Java_SafeBrowsingApiBridge_create(AttachCurrentThread());
+    j_api_handler_ = base::android::ScopedJavaGlobalRef<jobject>(
+        Java_SafeBrowsingApiBridge_create(AttachCurrentThread()));
     checked_api_support_ = true;
   }
   return j_api_handler_.obj() != nullptr;
 }
 
-void SafeBrowsingApiHandlerBridge::StartURLCheck(
-    const SafeBrowsingApiHandler::URLCheckCallbackMeta& callback,
+void SafeBrowsingApiHandlerBridge::Core::StartURLCheck(
+    const URLCheckCallbackMeta& callback,
     const GURL& url,
     const SBThreatTypeSet& threat_types) {
-  DCHECK_CURRENTLY_ON(BrowserThread::IO);
-
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  TRACE_EVENT0("safe_browsing",
+               "SafeBrowsingApiHandlerBridge::StartURLCheckAsync");
   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.

components/safe_browsing/android/safe_browsing_api_handler_bridge.h

@@ -9,11 +9,15 @@
 
 #include <jni.h>
 
+#include <memory>
 #include <string>
 #include <vector>
 
 #include "base/android/jni_android.h"
 #include "base/macros.h"
+#include "base/memory/ref_counted.h"
+#include "base/sequence_checker.h"
+#include "base/sequenced_task_runner.h"
 #include "components/safe_browsing/android/safe_browsing_api_handler.h"
 #include "components/safe_browsing/db/v4_protocol_manager_util.h"
 #include "url/gurl.h"
@@ -30,15 +34,42 @@ class SafeBrowsingApiHandlerBridge : public SafeBrowsingApiHandler {
                      const SBThreatTypeSet& threat_types) override;
 
  private:
-  // Creates the j_api_handler_ if it hasn't been already.  If the API is not
-  // supported, this will return false and j_api_handler_ will remain NULL.
-  bool CheckApiIsSupported();
+  void Initialize();
 
-  // The Java-side SafeBrowsingApiHandler. Must call CheckApiIsSupported first.
-  base::android::ScopedJavaLocalRef<jobject> j_api_handler_;
+  // Responsible for calling into Java from a separate sequence than the
+  // SafeBrowsingApiHandlerBridge.
+  class Core {
+   public:
+    Core();
+    ~Core();
 
-  // True if we've once tried to create the above object.
-  bool checked_api_support_;
+    void StartURLCheck(const URLCheckCallbackMeta& callback,
+                       const GURL& url,
+                       const SBThreatTypeSet& threat_types);
+
+   private:
+    // Creates the j_api_handler_ if it hasn't been already.  If the API is not
+    // supported, this will return false and j_api_handler_ will remain NULL.
+    bool CheckApiIsSupported();
+
+    // The Java-side SafeBrowsingApiHandler. Must call CheckApiIsSupported
+    // first.
+    base::android::ScopedJavaGlobalRef<jobject> j_api_handler_;
+
+    // True if we've once tried to create the above object.
+    bool checked_api_support_ = false;
+
+    SEQUENCE_CHECKER(sequence_checker_);
+    DISALLOW_COPY_AND_ASSIGN(Core);
+  };
+
+  // nullptr if |this| and |core_| should live on the same sequence. Otherwise
+  // it will be the sequence that |core_| lives on.
+  scoped_refptr<base::SequencedTaskRunner> api_task_runner_;
+
+  // Lives on |api_task_runner_|, unless DispatchSafetyNetCheckOffThread is
+  // disabled.
+  std::unique_ptr<Core> core_;
 
   DISALLOW_COPY_AND_ASSIGN(SafeBrowsingApiHandlerBridge);
 };