diff options
| -rw-r--r-- | location/java/android/location/LocationManager.java | 43 |
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() { |