From 524ae21e70c9cb9e8e73d3e182051ee522be0afd Mon Sep 17 00:00:00 2001
From: Florence Yang <florenceyang@google.com>
Date: Thu, 29 Sep 2022 20:48:44 +0000
Subject: Fix RegionSamplingHelper race condition

mSamplingRequestBounds was causing a race condition in RegionSamplingHelper because it becomes an empty Rectangle in the main thread when the background thread needs it to be non-empty for CompositionSamplingListener.register().To fix this, a final copy of mSamplingRequestBounds is passed to the background thread, so that it will always be working with a non-empty Rectangle.

Test: atest testCompositionSamplingListener_has_nonEmptyRect
Fixes: 247690841
Change-Id: Ib0139c8be80f828172e1909503dcf57a59673717
---
 .../shared/navigationbar/RegionSamplingHelper.java |  6 ++-
 .../navigationbar/RegionSamplingHelperTest.kt      | 50 ++++++++++++++++++++--
 2 files changed, 52 insertions(+), 4 deletions(-)

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 c7d5ffe1b84c..023ef319352e 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
@@ -217,12 +217,16 @@ public class RegionSamplingHelper implements View.OnAttachStateChangeListener,
                 unregisterSamplingListener();
                 mSamplingListenerRegistered = true;
                 SurfaceControl wrappedStopLayer = wrap(stopLayerControl);
+
+                // pass this to background thread to avoid empty Rect race condition
+                final Rect boundsCopy = new Rect(mSamplingRequestBounds);
+
                 mBackgroundExecutor.execute(() -> {
                     if (wrappedStopLayer != null && !wrappedStopLayer.isValid()) {
                         return;
                     }
                     mCompositionSamplingListener.register(mSamplingListener, DEFAULT_DISPLAY,
-                            wrappedStopLayer, mSamplingRequestBounds);
+                            wrappedStopLayer, boundsCopy);
                 });
                 mRegisteredSamplingBounds.set(mSamplingRequestBounds);
                 mRegisteredStopLayer = stopLayerControl;
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
index 8bc438bce5cc..5fc09c7d5563 100644
--- a/packages/SystemUI/tests/src/com/android/systemui/shared/navigationbar/RegionSamplingHelperTest.kt
+++ b/packages/SystemUI/tests/src/com/android/systemui/shared/navigationbar/RegionSamplingHelperTest.kt
@@ -15,6 +15,7 @@
  */
 
 package com.android.systemui.shared.navigationbar
+
 import android.graphics.Rect
 import android.testing.AndroidTestingRunner
 import android.testing.TestableLooper.RunWithLooper
@@ -24,15 +25,23 @@ import android.view.ViewRootImpl
 import androidx.concurrent.futures.DirectExecutor
 import androidx.test.filters.SmallTest
 import com.android.systemui.SysuiTestCase
+import com.android.systemui.util.concurrency.FakeExecutor
+import com.android.systemui.util.mockito.any
+import com.android.systemui.util.mockito.argumentCaptor
+import com.android.systemui.util.time.FakeSystemClock
+import com.google.common.truth.Truth.assertThat
 import org.junit.Before
 import org.junit.Rule
 import org.junit.Test
 import org.junit.runner.RunWith
+import org.mockito.ArgumentMatchers.anyInt
 import org.mockito.ArgumentMatchers.eq
 import org.mockito.Mock
-import org.mockito.Mockito.*
-import org.mockito.junit.MockitoJUnit
+import org.mockito.Mockito.mock
+import org.mockito.Mockito.never
+import org.mockito.Mockito.verify
 import org.mockito.Mockito.`when` as whenever
+import org.mockito.junit.MockitoJUnit
 
 @RunWith(AndroidTestingRunner::class)
 @SmallTest
@@ -99,4 +108,39 @@ class RegionSamplingHelperTest : SysuiTestCase() {
         regionSamplingHelper.stopAndDestroy()
         verify(compositionListener).unregister(any())
     }
-}
\ No newline at end of file
+
+    @Test
+    fun testCompositionSamplingListener_has_nonEmptyRect() {
+        // simulate race condition
+        val fakeExecutor = FakeExecutor(FakeSystemClock()) // pass in as backgroundExecutor
+        val fakeSamplingCallback = mock(RegionSamplingHelper.SamplingCallback::class.java)
+
+        whenever(fakeSamplingCallback.isSamplingEnabled).thenReturn(true)
+        whenever(wrappedSurfaceControl.isValid).thenReturn(true)
+
+        regionSamplingHelper = object : RegionSamplingHelper(sampledView, fakeSamplingCallback,
+                DirectExecutor.INSTANCE, fakeExecutor, compositionListener) {
+            override fun wrap(stopLayerControl: SurfaceControl?): SurfaceControl {
+                return wrappedSurfaceControl
+            }
+        }
+        regionSamplingHelper.setWindowVisible(true)
+        regionSamplingHelper.start(Rect(0, 0, 100, 100))
+
+        // make sure background task is enqueued
+        assertThat(fakeExecutor.numPending()).isEqualTo(1)
+
+        // make sure regionSamplingHelper will have empty Rect
+        whenever(fakeSamplingCallback.getSampledRegion(any())).thenReturn(Rect(0, 0, 0, 0))
+        regionSamplingHelper.onLayoutChange(sampledView, 0, 0, 0, 0, 0, 0, 0, 0)
+
+        // resume running of background thread
+        fakeExecutor.runAllReady()
+
+        // grab Rect passed into compositionSamplingListener and make sure it's not empty
+        val argumentGrabber = argumentCaptor<Rect>()
+        verify(compositionListener).register(any(), anyInt(), eq(wrappedSurfaceControl),
+                argumentGrabber.capture())
+        assertThat(argumentGrabber.value.isEmpty).isFalse()
+    }
+}
-- 
cgit v1.2.3-59-g8ed1b