Merge "Refactor the VDM locks" into main
diff --git a/services/companion/java/com/android/server/companion/virtual/VirtualDeviceImpl.java b/services/companion/java/com/android/server/companion/virtual/VirtualDeviceImpl.java
index f03e8c7..28efdfc 100644
--- a/services/companion/java/com/android/server/companion/virtual/VirtualDeviceImpl.java
+++ b/services/companion/java/com/android/server/companion/virtual/VirtualDeviceImpl.java
@@ -163,6 +163,16 @@
*/
private static final long PENDING_TRAMPOLINE_TIMEOUT_MS = 5000;
+ /**
+ * Global lock for this Virtual Device.
+ *
+ * Never call outside this class while holding this lock. A number of other system services like
+ * WindowManager, DisplayManager, etc. call into this device to get device-specific information,
+ * while holding their own global locks.
+ *
+ * Making a call to another service while holding this lock creates lock order inversion and
+ * will potentially cause a deadlock.
+ */
private final Object mVirtualDeviceLock = new Object();
private final int mBaseVirtualDisplayFlags;
@@ -179,14 +189,6 @@
private final int mDeviceId;
@Nullable
private final String mPersistentDeviceId;
- // Thou shall not hold the mVirtualDeviceLock over the mInputController calls.
- // Holding the lock can lead to lock inversion with GlobalWindowManagerLock.
- // 1. After display is created the window manager calls into VDM during construction
- // of display specific context to fetch device id corresponding to the display.
- // mVirtualDeviceLock will be held while this is done.
- // 2. InputController interactions result in calls to DisplayManager (to set IME,
- // possibly more indirect calls), and those attempt to lock GlobalWindowManagerLock which
- // creates lock inversion.
private final InputController mInputController;
private final SensorController mSensorController;
private final CameraAccessController mCameraAccessController;
@@ -205,19 +207,23 @@
private final DisplayManagerGlobal mDisplayManager;
private final DisplayManagerInternal mDisplayManagerInternal;
private final PowerManager mPowerManager;
- @GuardedBy("mVirtualDeviceLock")
+ @GuardedBy("mIntentInterceptors")
private final Map<IBinder, IntentFilter> mIntentInterceptors = new ArrayMap<>();
@NonNull
private final Consumer<ArraySet<Integer>> mRunningAppsChangedCallback;
+
// The default setting for showing the pointer on new displays.
@GuardedBy("mVirtualDeviceLock")
private boolean mDefaultShowPointerIcon = true;
@GuardedBy("mVirtualDeviceLock")
@Nullable
private LocaleList mLocaleList = null;
- @GuardedBy("mVirtualDeviceLock")
+
+ // Lock for power operations for this virtual device that allow calling PowerManager.
+ private final Object mPowerLock = new Object();
+ @GuardedBy("mPowerLock")
private boolean mLockdownActive = false;
- @GuardedBy("mVirtualDeviceLock")
+ @GuardedBy("mPowerLock")
private boolean mRequestedToBeAwake = true;
@NonNull
@@ -334,7 +340,7 @@
*/
@Override
public boolean shouldInterceptIntent(@NonNull Intent intent) {
- synchronized (mVirtualDeviceLock) {
+ synchronized (mIntentInterceptors) {
boolean hasInterceptedIntent = false;
for (Map.Entry<IBinder, IntentFilter> interceptor
: mIntentInterceptors.entrySet()) {
@@ -500,7 +506,7 @@
}
void onLockdownChanged(boolean lockdownActive) {
- synchronized (mVirtualDeviceLock) {
+ synchronized (mPowerLock) {
if (lockdownActive != mLockdownActive) {
mLockdownActive = lockdownActive;
if (mLockdownActive) {
@@ -610,7 +616,7 @@
@Override // Binder call
public void goToSleep() {
checkCallerIsDeviceOwner();
- synchronized (mVirtualDeviceLock) {
+ synchronized (mPowerLock) {
mRequestedToBeAwake = false;
}
final long ident = Binder.clearCallingIdentity();
@@ -624,7 +630,7 @@
@Override // Binder call
public void wakeUp() {
checkCallerIsDeviceOwner();
- synchronized (mVirtualDeviceLock) {
+ synchronized (mPowerLock) {
mRequestedToBeAwake = true;
if (mLockdownActive) {
Slog.w(TAG, "Cannot wake up device during lockdown.");
@@ -850,8 +856,8 @@
checkDisplayOwnedByVirtualDeviceLocked(displayId);
if (mVirtualAudioController == null) {
mVirtualAudioController = new VirtualAudioController(mContext, mAttributionSource);
- GenericWindowPolicyController gwpc = mVirtualDisplays.get(
- displayId).getWindowPolicyController();
+ GenericWindowPolicyController gwpc =
+ mVirtualDisplays.get(displayId).getWindowPolicyController();
mVirtualAudioController.startListening(gwpc, routingCallback,
configChangedCallback);
}
@@ -1293,7 +1299,7 @@
checkCallerIsDeviceOwner();
Objects.requireNonNull(intentInterceptor);
Objects.requireNonNull(filter);
- synchronized (mVirtualDeviceLock) {
+ synchronized (mIntentInterceptors) {
mIntentInterceptors.put(intentInterceptor.asBinder(), filter);
}
}
@@ -1303,7 +1309,7 @@
@NonNull IVirtualDeviceIntentInterceptor intentInterceptor) {
checkCallerIsDeviceOwner();
Objects.requireNonNull(intentInterceptor);
- synchronized (mVirtualDeviceLock) {
+ synchronized (mIntentInterceptors) {
mIntentInterceptors.remove(intentInterceptor.asBinder());
}
}
@@ -1653,6 +1659,7 @@
void goToSleepInternal(@PowerManager.GoToSleepReason int reason) {
final long now = SystemClock.uptimeMillis();
+ IntArray displayIds = new IntArray();
synchronized (mVirtualDeviceLock) {
for (int i = 0; i < mVirtualDisplays.size(); i++) {
VirtualDisplayWrapper wrapper = mVirtualDisplays.valueAt(i);
@@ -1660,13 +1667,17 @@
continue;
}
int displayId = mVirtualDisplays.keyAt(i);
- mPowerManager.goToSleep(displayId, now, reason, /* flags= */ 0);
+ displayIds.add(displayId);
}
}
+ for (int i = 0; i < displayIds.size(); ++i) {
+ mPowerManager.goToSleep(displayIds.get(i), now, reason, /* flags= */ 0);
+ }
}
void wakeUpInternal(@PowerManager.WakeReason int reason, String details) {
final long now = SystemClock.uptimeMillis();
+ IntArray displayIds = new IntArray();
synchronized (mVirtualDeviceLock) {
for (int i = 0; i < mVirtualDisplays.size(); i++) {
VirtualDisplayWrapper wrapper = mVirtualDisplays.valueAt(i);
@@ -1674,9 +1685,12 @@
continue;
}
int displayId = mVirtualDisplays.keyAt(i);
- mPowerManager.wakeUp(now, reason, details, displayId);
+ displayIds.add(displayId);
}
}
+ for (int i = 0; i < displayIds.size(); ++i) {
+ mPowerManager.wakeUp(now, reason, details, displayIds.get(i));
+ }
}
/**
diff --git a/services/companion/java/com/android/server/companion/virtual/VirtualDeviceManagerService.java b/services/companion/java/com/android/server/companion/virtual/VirtualDeviceManagerService.java
index ff82ca0..139bbae 100644
--- a/services/companion/java/com/android/server/companion/virtual/VirtualDeviceManagerService.java
+++ b/services/companion/java/com/android/server/companion/virtual/VirtualDeviceManagerService.java
@@ -117,7 +117,18 @@
*/
static final int CDM_ASSOCIATION_ID_NONE = 0;
+ /**
+ * Global VDM lock.
+ *
+ * Never call outside this class while holding this lock. A number of other system services like
+ * WindowManager, DisplayManager, etc. call into VDM to get device-specific information, while
+ * holding their own global locks.
+ *
+ * Making a call to another service while holding this lock creates lock order inversion and
+ * will potentially cause a deadlock.
+ */
private final Object mVirtualDeviceManagerLock = new Object();
+
private final VirtualDeviceManagerImpl mImpl;
private final VirtualDeviceManagerNativeImpl mNativeImpl;
private final VirtualDeviceManagerInternal mLocalService;
@@ -235,10 +246,9 @@
// Called when the global lockdown state changes, i.e. lockdown is considered active if any user
// is in lockdown mode, and inactive if no users are in lockdown mode.
void onLockdownChanged(boolean lockdownActive) {
- synchronized (mVirtualDeviceManagerLock) {
- for (int i = 0; i < mVirtualDevices.size(); i++) {
- mVirtualDevices.valueAt(i).onLockdownChanged(lockdownActive);
- }
+ ArrayList<VirtualDeviceImpl> virtualDevicesSnapshot = getVirtualDevicesSnapshot();
+ for (int i = 0; i < virtualDevicesSnapshot.size(); i++) {
+ virtualDevicesSnapshot.get(i).onLockdownChanged(lockdownActive);
}
}
@@ -264,16 +274,14 @@
return null;
}
int userId = userHandle.getIdentifier();
- synchronized (mVirtualDeviceManagerLock) {
- for (int i = 0; i < mVirtualDevices.size(); i++) {
- final CameraAccessController cameraAccessController =
- mVirtualDevices.valueAt(i).getCameraAccessController();
- if (cameraAccessController.getUserId() == userId) {
- return cameraAccessController;
- }
+ ArrayList<VirtualDeviceImpl> virtualDevicesSnapshot = getVirtualDevicesSnapshot();
+ for (int i = 0; i < virtualDevicesSnapshot.size(); i++) {
+ final CameraAccessController cameraAccessController =
+ virtualDevicesSnapshot.get(i).getCameraAccessController();
+ if (cameraAccessController.getUserId() == userId) {
+ return cameraAccessController;
}
- }
- Context userContext = getContext().createContextAsUser(userHandle, 0);
+ } Context userContext = getContext().createContextAsUser(userHandle, 0);
return new CameraAccessController(userContext, mLocalService, this::onCameraAccessBlocked);
}
@@ -520,11 +528,10 @@
@Override // Binder call
public List<VirtualDevice> getVirtualDevices() {
List<VirtualDevice> virtualDevices = new ArrayList<>();
- synchronized (mVirtualDeviceManagerLock) {
- for (int i = 0; i < mVirtualDevices.size(); i++) {
- final VirtualDeviceImpl device = mVirtualDevices.valueAt(i);
- virtualDevices.add(device.getPublicVirtualDeviceObject());
- }
+ ArrayList<VirtualDeviceImpl> virtualDevicesSnapshot = getVirtualDevicesSnapshot();
+ for (int i = 0; i < virtualDevicesSnapshot.size(); i++) {
+ VirtualDeviceImpl device = virtualDevicesSnapshot.get(i);
+ virtualDevices.add(device.getPublicVirtualDeviceObject());
}
return virtualDevices;
}
@@ -834,15 +841,17 @@
@Nullable
public LocaleList getPreferredLocaleListForUid(int uid) {
// TODO: b/263188984 support the case where an app is running on multiple VDs
+ VirtualDeviceImpl virtualDevice = null;
synchronized (mVirtualDeviceManagerLock) {
for (int i = 0; i < mAppsOnVirtualDevices.size(); i++) {
if (mAppsOnVirtualDevices.valueAt(i).contains(uid)) {
int deviceId = mAppsOnVirtualDevices.keyAt(i);
- return mVirtualDevices.get(deviceId).getDeviceLocaleList();
+ virtualDevice = mVirtualDevices.get(deviceId);
+ break;
}
}
}
- return null;
+ return virtualDevice == null ? null : virtualDevice.getDeviceLocaleList();
}
@Override