Fix LM deadlock possibility

Put onRemoved callbacks on the same thread as all other callbacks so
that they cannot be reordered and introduce deadlock or other bugs.

Test: n/a
Bug: 144487580
Change-Id: I60ad76c40348138bf54d39e966c133fa5db89861

1 files changed, 40 insertions, 3 deletions

diff --git a/location/java/android/location/LocationManager.java b/location/java/android/location/LocationManager.java
index 39a7e25cb68b..5030e6625d5d 100644
--- a/location/java/android/location/LocationManager.java
+++ b/location/java/android/location/LocationManager.java
@@ -2642,10 +2642,47 @@ public class LocationManager {
 
         @Override
         public void onRemoved() {
-            unregister();
-            synchronized (mListeners) {
-                mListeners.remove(mListener, this);
+            // TODO: onRemoved is necessary to GC hanging listeners, but introduces some interesting
+            //  broken edge cases. luckily these edge cases are quite unlikely. consider the
+            //  following interleaving for instance:
+            //    1) client adds single shot location request (A)
+            //    2) client gets removal callback, and schedules it for execution
+            //    3) client replaces single shot request with a different location request (B)
+            //    4) prior removal callback is executed, removing location request (B) incorrectly
+            //  what's needed is a way to identify which listener a callback belongs to. currently
+            //  we reuse the same transport object for the same listeners (so that we don't leak
+            //  transport objects on the server side). there seem to be two solutions:
+            //    1) when reregistering a request, first unregister the current transport, then
+            //       register with a new transport object (never reuse transport objects) - the
+            //       downside is that this breaks the server's knowledge that the request is the
+            //       same object, and thus breaks optimizations such as reusing the same transport
+            //       state.
+            //    2) pass some other type of marker in addition to the transport (for example an
+            //       incrementing integer representing the transport "version"), and pass this
+            //       marker back into callbacks so that each callback knows which transport
+            //       "version" it belongs to and can not execute itself if the version does not
+            //       match.
+            //  (1) seems like the preferred solution as it's simpler to implement and the above
+            //  mentioned server optimizations are not terribly important (they can be bypassed by
+            //  clients that use a new listener every time anyways).
+
+            Executor currentExecutor = mExecutor;
+            if (currentExecutor == null) {
+                // we've already been unregistered, no work to do anyways
+                return;
             }
+
+            // must be executed on the same executor so callback execution cannot be reordered
+            currentExecutor.execute(() -> {
+                if (currentExecutor != mExecutor) {
+                    return;
+                }
+
+                unregister();
+                synchronized (mListeners) {
+                    mListeners.remove(mListener, this);
+                }
+            });
         }
 
         private void locationCallbackFinished() {