webview: fix minor race in init and clean up code.

If an app called a thread-safe WebView function and then, before the
init task posted to the UI thread had a chance to complete, another
thread also called a WebView function, then we would attempt to set the
UI thread twice. In almost all cases this is benign as it would be set
to the main looper both times, but edge cases may exist that would
result in an attempt to *change* the UI thread, causing a crash in
ThreadUtils.setUiThread.

Fix this by replacing mStarted with a state enum. All existing uses of
mStarted now refer to INIT_FINISHED; INIT_STARTED is (currently) only
used to determine whether the UI thread has already been chosen or not.

To make the code easier to understand, rename the "onMainThread"
parameter to "fromThreadSafeFunction" to more clearly reflect what it
actually means: if true, the caller was a thread-safe function and so
init must be started on the main looper, regardless of the current
thread. This does *not* change the value at any callsites; this was
already the meaning, just with a bad name.

Flip the ternary operator usage so that the condition is not negated,
making it easier to read.

Add comments describing the process in more detail, as WebView threading
requirements are not obvious.

Change-Id: I8f916bfcd7eae04ba97d87f2fa9655844fd50663
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2626516
Commit-Queue: Richard Coles <[email protected]>
Reviewed-by: Nate Fischer <[email protected]>
Cr-Commit-Position: refs/heads/master@{#847673}

android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumAwInit.java

@@ -88,8 +88,12 @@ public class WebViewChromiumAwInit {
     // it shouldn't be accessed from anywhere else.
     /* package */ final Object mLock = new Object();
 
-    // Read/write protected by mLock.
-    private boolean mStarted;
+    // mInitState should only transition INIT_NOT_STARTED -> INIT_STARTED -> INIT_FINISHED
+    private static final int INIT_NOT_STARTED = 0;
+    private static final int INIT_STARTED = 1;
+    private static final int INIT_FINISHED = 2;
+    // Read/write protected by mLock
+    private int mInitState;
     private Looper mFirstWebViewConstructedOn;
 
     private final WebViewChromiumFactoryProvider mFactory;
@@ -135,7 +139,7 @@ protected void startChromiumLocked() {
             // return paths. (Other threads will not wake-up until we release |mLock|, whatever).
             mLock.notifyAll();
 
-            if (mStarted) {
+            if (mInitState == INIT_FINISHED) {
                 return;
             }
 
@@ -180,7 +184,7 @@ protected void startChromiumLocked() {
                 mSharedStatics.setWebContentsDebuggingEnabledUnconditionally(true);
             }
 
-            mStarted = true;
+            mInitState = INIT_FINISHED;
 
             RecordHistogram.recordSparseHistogram("Android.WebView.TargetSdkVersion",
                     context.getApplicationInfo().targetSdkVersion);
@@ -249,7 +253,7 @@ private void setUpResources(int packageId, Context context) {
     // Only called for apps which target <JB MR2, and which construct WebView on a non-main thread.
     void setFirstWebViewConstructedOn(Looper looper) {
         synchronized (mLock) {
-            if (!mStarted && mFirstWebViewConstructedOn == null) {
+            if (mInitState != INIT_FINISHED && mFirstWebViewConstructedOn == null) {
                 mFirstWebViewConstructedOn = looper;
             }
         }
@@ -273,54 +277,42 @@ private static void recordActualUiThread(@ActualUiThread int value) {
     }
 
     boolean hasStarted() {
-        return mStarted;
+        return mInitState == INIT_FINISHED;
     }
 
-    void startYourEngines(boolean onMainThread) {
+    void startYourEngines(boolean fromThreadSafeFunction) {
         synchronized (mLock) {
-            ensureChromiumStartedLocked(onMainThread);
+            ensureChromiumStartedLocked(fromThreadSafeFunction);
         }
     }
 
     // This method is not private only because the downstream subclass needs to access it,
     // it shouldn't be accessed from anywhere else.
-    /* package */ void ensureChromiumStartedLocked(boolean onMainThread) {
+    /* package */ void ensureChromiumStartedLocked(boolean fromThreadSafeFunction) {
         assert Thread.holdsLock(mLock);
 
-        if (mStarted) { // Early-out for the common case.
+        if (mInitState == INIT_FINISHED) { // Early-out for the common case.
             return;
         }
 
-        Looper looper = !onMainThread ? Looper.myLooper() : Looper.getMainLooper();
-        Log.v(TAG, "Binding Chromium to "
-                        + (Looper.getMainLooper().equals(looper) ? "main" : "background")
-                        + " looper " + looper);
-        ThreadUtils.setUiThread(looper);
-
-        // For apps targeting <JBMR2 which aren't required to commit to a thread in
-        // WebViewChromium.init, record a metric stating which thread we picked.
-        if (mFirstWebViewConstructedOn != null) {
-            if (looper == mFirstWebViewConstructedOn) {
-                // Using the same thread that the first WebView was constructed on.
-                recordActualUiThread(ActualUiThread.FIRST_WEBVIEW_CONSTRUCTED);
-            } else if (looper == Looper.getMainLooper()) {
-                // Using the main looper.
-                recordActualUiThread(ActualUiThread.MAIN_LOOPER);
-            } else {
-                // Using some other thread.
-                recordActualUiThread(ActualUiThread.OTHER);
-            }
-            // Reset to null to avoid leaking the app's looper.
-            mFirstWebViewConstructedOn = null;
+        if (mInitState == INIT_NOT_STARTED) {
+            // If we're the first thread to enter ensureChromiumStartedLocked, we need to determine
+            // which thread will be the UI thread; declare init has started so that no other thread
+            // will try to do this.
+            mInitState = INIT_STARTED;
+            setChromiumUiThreadLocked(fromThreadSafeFunction);
         }
 
         if (ThreadUtils.runningOnUiThread()) {
+            // If we are currently running on the UI thread then we must do init now. If there was
+            // already a task posted to the UI thread from another thread to do it, it will just
+            // no-op when it runs.
             startChromiumLocked();
             return;
         }
 
-        // We must post to the UI thread to cover the case that the user has invoked Chromium
-        // startup by using the (thread-safe) CookieManager rather than creating a WebView.
+        // If we're not running on the UI thread (because init was triggered by a thread-safe
+        // function), post init to the UI thread, since init is *not* thread-safe.
         AwThreadUtils.postToUiThreadLooper(new Runnable() {
             @Override
             public void run() {
@@ -329,13 +321,49 @@ public void run() {
                 }
             }
         });
-        while (!mStarted) {
+
+        // Wait for the UI thread to finish init.
+        while (mInitState != INIT_FINISHED) {
             try {
-                // Important: wait() releases |mLock| the UI thread can take it :-)
                 mLock.wait();
             } catch (InterruptedException e) {
-                // Keep trying... eventually the UI thread will process the task we sent it.
+                // Keep trying; we can't abort init as WebView APIs do not declare that they throw
+                // InterruptedException.
+            }
+        }
+    }
+
+    private void setChromiumUiThreadLocked(boolean fromThreadSafeFunction) {
+        // If we're being started from a function that's allowed to be called on any thread,
+        // then we can't just assume the current thread is the UI thread; instead we assume the
+        // process's main looper will be the UI thread, because that's the case for almost all
+        // Android apps.
+        //
+        // If we're being started from a function that must be called from the UI
+        // thread, then by definition the current thread is the UI thread whether it's the main
+        // looper or not.
+        Looper looper = fromThreadSafeFunction ? Looper.getMainLooper() : Looper.myLooper();
+        Log.v(TAG,
+                "Binding Chromium to "
+                        + (Looper.getMainLooper().equals(looper) ? "main" : "background")
+                        + " looper " + looper);
+        ThreadUtils.setUiThread(looper);
+
+        // For apps targeting <JBMR2 which aren't required to commit to a thread in
+        // WebViewChromium.init, record a metric stating which thread we picked.
+        if (mFirstWebViewConstructedOn != null) {
+            if (looper == mFirstWebViewConstructedOn) {
+                // Using the same thread that the first WebView was constructed on.
+                recordActualUiThread(ActualUiThread.FIRST_WEBVIEW_CONSTRUCTED);
+            } else if (looper == Looper.getMainLooper()) {
+                // Using the main looper.
+                recordActualUiThread(ActualUiThread.MAIN_LOOPER);
+            } else {
+                // Using some other thread.
+                recordActualUiThread(ActualUiThread.OTHER);
             }
+            // Reset to null to avoid leaking the app's looper.
+            mFirstWebViewConstructedOn = null;
         }
     }
 
@@ -377,7 +405,7 @@ public AwTracingController getTracingController() {
 
     // Only on UI thread.
     AwBrowserContext getBrowserContextOnUiThread() {
-        assert mStarted;
+        assert mInitState == INIT_FINISHED;
 
         if (BuildConfig.DCHECK_IS_ON && !ThreadUtils.runningOnUiThread()) {
             throw new RuntimeException(