From a6ab5b715c3213d003fcb950a9d68b2c9e919206 Mon Sep 17 00:00:00 2001
From: Fabian Kozynski <kozynski@google.com>
Date: Tue, 15 Nov 2022 13:14:05 -0500
Subject: [PATCH] Do not capture argument from ServiceListing.Callback

As the argument passed is the internal arraylist (mutable), capturing it
in the Runnable passed to the executor could cause a
ConcurrentModificationException.

Instead, map it before defining the Runnable so by the time the callback
returns, we don't have any copies of it.

The test checks that the original argument passed is the one that's
mapped and not a modified copy.

Test: atest ControlsListingControllerImpl
Fixes: 259222196
Change-Id: I6fff508dca50f0f9ac6981ad94e7cc9212cafda9
---
 .../management/ControlsListingControllerImpl.kt    |  7 ++++---
 .../ControlsListingControllerImplTest.kt           | 14 ++++++++++++++
 2 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/packages/SystemUI/src/com/android/systemui/controls/management/ControlsListingControllerImpl.kt b/packages/SystemUI/src/com/android/systemui/controls/management/ControlsListingControllerImpl.kt
index 115edd115ffe3..c6428ef6eb8e3 100644
--- a/packages/SystemUI/src/com/android/systemui/controls/management/ControlsListingControllerImpl.kt
+++ b/packages/SystemUI/src/com/android/systemui/controls/management/ControlsListingControllerImpl.kt
@@ -91,11 +91,12 @@ class ControlsListingControllerImpl @VisibleForTesting constructor(
     override var currentUserId = userTracker.userId
         private set
 
-    private val serviceListingCallback = ServiceListing.Callback {
+    private val serviceListingCallback = ServiceListing.Callback { list ->
+        Log.d(TAG, "ServiceConfig reloaded, count: ${list.size}")
+        val newServices = list.map { ControlsServiceInfo(userTracker.userContext, it) }
+        // After here, `list` is not captured, so we don't risk modifying it outside of the callback
         backgroundExecutor.execute {
             if (userChangeInProgress.get() > 0) return@execute
-            Log.d(TAG, "ServiceConfig reloaded, count: ${it.size}")
-            val newServices = it.map { ControlsServiceInfo(userTracker.userContext, it) }
             if (featureFlags.isEnabled(Flags.USE_APP_PANELS)) {
                 newServices.forEach(ControlsServiceInfo::resolvePanelActivity)
             }
diff --git a/packages/SystemUI/tests/src/com/android/systemui/controls/management/ControlsListingControllerImplTest.kt b/packages/SystemUI/tests/src/com/android/systemui/controls/management/ControlsListingControllerImplTest.kt
index dedc7239bf856..98ff8d1d8845f 100644
--- a/packages/SystemUI/tests/src/com/android/systemui/controls/management/ControlsListingControllerImplTest.kt
+++ b/packages/SystemUI/tests/src/com/android/systemui/controls/management/ControlsListingControllerImplTest.kt
@@ -46,6 +46,7 @@ import com.android.systemui.util.time.FakeSystemClock
 import org.junit.After
 import org.junit.Assert.assertEquals
 import org.junit.Assert.assertNull
+import org.junit.Assert.assertTrue
 import org.junit.Before
 import org.junit.Test
 import org.junit.runner.RunWith
@@ -480,6 +481,19 @@ class ControlsListingControllerImplTest : SysuiTestCase() {
         assertNull(controller.getCurrentServices()[0].panelActivity)
     }
 
+    @Test
+    fun testListingsNotModifiedByCallback() {
+        // This test checks that if the list passed to the callback is modified, it has no effect
+        // in the resulting services
+        val list = mutableListOf<ServiceInfo>()
+        serviceListingCallbackCaptor.value.onServicesReloaded(list)
+
+        list.add(ServiceInfo(ComponentName("a", "b")))
+        executor.runAllReady()
+
+        assertTrue(controller.getCurrentServices().isEmpty())
+    }
+
     private fun ServiceInfo(
             componentName: ComponentName,
             panelActivityComponentName: ComponentName? = null