From 2fa48adb1ef3cd2e6987ccaa6c1395da9f792911 Mon Sep 17 00:00:00 2001 From: Eric Lin Date: Mon, 18 Nov 2024 17:51:55 +0800 Subject: Revert DeviceState callback to main thread in FFP. This partially reverts commit I8a4af1c5ac98f5653e4293a049291ba5ff7cd464. Revert device state callback to the main thread in FFP to fix crashes in Chrome and Photos. The original change allowed callbacks off the main thread but this caused crashes because: 1. Chrome loads window extension directly; it crashs when receiving callbacks on binder thread. 2. Photos uses Java API with Runnable::run, breaking the flowOn main operator; it crashs when receiving callbacks on the binder thread. Window layout info remains available via getter because the device state is available synchronously in the DeviceStateManagerGlobal constructor, so this rollback does not impact functionality. Bug: 337820752 Bug: 379501835 Test: atest WMJetpackUnitTests:DeviceStateManagerFoldingFeatureProducerTest Test: Run with latest Chrome (131.0.6778.39) Flag: com.android.window.flags.wlinfo_oncreate Change-Id: Ifc5af9955a27b6cfb5405e63c0763f3c55a5a75e --- .../DeviceStateManagerFoldingFeatureProducer.java | 65 +++++----------------- ...DeviceStateManagerFoldingFeatureProducerTest.kt | 30 +--------- 2 files changed, 14 insertions(+), 81 deletions(-) (limited to 'libs') diff --git a/libs/WindowManager/Jetpack/src/androidx/window/common/DeviceStateManagerFoldingFeatureProducer.java b/libs/WindowManager/Jetpack/src/androidx/window/common/DeviceStateManagerFoldingFeatureProducer.java index 089613853555..5ea38431829e 100644 --- a/libs/WindowManager/Jetpack/src/androidx/window/common/DeviceStateManagerFoldingFeatureProducer.java +++ b/libs/WindowManager/Jetpack/src/androidx/window/common/DeviceStateManagerFoldingFeatureProducer.java @@ -30,8 +30,6 @@ import android.text.TextUtils; import android.util.Log; import android.util.SparseIntArray; -import androidx.annotation.BinderThread; -import androidx.annotation.GuardedBy; import androidx.annotation.MainThread; import androidx.annotation.NonNull; import androidx.annotation.VisibleForTesting; @@ -39,14 +37,12 @@ import androidx.window.common.layout.CommonFoldingFeature; import androidx.window.common.layout.DisplayFoldFeatureCommon; import com.android.internal.R; -import com.android.window.flags.Flags; import java.util.ArrayList; import java.util.Collections; import java.util.List; import java.util.Objects; import java.util.Optional; -import java.util.concurrent.Executor; import java.util.function.Consumer; /** @@ -70,20 +66,8 @@ public final class DeviceStateManagerFoldingFeatureProducer * "rear display". Concurrent mode for example is activated via public API and can be active in * both the "open" and "half folded" device states. */ - // TODO: b/337820752 - Add @GuardedBy("mCurrentDeviceStateLock") after flag cleanup. private DeviceState mCurrentDeviceState = INVALID_DEVICE_STATE; - /** - * Lock to synchronize access to {@link #mCurrentDeviceState}. - * - *

