Improve DreamOverlayService thread safety.
Local states like the lifecycle state should and are now only accessed from the main thread. Also add "locked" to names of local functions that should only be called synchronously.
Test: atest DreamOverlayServiceTest
Bug: 255203719
Change-Id: Iaa7f952e84b5c4c33f48238baeac0a4124233e2c
diff --git a/packages/SystemUI/src/com/android/systemui/dreams/DreamOverlayService.java b/packages/SystemUI/src/com/android/systemui/dreams/DreamOverlayService.java
index d1b7368..1e53909 100644
--- a/packages/SystemUI/src/com/android/systemui/dreams/DreamOverlayService.java
+++ b/packages/SystemUI/src/com/android/systemui/dreams/DreamOverlayService.java
@@ -90,13 +90,15 @@
new KeyguardUpdateMonitorCallback() {
@Override
public void onShadeExpandedChanged(boolean expanded) {
- if (mLifecycleRegistry.getCurrentState() != Lifecycle.State.RESUMED
- && mLifecycleRegistry.getCurrentState() != Lifecycle.State.STARTED) {
- return;
- }
+ mExecutor.execute(() -> {
+ if (getCurrentStateLocked() != Lifecycle.State.RESUMED
+ && getCurrentStateLocked() != Lifecycle.State.STARTED) {
+ return;
+ }
- mLifecycleRegistry.setCurrentState(
- expanded ? Lifecycle.State.STARTED : Lifecycle.State.RESUMED);
+ setCurrentStateLocked(
+ expanded ? Lifecycle.State.STARTED : Lifecycle.State.RESUMED);
+ });
}
};
@@ -146,29 +148,30 @@
() -> mExecutor.execute(DreamOverlayService.this::requestExit);
mDreamOverlayComponent = dreamOverlayComponentFactory.create(viewModelStore, host);
mLifecycleRegistry = mDreamOverlayComponent.getLifecycleRegistry();
- setCurrentState(Lifecycle.State.CREATED);
- }
- private void setCurrentState(Lifecycle.State state) {
- mExecutor.execute(() -> mLifecycleRegistry.setCurrentState(state));
+ mExecutor.execute(() -> setCurrentStateLocked(Lifecycle.State.CREATED));
}
@Override
public void onDestroy() {
mKeyguardUpdateMonitor.removeCallback(mKeyguardCallback);
- setCurrentState(Lifecycle.State.DESTROYED);
- resetCurrentDreamOverlay();
+ mExecutor.execute(() -> {
+ setCurrentStateLocked(Lifecycle.State.DESTROYED);
- mDestroyed = true;
+ resetCurrentDreamOverlayLocked();
+
+ mDestroyed = true;
+ });
+
super.onDestroy();
}
@Override
public void onStartDream(@NonNull WindowManager.LayoutParams layoutParams) {
- setCurrentState(Lifecycle.State.STARTED);
-
mExecutor.execute(() -> {
+ setCurrentStateLocked(Lifecycle.State.STARTED);
+
mUiEventLogger.log(DreamOverlayEvent.DREAM_OVERLAY_ENTER_START);
if (mDestroyed) {
@@ -181,7 +184,7 @@
// Reset the current dream overlay before starting a new one. This can happen
// when two dreams overlap (briefly, for a smoother dream transition) and both
// dreams are bound to the dream overlay service.
- resetCurrentDreamOverlay();
+ resetCurrentDreamOverlayLocked();
}
mDreamOverlayContainerViewController =
@@ -191,7 +194,7 @@
mStateController.setShouldShowComplications(shouldShowComplications());
addOverlayWindowLocked(layoutParams);
- setCurrentState(Lifecycle.State.RESUMED);
+ setCurrentStateLocked(Lifecycle.State.RESUMED);
mStateController.setOverlayActive(true);
final ComponentName dreamComponent = getDreamComponent();
mStateController.setLowLightActive(
@@ -202,6 +205,14 @@
});
}
+ private Lifecycle.State getCurrentStateLocked() {
+ return mLifecycleRegistry.getCurrentState();
+ }
+
+ private void setCurrentStateLocked(Lifecycle.State state) {
+ mLifecycleRegistry.setCurrentState(state);
+ }
+
/**
* Inserts {@link Window} to host the dream overlay into the dream's parent window. Must be
* called from the main executing thread. The window attributes closely mirror those that are
@@ -231,13 +242,13 @@
// Make extra sure the container view has been removed from its old parent (otherwise we
// risk an IllegalStateException in some cases when setting the container view as the
// window's content view and the container view hasn't been properly removed previously).
- removeContainerViewFromParent();
+ removeContainerViewFromParentLocked();
mWindow.setContentView(mDreamOverlayContainerViewController.getContainerView());
mWindowManager.addView(mWindow.getDecorView(), mWindow.getAttributes());
}
- private void removeContainerViewFromParent() {
+ private void removeContainerViewFromParentLocked() {
View containerView = mDreamOverlayContainerViewController.getContainerView();
if (containerView == null) {
return;
@@ -250,7 +261,7 @@
parentView.removeView(containerView);
}
- private void resetCurrentDreamOverlay() {
+ private void resetCurrentDreamOverlayLocked() {
if (mStarted && mWindow != null) {
mWindowManager.removeView(mWindow.getDecorView());
}
diff --git a/packages/SystemUI/tests/src/com/android/systemui/dreams/DreamOverlayServiceTest.java b/packages/SystemUI/tests/src/com/android/systemui/dreams/DreamOverlayServiceTest.java
index f370be1..2cdbef7 100644
--- a/packages/SystemUI/tests/src/com/android/systemui/dreams/DreamOverlayServiceTest.java
+++ b/packages/SystemUI/tests/src/com/android/systemui/dreams/DreamOverlayServiceTest.java
@@ -273,27 +273,31 @@
@Test
public void testDecorViewNotAddedToWindowAfterDestroy() throws Exception {
- when(mDreamOverlayContainerView.getParent())
- .thenReturn(mDreamOverlayContainerViewParent)
- .thenReturn(null);
-
final IBinder proxy = mService.onBind(new Intent());
final IDreamOverlay overlay = IDreamOverlay.Stub.asInterface(proxy);
+ // Destroy the service.
+ mService.onDestroy();
+ mMainExecutor.runAllReady();
+
// Inform the overlay service of dream starting.
overlay.startDream(mWindowParams, mDreamOverlayCallback, DREAM_COMPONENT,
false /*shouldShowComplication*/);
-
- // Destroy the service.
- mService.onDestroy();
-
- // Run executor tasks.
mMainExecutor.runAllReady();
verify(mWindowManager, never()).addView(any(), any());
}
@Test
+ public void testNeverRemoveDecorViewIfNotAdded() {
+ // Service destroyed before dream started.
+ mService.onDestroy();
+ mMainExecutor.runAllReady();
+
+ verify(mWindowManager, never()).removeView(any());
+ }
+
+ @Test
public void testResetCurrentOverlayWhenConnectedToNewDream() throws RemoteException {
final IBinder proxy = mService.onBind(new Intent());
final IDreamOverlay overlay = IDreamOverlay.Stub.asInterface(proxy);
