[Cronet] Move initialization to a new thread rather than the UI thread.

components/cronet/android/java/src/org/chromium/net/impl/CronetLibraryLoader.java

Index: components/cronet/android/java/src/org/chromium/net/impl/CronetLibraryLoader.java
diff --git a/components/cronet/android/java/src/org/chromium/net/impl/CronetLibraryLoader.java b/components/cronet/android/java/src/org/chromium/net/impl/CronetLibraryLoader.java
index c7a7cb46b52ed2ece1448634f14f36b89cea496d..810f5bcf8b99da5473a0371eab623b3cfb8b3ee5 100644
--- a/components/cronet/android/java/src/org/chromium/net/impl/CronetLibraryLoader.java
+++ b/components/cronet/android/java/src/org/chromium/net/impl/CronetLibraryLoader.java
@@ -6,6 +6,7 @@ package org.chromium.net.impl;
 
 import android.content.Context;
 import android.os.Handler;
+import android.os.HandlerThread;
 import android.os.Looper;
 
 import org.chromium.base.ContextUtils;
@@ -15,7 +16,7 @@ import org.chromium.base.annotations.JNINamespace;
 import org.chromium.net.NetworkChangeNotifier;
 
 /**
- * CronetLibraryLoader loads and initializes native library on main thread.
+ * CronetLibraryLoader loads and initializes native library on init thread.
  */
 @JNINamespace("cronet")
 @VisibleForTesting
@@ -24,14 +25,17 @@ public class CronetLibraryLoader {
     private static final Object sLoadLock = new Object();
     private static final String LIBRARY_NAME = "cronet." + ImplVersion.getCronetVersion();
     private static final String TAG = CronetLibraryLoader.class.getSimpleName();
+    // Thread used for initialization. This thread lives forever as things like the
+    // global singleton NetworkChangeNotifier live on it and are never killed.
+    private static final HandlerThread sInitThread = new HandlerThread("CronetInit");

[xunjieli, 2017/04/13 18:44:54]

I am fine with the word "init thread." However, since the thread lives forever
and handles messages from the UI thread, calling it initialization thread is a
slightly misleading. 

[pauljensen, 2017/05/01 15:35:53]

True, I've updated the comment appropriately.  I couldn't think of a variable
name that wasn't ridiculously long so I've left it called sInitThread.  Let me
know if you think I should rename the variable too.

     // Has library loading commenced?  Setting guarded by sLoadLock.
     private static volatile boolean sLibraryLoaded = false;
-    // Has ensureMainThreadInitialized() completed?  Only accessed on main thread.
-    private static volatile boolean sMainThreadInitDone = false;
+    // Has ensureInitThreadInitialized() completed?  Only accessed on init thread.
+    private static volatile boolean sInitThreadInitDone = false;
 
     /**
      * Ensure that native library is loaded and initialized. Can be called from
-     * any thread, the load and initialization is performed on main thread.
+     * any thread, the load and initialization is performed on init thread.
      */
     public static void ensureInitialized(
             final Context applicationContext, final CronetEngineBuilderImpl builder) {
@@ -55,35 +59,29 @@ public class CronetLibraryLoader {
                 sLibraryLoaded = true;
             }
 
-            if (!sMainThreadInitDone) {
-                // Init native Chromium CronetEngine on Main UI thread.
-                Runnable task = new Runnable() {
+            if (!sInitThreadInitDone) {
+                if (!sInitThread.isAlive()) {

[xunjieli, 2017/04/13 18:44:54]

We could also lazily create sInitThread here to avoid the isAlive() check/state.

if (!sInitThread) {
   sInitThread = new HandlerThread();
   sInitThread.start();
}

[pauljensen, 2017/05/01 15:35:53]

We could but this would require making sInitThread non-final which somewhat
complicates reasoning about sInitThread.  For example one might then wonder if
synchronization to access sInitThread is necessary in postToInitThread() and
ensureInitializedOnInitThread() which I'd like to avoid as it goes against the
Chromium threading approach: "We discourage locking and threadsafe objects."
https://chromium.googlesource.com/chromium/src/+/master/docs/design/threading.md

I think sInitThread.isAlive() will generally only be called once, so I don't
think the performance impact will be significant.

+                    sInitThread.start();
+                }
+                postToInitThread(new Runnable() {
                     @Override
                     public void run() {
-                        ensureInitializedOnMainThread(applicationContext);
+                        ensureInitializedOnInitThread(applicationContext);
                     }
-                };
-                // Run task immediately or post it to the UI thread.
-                if (Looper.getMainLooper() == Looper.myLooper()) {
-                    task.run();
-                } else {
-                    // The initOnMainThread will complete on the main thread prior
-                    // to other tasks posted to the main thread.
-                    new Handler(Looper.getMainLooper()).post(task);
-                }
+                });
             }
         }
     }
 
     /**
-     * Ensure that the main thread initialization has completed. Can only be called from
-     * the main thread. Ensures that the NetworkChangeNotifier is initialzied and the
-     * main thread native MessageLoop is initialized.
+     * Ensure that the init thread initialization has completed. Can only be called from
+     * the init thread. Ensures that the NetworkChangeNotifier is initialzied and the
+     * init thread native MessageLoop is initialized.
      */
-    static void ensureInitializedOnMainThread(Context context) {
+    static void ensureInitializedOnInitThread(Context context) {
         assert sLibraryLoaded;
-        assert Looper.getMainLooper() == Looper.myLooper();
-        if (sMainThreadInitDone) {
+        assert sInitThread.getLooper() == Looper.myLooper();
+        if (sInitThreadInitDone) {
             return;
         }
         NetworkChangeNotifier.init(context);
@@ -97,11 +95,18 @@ public class CronetLibraryLoader {
         // NetworkChangeNotifierAndroid is created, so as to avoid receiving
         // the undesired initial network change observer notification, which
         // will cause active requests to fail with ERR_NETWORK_CHANGED.
-        nativeCronetInitOnMainThread();
-        sMainThreadInitDone = true;
+        nativeCronetInitOnInitThread();
+        sInitThreadInitDone = true;
+    }
+
+    /**
+     * Run {@code r} on the initialization thread.
+     */
+    static void postToInitThread(Runnable r) {
+        new Handler(sInitThread.getLooper()).post(r);

[xunjieli, 2017/04/13 18:44:54]

Do we need to test to see if we aren't already on Init thread?
I know that embedder won't have access to Init thread, but if in the future we
might end up posting from the same thread. Maybe add a check so we can detect
that.

[pauljensen, 2017/05/01 15:35:53]

Done.

     }
 
     // Native methods are implemented in cronet_library_loader.cc.
-    private static native void nativeCronetInitOnMainThread();
+    private static native void nativeCronetInitOnInitThread();
     private static native String nativeGetCronetVersion();
 }