From 6b104eec9a2878fd4128e8ed5a8c7569a15c6397 Mon Sep 17 00:00:00 2001 From: Chilun Date: Thu, 23 Dec 2021 15:51:07 +0800 Subject: [PATCH] Store pending override config instead of creating ActivityClientRecord Create ActivityClientRecord early in preExecute may cause NullPointerException. If two LaunchActivityItem using the same token and the 1st postExecute() comes after 2nd preExecute(), the corresponding launching activity will be removed and cause 2nd execute() get NullPointerException. Since the only use case to get ActivityClientRecord in preExecute() is just to use it to store the pending override config. We can directly store the latest pending override config from preExecute() in ActivityThread instead of creating ActivityClientRecord. Bug: 201668069 Test: atest TransactionExecutorTests atest ActivityThreadTest Change-Id: If350e942254e54c9ec90bc63a6e50eb67d038183 --- core/java/android/app/ActivityThread.java | 69 ++++++++----------- .../android/app/ClientTransactionHandler.java | 25 ++----- .../ActivityConfigurationChangeItem.java | 4 +- .../ActivityTransactionItem.java | 32 ++------- .../servertransaction/LaunchActivityItem.java | 15 ++-- .../servertransaction/MoveToDisplayItem.java | 4 +- .../app/activity/ActivityThreadTest.java | 8 ++- 7 files changed, 52 insertions(+), 105 deletions(-) diff --git a/core/java/android/app/ActivityThread.java b/core/java/android/app/ActivityThread.java index a8894dcd06614..1778ea4a32e23 100644 --- a/core/java/android/app/ActivityThread.java +++ b/core/java/android/app/ActivityThread.java @@ -243,6 +243,7 @@ import java.util.Map; import java.util.Objects; import java.util.TimeZone; import java.util.concurrent.Executor; +import java.util.concurrent.atomic.AtomicInteger; import java.util.function.Consumer; /** @@ -344,11 +345,9 @@ public final class ActivityThread extends ClientTransactionHandler */ @UnsupportedAppUsage final ArrayMap mActivities = new ArrayMap<>(); - /** - * Maps from activity token to local record of the activities that are preparing to be launched. - */ - final Map mLaunchingActivities = - Collections.synchronizedMap(new ArrayMap()); + /** Maps from activity token to the pending override configuration. */ + @GuardedBy("mPendingOverrideConfigs") + private final ArrayMap mPendingOverrideConfigs = new ArrayMap<>(); /** The activities to be truly destroyed (not include relaunch). */ final Map mActivitiesToBeDestroyed = Collections.synchronizedMap(new ArrayMap()); @@ -358,6 +357,7 @@ public final class ActivityThread extends ClientTransactionHandler // Number of activities that are currently visible on-screen. @UnsupportedAppUsage int mNumVisibleActivities = 0; + private final AtomicInteger mNumLaunchingActivities = new AtomicInteger(); @GuardedBy("mAppThread") private int mLastProcessState = PROCESS_STATE_UNKNOWN; @GuardedBy("mAppThread") @@ -555,10 +555,6 @@ public final class ActivityThread extends ClientTransactionHandler boolean hideForNow; Configuration createdConfig; Configuration overrideConfig; - // Used to save the last reported configuration from server side so that activity - // configuration transactions can always use the latest configuration. - @GuardedBy("this") - private Configuration mPendingOverrideConfig; // Used for consolidating configs before sending on to Activity. private Configuration tmpConfig = new Configuration(); // Callback used for updating activity override config and camera compat control state. @@ -3360,21 +3356,6 @@ public final class ActivityThread extends ClientTransactionHandler return activityRecord != null ? activityRecord.activity : null; } - @Override - public void addLaunchingActivity(IBinder token, ActivityClientRecord activity) { - mLaunchingActivities.put(token, activity); - } - - @Override - public ActivityClientRecord getLaunchingActivity(IBinder token) { - return mLaunchingActivities.get(token); - } - - @Override - public void removeLaunchingActivity(IBinder token) { - mLaunchingActivities.remove(token); - } - @Override public ActivityClientRecord getActivityClient(IBinder token) { return mActivities.get(token); @@ -3419,7 +3400,7 @@ public final class ActivityThread extends ClientTransactionHandler // Defer the top state for VM to avoid aggressive JIT compilation affecting activity // launch time. if (processState == ActivityManager.PROCESS_STATE_TOP - && !mLaunchingActivities.isEmpty()) { + && mNumLaunchingActivities.get() > 0) { mPendingProcessState = processState; mH.postDelayed(this::applyPendingProcessState, PENDING_TOP_PROCESS_STATE_TIMEOUT); } else { @@ -3435,7 +3416,7 @@ public final class ActivityThread extends ClientTransactionHandler // Handle the pending configuration if the process state is changed from cached to // non-cached. Except the case where there is a launching activity because the // LaunchActivityItem will handle it. - if (wasCached && !isCachedProcessState() && mLaunchingActivities.isEmpty()) { + if (wasCached && !isCachedProcessState() && mNumLaunchingActivities.get() == 0) { final Configuration pendingConfig = mConfigurationController.getPendingConfiguration(false /* clearPending */); if (pendingConfig == null) { @@ -3473,6 +3454,11 @@ public final class ActivityThread extends ClientTransactionHandler } } + @Override + public void countLaunchingActivities(int num) { + mNumLaunchingActivities.getAndAdd(num); + } + @UnsupportedAppUsage public final void sendActivityResult( IBinder token, String id, int requestCode, @@ -6096,31 +6082,31 @@ public final class ActivityThread extends ClientTransactionHandler } /** - * Sets the supplied {@code overrideConfig} as pending for the {@code activityToken}. Calling + * Sets the supplied {@code overrideConfig} as pending for the {@code token}. Calling * this method prevents any calls to * {@link #handleActivityConfigurationChanged(ActivityClientRecord, Configuration, int)} from * processing any configurations older than {@code overrideConfig}. */ @Override - public void updatePendingActivityConfiguration(ActivityClientRecord r, - Configuration overrideConfig) { - synchronized (r) { - if (r.mPendingOverrideConfig != null - && !r.mPendingOverrideConfig.isOtherSeqNewer(overrideConfig)) { + public void updatePendingActivityConfiguration(IBinder token, Configuration overrideConfig) { + synchronized (mPendingOverrideConfigs) { + final Configuration pendingOverrideConfig = mPendingOverrideConfigs.get(token); + if (pendingOverrideConfig != null + && !pendingOverrideConfig.isOtherSeqNewer(overrideConfig)) { if (DEBUG_CONFIGURATION) { - Slog.v(TAG, "Activity has newer configuration pending so drop this" - + " transaction. overrideConfig=" + overrideConfig - + " r.mPendingOverrideConfig=" + r.mPendingOverrideConfig); + Slog.v(TAG, "Activity has newer configuration pending so this transaction will" + + " be dropped. overrideConfig=" + overrideConfig + + " pendingOverrideConfig=" + pendingOverrideConfig); } return; } - r.mPendingOverrideConfig = overrideConfig; + mPendingOverrideConfigs.put(token, overrideConfig); } } /** * Handle new activity configuration and/or move to a different display. This method is a noop - * if {@link #updatePendingActivityConfiguration(ActivityClientRecord, Configuration)} has been + * if {@link #updatePendingActivityConfiguration(IBinder, Configuration)} has been * called with a newer config than {@code overrideConfig}. * * @param r Target activity record. @@ -6131,16 +6117,17 @@ public final class ActivityThread extends ClientTransactionHandler @Override public void handleActivityConfigurationChanged(ActivityClientRecord r, @NonNull Configuration overrideConfig, int displayId) { - synchronized (r) { - if (overrideConfig.isOtherSeqNewer(r.mPendingOverrideConfig)) { + synchronized (mPendingOverrideConfigs) { + final Configuration pendingOverrideConfig = mPendingOverrideConfigs.get(r.token); + if (overrideConfig.isOtherSeqNewer(pendingOverrideConfig)) { if (DEBUG_CONFIGURATION) { Slog.v(TAG, "Activity has newer configuration pending so drop this" + " transaction. overrideConfig=" + overrideConfig - + " r.mPendingOverrideConfig=" + r.mPendingOverrideConfig); + + " pendingOverrideConfig=" + pendingOverrideConfig); } return; } - r.mPendingOverrideConfig = null; + mPendingOverrideConfigs.remove(r.token); } if (displayId == INVALID_DISPLAY) { diff --git a/core/java/android/app/ClientTransactionHandler.java b/core/java/android/app/ClientTransactionHandler.java index c743f6572d5e5..d365269ed1a63 100644 --- a/core/java/android/app/ClientTransactionHandler.java +++ b/core/java/android/app/ClientTransactionHandler.java @@ -83,6 +83,9 @@ public abstract class ClientTransactionHandler { /** Set current process state. */ public abstract void updateProcessState(int processState, boolean fromIpc); + /** Count how many activities are launching. */ + public abstract void countLaunchingActivities(int num); + // Execute phase related logic and handlers. Methods here execute actual lifecycle transactions // and deliver callbacks. @@ -139,7 +142,7 @@ public abstract class ClientTransactionHandler { public abstract void performRestartActivity(@NonNull ActivityClientRecord r, boolean start); /** Set pending activity configuration in case it will be updated by other transaction item. */ - public abstract void updatePendingActivityConfiguration(@NonNull ActivityClientRecord r, + public abstract void updatePendingActivityConfiguration(@NonNull IBinder token, Configuration overrideConfig); /** Deliver activity (override) configuration change. */ @@ -188,26 +191,6 @@ public abstract class ClientTransactionHandler { public abstract void handleFixedRotationAdjustments(IBinder token, FixedRotationAdjustments fixedRotationAdjustments); - /** - * Add {@link ActivityClientRecord} that is preparing to be launched. - * @param token Activity token. - * @param activity An initialized instance of {@link ActivityClientRecord} to use during launch. - */ - public abstract void addLaunchingActivity(IBinder token, ActivityClientRecord activity); - - /** - * Get {@link ActivityClientRecord} that is preparing to be launched. - * @param token Activity token. - * @return An initialized instance of {@link ActivityClientRecord} to use during launch. - */ - public abstract ActivityClientRecord getLaunchingActivity(IBinder token); - - /** - * Remove {@link ActivityClientRecord} from the launching activity list. - * @param token Activity token. - */ - public abstract void removeLaunchingActivity(IBinder token); - /** * Get {@link android.app.ActivityThread.ActivityClientRecord} instance that corresponds to the * provided token. diff --git a/core/java/android/app/servertransaction/ActivityConfigurationChangeItem.java b/core/java/android/app/servertransaction/ActivityConfigurationChangeItem.java index 032b57e654588..5a3ad310b4d65 100644 --- a/core/java/android/app/servertransaction/ActivityConfigurationChangeItem.java +++ b/core/java/android/app/servertransaction/ActivityConfigurationChangeItem.java @@ -40,11 +40,9 @@ public class ActivityConfigurationChangeItem extends ActivityTransactionItem { @Override public void preExecute(android.app.ClientTransactionHandler client, IBinder token) { - final ActivityClientRecord r = getActivityClientRecord(client, token, - true /* includeLaunching */); // 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(r, mConfiguration); + client.updatePendingActivityConfiguration(token, mConfiguration); } @Override diff --git a/core/java/android/app/servertransaction/ActivityTransactionItem.java b/core/java/android/app/servertransaction/ActivityTransactionItem.java index 186f25deab67e..6a6d76d202598 100644 --- a/core/java/android/app/servertransaction/ActivityTransactionItem.java +++ b/core/java/android/app/servertransaction/ActivityTransactionItem.java @@ -53,43 +53,23 @@ public abstract class ActivityTransactionItem extends ClientTransactionItem { public abstract void execute(@NonNull ClientTransactionHandler client, @NonNull ActivityClientRecord r, PendingTransactionActions pendingActions); - @NonNull ActivityClientRecord getActivityClientRecord( - @NonNull ClientTransactionHandler client, IBinder token) { - return getActivityClientRecord(client, token, false /* includeLaunching */); - } - /** * Gets the {@link ActivityClientRecord} instance that corresponds to the provided token. * @param client Target client handler. * @param token Target activity token. - * @param includeLaunching Indicate to find the {@link ActivityClientRecord} in launching - * activity list. - *

