InstalledAppProviderImpl: Assert that code is run off the UI thread.

base/android/java/src/org/chromium/base/ThreadUtils.java

 // Copyright 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 package org.chromium.base;
 
 import android.os.Handler;
 import android.os.Looper;
 import android.os.Process;
 
 import org.chromium.base.annotations.CalledByNative;
 
 import java.util.concurrent.Callable;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.FutureTask;
 
 /**
  * Helper methods to deal with threading related tasks.
  */
 public class ThreadUtils {
 
     private static final Object sLock = new Object();
 
     private static boolean sWillOverride;
 
     private static Handler sUiThreadHandler;
 
+    private static boolean sThreadAssertsEnabled = true;

[agrieve, 2017/05/05 15:00:48]

nit: Generally better to have things default-initialized if possible, since it
requires less code to run on start-up. E.g. could just call this
sThreadAssertsDisabled, then wouldn't need to be initialized.

[Matt Giuca, 2017/05/08 01:18:10]

Done.

+
     public static void setWillOverrideUiThread() {
         synchronized (sLock) {
             sWillOverride = true;
         }
     }
 
     @VisibleForTesting
     public static void setUiThread(Looper looper) {
         synchronized (sLock) {
             if (looper == null) {
@@ skipping 146 matching lines @@
      *
      * @param task The Runnable to run
      * @param delayMillis The delay in milliseconds until the Runnable will be run
      */
     @VisibleForTesting
     public static void postOnUiThreadDelayed(Runnable task, long delayMillis) {
         getUiThreadHandler().postDelayed(task, delayMillis);
     }
 
     /**
-     * Asserts that the current thread is running on the main thread.
+     * Asserts that the current thread is the main (UI) thread.
+     *
+     * Can be disabled by setThreadAssertsEnabled(false).
      */
     public static void assertOnUiThread() {
-        if (BuildConfig.DCHECK_IS_ON && !runningOnUiThread()) {
-            throw new IllegalStateException("Must be called on the Ui thread.");
+        if (!sThreadAssertsEnabled) {
+            return;
         }
+
+        assert runningOnUiThread() : "Must be called on the UI thread.";
     }
 
     /**
+     * Asserts that the current thread is NOT the main (UI) thread.
+     *
+     * Can be disabled by setThreadAssertsEnabled(false).
+     */
+    public static void assertOnBackgroundThread() {
+        if (!sThreadAssertsEnabled) {
+            return;
+        }
+
+        assert !runningOnUiThread() : "Must be called on a thread other than UI.";
+    }
+
+    /**
+     * Disables thread asserts.
+     *
+     * Can be used by tests where code that normally runs multi-threaded is going to run
+     * single-threaded for the test (otherwise asserts that are valid in production would fail in
+     * those tests).
+     */
+    @VisibleForTesting
+    public static void setThreadAssertsEnabled(boolean enabled) {

[Ted C, 2017/05/05 14:09:39]

I would add "ForTesting" to the end of the name to make sure this doesn't leak
into prod code usage.

[agrieve, 2017/05/05 15:00:49]

Along the same lines, probably would drop "VisibleForTesting", since the
visibility has not been altered for tests (it exists only for tests, which is
what changing the name will convey)

[Matt Giuca, 2017/05/08 01:18:10]

I'm confused about both of these suggestions... you're the experts on
Java-in-Chrome so I'll defer to you, but my understanding was that in Java we
use @VisibleForTesting as a more formalized alternative to a "ForTesting" name
suffix.

On 2017/05/05 15:00:49, agrieve wrote:
Are you suggesting that if a method is used *both* internally as well as being
exposed publicly for testing, it should have @VisibleForTesting, but if a method
is *only* exposed for testing (and isn't otherwise used internally), it should
have a "ForTesting" suffix instead, and not the annotation?

That seems like a pretty arbitrary distinction; from an external perspective, it
doesn't matter whether it's used internally or not, and surely we should have a
consistent way to convey something shouldn't be used other than by tests?

Also, there are three other methods in this class (setUiThread,
runOnUiThreadBlockingNoException and postOnUiThreadDelayed) which are annotated
@VisibleForTesting but aren't used internally. I'm changing it but please
confirm this is OK because it seems wrong to me.

[agrieve, 2017/05/08 14:18:15]

Consistency is certainly not our strong suite. To me, seeing @VisibleForTesting
does not answer whether the function is used outside of tests, but the name
suffix makes it crystal clear.

Thinking about this change again though... Is there a reason the tests can't
just run on the UI thread? It's generally better to have tests match reality as
much as possible. E.g. just call ThreadUtils.runOnUiThreadBlocking() within the
tests.

+        sThreadAssertsEnabled = enabled;
+    }
+
+    /**
      * @return true iff the current thread is the main (UI) thread.
      */
     public static boolean runningOnUiThread() {
         return getUiThreadHandler().getLooper() == Looper.myLooper();
     }
 
     public static Looper getUiThreadLooper() {
         return getUiThreadHandler().getLooper();
     }
 
     /**
      * Set thread priority to audio.
      */
     @CalledByNative
     public static void setThreadPriorityAudio(int tid) {
         Process.setThreadPriority(tid, Process.THREAD_PRIORITY_AUDIO);
     }
 
     /**
      * Checks whether Thread priority is THREAD_PRIORITY_AUDIO or not.
      * @param tid Thread id.
      * @return true for THREAD_PRIORITY_AUDIO and false otherwise.
      */
     @CalledByNative
     private static boolean isThreadPriorityAudio(int tid) {
         return Process.getThreadPriority(tid) == Process.THREAD_PRIORITY_AUDIO;
     }
 }