Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit f2ae0e31 authored by Chris Li's avatar Chris Li
Browse files

Fix ConcurrentModificationException on WindowToken 

Because there can be multiple different UI threads for different
View windows, the corresponding configuration changed callback can be
triggered in different threads.

Add lock to make it thread-safe.

Fix: 335967960
Test: Refactor pass existing tests
Flag: com.android.window.flags.window_token_config_thread_safe
Change-Id: I504731bf60f029010ebd370bb2b2e99199998f2f
parent e70357a6

core/java/android/app/servertransaction/ClientTransactionListenerController.java

+58 −55
Original line number Diff line number Diff line
@@ -17,14 +17,13 @@ 
package android.app.servertransaction;



import static android.app.WindowConfiguration.areConfigurationsEqualForDisplay;

import static android.view.Display.INVALID_DISPLAY;



import static com.android.window.flags.Flags.activityWindowInfoFlag;

import static com.android.window.flags.Flags.bundleClientTransactionFlag;



import static java.util.Objects.requireNonNull;



import android.annotation.AnyThread;

import android.annotation.MainThread;

import android.annotation.NonNull;

import android.app.Activity;

import android.app.ActivityThread;
@@ -62,9 +61,11 @@ public class ClientTransactionListenerController { 
     * Keeps track of the Context whose Configuration will get updated, mapping to the config before

     * the change.

     */

    @GuardedBy("mLock")

    private final ArrayMap<Context, Configuration> mContextToPreChangedConfigMap = new ArrayMap<>();



    /** Whether there is an {@link ClientTransaction} being executed. */

    @GuardedBy("mLock")

    private boolean mIsClientTransactionExecuting;



    /** Gets the singleton controller. */
@@ -96,7 +97,6 @@ public class ClientTransactionListenerController { 
     * The listener will be invoked with two parameters: {@link Activity#getActivityToken()} and

     * {@link ActivityWindowInfo}.

     */

    @AnyThread

    public void registerActivityWindowInfoChangedListener(

            @NonNull BiConsumer<IBinder, ActivityWindowInfo> listener) {

        if (!activityWindowInfoFlag()) {
@@ -111,7 +111,6 @@ public class ClientTransactionListenerController { 
     * Unregisters the listener that was previously registered via

     * {@link #registerActivityWindowInfoChangedListener(BiConsumer)}

     */

    @AnyThread

    public void unregisterActivityWindowInfoChangedListener(

            @NonNull BiConsumer<IBinder, ActivityWindowInfo> listener) {

        if (!activityWindowInfoFlag()) {
@@ -126,7 +125,6 @@ public class ClientTransactionListenerController { 
     * Called when receives a {@link ClientTransaction} that is updating an activity's

     * {@link ActivityWindowInfo}.

     */

    @MainThread

    public void onActivityWindowInfoChanged(@NonNull IBinder activityToken,

            @NonNull ActivityWindowInfo activityWindowInfo) {

        if (!activityWindowInfoFlag()) {
@@ -146,25 +144,55 @@ public class ClientTransactionListenerController { 
    }



    /** Called when starts executing a remote {@link ClientTransaction}. */

    @MainThread

    public void onClientTransactionStarted() {

        synchronized (mLock) {

            mIsClientTransactionExecuting = true;

        }

    }



    /** Called when finishes executing a remote {@link ClientTransaction}. */

    @MainThread

    public void onClientTransactionFinished() {

        notifyDisplayManagerIfNeeded();

        final ArraySet<Integer> configUpdatedDisplayIds;

        synchronized (mLock) {

            mIsClientTransactionExecuting = false;



            // When {@link Configuration} is changed, we want to trigger display change callback as

            // well, because Display reads some fields from {@link Configuration}.

            if (mContextToPreChangedConfigMap.isEmpty()) {

                return;

            }



            // Calculate display ids that have config changed.

            configUpdatedDisplayIds = new ArraySet<>();

            final int contextCount = mContextToPreChangedConfigMap.size();

            try {

                for (int i = 0; i < contextCount; i++) {

                    final Context context = mContextToPreChangedConfigMap.keyAt(i);

                    final Configuration preChangedConfig = mContextToPreChangedConfigMap.valueAt(i);

                    if (shouldReportDisplayChange(context, preChangedConfig)) {

                        configUpdatedDisplayIds.add(context.getDisplayId());

                    }

                }

            } finally {

                mContextToPreChangedConfigMap.clear();

            }

        }



        // Dispatch the display changed callbacks.

        final int displayCount = configUpdatedDisplayIds.size();

        for (int i = 0; i < displayCount; i++) {

            final int displayId = configUpdatedDisplayIds.valueAt(i);

            onDisplayChanged(displayId);

        }

    }



    /** Called before updating the Configuration of the given {@code context}. */

    @MainThread

    public void onContextConfigurationPreChanged(@NonNull Context context) {

        if (!bundleClientTransactionFlag() || ActivityThread.isSystem()) {

            // Not enable for system server.

            return;

        }

        synchronized (mLock) {

            if (mContextToPreChangedConfigMap.containsKey(context)) {

                // There is an earlier change that hasn't been reported yet.

                return;
@@ -172,14 +200,16 @@ public class ClientTransactionListenerController { 
            mContextToPreChangedConfigMap.put(context,

                    new Configuration(context.getResources().getConfiguration()));

        }

    }



    /** Called after updating the Configuration of the given {@code context}. */

    @MainThread

    public void onContextConfigurationPostChanged(@NonNull Context context) {

        if (!bundleClientTransactionFlag() || ActivityThread.isSystem()) {

            // Not enable for system server.

            return;

        }

        int changedDisplayId = INVALID_DISPLAY;

        synchronized (mLock) {

            if (mIsClientTransactionExecuting) {

                // Wait until #onClientTransactionFinished to prevent it from triggering the same

                // #onDisplayChanged multiple times within the same ClientTransaction.
@@ -187,39 +217,12 @@ public class ClientTransactionListenerController { 
            }

            final Configuration preChangedConfig = mContextToPreChangedConfigMap.remove(context);

            if (preChangedConfig != null && shouldReportDisplayChange(context, preChangedConfig)) {

            onDisplayChanged(context.getDisplayId());

                changedDisplayId = context.getDisplayId();

            }

        }



    /**

     * When {@link Configuration} is changed, we want to trigger display change callback as well,

     * because Display reads some fields from {@link Configuration}.

     */

    private void notifyDisplayManagerIfNeeded() {

        if (mContextToPreChangedConfigMap.isEmpty()) {

            return;

        }

        // Whether the configuration change should trigger DisplayListener#onDisplayChanged.

        try {

            // Calculate display ids that have config changed.

            final ArraySet<Integer> configUpdatedDisplayIds = new ArraySet<>();

            final int contextCount = mContextToPreChangedConfigMap.size();

            for (int i = 0; i < contextCount; i++) {

                final Context context = mContextToPreChangedConfigMap.keyAt(i);

                final Configuration preChangedConfig = mContextToPreChangedConfigMap.valueAt(i);

                if (shouldReportDisplayChange(context, preChangedConfig)) {

                    configUpdatedDisplayIds.add(context.getDisplayId());

                }

            }



            // Dispatch the display changed callbacks.

            final int displayCount = configUpdatedDisplayIds.size();

            for (int i = 0; i < displayCount; i++) {

                final int displayId = configUpdatedDisplayIds.valueAt(i);

                onDisplayChanged(displayId);

            }

        } finally {

            mContextToPreChangedConfigMap.clear();

        if (changedDisplayId != INVALID_DISPLAY) {

            onDisplayChanged(changedDisplayId);

        }

    }

core/java/android/window/WindowTokenClient.java

+4 −3
Original line number Diff line number Diff line
@@ -19,6 +19,8 @@ import static android.window.ConfigurationHelper.freeTextLayoutCachesIfNeeded; 
import static android.window.ConfigurationHelper.isDifferentDisplay;

import static android.window.ConfigurationHelper.shouldUpdateResources;



import static com.android.window.flags.Flags.windowTokenConfigThreadSafe;



import android.annotation.AnyThread;

import android.annotation.MainThread;

import android.annotation.NonNull;
@@ -144,9 +146,8 @@ public class WindowTokenClient extends Binder { 
        if (context == null) {

            return;

        }

        if (shouldReportConfigChange) {

            // Only report to ClientTransactionListenerController when shouldReportConfigChange,

            // which is on the MainThread.

        if (shouldReportConfigChange && windowTokenConfigThreadSafe()) {

            // Only report to ClientTransactionListenerController when shouldReportConfigChange.

            final ClientTransactionListenerController controller =

                    getClientTransactionListenerController();

            controller.onContextConfigurationPreChanged(context);

core/java/android/window/flags/windowing_sdk.aconfig

+10 −0
Original line number Diff line number Diff line
@@ -91,6 +91,16 @@ flag { 
    is_fixed_read_only: true

}



flag {

    namespace: "windowing_sdk"

    name: "window_token_config_thread_safe"

    description: "Ensure the Configuration pre/post changed is thread safe"

    bug: "334285008"

    metadata {

        purpose: PURPOSE_BUGFIX

    }

}



flag {

    namespace: "windowing_sdk"

    name: "always_defer_transition_when_apply_wct"

core/tests/coretests/src/android/app/servertransaction/ClientTransactionListenerControllerTest.java

+3 −0
Original line number Diff line number Diff line
@@ -21,6 +21,7 @@ import static android.platform.test.flag.junit.SetFlagsRule.DefaultInitValueType 
import static androidx.test.platform.app.InstrumentationRegistry.getInstrumentation;



import static com.android.window.flags.Flags.FLAG_BUNDLE_CLIENT_TRANSACTION_FLAG;

import static com.android.window.flags.Flags.FLAG_WINDOW_TOKEN_CONFIG_THREAD_SAFE;



import static org.mockito.ArgumentMatchers.any;

import static org.mockito.ArgumentMatchers.anyBoolean;
@@ -189,6 +190,8 @@ public class ClientTransactionListenerControllerTest { 


    @Test

    public void testWindowTokenClient_onConfigurationChanged() {

        mSetFlagsRule.enableFlags(FLAG_WINDOW_TOKEN_CONFIG_THREAD_SAFE);



        doNothing().when(mController).onContextConfigurationPreChanged(any());

        doNothing().when(mController).onContextConfigurationPostChanged(any());