Merge "Do not use Map.Entry outside of lock" into udc-dev

services/core/java/com/android/server/wm/DeviceStateController.java

@@ -20,6 +20,7 @@ import android.annotation.CallbackExecutor;
 import android.annotation.NonNull;
 import android.content.Context;
 import android.util.ArrayMap;
+import android.util.Pair;
 
 import com.android.internal.R;
 import com.android.internal.annotations.GuardedBy;
@@ -165,19 +166,28 @@ final class DeviceStateController {
 
             // Make a copy here because it's possible that the consumer tries to remove a callback
             // while we're still iterating through the list, which would end up in a
-            // ConcurrentModificationException.
-            final List<Map.Entry<Consumer<DeviceState>, Executor>> entries = new ArrayList<>();
-            synchronized (mWmLock) {
-                for (Map.Entry<Consumer<DeviceState>, Executor> entry
-                        : mDeviceStateCallbacks.entrySet()) {
-                    entries.add(entry);
-                }
-            }
+            // ConcurrentModificationException. Note that cannot use a List<Map.Entry> because the
+            // entries are tied to the backing map. So, if a client removes a callback while
+            // we are notifying clients, we will get a NPE.
+            final List<Pair<Consumer<DeviceState>, Executor>> entries = copyDeviceStateCallbacks();
 
             for (int i = 0; i < entries.size(); i++) {
-                Map.Entry<Consumer<DeviceState>, Executor> entry = entries.get(i);
-                entry.getValue().execute(() -> entry.getKey().accept(mCurrentDeviceState));
+                final Pair<Consumer<DeviceState>, Executor> entry = entries.get(i);
+                entry.second.execute(() -> entry.first.accept(deviceState));
             }
         }
     }
+
+    @VisibleForTesting
+    @NonNull
+    List<Pair<Consumer<DeviceState>, Executor>> copyDeviceStateCallbacks() {
+        final List<Pair<Consumer<DeviceState>, Executor>> entries = new ArrayList<>();
+
+        synchronized (mWmLock) {
+            mDeviceStateCallbacks.forEach((deviceStateConsumer, executor) -> {
+                entries.add(new Pair<>(deviceStateConsumer, executor));
+            });
+        }
+        return entries;
+    }
 }

services/tests/wmtests/src/com/android/server/wm/DeviceStateControllerTests.java

@@ -17,17 +17,16 @@
 package com.android.server.wm;
 
 import static com.android.dx.mockito.inline.extended.ExtendedMockito.mock;
-import static com.android.dx.mockito.inline.extended.ExtendedMockito.verify;
 import static com.android.dx.mockito.inline.extended.ExtendedMockito.when;
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertTrue;
-import static org.mockito.ArgumentMatchers.any;
 
 import android.content.Context;
 import android.content.res.Resources;
 import android.hardware.devicestate.DeviceStateManager;
 import android.platform.test.annotations.Presubmit;
+import android.util.Pair;
 
 import androidx.test.filters.SmallTest;
 
@@ -38,6 +37,8 @@ import com.google.common.util.concurrent.MoreExecutors;
 import org.junit.Before;
 import org.junit.Test;
 
+import java.util.List;
+import java.util.concurrent.Executor;
 import java.util.function.Consumer;
 
 /**
@@ -58,6 +59,7 @@ public class DeviceStateControllerTests {
     private DeviceStateController.DeviceState mCurrentState =
             DeviceStateController.DeviceState.UNKNOWN;
     private Consumer<DeviceStateController.DeviceState> mDelegate;
+    private Executor mExecutor = MoreExecutors.directExecutor();
 
     @Before
     public void setUp() {
@@ -124,13 +126,31 @@ public class DeviceStateControllerTests {
         mTarget.onDeviceStateReceivedByDisplayManager(mFoldedStates[0]);
         assertEquals(DeviceStateController.DeviceState.FOLDED, mCurrentState);
 
-        // The callback should not receive state change when the it is unregistered.
+        // The callback should not receive state change when it is unregistered.
         mTarget.unregisterDeviceStateCallback(mDelegate);
         assertTrue(mTarget.mDeviceStateCallbacks.isEmpty());
         mTarget.onDeviceStateReceivedByDisplayManager(mOpenDeviceStates[0]);
         assertEquals(DeviceStateController.DeviceState.FOLDED /* unchanged */, mCurrentState);
     }
 
+    @Test
+    public void testCopyDeviceStateCallbacks() {
+        initialize(true /* supportFold */, true /* supportHalfFolded */);
+        assertEquals(1, mTarget.mDeviceStateCallbacks.size());
+        assertTrue(mTarget.mDeviceStateCallbacks.containsKey(mDelegate));
+
+        List<Pair<Consumer<DeviceStateController.DeviceState>, Executor>> entries =
+                mTarget.copyDeviceStateCallbacks();
+        mTarget.unregisterDeviceStateCallback(mDelegate);
+
+        // In contrast to List<Map.Entry> where the entries are tied to changes in the backing map,
+        // List<Pair> should still contain non-null callbacks and executors even though they were
+        // removed from the backing map via the unregister method above.
+        assertEquals(1, entries.size());
+        assertEquals(mDelegate, entries.get(0).first);
+        assertEquals(mExecutor, entries.get(0).second);
+    }
+
     private final int[] mFoldedStates = {0};
     private final int[] mOpenDeviceStates = {1};
     private final int[] mHalfFoldedStates = {2};
@@ -194,7 +214,7 @@ public class DeviceStateControllerTests {
             when(mMockContext.getResources()).thenReturn((mockRes));
             mockFold(mSupportFold, mSupportHalfFold);
             mTarget = new DeviceStateController(mMockContext, new WindowManagerGlobalLock());
-            mTarget.registerDeviceStateCallback(mDelegate, MoreExecutors.directExecutor());
+            mTarget.registerDeviceStateCallback(mDelegate, mExecutor);
         }
     }
 }