Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit d73a4f01 authored by Soonil Nagarkar's avatar Soonil Nagarkar
Browse files

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
parent 2dbc22cb
Loading
Loading
Loading
Loading
+40 −3
Original line number Diff line number Diff line
@@ -2642,10 +2642,47 @@ public class LocationManager {

        @Override
        public void onRemoved() {
            // 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() {