Fix CancellationSignal deadlock.

Bug: 6398945
Change-Id: I5a3d4dce74009d6f1d284a8cc54c9f68daa6a9c2

2 files changed, 116 insertions, 74 deletions

diff --git a/core/java/android/content/CancellationSignal.java b/core/java/android/content/CancellationSignal.java
index 2dbbe54..dcaeeb7 100644
--- a/core/java/android/content/CancellationSignal.java
+++ b/core/java/android/content/CancellationSignal.java
@@ -25,6 +25,7 @@ public final class CancellationSignal {
     private boolean mIsCanceled;
     private OnCancelListener mOnCancelListener;
     private ICancellationSignal mRemote;
+    private boolean mCancelInProgress;
 
     /**
      * Creates a cancellation signal, initially not canceled.
@@ -59,19 +60,33 @@ public final class CancellationSignal {
      * If the operation has not yet started, then it will be canceled as soon as it does.
      */
     public void cancel() {
+        final OnCancelListener listener;
+        final ICancellationSignal remote;
         synchronized (this) {
-            if (!mIsCanceled) {
-                mIsCanceled = true;
-                if (mOnCancelListener != null) {
-                    mOnCancelListener.onCancel();
-                }
-                if (mRemote != null) {
-                    try {
-                        mRemote.cancel();
-                    } catch (RemoteException ex) {
-                    }
+            if (mIsCanceled) {
+                return;
+            }
+            mIsCanceled = true;
+            mCancelInProgress = true;
+            listener = mOnCancelListener;
+            remote = mRemote;
+        }
+
+        try {
+            if (listener != null) {
+                listener.onCancel();
+            }
+            if (remote != null) {
+                try {
+                    remote.cancel();
+                } catch (RemoteException ex) {
                 }
             }
+        } finally {
+            synchronized (this) {
+                mCancelInProgress = false;
+                notifyAll();
+            }
         }
     }
 
@@ -86,38 +101,62 @@ public final class CancellationSignal {
      * If {@link CancellationSignal#cancel} has already been called, then the provided
      * listener is invoked immediately.
      *
-     * The listener is called while holding the cancellation signal's lock which is
-     * also held while registering or unregistering the listener.  Because of the lock,
-     * it is not possible for the listener to run after it has been unregistered.
-     * This design choice makes it easier for clients of {@link CancellationSignal} to
-     * prevent race conditions related to listener registration and unregistration.
+     * This method is guaranteed that the listener will not be called after it
+     * has been removed.
      *
      * @param listener The cancellation listener, or null to remove the current listener.
      */
     public void setOnCancelListener(OnCancelListener listener) {
         synchronized (this) {
+            waitForCancelFinishedLocked();
+
+            if (mOnCancelListener == listener) {
+                return;
+            }
             mOnCancelListener = listener;
-            if (mIsCanceled && listener != null) {
-                listener.onCancel();
+            if (!mIsCanceled || listener == null) {
+                return;
             }
         }
+        listener.onCancel();
     }
 
     /**
      * Sets the remote transport.
      *
+     * If {@link CancellationSignal#cancel} has already been called, then the provided
+     * remote transport is canceled immediately.
+     *
+     * This method is guaranteed that the remote transport will not be called after it
+     * has been removed.
+     *
      * @param remote The remote transport, or null to remove.
      *
      * @hide
      */
     public void setRemote(ICancellationSignal remote) {
         synchronized (this) {
+            waitForCancelFinishedLocked();
+
+            if (mRemote == remote) {
+                return;
+            }
             mRemote = remote;
-            if (mIsCanceled && remote != null) {
-                try {
-                    remote.cancel();
-                } catch (RemoteException ex) {
-                }
+            if (!mIsCanceled || remote == null) {
+                return;
+            }
+        }
+        try {
+            remote.cancel();
+        } catch (RemoteException ex) {
+        }
+    }
+
+    private void waitForCancelFinishedLocked() {
+        while (mCancelInProgress) {
+            try {
+                wait();
+            } catch (InterruptedException ex) {
             }
         }
     }
diff --git a/core/java/android/database/sqlite/SQLiteConnectionPool.java b/core/java/android/database/sqlite/SQLiteConnectionPool.java
index 5c8e38b..a175662 100644
--- a/core/java/android/database/sqlite/SQLiteConnectionPool.java
+++ b/core/java/android/database/sqlite/SQLiteConnectionPool.java
@@ -594,6 +594,7 @@ public final class SQLiteConnectionPool implements Closeable {
                 (connectionFlags & CONNECTION_FLAG_PRIMARY_CONNECTION_AFFINITY) != 0;
 
         final ConnectionWaiter waiter;
+        final int nonce;
         synchronized (mLock) {
             throwIfClosedLocked();
 
@@ -636,73 +637,75 @@ public final class SQLiteConnectionPool implements Closeable {
                 mConnectionWaiterQueue = waiter;
             }
 
-            if (cancellationSignal != null) {
-                final int nonce = waiter.mNonce;
-                cancellationSignal.setOnCancelListener(new CancellationSignal.OnCancelListener() {
-                    @Override
-                    public void onCancel() {
-                        synchronized (mLock) {
-                            cancelConnectionWaiterLocked(waiter, nonce);
+            nonce = waiter.mNonce;
+        }
+
+        // Set up the cancellation listener.
+        if (cancellationSignal != null) {
+            cancellationSignal.setOnCancelListener(new CancellationSignal.OnCancelListener() {
+                @Override
+                public void onCancel() {
+                    synchronized (mLock) {
+                        if (waiter.mNonce == nonce) {
+                            cancelConnectionWaiterLocked(waiter);
                         }
                     }
-                });
-            }
+                }
+            });
         }
-
-        // Park the thread until a connection is assigned or the pool is closed.
-        // Rethrow an exception from the wait, if we got one.
-        long busyTimeoutMillis = CONNECTION_POOL_BUSY_MILLIS;
-        long nextBusyTimeoutTime = waiter.mStartTime + busyTimeoutMillis;
-        for (;;) {
-            // Detect and recover from connection leaks.
-            if (mConnectionLeaked.compareAndSet(true, false)) {
-                synchronized (mLock) {
-                    wakeConnectionWaitersLocked();
+        try {
+            // Park the thread until a connection is assigned or the pool is closed.
+            // Rethrow an exception from the wait, if we got one.
+            long busyTimeoutMillis = CONNECTION_POOL_BUSY_MILLIS;
+            long nextBusyTimeoutTime = waiter.mStartTime + busyTimeoutMillis;
+            for (;;) {
+                // Detect and recover from connection leaks.
+                if (mConnectionLeaked.compareAndSet(true, false)) {
+                    synchronized (mLock) {
+                        wakeConnectionWaitersLocked();
+                    }
                 }
-            }
 
-            // Wait to be unparked (may already have happened), a timeout, or interruption.
-            LockSupport.parkNanos(this, busyTimeoutMillis * 1000000L);
+                // Wait to be unparked (may already have happened), a timeout, or interruption.
+                LockSupport.parkNanos(this, busyTimeoutMillis * 1000000L);
 
-            // Clear the interrupted flag, just in case.
-            Thread.interrupted();
+                // Clear the interrupted flag, just in case.
+                Thread.interrupted();
 
-            // Check whether we are done waiting yet.
-            synchronized (mLock) {
-                throwIfClosedLocked();
-
-                final SQLiteConnection connection = waiter.mAssignedConnection;
-                final RuntimeException ex = waiter.mException;
-                if (connection != null || ex != null) {
-                    if (cancellationSignal != null) {
-                        cancellationSignal.setOnCancelListener(null);
-                    }
-                    recycleConnectionWaiterLocked(waiter);
-                    if (connection != null) {
-                        return connection;
+                // Check whether we are done waiting yet.
+                synchronized (mLock) {
+                    throwIfClosedLocked();
+
+                    final SQLiteConnection connection = waiter.mAssignedConnection;
+                    final RuntimeException ex = waiter.mException;
+                    if (connection != null || ex != null) {
+                        recycleConnectionWaiterLocked(waiter);
+                        if (connection != null) {
+                            return connection;
+                        }
+                        throw ex; // rethrow!
                     }
-                    throw ex; // rethrow!
-                }
 
-                final long now = SystemClock.uptimeMillis();
-                if (now < nextBusyTimeoutTime) {
-                    busyTimeoutMillis = now - nextBusyTimeoutTime;
-                } else {
-                    logConnectionPoolBusyLocked(now - waiter.mStartTime, connectionFlags);
-                    busyTimeoutMillis = CONNECTION_POOL_BUSY_MILLIS;
-                    nextBusyTimeoutTime = now + busyTimeoutMillis;
+                    final long now = SystemClock.uptimeMillis();
+                    if (now < nextBusyTimeoutTime) {
+                        busyTimeoutMillis = now - nextBusyTimeoutTime;
+                    } else {
+                        logConnectionPoolBusyLocked(now - waiter.mStartTime, connectionFlags);
+                        busyTimeoutMillis = CONNECTION_POOL_BUSY_MILLIS;
+                        nextBusyTimeoutTime = now + busyTimeoutMillis;
+                    }
                 }
             }
+        } finally {
+            // Remove the cancellation listener.
+            if (cancellationSignal != null) {
+                cancellationSignal.setOnCancelListener(null);
+            }
         }
     }
 
     // Can't throw.
-    private void cancelConnectionWaiterLocked(ConnectionWaiter waiter, int nonce) {
-        if (waiter.mNonce != nonce) {
-            // Waiter already removed and recycled.
-            return;
-        }
-
+    private void cancelConnectionWaiterLocked(ConnectionWaiter waiter) {
         if (waiter.mAssignedConnection != null || waiter.mException != null) {
             // Waiter is done waiting but has not woken up yet.
             return;