From dafd01de26cbd3fd930153b262eb16926b80e5f2 Mon Sep 17 00:00:00 2001 From: Lucas Dupin Date: Tue, 11 Jan 2022 15:56:21 -0800 Subject: [PATCH] Always destroy RegionSamplingHelper on same thread stopAndDestroy() was being called from the main thread, while registration was happening on the background. This would lead to concurrency bugs because this class is not thread safe. Test: atest RegionSamplingHelperTest Fixes: 201962411 Change-Id: I01290f5931972ad3eb322e7dddc44e002324e405 --- .../navigationbar/RegionSamplingHelper.java | 46 ++++++-- .../navigationbar/RegionSamplingHelperTest.kt | 102 ++++++++++++++++++ 2 files changed, 141 insertions(+), 7 deletions(-) create mode 100644 packages/SystemUI/tests/src/com/android/systemui/shared/navigationbar/RegionSamplingHelperTest.kt diff --git a/packages/SystemUI/shared/src/com/android/systemui/shared/navigationbar/RegionSamplingHelper.java b/packages/SystemUI/shared/src/com/android/systemui/shared/navigationbar/RegionSamplingHelper.java index 98083742d7071..1d2caf9ab5459 100644 --- a/packages/SystemUI/shared/src/com/android/systemui/shared/navigationbar/RegionSamplingHelper.java +++ b/packages/SystemUI/shared/src/com/android/systemui/shared/navigationbar/RegionSamplingHelper.java @@ -28,6 +28,8 @@ import android.view.View; import android.view.ViewRootImpl; import android.view.ViewTreeObserver; +import androidx.annotation.VisibleForTesting; + import java.io.PrintWriter; import java.util.concurrent.Executor; @@ -60,6 +62,7 @@ public class RegionSamplingHelper implements View.OnAttachStateChangeListener, private final Rect mRegisteredSamplingBounds = new Rect(); private final SamplingCallback mCallback; private final Executor mBackgroundExecutor; + private final SysuiCompositionSamplingListener mCompositionSamplingListener; private boolean mSamplingEnabled = false; private boolean mSamplingListenerRegistered = false; @@ -91,9 +94,17 @@ public class RegionSamplingHelper implements View.OnAttachStateChangeListener, public RegionSamplingHelper(View sampledView, SamplingCallback samplingCallback, Executor backgroundExecutor) { + this(sampledView, samplingCallback, sampledView.getContext().getMainExecutor(), + backgroundExecutor, new SysuiCompositionSamplingListener()); + } + + @VisibleForTesting + RegionSamplingHelper(View sampledView, SamplingCallback samplingCallback, + Executor mainExecutor, Executor backgroundExecutor, + SysuiCompositionSamplingListener compositionSamplingListener) { mBackgroundExecutor = backgroundExecutor; - mSamplingListener = new CompositionSamplingListener( - sampledView.getContext().getMainExecutor()) { + mCompositionSamplingListener = compositionSamplingListener; + mSamplingListener = new CompositionSamplingListener(mainExecutor) { @Override public void onSampleCollected(float medianLuma) { if (mSamplingEnabled) { @@ -136,7 +147,7 @@ public class RegionSamplingHelper implements View.OnAttachStateChangeListener, public void stopAndDestroy() { stop(); - mSamplingListener.destroy(); + mBackgroundExecutor.execute(mSamplingListener::destroy); mIsDestroyed = true; } @@ -189,13 +200,12 @@ public class RegionSamplingHelper implements View.OnAttachStateChangeListener, // We only want to re-register if something actually changed unregisterSamplingListener(); mSamplingListenerRegistered = true; - SurfaceControl wrappedStopLayer = stopLayerControl == null - ? null : new SurfaceControl(stopLayerControl, "regionSampling"); + SurfaceControl wrappedStopLayer = wrap(stopLayerControl); mBackgroundExecutor.execute(() -> { if (wrappedStopLayer != null && !wrappedStopLayer.isValid()) { return; } - CompositionSamplingListener.register(mSamplingListener, DEFAULT_DISPLAY, + mCompositionSamplingListener.register(mSamplingListener, DEFAULT_DISPLAY, wrappedStopLayer, mSamplingRequestBounds); }); mRegisteredSamplingBounds.set(mSamplingRequestBounds); @@ -208,14 +218,21 @@ public class RegionSamplingHelper implements View.OnAttachStateChangeListener, } } + @VisibleForTesting + protected SurfaceControl wrap(SurfaceControl stopLayerControl) { + return stopLayerControl == null ? null : new SurfaceControl(stopLayerControl, + "regionSampling"); + } + private void unregisterSamplingListener() { if (mSamplingListenerRegistered) { mSamplingListenerRegistered = false; SurfaceControl wrappedStopLayer = mWrappedStopLayer; mRegisteredStopLayer = null; + mWrappedStopLayer = null; mRegisteredSamplingBounds.setEmpty(); mBackgroundExecutor.execute(() -> { - CompositionSamplingListener.unregister(mSamplingListener); + mCompositionSamplingListener.unregister(mSamplingListener); if (wrappedStopLayer != null && wrappedStopLayer.isValid()) { wrappedStopLayer.release(); } @@ -299,4 +316,19 @@ public class RegionSamplingHelper implements View.OnAttachStateChangeListener, return true; } } + + @VisibleForTesting + public static class SysuiCompositionSamplingListener { + public void register(CompositionSamplingListener listener, + int displayId, SurfaceControl stopLayer, Rect samplingArea) { + CompositionSamplingListener.register(listener, displayId, stopLayer, samplingArea); + } + + /** + * Unregisters a sampling listener. + */ + public void unregister(CompositionSamplingListener listener) { + CompositionSamplingListener.unregister(listener); + } + } } diff --git a/packages/SystemUI/tests/src/com/android/systemui/shared/navigationbar/RegionSamplingHelperTest.kt b/packages/SystemUI/tests/src/com/android/systemui/shared/navigationbar/RegionSamplingHelperTest.kt new file mode 100644 index 0000000000000..8bc438bce5ccf --- /dev/null +++ b/packages/SystemUI/tests/src/com/android/systemui/shared/navigationbar/RegionSamplingHelperTest.kt @@ -0,0 +1,102 @@ +/* + * Copyright (C) 2022 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.android.systemui.shared.navigationbar +import android.graphics.Rect +import android.testing.AndroidTestingRunner +import android.testing.TestableLooper.RunWithLooper +import android.view.SurfaceControl +import android.view.View +import android.view.ViewRootImpl +import androidx.concurrent.futures.DirectExecutor +import androidx.test.filters.SmallTest +import com.android.systemui.SysuiTestCase +import org.junit.Before +import org.junit.Rule +import org.junit.Test +import org.junit.runner.RunWith +import org.mockito.ArgumentMatchers.eq +import org.mockito.Mock +import org.mockito.Mockito.* +import org.mockito.junit.MockitoJUnit +import org.mockito.Mockito.`when` as whenever + +@RunWith(AndroidTestingRunner::class) +@SmallTest +@RunWithLooper +class RegionSamplingHelperTest : SysuiTestCase() { + + @Mock + lateinit var sampledView: View + @Mock + lateinit var samplingCallback: RegionSamplingHelper.SamplingCallback + @Mock + lateinit var compositionListener: RegionSamplingHelper.SysuiCompositionSamplingListener + @Mock + lateinit var viewRootImpl: ViewRootImpl + @Mock + lateinit var surfaceControl: SurfaceControl + @Mock + lateinit var wrappedSurfaceControl: SurfaceControl + @JvmField @Rule + var rule = MockitoJUnit.rule() + lateinit var regionSamplingHelper: RegionSamplingHelper + + @Before + fun setup() { + whenever(sampledView.isAttachedToWindow).thenReturn(true) + whenever(sampledView.viewRootImpl).thenReturn(viewRootImpl) + whenever(viewRootImpl.surfaceControl).thenReturn(surfaceControl) + whenever(surfaceControl.isValid).thenReturn(true) + whenever(wrappedSurfaceControl.isValid).thenReturn(true) + whenever(samplingCallback.isSamplingEnabled).thenReturn(true) + regionSamplingHelper = object : RegionSamplingHelper(sampledView, samplingCallback, + DirectExecutor.INSTANCE, DirectExecutor.INSTANCE, compositionListener) { + override fun wrap(stopLayerControl: SurfaceControl?): SurfaceControl { + return wrappedSurfaceControl + } + } + regionSamplingHelper.setWindowVisible(true) + } + + @Test + fun testStart_register() { + regionSamplingHelper.start(Rect(0, 0, 100, 100)) + verify(compositionListener).register(any(), anyInt(), eq(wrappedSurfaceControl), any()) + } + + @Test + fun testStart_unregister() { + regionSamplingHelper.start(Rect(0, 0, 100, 100)) + regionSamplingHelper.setWindowVisible(false) + verify(compositionListener).unregister(any()) + } + + @Test + fun testStart_hasBlur_neverRegisters() { + regionSamplingHelper.setWindowHasBlurs(true) + regionSamplingHelper.start(Rect(0, 0, 100, 100)) + verify(compositionListener, never()) + .register(any(), anyInt(), eq(wrappedSurfaceControl), any()) + } + + @Test + fun testStart_stopAndDestroy() { + regionSamplingHelper.start(Rect(0, 0, 100, 100)) + regionSamplingHelper.stopAndDestroy() + verify(compositionListener).unregister(any()) + } +} \ No newline at end of file