From 0003ce7a48a148f3f4b842e473d9ff8854af281d Mon Sep 17 00:00:00 2001 From: Naomi Musgrave Date: Mon, 24 Jul 2023 15:38:41 +0000 Subject: [MediaProjection] Address deadlock between multiple services Below outlines acceptable lock ordering: * WindowManagerService -> MediaProjectionManagerService * ActivityManagerService -> WindowManagerService This change addresses deadlock caused by MediaProjectionManagerService -> ActivityManagerService by ensuring that MPMS -> AMS is never done while holding the MPMS lock. Bug: 291826358 Test: Manual & asking partner to validate monkey test Change-Id: I63037ea099699ac9dba87086ea8caf0082e0c721 Merged-In: I9c21acf069c66f1ade22c49b095ad5339fe13eca --- .../projection/MediaProjectionManagerService.java | 41 ++++++++++++++-------- .../server/media/projection/mediaprojection.md | 5 +++ 2 files changed, 32 insertions(+), 14 deletions(-) diff --git a/services/core/java/com/android/server/media/projection/MediaProjectionManagerService.java b/services/core/java/com/android/server/media/projection/MediaProjectionManagerService.java index 65e34e682724..38aac4648e34 100644 --- a/services/core/java/com/android/server/media/projection/MediaProjectionManagerService.java +++ b/services/core/java/com/android/server/media/projection/MediaProjectionManagerService.java @@ -117,6 +117,10 @@ public final class MediaProjectionManagerService extends SystemService // WindowManagerService -> MediaProjectionManagerService -> DisplayManagerService // See mediaprojection.md private final Object mLock = new Object(); + // A handler for posting tasks that must interact with a service holding another lock, + // especially for services that will eventually acquire the WindowManager lock. + @NonNull private final Handler mHandler; + private final Map mDeathEaters; private final CallbackDelegate mCallbackDelegate; @@ -145,6 +149,8 @@ public final class MediaProjectionManagerService extends SystemService super(context); mContext = context; mInjector = injector; + // Post messages on the main thread; no need for a separate thread. + mHandler = new Handler(Looper.getMainLooper()); mClock = injector.createClock(); mDeathEaters = new ArrayMap(); mCallbackDelegate = new CallbackDelegate(); @@ -238,15 +244,20 @@ public final class MediaProjectionManagerService extends SystemService if (!mProjectionGrant.requiresForegroundService()) { return; } + } - if (mActivityManagerInternal.hasRunningForegroundService( - uid, ServiceInfo.FOREGROUND_SERVICE_TYPE_MEDIA_PROJECTION)) { - // If there is any process within this UID running a FGS - // with the mediaProjection type, that's Okay. - return; - } + // Run outside the lock when calling into ActivityManagerService. + if (mActivityManagerInternal.hasRunningForegroundService( + uid, ServiceInfo.FOREGROUND_SERVICE_TYPE_MEDIA_PROJECTION)) { + // If there is any process within this UID running a FGS + // with the mediaProjection type, that's Okay. + return; + } - mProjectionGrant.stop(); + synchronized (mLock) { + if (mProjectionGrant != null) { + mProjectionGrant.stop(); + } } } @@ -854,7 +865,6 @@ public final class MediaProjectionManagerService extends SystemService mTargetSdkVersion = targetSdkVersion; mIsPrivileged = isPrivileged; mCreateTimeMs = mClock.uptimeMillis(); - // TODO(b/267740338): Add unit test. mActivityManagerInternal.notifyMediaProjectionEvent(uid, asBinder(), MEDIA_PROJECTION_TOKEN_EVENT_CREATED); } @@ -911,6 +921,10 @@ public final class MediaProjectionManagerService extends SystemService if (callback == null) { throw new IllegalArgumentException("callback must not be null"); } + // Cache result of calling into ActivityManagerService outside of the lock, to prevent + // deadlock with WindowManagerService. + final boolean hasFGS = mActivityManagerInternal.hasRunningForegroundService( + uid, ServiceInfo.FOREGROUND_SERVICE_TYPE_MEDIA_PROJECTION); synchronized (mLock) { if (isCurrentProjection(asBinder())) { Slog.w(TAG, "UID " + Binder.getCallingUid() @@ -922,9 +936,7 @@ public final class MediaProjectionManagerService extends SystemService } if (REQUIRE_FG_SERVICE_FOR_PROJECTION - && requiresForegroundService() - && !mActivityManagerInternal.hasRunningForegroundService( - uid, ServiceInfo.FOREGROUND_SERVICE_TYPE_MEDIA_PROJECTION)) { + && requiresForegroundService() && !hasFGS) { throw new SecurityException("Media projections require a foreground service" + " of type ServiceInfo.FOREGROUND_SERVICE_TYPE_MEDIA_PROJECTION"); } @@ -1013,10 +1025,11 @@ public final class MediaProjectionManagerService extends SystemService mToken = null; unregisterCallback(mCallback); mCallback = null; - // TODO(b/267740338): Add unit test. - mActivityManagerInternal.notifyMediaProjectionEvent(uid, asBinder(), - MEDIA_PROJECTION_TOKEN_EVENT_DESTROYED); } + // Run on a separate thread, to ensure no lock is held when calling into + // ActivityManagerService. + mHandler.post(() -> mActivityManagerInternal.notifyMediaProjectionEvent(uid, asBinder(), + MEDIA_PROJECTION_TOKEN_EVENT_DESTROYED)); } @Override // Binder call diff --git a/services/core/java/com/android/server/media/projection/mediaprojection.md b/services/core/java/com/android/server/media/projection/mediaprojection.md index bccdf3411903..34e7ecc6c6c5 100644 --- a/services/core/java/com/android/server/media/projection/mediaprojection.md +++ b/services/core/java/com/android/server/media/projection/mediaprojection.md @@ -11,6 +11,11 @@ Calls must follow the below invocation order while holding locks: `WindowManagerService -> MediaProjectionManagerService -> DisplayManagerService` +`MediaProjectionManagerService` should never lock when calling into a service that may acquire +the `WindowManagerService` global lock (for example, +`MediaProjectionManagerService -> ActivityManagerService` may result in deadlock, since +`ActivityManagerService -> WindowManagerService`). + ### Justification `MediaProjectionManagerService` calls into `WindowManagerService` in the below cases. While handling -- cgit v1.2.3-59-g8ed1b