Change getDeviceEnterpriseInfo to purely async

getDeviceEnterpriseInfo() could run callbacks synchronously if a result
is cached. This could lead some reentrancy problems for consumers of the
class.

This CL modified getDeviceEnterpriseInfo() to always run callbacks
asynchronously.

Testing: Built and ran Chrome(ium) on a managed Android emulator
and observed that "EnterpriseCheck.IsFullyManaged2" indicates
managed.

Bug: 1120955
Change-Id: I122ecbc07946129220776de063a224d5173f1540
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2385640
Commit-Queue: Steven Bingler <bingler@chromium.org>
Reviewed-by: Theresa  <twellington@chromium.org>
Reviewed-by: Eric Orth <ericorth@chromium.org>
Reviewed-by: Sky Malice <skym@chromium.org>
Cr-Commit-Position: refs/heads/master@{#804016}

chrome/android/java/src/org/chromium/chrome/browser/policy/EnterpriseInfo.java

@@ -8,6 +8,8 @@ import android.app.admin.DevicePolicyManager;
 import android.content.Context;
 import android.content.pm.PackageInfo;
 import android.content.pm.PackageManager;
+import android.os.Handler;
+import android.os.Looper;
 import android.os.SystemClock;
 
 import androidx.annotation.VisibleForTesting;
@@ -33,6 +35,7 @@ import java.util.concurrent.RejectedExecutionException;
  */
 public class EnterpriseInfo {
     private static final String TAG = "EnterpriseInfo";
+    private final Handler mHandler;
 
     private static EnterpriseInfo sInstance;
 
@@ -86,21 +89,26 @@ public class EnterpriseInfo {
         ThreadUtils.assertOnUiThread();
         assert callback != null;
 
+        // If there is already a cached result post a task to return it.
         if (mOwnedState != null) {
-            callback.onResult(mOwnedState);
+            mHandler.post(() -> callback.onResult(mOwnedState));
             return;
         }
 
+        // We need to make sure that nothing gets added to mCallbackList once there is a cached
+        // result as nothing on this list will be ran again.
         mCallbackList.add(callback);
 
         if (mCallbackList.size() > 1) {
-            // A pending callback is already being worked on, no need to start up a new thread.
+            // A pending callback is already being worked on, just add to the list and wait.
             return;
         }
 
+        // Skip querying the device if we're testing.
         if (mSkipAsyncCheckForTesting) return;
 
-        // This is the first request, spin up a thread.
+        // There is no cached value and this is the first request, spin up a thread to query the
+        // device.
         try {
             new AsyncTask<OwnedState>() {
                 // TODO: Unit test this function. https://crbug.com/1099262
@@ -145,7 +153,8 @@ public class EnterpriseInfo {
 
                 @Override
                 protected void onPostExecute(OwnedState result) {
-                    onEnterpriseInfoResult(result);
+                    setCacheResult(result);
+                    onEnterpriseInfoResultAvailable();
                 }
             }.executeWithTaskTraits(TaskTraits.USER_VISIBLE);
         } catch (RejectedExecutionException e) {
@@ -155,7 +164,8 @@ public class EnterpriseInfo {
 
             // There will only ever be a single item in the queue as we only try()/catch() on the
             // first item.
-            mCallbackList.remove().onResult(null);
+            Callback<OwnedState> failedRunCallback = mCallbackList.remove();
+            mHandler.post(() -> { failedRunCallback.onResult(null); });
         }
     }
 
@@ -173,23 +183,23 @@ public class EnterpriseInfo {
     private EnterpriseInfo() {
         mOwnedState = null;
         mCallbackList = new LinkedList<Callback<OwnedState>>();
+        mHandler = new Handler(Looper.myLooper());
     }
 
     @VisibleForTesting
-    void onEnterpriseInfoResult(OwnedState result) {
+    void setCacheResult(OwnedState result) {
         ThreadUtils.assertOnUiThread();
         assert result != null;
-
-        // Set the cached value.
         mOwnedState = result;
+    }
+
+    @VisibleForTesting
+    void onEnterpriseInfoResultAvailable() {
+        ThreadUtils.assertOnUiThread();
+        assert mOwnedState != null;
 
         // Service every waiting callback.
-        while (mCallbackList.size() > 0) {
-            // This implementation assumes that ever future call to getDeviceEnterpriseInfo(), from
-            // this point, will result in the cached value being returned immediately. This means we
-            // can ignore the issue of re-entrant callbacks.
-            mCallbackList.remove().onResult(mOwnedState);
-        }
+        while (mCallbackList.size() > 0) mCallbackList.remove().onResult(mOwnedState);
     }
 
     private void recordManagementHistograms(OwnedState state) {
@@ -200,6 +210,14 @@ public class EnterpriseInfo {
                 "EnterpriseCheck.IsFullyManaged2", state.mDeviceOwned);
     }
 
+    /**
+     * When true the check if a device/profile is managed is skipped, meaning that the callback
+     * provided to getDeviceEnterpriseInfo is only added to mCallbackList. setCacheResult and
+     * onEnterpriseInfoResultAvailable must be called manually.
+     *
+     * If mOwnedState != null then this function has no effect and a task to service the
+     * callback will be posted immediately.
+     */
     @VisibleForTesting
     void setSkipAsyncCheckForTesting(boolean skip) {
         mSkipAsyncCheckForTesting = skip;

chrome/android/junit/src/org/chromium/chrome/browser/policy/EnterpriseInfoTest.java

@@ -4,6 +4,8 @@
 
 package org.chromium.chrome.browser.policy;
 
+import android.os.Handler;
+import android.os.Looper;
 import android.support.test.filters.SmallTest;
 
 import org.junit.Assert;
@@ -52,7 +54,8 @@ public class EnterpriseInfoTest {
         // Make the request and service the callbacks.
         instance.getDeviceEnterpriseInfo(callback);
         instance.getDeviceEnterpriseInfo(callback2);
-        instance.onEnterpriseInfoResult(stateIn);
+        instance.setCacheResult(stateIn);
+        instance.onEnterpriseInfoResultAvailable();
 
         // Results should be the same for all callbacks.
         Assert.assertEquals("Callback doesn't match the expected result on servicing.",
@@ -67,11 +70,14 @@ public class EnterpriseInfoTest {
         Assert.assertNotEquals("Callback wasn't reset properly.", callback2.result, stateIn);
 
         // Check the cached value is returned correctly.
+        // Cached results are immediately added to the message queue. With the Roboelectric
+        // framework these async tasks are run synchronously. Meaning as soon as we make the call
+        // we'll have the result when it returns.
         instance.getDeviceEnterpriseInfo(callback);
-        instance.getDeviceEnterpriseInfo(callback2);
-        // This should happen immediately, see testCallbackServicedWhenResultCached.
         Assert.assertEquals(
                 "Callback doesn't match the expected cached result.", callback.result, stateIn);
+
+        instance.getDeviceEnterpriseInfo(callback2);
         Assert.assertEquals(
                 "Callback doesn't match the expected cached result.", callback2.result, stateIn);
     }
@@ -101,40 +107,68 @@ public class EnterpriseInfoTest {
                 "Callbacks were serviced before they were meant to be.", 0, helper.getCallCount());
 
         // Do it. The result value here is irrelevant, put anything.
-        instance.onEnterpriseInfoResult(new EnterpriseInfo.OwnedState(true, true));
+        instance.setCacheResult(new EnterpriseInfo.OwnedState(true, true));
+        instance.onEnterpriseInfoResultAvailable();
 
         Assert.assertEquals(
                 "The wrong number of callbacks were serviced.", count, helper.getCallCount());
     }
 
     /**
-     * Tests that a callback is only serviced immediately if there is a cached result.
+     * Tests that a reentrant callback doesn't cause a synchronous reentry.
      */
     @Test
     @SmallTest
-    public void testCallbackServicedWhenResultCached() {
+    public void testReentrantCallback() {
         EnterpriseInfo instance = EnterpriseInfo.getInstance();
         CallbackHelper helper = new CallbackHelper();
 
+        // Make sure there is a cached value so that the getDeviceEnterpriseInfo() calls below will
+        // post() immediately with a result. The value itself doesn't matter.
+        instance.setCacheResult(new EnterpriseInfo.OwnedState(false, true));
+
         Callback<EnterpriseInfo.OwnedState> callback = (result) -> {
             // We don't care about the result in this test.
             helper.notifyCalled();
         };
 
-        // First: Callback should not be serviced if nothing is cached.
-        instance.getDeviceEnterpriseInfo(callback);
-        Assert.assertEquals(
-                "Callbacks were serviced before they were meant to be.", 0, helper.getCallCount());
-
-        // Set a cached result (of any value) and process the callback.
-        instance.onEnterpriseInfoResult(new EnterpriseInfo.OwnedState(false, false));
-        Assert.assertEquals("Only a single callback should have been serviced at this point.", 1,
-                helper.getCallCount());
+        Callback<EnterpriseInfo.OwnedState> reentrantCallback = (result) -> {
+            // We don't care about the result in this test.
+            helper.notifyCalled();
+            instance.getDeviceEnterpriseInfo(callback);
+        };
 
-        // Second: Callback should now be serviced immediately.
-        instance.getDeviceEnterpriseInfo(callback);
-        Assert.assertEquals(
-                "Callback wasn't serviced by the cached result.", 2, helper.getCallCount());
+        Handler handler = new Handler(Looper.myLooper());
+
+        // Roboelectric synchronously calls post() functions in its Looper, but we can still use it
+        // to test for async behavior by inserting a post() with our assert at the correct point.
+        handler.post(() -> {
+            // getDeviceEnterpriseInfo should insert a post() to run its callback. This post() will
+            // run after the outer post() is finished.
+            instance.getDeviceEnterpriseInfo(reentrantCallback);
+
+            // This inner post() will be inserted after the one from
+            // getDeviceEnterpriseInfo(reentrantCallback). Therefore it will run after
+            // |reentrantCallback| is invoked. When |reentrantCallback| is invoked it will run its
+            // own getDeviceEnterpriseInfo() which will in turn insert its own post(). If all goes
+            // as expected this assert should check after |helper| is notified once by
+            // |reentrantCallback| but before it's notified a second time by |callback|.
+            handler.post(() -> {
+                Assert.assertEquals(
+                        "Reentrant callback wasn't executed as expect.", 1, helper.getCallCount());
+            });
+
+            /* At this point the message queue should look like:
+               -------------------------------
+               Outer post() // Being run now.
+               post(reentrantCallback) // Inserts the `post(callback)`.
+               post(Assert)
+               post(callback) // Not yet inserted.
+               -------------------------------
+            */
+        });
+        // By this point all post()s should have been run, including |callback|'s.
+        Assert.assertEquals("Second callback wasn't executed.", 2, helper.getCallCount());
     }
 
     /**