This lock is used to ensure thread-safety when accessing and modifying the - * {@link #mCurrentDeviceState} field. It is acquired by both the binder thread (if - * {@link Flags#wlinfoOncreate()} is enabled) and the main thread (if - * {@link Flags#wlinfoOncreate()} is disabled) to prevent race conditions and - * ensure data consistency. - */ - private final Object mCurrentDeviceStateLock = new Object(); - @NonNull private final RawFoldingFeatureProducer mRawFoldSupplier; @@ -95,15 +79,12 @@ public final class DeviceStateManagerFoldingFeatureProducer // The GuardedBy analysis is intra-procedural, meaning it doesn’t consider the getData() // implementation. See https://errorprone.info/bugpattern/GuardedBy for limitations. @SuppressWarnings("GuardedBy") - @BinderThread // When Flags.wlinfoOncreate() is enabled. - @MainThread // When Flags.wlinfoOncreate() is disabled. + @MainThread @Override public void onDeviceStateChanged(@NonNull DeviceState state) { - synchronized (mCurrentDeviceStateLock) { - mCurrentDeviceState = state; - mRawFoldSupplier.getData(DeviceStateManagerFoldingFeatureProducer.this - ::notifyFoldingFeatureChangeLocked); - } + mCurrentDeviceState = state; + mRawFoldSupplier.getData(DeviceStateManagerFoldingFeatureProducer.this + ::notifyFoldingFeatureChangeLocked); } }; @@ -115,10 +96,8 @@ public final class DeviceStateManagerFoldingFeatureProducer new DeviceStateMapper(context, deviceStateManager.getSupportedDeviceStates()); if (!mDeviceStateMapper.isDeviceStateToPostureMapEmpty()) { - final Executor executor = - Flags.wlinfoOncreate() ? Runnable::run : context.getMainExecutor(); Objects.requireNonNull(deviceStateManager) - .registerCallback(executor, mDeviceStateCallback); + .registerCallback(context.getMainExecutor(), mDeviceStateCallback); } } @@ -145,21 +124,17 @@ public final class DeviceStateManagerFoldingFeatureProducer @Override protected void onListenersChanged() { super.onListenersChanged(); - synchronized (mCurrentDeviceStateLock) { - if (hasListeners()) { - mRawFoldSupplier.addDataChangedCallback(this::notifyFoldingFeatureChangeLocked); - } else { - mCurrentDeviceState = INVALID_DEVICE_STATE; - mRawFoldSupplier.removeDataChangedCallback(this::notifyFoldingFeatureChangeLocked); - } + if (hasListeners()) { + mRawFoldSupplier.addDataChangedCallback(this::notifyFoldingFeatureChangeLocked); + } else { + mCurrentDeviceState = INVALID_DEVICE_STATE; + mRawFoldSupplier.removeDataChangedCallback(this::notifyFoldingFeatureChangeLocked); } } @NonNull private DeviceState getCurrentDeviceState() { - synchronized (mCurrentDeviceStateLock) { - return mCurrentDeviceState; - } + return mCurrentDeviceState; } @NonNull @@ -231,7 +206,6 @@ public final class DeviceStateManagerFoldingFeatureProducer }); } - @GuardedBy("mCurrentDeviceStateLock") private void notifyFoldingFeatureChangeLocked(String displayFeaturesString) { final DeviceState state = mCurrentDeviceState; if (!mDeviceStateMapper.isDeviceStateValid(state)) { @@ -252,29 +226,16 @@ public final class DeviceStateManagerFoldingFeatureProducer return parseListFromString(displayFeaturesString, hingeState); } - /** - * Internal class to map device states to corresponding postures. - * - *

This class encapsulates the logic for mapping device states to postures. The mapping is - * immutable after initialization to ensure thread safety. - */ + /** Internal class to map device states to corresponding postures. */ private static class DeviceStateMapper { /** * Emulated device state * {@link DeviceStateManager.DeviceStateCallback#onDeviceStateChanged(DeviceState)} to * {@link CommonFoldingFeature.State} map. - * - *

This map must be immutable after initialization to ensure thread safety, as it may be - * accessed from multiple threads. Modifications should only occur during object - * construction. */ private final SparseIntArray mDeviceStateToPostureMap = new SparseIntArray(); - /** - * The list of device states that are supported. - * - *

This list must be immutable after initialization to ensure thread safety. - */ + /** The list of device states that are supported. */ @NonNull private final List mSupportedStates; diff --git a/libs/WindowManager/Jetpack/tests/unittest/src/androidx/window/common/DeviceStateManagerFoldingFeatureProducerTest.kt b/libs/WindowManager/Jetpack/tests/unittest/src/androidx/window/common/DeviceStateManagerFoldingFeatureProducerTest.kt index 90887a747a6f..fb01cd88158c 100644 --- a/libs/WindowManager/Jetpack/tests/unittest/src/androidx/window/common/DeviceStateManagerFoldingFeatureProducerTest.kt +++ b/libs/WindowManager/Jetpack/tests/unittest/src/androidx/window/common/DeviceStateManagerFoldingFeatureProducerTest.kt @@ -20,9 +20,6 @@ import android.content.Context import android.content.res.Resources import android.hardware.devicestate.DeviceState import android.hardware.devicestate.DeviceStateManager -import android.platform.test.annotations.DisableFlags -import android.platform.test.annotations.EnableFlags -import android.platform.test.flag.junit.SetFlagsRule import androidx.test.ext.junit.runners.AndroidJUnit4 import androidx.window.common.layout.CommonFoldingFeature import androidx.window.common.layout.CommonFoldingFeature.COMMON_STATE_FLAT @@ -34,15 +31,11 @@ import androidx.window.common.layout.DisplayFoldFeatureCommon import androidx.window.common.layout.DisplayFoldFeatureCommon.DISPLAY_FOLD_FEATURE_PROPERTY_SUPPORTS_HALF_OPENED import androidx.window.common.layout.DisplayFoldFeatureCommon.DISPLAY_FOLD_FEATURE_TYPE_SCREEN_FOLD_IN import com.android.internal.R -import com.android.window.flags.Flags import com.google.common.truth.Truth.assertThat import java.util.Optional -import java.util.concurrent.Executor import java.util.function.Consumer -import org.junit.Rule import org.junit.Test import org.junit.runner.RunWith -import org.mockito.ArgumentCaptor import org.mockito.kotlin.any import org.mockito.kotlin.doAnswer import org.mockito.kotlin.doReturn @@ -60,9 +53,6 @@ import org.mockito.kotlin.verify */ @RunWith(AndroidJUnit4::class) class DeviceStateManagerFoldingFeatureProducerTest { - @get:Rule - val setFlagsRule: SetFlagsRule = SetFlagsRule() - private val mMockDeviceStateManager = mock() private val mMockResources = mock { on { getStringArray(R.array.config_device_state_postures) } doReturn DEVICE_STATE_POSTURES @@ -79,8 +69,7 @@ class DeviceStateManagerFoldingFeatureProducerTest { } @Test - @DisableFlags(Flags.FLAG_WLINFO_ONCREATE) - fun testRegisterCallback_whenWlinfoOncreateIsDisabled_usesMainExecutor() { + fun testRegisterCallback_usesMainExecutor() { DeviceStateManagerFoldingFeatureProducer( mMockContext, mRawFoldSupplier, @@ -90,23 +79,6 @@ class DeviceStateManagerFoldingFeatureProducerTest { verify(mMockDeviceStateManager).registerCallback(eq(mMockContext.mainExecutor), any()) } - @Test - @EnableFlags(Flags.FLAG_WLINFO_ONCREATE) - fun testRegisterCallback_whenWlinfoOncreateIsEnabled_usesRunnableRun() { - val executorCaptor = ArgumentCaptor.forClass(Executor::class.java) - val runnable = mock() - - DeviceStateManagerFoldingFeatureProducer( - mMockContext, - mRawFoldSupplier, - mMockDeviceStateManager, - ) - - verify(mMockDeviceStateManager).registerCallback(executorCaptor.capture(), any()) - executorCaptor.value.execute(runnable) - verify(runnable).run() - } - @Test fun testGetCurrentData_validCurrentState_returnsFoldingFeatureWithState() { val ffp = DeviceStateManagerFoldingFeatureProducer( -- cgit v1.2.3-59-g8ed1b