From 91d351bf67091615e9c143a1342d228d70669db4 Mon Sep 17 00:00:00 2001 From: Tyler Freeman Date: Mon, 8 May 2023 12:41:01 -0700 Subject: [PATCH] fix(magnifier): fix deadlock when feature flag changes Handle feature flag changes on a background thread so we're not blocking the main thread which might already be locked. Fix: 281132229 Test: 0. Complete initial SUW without connecting WiFi, no account added and no lock screen set. 1. On target device, tap on deferred SUW notification and proceed. 2. Tap on "Get started" on Welcome screen. Quick Start User Guide screen is displayed. 3. Connect source and target devices by scanning the QR Code with scanner 4. WiFi setting is transferred and the target device connected to internet. Getting your phone ready screen is shown 6. Wait and observe. SysUi should not hang / black screen. Change-Id: Ic2cf5cfb34041671468f0036a243e24005d064c9 --- .../AccessibilityManagerService.java | 10 +++++-- .../FullScreenMagnificationController.java | 13 ++++++---- .../MagnificationController.java | 26 +++++++++++++------ .../MagnificationFeatureFlagBase.java | 3 +++ ...FullScreenMagnificationControllerTest.java | 4 ++- ...ScreenMagnificationGestureHandlerTest.java | 5 ++-- .../MagnificationControllerTest.java | 7 +++-- 7 files changed, 48 insertions(+), 20 deletions(-) diff --git a/services/accessibility/java/com/android/server/accessibility/AccessibilityManagerService.java b/services/accessibility/java/com/android/server/accessibility/AccessibilityManagerService.java index 2d1290caf32d8..ad038d1c03615 100644 --- a/services/accessibility/java/com/android/server/accessibility/AccessibilityManagerService.java +++ b/services/accessibility/java/com/android/server/accessibility/AccessibilityManagerService.java @@ -179,6 +179,7 @@ import java.util.Iterator; import java.util.List; import java.util.Map; import java.util.Set; +import java.util.concurrent.Executors; import java.util.function.Consumer; import java.util.function.Function; import java.util.function.Predicate; @@ -494,8 +495,13 @@ public class AccessibilityManagerService extends IAccessibilityManager.Stub mA11yWindowManager = new AccessibilityWindowManager(mLock, mMainHandler, mWindowManagerService, this, mSecurityPolicy, this, mTraceManager); mA11yDisplayListener = new AccessibilityDisplayListener(mContext, mMainHandler); - mMagnificationController = new MagnificationController(this, mLock, mContext, - new MagnificationScaleProvider(mContext)); + mMagnificationController = new MagnificationController( + this, + mLock, + mContext, + new MagnificationScaleProvider(mContext), + Executors.newSingleThreadExecutor() + ); mMagnificationProcessor = new MagnificationProcessor(mMagnificationController); mCaptioningManagerImpl = new CaptioningManagerImpl(mContext); mProxyManager = new ProxyManager(mLock, mA11yWindowManager, mContext, mMainHandler, diff --git a/services/accessibility/java/com/android/server/accessibility/magnification/FullScreenMagnificationController.java b/services/accessibility/java/com/android/server/accessibility/magnification/FullScreenMagnificationController.java index 874fb0164b017..b1cdc5053c27f 100644 --- a/services/accessibility/java/com/android/server/accessibility/magnification/FullScreenMagnificationController.java +++ b/services/accessibility/java/com/android/server/accessibility/magnification/FullScreenMagnificationController.java @@ -56,7 +56,6 @@ import com.android.internal.R; import com.android.internal.accessibility.common.MagnificationConstants; import com.android.internal.annotations.GuardedBy; import com.android.internal.annotations.VisibleForTesting; -import com.android.internal.util.ConcurrentUtils; import com.android.internal.util.function.pooled.PooledLambda; import com.android.server.LocalServices; import com.android.server.accessibility.AccessibilityManagerService; @@ -65,6 +64,7 @@ import com.android.server.wm.WindowManagerInternal; import java.util.ArrayList; import java.util.Locale; +import java.util.concurrent.Executor; import java.util.function.Supplier; /** @@ -774,7 +774,8 @@ public class FullScreenMagnificationController implements public FullScreenMagnificationController(@NonNull Context context, @NonNull AccessibilityTraceManager traceManager, @NonNull Object lock, @NonNull MagnificationInfoChangedCallback magnificationInfoChangedCallback, - @NonNull MagnificationScaleProvider scaleProvider) { + @NonNull MagnificationScaleProvider scaleProvider, + @NonNull Executor backgroundExecutor) { this( new ControllerContext( context, @@ -785,7 +786,8 @@ public class FullScreenMagnificationController implements lock, magnificationInfoChangedCallback, scaleProvider, - /* thumbnailSupplier= */ null); + /* thumbnailSupplier= */ null, + backgroundExecutor); } /** Constructor for tests */ @@ -795,7 +797,8 @@ public class FullScreenMagnificationController implements @NonNull Object lock, @NonNull MagnificationInfoChangedCallback magnificationInfoChangedCallback, @NonNull MagnificationScaleProvider scaleProvider, - Supplier thumbnailSupplier) { + Supplier thumbnailSupplier, + @NonNull Executor backgroundExecutor) { mControllerCtx = ctx; mLock = lock; mMainThreadId = mControllerCtx.getContext().getMainLooper().getThread().getId(); @@ -805,7 +808,7 @@ public class FullScreenMagnificationController implements mDisplayManagerInternal = LocalServices.getService(DisplayManagerInternal.class); mMagnificationThumbnailFeatureFlag = new MagnificationThumbnailFeatureFlag(); mMagnificationThumbnailFeatureFlag.addOnChangedListener( - ConcurrentUtils.DIRECT_EXECUTOR, this::onMagnificationThumbnailFeatureFlagChanged); + backgroundExecutor, this::onMagnificationThumbnailFeatureFlagChanged); if (thumbnailSupplier != null) { mThumbnailSupplier = thumbnailSupplier; } else { diff --git a/services/accessibility/java/com/android/server/accessibility/magnification/MagnificationController.java b/services/accessibility/java/com/android/server/accessibility/magnification/MagnificationController.java index fee20c89c106e..ee465b85b0a29 100644 --- a/services/accessibility/java/com/android/server/accessibility/magnification/MagnificationController.java +++ b/services/accessibility/java/com/android/server/accessibility/magnification/MagnificationController.java @@ -23,7 +23,6 @@ import static android.provider.Settings.Secure.ACCESSIBILITY_MAGNIFICATION_MODE_ import static android.provider.Settings.Secure.ACCESSIBILITY_MAGNIFICATION_MODE_FULLSCREEN; import static android.provider.Settings.Secure.ACCESSIBILITY_MAGNIFICATION_MODE_NONE; import static android.provider.Settings.Secure.ACCESSIBILITY_MAGNIFICATION_MODE_WINDOW; -import static android.view.accessibility.MagnificationAnimationCallback.STUB_ANIMATION_CALLBACK; import static com.android.server.accessibility.AccessibilityManagerService.MAGNIFICATION_GESTURE_HANDLER_ID; @@ -48,11 +47,12 @@ import android.view.accessibility.MagnificationAnimationCallback; import com.android.internal.accessibility.util.AccessibilityStatsLogUtils; import com.android.internal.annotations.GuardedBy; import com.android.internal.annotations.VisibleForTesting; -import com.android.internal.util.ConcurrentUtils; import com.android.server.LocalServices; import com.android.server.accessibility.AccessibilityManagerService; import com.android.server.wm.WindowManagerInternal; +import java.util.concurrent.Executor; + /** * Handles all magnification controllers initialization, generic interactions, * magnification mode transition and magnification switch UI show/hide logic @@ -101,6 +101,8 @@ public class MagnificationController implements WindowMagnificationManager.Callb /** Whether the platform supports window magnification feature. */ private final boolean mSupportWindowMagnification; + private final Executor mBackgroundExecutor; + @GuardedBy("mLock") private final SparseIntArray mCurrentMagnificationModeArray = new SparseIntArray(); @GuardedBy("mLock") @@ -142,11 +144,13 @@ public class MagnificationController implements WindowMagnificationManager.Callb } public MagnificationController(AccessibilityManagerService ams, Object lock, - Context context, MagnificationScaleProvider scaleProvider) { + Context context, MagnificationScaleProvider scaleProvider, + Executor backgroundExecutor) { mAms = ams; mLock = lock; mContext = context; mScaleProvider = scaleProvider; + mBackgroundExecutor = backgroundExecutor; LocalServices.getService(WindowManagerInternal.class) .getAccessibilityController().setUiChangesForAccessibilityCallbacks(this); mSupportWindowMagnification = context.getPackageManager().hasSystemFeature( @@ -154,15 +158,15 @@ public class MagnificationController implements WindowMagnificationManager.Callb mAlwaysOnMagnificationFeatureFlag = new AlwaysOnMagnificationFeatureFlag(); mAlwaysOnMagnificationFeatureFlag.addOnChangedListener( - ConcurrentUtils.DIRECT_EXECUTOR, mAms::updateAlwaysOnMagnification); + mBackgroundExecutor, mAms::updateAlwaysOnMagnification); } @VisibleForTesting public MagnificationController(AccessibilityManagerService ams, Object lock, Context context, FullScreenMagnificationController fullScreenMagnificationController, WindowMagnificationManager windowMagnificationManager, - MagnificationScaleProvider scaleProvider) { - this(ams, lock, context, scaleProvider); + MagnificationScaleProvider scaleProvider, Executor backgroundExecutor) { + this(ams, lock, context, scaleProvider, backgroundExecutor); mFullScreenMagnificationController = fullScreenMagnificationController; mWindowMagnificationMgr = windowMagnificationManager; } @@ -757,8 +761,14 @@ public class MagnificationController implements WindowMagnificationManager.Callb public FullScreenMagnificationController getFullScreenMagnificationController() { synchronized (mLock) { if (mFullScreenMagnificationController == null) { - mFullScreenMagnificationController = new FullScreenMagnificationController(mContext, - mAms.getTraceManager(), mLock, this, mScaleProvider); + mFullScreenMagnificationController = new FullScreenMagnificationController( + mContext, + mAms.getTraceManager(), + mLock, + this, + mScaleProvider, + mBackgroundExecutor + ); } } return mFullScreenMagnificationController; diff --git a/services/accessibility/java/com/android/server/accessibility/magnification/MagnificationFeatureFlagBase.java b/services/accessibility/java/com/android/server/accessibility/magnification/MagnificationFeatureFlagBase.java index 2965887d8eb3c..522f49d092da8 100644 --- a/services/accessibility/java/com/android/server/accessibility/magnification/MagnificationFeatureFlagBase.java +++ b/services/accessibility/java/com/android/server/accessibility/magnification/MagnificationFeatureFlagBase.java @@ -80,6 +80,9 @@ abstract class MagnificationFeatureFlagBase { * *

