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

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

Index: base/android/java/src/org/chromium/base/ThreadUtils.java
diff --git a/base/android/java/src/org/chromium/base/ThreadUtils.java b/base/android/java/src/org/chromium/base/ThreadUtils.java
index dbad59d093fa695f43c060bc1e93873a95295761..4979c5f7fcb496a7b44e882dcd366c209e1d705f 100644
--- a/base/android/java/src/org/chromium/base/ThreadUtils.java
+++ b/base/android/java/src/org/chromium/base/ThreadUtils.java
@@ -25,6 +25,8 @@ public class ThreadUtils {
 
     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;
@@ -191,12 +193,41 @@ public class ThreadUtils {
     }
 
     /**
-     * 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;
     }
 
     /**