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
diff --git a/services/core/java/com/android/server/wm/DragDropController.java b/services/core/java/com/android/server/wm/DragDropController.java
index 65951dc..9cba533 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 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.
@@ -67,42 +68,10 @@
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<IDragDropCallback> mCallback = new AtomicReference<>(
+ new IDragDropCallback() {});
boolean dragDropActiveLocked() {
return mDragState != null;
@@ -114,9 +83,7 @@
void registerCallback(IDragDropCallback callback) {
Preconditions.checkNotNull(callback);
- synchronized (mWriteLock) {
- mCallback = callback;
- }
+ mCallback.set(callback);
}
DragDropController(WindowManagerService service, Looper looper) {
@@ -136,45 +103,43 @@
+ " 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 @@
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 @@
mDragState.notifyLocationLocked(touchX, touchY);
}
+ return true; // success!
+ } finally {
+ mCallback.get().postPerformDrag();
}
-
- return true; // success!
}
void reportDropResult(IWindow window, boolean consumed) {
@@ -271,8 +237,8 @@
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 @@
return; // !!! TODO: throw here?
}
-
mDragState.mDragResult = consumed;
mDragState.endDragLocked();
}
+ } finally {
+ mCallback.get().postReportDropResult();
}
}
@@ -309,8 +276,8 @@
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 @@
mDragState.mDragResult = false;
mDragState.cancelDragLocked();
}
+ } finally {
+ mCallback.get().postCancelDragAndDrop();
}
}
@@ -338,20 +307,18 @@
* @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 @@
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 @@
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 @@
}
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;
}