From bb28efb6311f203f74e2dbc5de439d53b1ea8cd5 Mon Sep 17 00:00:00 2001 From: Daichi Hirono Date: Tue, 21 Nov 2017 15:11:01 +0900 Subject: [PATCH] Add pre/post callback to IDragDropCallback Previoulsy we uses mWriteLock to avoid race condition between DragDropController and IDragDropCallback. However it turns out we have dead lock here. 1. DragDropController invokes performDrag for drag operation B with holding write lock. 2. The Callback initiates other drag operation A before processing B. 3. A does not complete because DragDropController holding write lock for B. B does not complete because the callback try to process request A before B. The CL removes write lock and separates the existing callback methods into two: pre and post callbacks. 1. DragDropController invokes prePerfromDrag for drag operaiton B, which blocks. 2. The callback initiates other drag operation A before processing B. 3. DragDropController invokes prePerfromDrag for drag operaiton A, which returns immediately. DragDropController processes A and invokes postPefromCallback for A. 4. The Callback unblocks prePerfromDrag for B. Bug: 65564090 Test: android.server.wm.CrossAppDragAndDropTests, manually check the drag and drop behavior on test app. Change-Id: I393a031cb276d6fa7ea56909959d0f2c5d621125 --- .../android/server/wm/DragDropController.java | 205 +++++++----------- .../server/wm/WindowManagerInternal.java | 27 ++- 2 files changed, 105 insertions(+), 127 deletions(-) diff --git a/services/core/java/com/android/server/wm/DragDropController.java b/services/core/java/com/android/server/wm/DragDropController.java index 65951dc634f81..9cba533310075 100644 --- a/services/core/java/com/android/server/wm/DragDropController.java +++ b/services/core/java/com/android/server/wm/DragDropController.java @@ -40,6 +40,7 @@ import android.view.View; import com.android.internal.util.Preconditions; import com.android.server.input.InputWindowHandle; import com.android.server.wm.WindowManagerInternal.IDragDropCallback; +import java.util.concurrent.atomic.AtomicReference; /** * Managing drag and drop operations initiated by View#startDragAndDrop. @@ -66,43 +67,11 @@ class DragDropController { private WindowManagerService mService; private final Handler mHandler; - /** - * Lock to preserve the order of state updates. - * The lock is used to process drag and drop state updates in order without having the window - * manager lock. - * - * Suppose DragDropController invokes a callback method A, then processes the following update - * A'. Same for a callback method B and the following update B'. The callback wants - * DragDropController to processes the updates in the order of A' then B'. - * - * Without mWriteLock: the following race can happen. - * - * 1. Thread a calls A. - * 2. Thread b calls B. - * 3. Thread b acquires the window manager lock - * 4. thread b processes the update B' - * 5. Thread a acquires the window manager lock - * 6. thread a processes the update A' - * - * With mWriteLock we can ensure the order of A' and B' - * - * 1. Thread a acquire mWriteLock - * 2. Thread a calls A - * 3. Thread a acquire the window manager lock - * 4. Thread a processes A' - * 5. Thread b acquire mWriteLock - * 6. Thread b calls B - * 7. Thread b acquire the window manager lock - * 8. Thread b processes B' - * - * Don't acquire the lock while holding the window manager lock, otherwise it causes a deadlock. - */ - private final Object mWriteLock = new Object(); - /** * Callback which is used to sync drag state with the vendor-specific code. */ - @NonNull private IDragDropCallback mCallback = new IDragDropCallback() {}; + @NonNull private AtomicReference mCallback = new AtomicReference<>( + new IDragDropCallback() {}); boolean dragDropActiveLocked() { return mDragState != null; @@ -114,9 +83,7 @@ class DragDropController { void registerCallback(IDragDropCallback callback) { Preconditions.checkNotNull(callback); - synchronized (mWriteLock) { - mCallback = callback; - } + mCallback.set(callback); } DragDropController(WindowManagerService service, Looper looper) { @@ -136,45 +103,43 @@ class DragDropController { + " asbinder=" + window.asBinder()); } - synchronized (mWriteLock) { - synchronized (mService.mWindowMap) { - if (dragDropActiveLocked()) { - Slog.w(TAG_WM, "Drag already in progress"); - return null; - } - - // TODO(multi-display): support other displays - final DisplayContent displayContent = - mService.getDefaultDisplayContentLocked(); - final Display display = displayContent.getDisplay(); - - final SurfaceControl surface = new SurfaceControl.Builder(session) - .setName("drag surface") - .setSize(width, height) - .setFormat(PixelFormat.TRANSLUCENT) - .build(); - surface.setLayerStack(display.getLayerStack()); - float alpha = 1; - if ((flags & View.DRAG_FLAG_OPAQUE) == 0) { - alpha = DRAG_SHADOW_ALPHA_TRANSPARENT; - } - surface.setAlpha(alpha); - - if (SHOW_TRANSACTIONS) - Slog.i(TAG_WM, " DRAG " + surface + ": CREATE"); - outSurface.copyFrom(surface); - final IBinder winBinder = window.asBinder(); - IBinder token = new Binder(); - mDragState = new DragState(mService, token, surface, flags, winBinder); - mDragState.mPid = callerPid; - mDragState.mUid = callerUid; - mDragState.mOriginalAlpha = alpha; - token = mDragState.mToken = new Binder(); - - // 5 second timeout for this window to actually begin the drag - sendTimeoutMessage(MSG_DRAG_START_TIMEOUT, winBinder); - return token; + synchronized (mService.mWindowMap) { + if (dragDropActiveLocked()) { + Slog.w(TAG_WM, "Drag already in progress"); + return null; } + + // TODO(multi-display): support other displays + final DisplayContent displayContent = + mService.getDefaultDisplayContentLocked(); + final Display display = displayContent.getDisplay(); + + final SurfaceControl surface = new SurfaceControl.Builder(session) + .setName("drag surface") + .setSize(width, height) + .setFormat(PixelFormat.TRANSLUCENT) + .build(); + surface.setLayerStack(display.getLayerStack()); + float alpha = 1; + if ((flags & View.DRAG_FLAG_OPAQUE) == 0) { + alpha = DRAG_SHADOW_ALPHA_TRANSPARENT; + } + surface.setAlpha(alpha); + + if (SHOW_TRANSACTIONS) + Slog.i(TAG_WM, " DRAG " + surface + ": CREATE"); + outSurface.copyFrom(surface); + final IBinder winBinder = window.asBinder(); + IBinder token = new Binder(); + mDragState = new DragState(mService, token, surface, flags, winBinder); + mDragState.mPid = callerPid; + mDragState.mUid = callerUid; + mDragState.mOriginalAlpha = alpha; + token = mDragState.mToken = new Binder(); + + // 5 second timeout for this window to actually begin the drag + sendTimeoutMessage(MSG_DRAG_START_TIMEOUT, winBinder); + return token; } } @@ -185,11 +150,11 @@ class DragDropController { Slog.d(TAG_WM, "perform drag: win=" + window + " data=" + data); } - synchronized (mWriteLock) { - if (!mCallback.performDrag(window, dragToken, touchSource, touchX, touchY, thumbCenterX, - thumbCenterY, data)) { - return false; - } + if (!mCallback.get().prePerformDrag(window, dragToken, touchSource, touchX, touchY, thumbCenterX, + thumbCenterY, data)) { + return false; + } + try { synchronized (mService.mWindowMap) { if (mDragState == null) { Slog.w(TAG_WM, "No drag prepared"); @@ -260,9 +225,10 @@ class DragDropController { mDragState.notifyLocationLocked(touchX, touchY); } + return true; // success! + } finally { + mCallback.get().postPerformDrag(); } - - return true; // success! } void reportDropResult(IWindow window, boolean consumed) { @@ -271,8 +237,8 @@ class DragDropController { Slog.d(TAG_WM, "Drop result=" + consumed + " reported by " + token); } - synchronized (mWriteLock) { - mCallback.reportDropResult(window, consumed); + mCallback.get().preReportDropResult(window, consumed); + try { synchronized (mService.mWindowMap) { if (mDragState == null) { // Most likely the drop recipient ANRed and we ended the drag @@ -297,10 +263,11 @@ class DragDropController { return; // !!! TODO: throw here? } - mDragState.mDragResult = consumed; mDragState.endDragLocked(); } + } finally { + mCallback.get().postReportDropResult(); } } @@ -309,8 +276,8 @@ class DragDropController { Slog.d(TAG_WM, "cancelDragAndDrop"); } - synchronized (mWriteLock) { - mCallback.cancelDragAndDrop(dragToken); + mCallback.get().preCancelDragAndDrop(dragToken); + try { synchronized (mService.mWindowMap) { if (mDragState == null) { Slog.w(TAG_WM, "cancelDragAndDrop() without prepareDrag()"); @@ -327,6 +294,8 @@ class DragDropController { mDragState.mDragResult = false; mDragState.cancelDragLocked(); } + } finally { + mCallback.get().postCancelDragAndDrop(); } } @@ -338,20 +307,18 @@ class DragDropController { * @param newY Y coordinate value in dp in the screen coordinate */ void handleMotionEvent(boolean keepHandling, float newX, float newY) { - synchronized (mWriteLock) { - synchronized (mService.mWindowMap) { - if (!dragDropActiveLocked()) { - // The drag has ended but the clean-up message has not been processed by - // window manager. Drop events that occur after this until window manager - // has a chance to clean-up the input handle. - return; - } + synchronized (mService.mWindowMap) { + if (!dragDropActiveLocked()) { + // The drag has ended but the clean-up message has not been processed by + // window manager. Drop events that occur after this until window manager + // has a chance to clean-up the input handle. + return; + } - if (keepHandling) { - mDragState.notifyMoveLocked(newX, newY); - } else { - mDragState.notifyDropLocked(newX, newY); - } + if (keepHandling) { + mDragState.notifyMoveLocked(newX, newY); + } else { + mDragState.notifyDropLocked(newX, newY); } } } @@ -414,12 +381,11 @@ class DragDropController { if (DEBUG_DRAG) { Slog.w(TAG_WM, "Timeout starting drag by win " + win); } - synchronized (mWriteLock) { - synchronized (mService.mWindowMap) { - // !!! TODO: ANR the app that has failed to start the drag in time - if (mDragState != null) { - mDragState.closeLocked(); - } + + synchronized (mService.mWindowMap) { + // !!! TODO: ANR the app that has failed to start the drag in time + if (mDragState != null) { + mDragState.closeLocked(); } } break; @@ -430,13 +396,12 @@ class DragDropController { if (DEBUG_DRAG) { Slog.w(TAG_WM, "Timeout ending drag to win " + win); } - synchronized (mWriteLock) { - synchronized (mService.mWindowMap) { - // !!! TODO: ANR the drag-receiving app - if (mDragState != null) { - mDragState.mDragResult = false; - mDragState.endDragLocked(); - } + + synchronized (mService.mWindowMap) { + // !!! TODO: ANR the drag-receiving app + if (mDragState != null) { + mDragState.mDragResult = false; + mDragState.endDragLocked(); } } break; @@ -455,15 +420,13 @@ class DragDropController { } case MSG_ANIMATION_END: { - synchronized (mWriteLock) { - synchronized (mService.mWindowMap) { - if (mDragState == null) { - Slog.wtf(TAG_WM, "mDragState unexpectedly became null while " + - "plyaing animation"); - return; - } - mDragState.closeLocked(); + synchronized (mService.mWindowMap) { + if (mDragState == null) { + Slog.wtf(TAG_WM, "mDragState unexpectedly became null while " + + "plyaing animation"); + return; } + mDragState.closeLocked(); } break; } diff --git a/services/core/java/com/android/server/wm/WindowManagerInternal.java b/services/core/java/com/android/server/wm/WindowManagerInternal.java index 036f7b0aea03b..62d2e7d8e1c5c 100644 --- a/services/core/java/com/android/server/wm/WindowManagerInternal.java +++ b/services/core/java/com/android/server/wm/WindowManagerInternal.java @@ -151,23 +151,38 @@ public abstract class WindowManagerInternal { */ public interface IDragDropCallback { /** - * Called when drag operation is started. + * Called when drag operation is starting. */ - default boolean performDrag(IWindow window, IBinder dragToken, + default boolean prePerformDrag(IWindow window, IBinder dragToken, int touchSource, float touchX, float touchY, float thumbCenterX, float thumbCenterY, ClipData data) { return true; } /** - * Called when drop result is reported. + * Called when drag operation is started. */ - default void reportDropResult(IWindow window, boolean consumed) {} + default void postPerformDrag() {} /** - * Called when drag operation is cancelled. + * Called when drop result is being reported. */ - default void cancelDragAndDrop(IBinder dragToken) {} + default void preReportDropResult(IWindow window, boolean consumed) {} + + /** + * Called when drop result was reported. + */ + default void postReportDropResult() {} + + /** + * Called when drag operation is being cancelled. + */ + default void preCancelDragAndDrop(IBinder dragToken) {} + + /** + * Called when drag operation was cancelled. + */ + default void postCancelDragAndDrop() {} } /**