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

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);
+        }
+    }
 }

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())
+    }
+}