Android: UserDataHost switches to throw RuntimeError

AssertionError-based tests are not guaranteed to work. This CL changes
the assertion exception used in UserDataHost class to runtime exceptions
to make it test-friendly. Items tested in the tests now throw
IllegalStateException or IllegalArgumentException.

Bug: 877918
Change-Id: I0f1d8c7d9b49a7b8c4020bdfaaf3e15479110b7d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1192484
Commit-Queue: Jinsuk Kim <jinsukkim@chromium.org>
Reviewed-by: Richard Coles <torne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#708871}

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

@@ -57,9 +57,20 @@ public final class UserDataHost {
 
     private HashMap<Class<? extends UserData>, UserData> mUserDataMap = new HashMap<>();
 
+    private static void checkArgument(boolean condition) {
+        if (!condition) {
+            throw new IllegalArgumentException(
+                    "Neither key nor object of UserDataHost can be null.");
+        }
+    }
+
     private void checkThreadAndState() {
-        assert mThreadId == Process.myTid() : "UserData must only be used on a single thread.";
-        assert mUserDataMap != null : "Operation is not allowed after destroy()";
+        if (mThreadId != Process.myTid()) {
+            throw new IllegalStateException("UserData must only be used on a single thread.");
+        }
+        if (mUserDataMap == null) {
+            throw new IllegalStateException("Operation is not allowed after destroy().");
+        }
     }
 
     /**
@@ -70,7 +81,7 @@ public final class UserDataHost {
      */
     public <T extends UserData> T setUserData(Class<T> key, T object) {
         checkThreadAndState();
-        assert key != null && object != null : "Neither key nor object of UserDataHost can be null";
+        checkArgument(key != null && object != null);
 
         mUserDataMap.put(key, object);
         return getUserData(key);
@@ -85,22 +96,24 @@ public final class UserDataHost {
      */
     public <T extends UserData> T getUserData(Class<T> key) {
         checkThreadAndState();
-        assert key != null : "UserDataHost key cannot be null";
+        checkArgument(key != null);
 
         return key.cast(mUserDataMap.get(key));
     }
 
     /**
-     * Removes the mapping for a key from this map. Assertion will be thrown if
+     * Removes the mapping for a key from this map. Exception will be thrown if
      * the given key has no mapping.
      * @param key Type token for which the specified object is to be removed.
      * @return The previous value associated with {@code key}.
      */
     public <T extends UserData> T removeUserData(Class<T> key) {
         checkThreadAndState();
-        assert key != null : "UserDataHost key cannot be null";
+        checkArgument(key != null);
 
-        assert mUserDataMap.containsKey(key) : "UserData for the key is not present";
+        if (!mUserDataMap.containsKey(key)) {
+            throw new IllegalStateException("UserData for the key is not present.");
+        }
         return key.cast(mUserDataMap.remove(key));
     }
 

base/android/javatests/src/org/chromium/base/UserDataHostTest.java

@@ -11,7 +11,6 @@ import org.junit.Test;
 import org.junit.runner.RunWith;
 
 import org.chromium.base.test.BaseJUnit4ClassRunner;
-import org.chromium.base.test.util.DisabledTest;
 
 /**
  * Test class for {@link UserDataHost}.
@@ -46,34 +45,31 @@ public class UserDataHostTest {
         }
     }
 
-    private <T extends UserData> void assertGetUserData(Class<T> key) {
-        boolean exception = false;
+    private <T extends UserData, E extends RuntimeException> void getUserDataException(
+            Class<T> key, Class<E> exceptionType) {
         try {
             mHost.getUserData(key);
-        } catch (AssertionError e) {
-            exception = true;
+        } catch (Exception e) {
+            if (!exceptionType.isInstance(e)) throw e;
         }
-        Assert.assertTrue(exception);
     }
 
-    private <T extends UserData> void assertSetUserData(Class<T> key, T obj) {
-        boolean exception = false;
+    private <T extends UserData, E extends RuntimeException> void setUserDataException(
+            Class<T> key, T obj, Class<E> exceptionType) {
         try {
             mHost.setUserData(key, obj);
-        } catch (AssertionError e) {
-            exception = true;
+        } catch (Exception e) {
+            if (!exceptionType.isInstance(e)) throw e;
         }
-        Assert.assertTrue(exception);
     }
 
-    private <T extends UserData> void assertRemoveUserData(Class<T> key) {
-        boolean exception = false;
+    private <T extends UserData, E extends RuntimeException> void removeUserDataException(
+            Class<T> key, Class<E> exceptionType) {
         try {
             mHost.removeUserData(key);
-        } catch (AssertionError e) {
-            exception = true;
+        } catch (Exception e) {
+            if (!exceptionType.isInstance(e)) throw e;
         }
-        Assert.assertTrue(exception);
     }
 
     /**
@@ -81,14 +77,13 @@ public class UserDataHostTest {
      */
     @Test
     @SmallTest
-    @DisabledTest
     public void testBasicOperations() {
         TestObjectA obj = new TestObjectA();
         mHost.setUserData(TestObjectA.class, obj);
         Assert.assertEquals(obj, mHost.getUserData(TestObjectA.class));
         Assert.assertEquals(obj, mHost.removeUserData(TestObjectA.class));
         Assert.assertNull(mHost.getUserData(TestObjectA.class));
-        assertRemoveUserData(TestObjectA.class);
+        removeUserDataException(TestObjectA.class, IllegalStateException.class);
     }
 
     /**
@@ -96,14 +91,13 @@ public class UserDataHostTest {
      */
     @Test
     @SmallTest
-    @DisabledTest
     public void testNullKeyOrDataAreDisallowed() {
         TestObjectA obj = new TestObjectA();
-        assertSetUserData(null, null);
-        assertSetUserData(TestObjectA.class, null);
-        assertSetUserData(null, obj);
-        assertGetUserData(null);
-        assertRemoveUserData(null);
+        setUserDataException(null, null, IllegalArgumentException.class);
+        setUserDataException(TestObjectA.class, null, IllegalArgumentException.class);
+        setUserDataException(null, obj, IllegalArgumentException.class);
+        getUserDataException(null, IllegalArgumentException.class);
+        removeUserDataException(null, IllegalArgumentException.class);
     }
 
     /**
@@ -121,6 +115,18 @@ public class UserDataHostTest {
         Assert.assertEquals(obj2, mHost.getUserData(TestObjectA.class));
     }
 
+    /**
+     * Verifies operation on a different thread is not allowed.
+     */
+    @Test
+    @SmallTest
+    public void testSingleThreadPolicy() {
+        TestObjectA obj = new TestObjectA();
+        mHost.setUserData(TestObjectA.class, obj);
+        ThreadUtils.runOnUiThreadBlocking(
+                () -> getUserDataException(TestObjectA.class, IllegalStateException.class));
+    }
+
     /**
      * Verifies {@link UserHostData#destroy()} detroyes each {@link UserData} object.
      */
@@ -144,15 +150,14 @@ public class UserDataHostTest {
      */
     @Test
     @SmallTest
-    @DisabledTest
     public void testOperationsDisallowedAfterDestroy() {
         TestObjectA obj = new TestObjectA();
         mHost.setUserData(TestObjectA.class, obj);
         Assert.assertEquals(obj, mHost.getUserData(TestObjectA.class));
 
         mHost.destroy();
-        assertGetUserData(TestObjectA.class);
-        assertSetUserData(TestObjectA.class, obj);
-        assertRemoveUserData(TestObjectA.class);
+        getUserDataException(TestObjectA.class, IllegalStateException.class);
+        setUserDataException(TestObjectA.class, obj, IllegalStateException.class);
+        removeUserDataException(TestObjectA.class, IllegalStateException.class);
     }
 }