Note that there is no {@link android.app.Activity} instance in - * {@link ActivityClientRecord} from the launching activity list. * @return The {@link ActivityClientRecord} instance that corresponds to the provided token. */ @NonNull ActivityClientRecord getActivityClientRecord( - @NonNull ClientTransactionHandler client, IBinder token, boolean includeLaunching) { - ActivityClientRecord r = null; - // Check launching Activity first to prevent race condition that activity instance has not - // yet set to ActivityClientRecord. - if (includeLaunching) { - r = client.getLaunchingActivity(token); - } - // Then if we don't want to find launching Activity or the ActivityClientRecord doesn't - // exist in launching Activity list. The ActivityClientRecord should have been initialized - // and put in the Activity list. - if (r == null) { - r = client.getActivityClient(token); - if (r != null && client.getActivity(token) == null) { - throw new IllegalArgumentException("Activity must not be null to execute " - + "transaction item"); - } - } + @NonNull ClientTransactionHandler client, IBinder token) { + final ActivityClientRecord r = client.getActivityClient(token); if (r == null) { throw new IllegalArgumentException("Activity client record must not be null to execute " + "transaction item"); } + if (client.getActivity(token) == null) { + throw new IllegalArgumentException("Activity must not be null to execute " + + "transaction item"); + } return r; } } diff --git a/core/java/android/app/servertransaction/LaunchActivityItem.java b/core/java/android/app/servertransaction/LaunchActivityItem.java index 4de608b50f6ff..0a2503c962d6a 100644 --- a/core/java/android/app/servertransaction/LaunchActivityItem.java +++ b/core/java/android/app/servertransaction/LaunchActivityItem.java @@ -82,12 +82,7 @@ public class LaunchActivityItem extends ClientTransactionItem { @Override public void preExecute(ClientTransactionHandler client, IBinder token) { - ActivityClientRecord r = new ActivityClientRecord(token, mIntent, mIdent, mInfo, - mOverrideConfig, mCompatInfo, mReferrer, mVoiceInteractor, mState, mPersistentState, - mPendingResults, mPendingNewIntents, mActivityOptions, mIsForward, mProfilerInfo, - client, mAssistToken, mFixedRotationAdjustments, mShareableActivityToken, - mLaunchedFromBubble); - client.addLaunchingActivity(token, r); + client.countLaunchingActivities(1); client.updateProcessState(mProcState, false); client.updatePendingConfiguration(mCurConfig); if (mActivityClientController != null) { @@ -99,7 +94,11 @@ public class LaunchActivityItem extends ClientTransactionItem { public void execute(ClientTransactionHandler client, IBinder token, PendingTransactionActions pendingActions) { Trace.traceBegin(TRACE_TAG_ACTIVITY_MANAGER, "activityStart"); - ActivityClientRecord r = client.getLaunchingActivity(token); + ActivityClientRecord r = new ActivityClientRecord(token, mIntent, mIdent, mInfo, + mOverrideConfig, mCompatInfo, mReferrer, mVoiceInteractor, mState, mPersistentState, + mPendingResults, mPendingNewIntents, mActivityOptions, mIsForward, mProfilerInfo, + client, mAssistToken, mFixedRotationAdjustments, mShareableActivityToken, + mLaunchedFromBubble); client.handleLaunchActivity(r, pendingActions, null /* customIntent */); Trace.traceEnd(TRACE_TAG_ACTIVITY_MANAGER); } @@ -107,7 +106,7 @@ public class LaunchActivityItem extends ClientTransactionItem { @Override public void postExecute(ClientTransactionHandler client, IBinder token, PendingTransactionActions pendingActions) { - client.removeLaunchingActivity(token); + client.countLaunchingActivities(-1); } diff --git a/core/java/android/app/servertransaction/MoveToDisplayItem.java b/core/java/android/app/servertransaction/MoveToDisplayItem.java index 4b8a3476262ef..2893ff21c3f28 100644 --- a/core/java/android/app/servertransaction/MoveToDisplayItem.java +++ b/core/java/android/app/servertransaction/MoveToDisplayItem.java @@ -40,11 +40,9 @@ public class MoveToDisplayItem extends ActivityTransactionItem { @Override public void preExecute(ClientTransactionHandler client, IBinder token) { - final ActivityClientRecord r = getActivityClientRecord(client, token, - true /* includeLaunching */); // 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(r, mConfiguration); + client.updatePendingActivityConfiguration(token, mConfiguration); } @Override diff --git a/core/tests/coretests/src/android/app/activity/ActivityThreadTest.java b/core/tests/coretests/src/android/app/activity/ActivityThreadTest.java index 5db6a3e421d5f..bfb2fd57975f8 100644 --- a/core/tests/coretests/src/android/app/activity/ActivityThreadTest.java +++ b/core/tests/coretests/src/android/app/activity/ActivityThreadTest.java @@ -273,13 +273,14 @@ public class ActivityThreadTest { newerConfig.orientation = orientation == ORIENTATION_LANDSCAPE ? ORIENTATION_PORTRAIT : ORIENTATION_LANDSCAPE; newerConfig.seq = seq + 2; - final ActivityClientRecord r = getActivityClientRecord(activity); - activityThread.updatePendingActivityConfiguration(r, newerConfig); + activityThread.updatePendingActivityConfiguration(activity.getActivityToken(), + newerConfig); final Configuration olderConfig = new Configuration(); olderConfig.orientation = orientation; olderConfig.seq = seq + 1; + final ActivityClientRecord r = getActivityClientRecord(activity); activityThread.handleActivityConfigurationChanged(r, olderConfig, INVALID_DISPLAY); assertEquals(numOfConfig, activity.mNumOfConfigChanges); assertEquals(olderConfig.orientation, activity.mConfig.orientation); @@ -504,7 +505,8 @@ public class ActivityThreadTest { ? ORIENTATION_LANDSCAPE : ORIENTATION_PORTRAIT; final ActivityClientRecord r = getActivityClientRecord(activity); - activityThread.updatePendingActivityConfiguration(r, newActivityConfig); + activityThread.updatePendingActivityConfiguration(activity.getActivityToken(), + newActivityConfig); activityThread.handleActivityConfigurationChanged(r, newActivityConfig, INVALID_DISPLAY);