{@see DeviceConfig#addOnPropertiesChangedListener( * String, Executor, DeviceConfig.OnPropertiesChangedListener)} + * + *

Note: be weary of using a DIRECT_EXECUTOR here. You may run into deadlocks! (see + * b/281132229) */ @NonNull public DeviceConfig.OnPropertiesChangedListener addOnChangedListener( diff --git a/services/tests/servicestests/src/com/android/server/accessibility/magnification/FullScreenMagnificationControllerTest.java b/services/tests/servicestests/src/com/android/server/accessibility/magnification/FullScreenMagnificationControllerTest.java index a01c7bdd51444..e6ef044d47916 100644 --- a/services/tests/servicestests/src/com/android/server/accessibility/magnification/FullScreenMagnificationControllerTest.java +++ b/services/tests/servicestests/src/com/android/server/accessibility/magnification/FullScreenMagnificationControllerTest.java @@ -60,6 +60,7 @@ import android.view.accessibility.MagnificationAnimationCallback; import androidx.test.InstrumentationRegistry; import androidx.test.runner.AndroidJUnit4; +import com.android.internal.util.ConcurrentUtils; import com.android.internal.util.test.FakeSettingsProvider; import com.android.server.LocalServices; import com.android.server.accessibility.AccessibilityTraceManager; @@ -160,7 +161,8 @@ public class FullScreenMagnificationControllerTest { new Object(), mRequestObserver, mScaleProvider, - () -> mMockThumbnail); + () -> mMockThumbnail, + ConcurrentUtils.DIRECT_EXECUTOR); } @After diff --git a/services/tests/servicestests/src/com/android/server/accessibility/magnification/FullScreenMagnificationGestureHandlerTest.java b/services/tests/servicestests/src/com/android/server/accessibility/magnification/FullScreenMagnificationGestureHandlerTest.java index cf5ba270999e7..2d4bf144155c1 100644 --- a/services/tests/servicestests/src/com/android/server/accessibility/magnification/FullScreenMagnificationGestureHandlerTest.java +++ b/services/tests/servicestests/src/com/android/server/accessibility/magnification/FullScreenMagnificationGestureHandlerTest.java @@ -54,6 +54,7 @@ import android.view.ViewConfiguration; import androidx.test.runner.AndroidJUnit4; +import com.android.internal.util.ConcurrentUtils; import com.android.server.accessibility.AccessibilityManagerService; import com.android.server.accessibility.AccessibilityTraceManager; import com.android.server.accessibility.EventStreamTransformation; @@ -170,8 +171,8 @@ public class FullScreenMagnificationGestureHandlerTest { new Object(), mMagnificationInfoChangedCallback, new MagnificationScaleProvider(mContext), - () -> null - ) { + () -> null, + ConcurrentUtils.DIRECT_EXECUTOR) { @Override public boolean magnificationRegionContains(int displayId, float x, float y) { return true; diff --git a/services/tests/servicestests/src/com/android/server/accessibility/magnification/MagnificationControllerTest.java b/services/tests/servicestests/src/com/android/server/accessibility/magnification/MagnificationControllerTest.java index 5f2db795f8bc7..f94ac98c3007e 100644 --- a/services/tests/servicestests/src/com/android/server/accessibility/magnification/MagnificationControllerTest.java +++ b/services/tests/servicestests/src/com/android/server/accessibility/magnification/MagnificationControllerTest.java @@ -64,6 +64,7 @@ import androidx.annotation.NonNull; import androidx.test.InstrumentationRegistry; import androidx.test.runner.AndroidJUnit4; +import com.android.internal.util.ConcurrentUtils; import com.android.internal.util.test.FakeSettingsProvider; import com.android.server.LocalServices; import com.android.server.accessibility.AccessibilityManagerService; @@ -199,7 +200,8 @@ public class MagnificationControllerTest { new Object(), mScreenMagnificationInfoChangedCallbackDelegate, mScaleProvider, - () -> null + () -> null, + ConcurrentUtils.DIRECT_EXECUTOR )); mScreenMagnificationController.register(TEST_DISPLAY); @@ -209,7 +211,8 @@ public class MagnificationControllerTest { mWindowMagnificationManager.setConnection(mMockConnection.getConnection()); mMagnificationController = spy(new MagnificationController(mService, globalLock, mContext, - mScreenMagnificationController, mWindowMagnificationManager, mScaleProvider)); + mScreenMagnificationController, mWindowMagnificationManager, mScaleProvider, + ConcurrentUtils.DIRECT_EXECUTOR)); mMagnificationController.setMagnificationCapabilities( Settings.Secure.ACCESSIBILITY_MAGNIFICATION_MODE_ALL);