Reuse transports if possible in LocationManager
Allow LocationManagerService to reuse the same listener transports, and
fix memory leak in event of RemoteException during registration.
Bug: 142024887
Test: manual
Change-Id: Ida25fe08c30e5936680c95c23cd03fbaf09ff574
diff --git a/location/java/android/location/LocationManager.java b/location/java/android/location/LocationManager.java
index 68480ce..f3d6875 100644
--- a/location/java/android/location/LocationManager.java
+++ b/location/java/android/location/LocationManager.java
@@ -940,9 +940,8 @@
@Nullable LocationRequest locationRequest,
@NonNull LocationListener listener,
@Nullable Looper looper) {
- requestLocationUpdates(locationRequest,
- new LocationListenerTransport(looper == null ? new Handler() : new Handler(looper),
- listener));
+ Handler handler = looper == null ? new Handler() : new Handler(looper);
+ requestLocationUpdates(locationRequest, new HandlerExecutor(handler), listener);
}
/**
@@ -969,7 +968,31 @@
@Nullable LocationRequest locationRequest,
@NonNull @CallbackExecutor Executor executor,
@NonNull LocationListener listener) {
- requestLocationUpdates(locationRequest, new LocationListenerTransport(executor, listener));
+ synchronized (mListeners) {
+ LocationListenerTransport transport = mListeners.get(listener);
+ if (transport != null) {
+ transport.unregister();
+ } else {
+ transport = new LocationListenerTransport(listener);
+ mListeners.put(listener, transport);
+ }
+ transport.register(executor);
+
+ boolean registered = false;
+ try {
+ mService.requestLocationUpdates(locationRequest, transport, null,
+ mContext.getPackageName());
+ registered = true;
+ } catch (RemoteException e) {
+ throw e.rethrowFromSystemServer();
+ } finally {
+ if (!registered) {
+ // allow gc after exception
+ transport.unregister();
+ mListeners.remove(listener);
+ }
+ }
+ }
}
/**
@@ -1009,23 +1032,6 @@
}
}
- private void requestLocationUpdates(@Nullable LocationRequest request,
- @NonNull LocationListenerTransport transport) {
- synchronized (mListeners) {
- LocationListenerTransport oldTransport = mListeners.put(transport.getKey(), transport);
- if (oldTransport != null) {
- oldTransport.unregisterListener();
- }
-
- try {
- mService.requestLocationUpdates(request, transport, null,
- mContext.getPackageName());
- } catch (RemoteException e) {
- throw e.rethrowFromSystemServer();
- }
- }
- }
-
/**
* Set the last known location with a new location.
*
@@ -1079,7 +1085,7 @@
if (transport == null) {
return;
}
- transport.unregisterListener();
+ transport.unregister();
try {
mService.removeUpdates(transport, null, mContext.getPackageName());
@@ -2237,49 +2243,45 @@
private class LocationListenerTransport extends ILocationListener.Stub {
- private final Executor mExecutor;
- @Nullable private volatile LocationListener mListener;
+ private final LocationListener mListener;
+ @Nullable private volatile Executor mExecutor = null;
- private LocationListenerTransport(@NonNull Handler handler,
- @NonNull LocationListener listener) {
- Preconditions.checkArgument(handler != null, "invalid null handler");
+ private LocationListenerTransport(@NonNull LocationListener listener) {
Preconditions.checkArgument(listener != null, "invalid null listener");
-
- mExecutor = new HandlerExecutor(handler);
mListener = listener;
}
- private LocationListenerTransport(@NonNull Executor executor,
- @NonNull LocationListener listener) {
- Preconditions.checkArgument(executor != null, "invalid null executor");
- Preconditions.checkArgument(listener != null, "invalid null listener");
-
- mExecutor = executor;
- mListener = listener;
- }
-
- private LocationListener getKey() {
+ public LocationListener getKey() {
return mListener;
}
- private void unregisterListener() {
- mListener = null;
+ public void register(@NonNull Executor executor) {
+ Preconditions.checkArgument(executor != null, "invalid null executor");
+ mExecutor = executor;
+ }
+
+ public void unregister() {
+ mExecutor = null;
}
@Override
public void onLocationChanged(Location location) {
+ Executor currentExecutor = mExecutor;
+ if (currentExecutor == null) {
+ return;
+ }
+
try {
- mExecutor.execute(() -> {
+ currentExecutor.execute(() -> {
try {
- LocationListener listener = mListener;
- if (listener == null) {
+ if (currentExecutor != mExecutor) {
return;
}
// we may be under the binder identity if a direct executor is used
long identity = Binder.clearCallingIdentity();
try {
- listener.onLocationChanged(location);
+ mListener.onLocationChanged(location);
} finally {
Binder.restoreCallingIdentity(identity);
}
@@ -2295,18 +2297,22 @@
@Override
public void onStatusChanged(String provider, int status, Bundle extras) {
+ Executor currentExecutor = mExecutor;
+ if (currentExecutor == null) {
+ return;
+ }
+
try {
- mExecutor.execute(() -> {
+ currentExecutor.execute(() -> {
try {
- LocationListener listener = mListener;
- if (listener == null) {
+ if (currentExecutor != mExecutor) {
return;
}
// we may be under the binder identity if a direct executor is used
long identity = Binder.clearCallingIdentity();
try {
- listener.onStatusChanged(provider, status, extras);
+ mListener.onStatusChanged(provider, status, extras);
} finally {
Binder.restoreCallingIdentity(identity);
}
@@ -2322,18 +2328,22 @@
@Override
public void onProviderEnabled(String provider) {
+ Executor currentExecutor = mExecutor;
+ if (currentExecutor == null) {
+ return;
+ }
+
try {
- mExecutor.execute(() -> {
+ currentExecutor.execute(() -> {
try {
- LocationListener listener = mListener;
- if (listener == null) {
+ if (currentExecutor != mExecutor) {
return;
}
// we may be under the binder identity if a direct executor is used
long identity = Binder.clearCallingIdentity();
try {
- listener.onProviderEnabled(provider);
+ mListener.onProviderEnabled(provider);
} finally {
Binder.restoreCallingIdentity(identity);
}
@@ -2349,18 +2359,22 @@
@Override
public void onProviderDisabled(String provider) {
+ Executor currentExecutor = mExecutor;
+ if (currentExecutor == null) {
+ return;
+ }
+
try {
- mExecutor.execute(() -> {
+ currentExecutor.execute(() -> {
try {
- LocationListener listener = mListener;
- if (listener == null) {
+ if (currentExecutor != mExecutor) {
return;
}
// we may be under the binder identity if a direct executor is used
long identity = Binder.clearCallingIdentity();
try {
- listener.onProviderDisabled(provider);
+ mListener.onProviderDisabled(provider);
} finally {
Binder.restoreCallingIdentity(identity);
}