Make sure config change items are executed in the order dispatched.

In the previous implementation a batch of process/activity config
changes would effectively be executed out of order. When the server
would dispatch changes in config in quick succession the config change
items would update the pending configs first through the preexecute()
calls and then apply the activity config before the process config
is applied even though the process config was dispatched before the activity
config change item. See b/148639784 for more detail.

Fixes: 148639784

Test: ActivityThreadTest#testHandleActivityConfigurationChanged_EnsureUpdatesProcessedInOrder
Test: ActivityThreadTest#testHandleActivityConfigurationChanged_SkipWhenNewerConfigurationPending

Change-Id: I3c926076ac8dba73eb0471c7bc91313df519cf92

5 files changed, 145 insertions, 30 deletions

diff --git a/core/java/android/app/ActivityThread.java b/core/java/android/app/ActivityThread.java
index eea1d69b6326..ea8b19028beb 100644
--- a/core/java/android/app/ActivityThread.java
+++ b/core/java/android/app/ActivityThread.java
@@ -5902,6 +5902,12 @@ public final class ActivityThread extends ClientTransactionHandler {
         }
     }
 
+    /**
+     * Sets the supplied {@code overrideConfig} as pending for the {@code activityToken}. Calling
+     * this method prevents any calls to
+     * {@link #handleActivityConfigurationChanged(IBinder, Configuration, int, boolean)} from
+     * processing any configurations older than {@code overrideConfig}.
+     */
     @Override
     public void updatePendingActivityConfiguration(IBinder activityToken,
             Configuration overrideConfig) {
@@ -5918,13 +5924,22 @@ public final class ActivityThread extends ClientTransactionHandler {
         }
 
         synchronized (r) {
+            if (r.mPendingOverrideConfig != null
+                    && !r.mPendingOverrideConfig.isOtherSeqNewer(overrideConfig)) {
+                if (DEBUG_CONFIGURATION) {
+                    Slog.v(TAG, "Activity has newer configuration pending so drop this"
+                            + " transaction. overrideConfig=" + overrideConfig
+                            + " r.mPendingOverrideConfig=" + r.mPendingOverrideConfig);
+                }
+                return;
+            }
             r.mPendingOverrideConfig = overrideConfig;
         }
     }
 
     @Override
     public void handleActivityConfigurationChanged(IBinder activityToken,
-            Configuration overrideConfig, int displayId) {
+            @NonNull Configuration overrideConfig, int displayId) {
         handleActivityConfigurationChanged(activityToken, overrideConfig, displayId,
                 // This is the only place that uses alwaysReportChange=true. The entry point should
                 // be from ActivityConfigurationChangeItem or MoveToDisplayItem, so the server side
@@ -5935,15 +5950,18 @@ public final class ActivityThread extends ClientTransactionHandler {
     }
 
     /**
-     * Handle new activity configuration and/or move to a different display.
+     * Handle new activity configuration and/or move to a different display. This method is a noop
+     * if {@link #updatePendingActivityConfiguration(IBinder, Configuration)} has been called with
+     * a newer config than {@code overrideConfig}.
+     *
      * @param activityToken Target activity token.
      * @param overrideConfig Activity override config.
      * @param displayId Id of the display where activity was moved to, -1 if there was no move and
      *                  value didn't change.
      * @param alwaysReportChange If the configuration is changed, always report to activity.
      */
-    void handleActivityConfigurationChanged(IBinder activityToken, Configuration overrideConfig,
-            int displayId, boolean alwaysReportChange) {
+    void handleActivityConfigurationChanged(IBinder activityToken,
+            @NonNull Configuration overrideConfig, int displayId, boolean alwaysReportChange) {
         ActivityClientRecord r = mActivities.get(activityToken);
         // Check input params.
         if (r == null || r.activity == null) {
@@ -5954,9 +5972,13 @@ public final class ActivityThread extends ClientTransactionHandler {
                 && displayId != r.activity.getDisplayId();
 
         synchronized (r) {
-            if (r.mPendingOverrideConfig != null
-                    && !r.mPendingOverrideConfig.isOtherSeqNewer(overrideConfig)) {
-                overrideConfig = r.mPendingOverrideConfig;
+            if (overrideConfig.isOtherSeqNewer(r.mPendingOverrideConfig)) {
+                if (DEBUG_CONFIGURATION) {
+                    Slog.v(TAG, "Activity has newer configuration pending so drop this"
+                            + " transaction. overrideConfig=" + overrideConfig
+                            + " r.mPendingOverrideConfig=" + r.mPendingOverrideConfig);
+                }
+                return;
             }
             r.mPendingOverrideConfig = null;
         }
diff --git a/core/java/android/app/servertransaction/ActivityConfigurationChangeItem.java b/core/java/android/app/servertransaction/ActivityConfigurationChangeItem.java
index 0d4e16bbb0f3..8b52242a6b6c 100644
--- a/core/java/android/app/servertransaction/ActivityConfigurationChangeItem.java
+++ b/core/java/android/app/servertransaction/ActivityConfigurationChangeItem.java
@@ -19,6 +19,7 @@ package android.app.servertransaction;
 import static android.os.Trace.TRACE_TAG_ACTIVITY_MANAGER;
 import static android.view.Display.INVALID_DISPLAY;
 
+import android.annotation.NonNull;
 import android.app.ClientTransactionHandler;
 import android.content.res.Configuration;
 import android.os.IBinder;
@@ -37,6 +38,8 @@ public class ActivityConfigurationChangeItem extends ClientTransactionItem {
 
     @Override
     public void preExecute(android.app.ClientTransactionHandler client, IBinder token) {
+        // Notify the client of an upcoming change in the token configuration. This ensures that
+        // batches of config change items only process the newest configuration.
         client.updatePendingActivityConfiguration(token, mConfiguration);
     }
 
@@ -55,7 +58,11 @@ public class ActivityConfigurationChangeItem extends ClientTransactionItem {
     private ActivityConfigurationChangeItem() {}
 
     /** Obtain an instance initialized with provided params. */
-    public static ActivityConfigurationChangeItem obtain(Configuration config) {
+    public static ActivityConfigurationChangeItem obtain(@NonNull Configuration config) {
+        if (config == null) {
+            throw new IllegalArgumentException("Config must not be null.");
+        }
+
         ActivityConfigurationChangeItem instance =
                 ObjectPool.obtain(ActivityConfigurationChangeItem.class);
         if (instance == null) {
@@ -68,7 +75,7 @@ public class ActivityConfigurationChangeItem extends ClientTransactionItem {
 
     @Override
     public void recycle() {
-        mConfiguration = null;
+        mConfiguration = Configuration.EMPTY;
         ObjectPool.recycle(this);
     }
 
diff --git a/core/java/android/app/servertransaction/MoveToDisplayItem.java b/core/java/android/app/servertransaction/MoveToDisplayItem.java
index f6d3dbdf5596..9a457a3aad40 100644
--- a/core/java/android/app/servertransaction/MoveToDisplayItem.java
+++ b/core/java/android/app/servertransaction/MoveToDisplayItem.java
@@ -18,6 +18,7 @@ package android.app.servertransaction;
 
 import static android.os.Trace.TRACE_TAG_ACTIVITY_MANAGER;
 
+import android.annotation.NonNull;
 import android.app.ClientTransactionHandler;
 import android.content.res.Configuration;
 import android.os.IBinder;
@@ -36,6 +37,13 @@ public class MoveToDisplayItem extends ClientTransactionItem {
     private Configuration mConfiguration;
 
     @Override
+    public void preExecute(ClientTransactionHandler client, IBinder token) {
+        // Notify the client of an upcoming change in the token configuration. This ensures that
+        // batches of config change items only process the newest configuration.
+        client.updatePendingActivityConfiguration(token, mConfiguration);
+    }
+
+    @Override
     public void execute(ClientTransactionHandler client, IBinder token,
             PendingTransactionActions pendingActions) {
         Trace.traceBegin(TRACE_TAG_ACTIVITY_MANAGER, "activityMovedToDisplay");
@@ -49,7 +57,12 @@ public class MoveToDisplayItem extends ClientTransactionItem {
     private MoveToDisplayItem() {}
 
     /** Obtain an instance initialized with provided params. */
-    public static MoveToDisplayItem obtain(int targetDisplayId, Configuration configuration) {
+    public static MoveToDisplayItem obtain(int targetDisplayId,
+            @NonNull Configuration configuration) {
+        if (configuration == null) {
+            throw new IllegalArgumentException("Configuration must not be null");
+        }
+
         MoveToDisplayItem instance = ObjectPool.obtain(MoveToDisplayItem.class);
         if (instance == null) {
             instance = new MoveToDisplayItem();
@@ -63,7 +76,7 @@ public class MoveToDisplayItem extends ClientTransactionItem {
     @Override
     public void recycle() {
         mTargetDisplayId = 0;
-        mConfiguration = null;
+        mConfiguration = Configuration.EMPTY;
         ObjectPool.recycle(this);
     }
 
diff --git a/core/tests/coretests/src/android/app/activity/ActivityThreadTest.java b/core/tests/coretests/src/android/app/activity/ActivityThreadTest.java
index a93dacf47050..000e870369db 100644
--- a/core/tests/coretests/src/android/app/activity/ActivityThreadTest.java
+++ b/core/tests/coretests/src/android/app/activity/ActivityThreadTest.java
@@ -20,6 +20,7 @@ import static android.content.Intent.ACTION_EDIT;
 import static android.content.Intent.ACTION_VIEW;
 import static android.content.res.Configuration.ORIENTATION_LANDSCAPE;
 import static android.content.res.Configuration.ORIENTATION_PORTRAIT;
+import static android.view.Display.INVALID_DISPLAY;
 
 import static com.google.common.truth.Truth.assertThat;
 
@@ -29,6 +30,7 @@ import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
 import static org.testng.Assert.assertFalse;
 
+import android.annotation.Nullable;
 import android.app.Activity;
 import android.app.ActivityThread;
 import android.app.IApplicationThread;
@@ -38,6 +40,7 @@ import android.app.servertransaction.ActivityConfigurationChangeItem;
 import android.app.servertransaction.ActivityRelaunchItem;
 import android.app.servertransaction.ClientTransaction;
 import android.app.servertransaction.ClientTransactionItem;
+import android.app.servertransaction.ConfigurationChangeItem;
 import android.app.servertransaction.NewIntentItem;
 import android.app.servertransaction.ResumeActivityItem;
 import android.app.servertransaction.StopActivityItem;
@@ -225,7 +228,7 @@ public class ActivityThreadTest {
     }
 
     @Test
-    public void testHandleActivityConfigurationChanged_PickNewerPendingConfiguration() {
+    public void testHandleActivityConfigurationChanged_SkipWhenNewerConfigurationPending() {
         final TestActivity activity = mActivityTestRule.launchActivity(new Intent());
 
         InstrumentationRegistry.getInstrumentation().runOnMainSync(() -> {
@@ -237,23 +240,88 @@ public class ActivityThreadTest {
 
             final ActivityThread activityThread = activity.getActivityThread();
 
-            final Configuration pendingConfig = new Configuration();
-            pendingConfig.orientation = orientation == ORIENTATION_LANDSCAPE
-                    ? ORIENTATION_PORTRAIT
-                    : ORIENTATION_LANDSCAPE;
-            pendingConfig.seq = seq + 2;
+            final Configuration newerConfig = new Configuration();
+            newerConfig.orientation = orientation == ORIENTATION_LANDSCAPE
+                    ? ORIENTATION_PORTRAIT : ORIENTATION_LANDSCAPE;
+            newerConfig.seq = seq + 2;
             activityThread.updatePendingActivityConfiguration(activity.getActivityToken(),
-                    pendingConfig);
+                    newerConfig);
 
-            final Configuration newConfig = new Configuration();
-            newConfig.orientation = orientation;
-            newConfig.seq = seq + 1;
+            final Configuration olderConfig = new Configuration();
+            olderConfig.orientation = orientation;
+            olderConfig.seq = seq + 1;
 
             activityThread.handleActivityConfigurationChanged(activity.getActivityToken(),
-                    newConfig, Display.INVALID_DISPLAY);
+                    olderConfig, INVALID_DISPLAY);
+            assertEquals(numOfConfig, activity.mNumOfConfigChanges);
+            assertEquals(olderConfig.orientation, activity.mConfig.orientation);
+
+            activityThread.handleActivityConfigurationChanged(activity.getActivityToken(),
+                    newerConfig, INVALID_DISPLAY);
             assertEquals(numOfConfig + 1, activity.mNumOfConfigChanges);
-            assertEquals(pendingConfig.orientation, activity.mConfig.orientation);
+            assertEquals(newerConfig.orientation, activity.mConfig.orientation);
+        });
+    }
+
+    @Test
+    public void testHandleActivityConfigurationChanged_EnsureUpdatesProcessedInOrder()
+            throws Exception {
+        final TestActivity activity = mActivityTestRule.launchActivity(new Intent());
+
+        final ActivityThread activityThread = activity.getActivityThread();
+        InstrumentationRegistry.getInstrumentation().runOnMainSync(() -> {
+            final Configuration config = new Configuration();
+            config.seq = BASE_SEQ;
+            config.orientation = ORIENTATION_PORTRAIT;
+
+            activityThread.handleActivityConfigurationChanged(activity.getActivityToken(),
+                    config, INVALID_DISPLAY);
         });
+
+        final IApplicationThread appThread = activityThread.getApplicationThread();
+        final int numOfConfig = activity.mNumOfConfigChanges;
+
+        final Configuration processConfigLandscape = new Configuration();
+        processConfigLandscape.windowConfiguration.setBounds(new Rect(0, 0, 100, 60));
+        processConfigLandscape.seq = BASE_SEQ + 1;
+
+        final Configuration activityConfigLandscape = new Configuration();
+        activityConfigLandscape.windowConfiguration.setBounds(new Rect(0, 0, 100, 50));
+        activityConfigLandscape.seq = BASE_SEQ + 2;
+
+        final Configuration processConfigPortrait = new Configuration();
+        processConfigPortrait.windowConfiguration.setBounds(new Rect(0, 0, 60, 100));
+        processConfigPortrait.seq = BASE_SEQ + 3;
+
+        final Configuration activityConfigPortrait = new Configuration();
+        activityConfigPortrait.windowConfiguration.setBounds(new Rect(0, 0, 50, 100));
+        activityConfigPortrait.seq = BASE_SEQ + 4;
+
+        activity.mConfigLatch = new CountDownLatch(1);
+        activity.mTestLatch = new CountDownLatch(1);
+
+        ClientTransaction transaction = newTransaction(activityThread, null);
+        transaction.addCallback(ConfigurationChangeItem.obtain(processConfigLandscape));
+        appThread.scheduleTransaction(transaction);
+
+        transaction = newTransaction(activityThread, activity.getActivityToken());
+        transaction.addCallback(ActivityConfigurationChangeItem.obtain(activityConfigLandscape));
+        transaction.addCallback(ConfigurationChangeItem.obtain(processConfigPortrait));
+        transaction.addCallback(ActivityConfigurationChangeItem.obtain(activityConfigPortrait));
+        appThread.scheduleTransaction(transaction);
+
+        activity.mTestLatch.await();
+        activity.mConfigLatch.countDown();
+
+        activity.mConfigLatch = null;
+        activity.mTestLatch = null;
+
+        // Check display metrics, bounds should match the portrait activity bounds.
+        final Rect bounds = activity.getWindowManager().getCurrentWindowMetrics().getBounds();
+        assertEquals(activityConfigPortrait.windowConfiguration.getBounds(), bounds);
+
+        // Ensure that Activity#onConfigurationChanged() is only called once.
+        assertEquals(numOfConfig + 1, activity.mNumOfConfigChanges);
     }
 
     @Test
@@ -268,7 +336,7 @@ public class ActivityThreadTest {
             config.orientation = ORIENTATION_PORTRAIT;
 
             activityThread.handleActivityConfigurationChanged(activity.getActivityToken(),
-                    config, Display.INVALID_DISPLAY);
+                    config, INVALID_DISPLAY);
         });
 
         final int numOfConfig = activity.mNumOfConfigChanges;
@@ -504,7 +572,7 @@ public class ActivityThreadTest {
         config.orientation = ORIENTATION_PORTRAIT;
         config.seq = seq;
         activityThread.handleActivityConfigurationChanged(activity.getActivityToken(), config,
-                Display.INVALID_DISPLAY);
+                INVALID_DISPLAY);
 
         if (activity.mNumOfConfigChanges > numOfConfig) {
             return config.seq;
@@ -514,7 +582,7 @@ public class ActivityThreadTest {
         config.orientation = ORIENTATION_LANDSCAPE;
         config.seq = seq + 1;
         activityThread.handleActivityConfigurationChanged(activity.getActivityToken(), config,
-                Display.INVALID_DISPLAY);
+                INVALID_DISPLAY);
 
         return config.seq;
     }
@@ -572,8 +640,12 @@ public class ActivityThreadTest {
     }
 
     private static ClientTransaction newTransaction(Activity activity) {
-        final IApplicationThread appThread = activity.getActivityThread().getApplicationThread();
-        return ClientTransaction.obtain(appThread, activity.getActivityToken());
+        return newTransaction(activity.getActivityThread(), activity.getActivityToken());
+    }
+
+    private static ClientTransaction newTransaction(ActivityThread activityThread,
+            @Nullable IBinder activityToken) {
+        return ClientTransaction.obtain(activityThread.getApplicationThread(), activityToken);
     }
 
     // Test activity
diff --git a/core/tests/coretests/src/android/app/servertransaction/ObjectPoolTests.java b/core/tests/coretests/src/android/app/servertransaction/ObjectPoolTests.java
index 6c23125aaf13..4654f63a2a91 100644
--- a/core/tests/coretests/src/android/app/servertransaction/ObjectPoolTests.java
+++ b/core/tests/coretests/src/android/app/servertransaction/ObjectPoolTests.java
@@ -63,7 +63,8 @@ public class ObjectPoolTests {
 
     @Test
     public void testRecycleActivityConfigurationChangeItem() {
-        ActivityConfigurationChangeItem emptyItem = ActivityConfigurationChangeItem.obtain(null);
+        ActivityConfigurationChangeItem emptyItem =
+                ActivityConfigurationChangeItem.obtain(Configuration.EMPTY);
         ActivityConfigurationChangeItem item = ActivityConfigurationChangeItem.obtain(config());
         assertNotSame(item, emptyItem);
         assertFalse(item.equals(emptyItem));
@@ -186,7 +187,7 @@ public class ObjectPoolTests {
 
     @Test
     public void testRecycleMoveToDisplayItem() {
-        MoveToDisplayItem emptyItem = MoveToDisplayItem.obtain(0, null);
+        MoveToDisplayItem emptyItem = MoveToDisplayItem.obtain(0, Configuration.EMPTY);
         MoveToDisplayItem item = MoveToDisplayItem.obtain(4, config());
         assertNotSame(item, emptyItem);
         assertFalse(item.equals(emptyItem));