Merge "Revert "Revert "Reduce allocations on location delivery""" into sc-v2-dev

services/core/java/com/android/server/location/provider/LocationProviderManager.java

@@ -177,7 +177,7 @@ public class LocationProviderManager extends
     protected interface LocationTransport {
 
         void deliverOnLocationChanged(LocationResult locationResult,
-                @Nullable Runnable onCompleteCallback) throws Exception;
+                @Nullable IRemoteCallback onCompleteCallback) throws Exception;
         void deliverOnFlushComplete(int requestCode) throws Exception;
     }
 
@@ -197,9 +197,8 @@ public class LocationProviderManager extends
 
         @Override
         public void deliverOnLocationChanged(LocationResult locationResult,
-                @Nullable Runnable onCompleteCallback) throws RemoteException {
+                @Nullable IRemoteCallback onCompleteCallback) throws RemoteException {
-            mListener.onLocationChanged(locationResult.asList(),
+            mListener.onLocationChanged(locationResult.asList(), onCompleteCallback);
-                    SingleUseCallback.wrap(onCompleteCallback));
         }
 
         @Override
@@ -227,7 +226,7 @@ public class LocationProviderManager extends
 
         @Override
         public void deliverOnLocationChanged(LocationResult locationResult,
-                @Nullable Runnable onCompleteCallback)
+                @Nullable IRemoteCallback onCompleteCallback)
                 throws PendingIntent.CanceledException {
             BroadcastOptions options = BroadcastOptions.makeBasic();
             options.setDontSendToRestrictedApps(true);
@@ -243,20 +242,34 @@ public class LocationProviderManager extends
                 intent.putExtra(KEY_LOCATIONS, locationResult.asList().toArray(new Location[0]));
             }
 
+            PendingIntent.OnFinished onFinished = null;
+
             // send() SHOULD only run the completion callback if it completes successfully. however,
-            // b/199464864 (which could not be fixed in the S timeframe) means that it's possible
+            // b/201299281 (which could not be fixed in the S timeframe) means that it's possible
             // for send() to throw an exception AND run the completion callback. if this happens, we
             // would over-release the wakelock... we take matters into our own hands to ensure that
             // the completion callback can only be run if send() completes successfully. this means
             // the completion callback may be run inline - but as we've never specified what thread
             // the callback is run on, this is fine.
-            GatedCallback gatedCallback = new GatedCallback(onCompleteCallback);
+            GatedCallback gatedCallback;
+            if (onCompleteCallback != null) {
+                gatedCallback = new GatedCallback(() -> {
+                    try {
+                        onCompleteCallback.sendResult(null);
+                    } catch (RemoteException e) {
+                        throw e.rethrowFromSystemServer();
+                    }
+                });
+                onFinished = (pI, i, rC, rD, rE) -> gatedCallback.run();
+            } else {
+                gatedCallback = new GatedCallback(null);
+            }
 
             mPendingIntent.send(
                     mContext,
                     0,
                     intent,
-                    (pI, i, rC, rD, rE) -> gatedCallback.run(),
+                    onFinished,
                     null,
                     null,
                     options.toBundle());
@@ -293,7 +306,7 @@ public class LocationProviderManager extends
 
         @Override
         public void deliverOnLocationChanged(@Nullable LocationResult locationResult,
-                @Nullable Runnable onCompleteCallback)
+                @Nullable IRemoteCallback onCompleteCallback)
                 throws RemoteException {
             // ILocationCallback doesn't currently support completion callbacks
             Preconditions.checkState(onCompleteCallback == null);
@@ -714,6 +727,13 @@ public class LocationProviderManager extends
 
         final PowerManager.WakeLock mWakeLock;
 
+        // b/206340085 - if we allocate a new wakelock releaser object for every delivery we
+        // increase the risk of resource starvation. if a client stops processing deliveries the
+        // system server binder allocation pool will be starved as we continue to queue up
+        // deliveries, each with a new allocation. in order to mitigate this, we use a single
+        // releaser object per registration rather than per delivery.
+        final ExternalWakeLockReleaser mWakeLockReleaser;
+
         private volatile ProviderTransport mProviderTransport;
         private int mNumLocationsDelivered = 0;
         private long mExpirationRealtimeMs = Long.MAX_VALUE;
@@ -727,6 +747,7 @@ public class LocationProviderManager extends
                     .newWakeLock(PowerManager.PARTIAL_WAKE_LOCK, WAKELOCK_TAG);
             mWakeLock.setReferenceCounted(true);
             mWakeLock.setWorkSource(request.getWorkSource());
+            mWakeLockReleaser = new ExternalWakeLockReleaser(identity, mWakeLock);
         }
 
         @Override
@@ -943,7 +964,7 @@ public class LocationProviderManager extends
                     }
 
                     listener.deliverOnLocationChanged(deliverLocationResult,
-                            mUseWakeLock ? mWakeLock::release : null);
+                            mUseWakeLock ? mWakeLockReleaser : null);
                     EVENT_LOG.logProviderDeliveredLocations(mName, locationResult.size(),
                             getIdentity());
                 }
@@ -2761,7 +2782,7 @@ public class LocationProviderManager extends
         @GuardedBy("this")
         private boolean mRun;
 
-        GatedCallback(Runnable callback) {
+        GatedCallback(@Nullable Runnable callback) {
             mCallback = callback;
         }
 
@@ -2796,4 +2817,24 @@ public class LocationProviderManager extends
             }
         }
     }
+
+    private static class ExternalWakeLockReleaser extends IRemoteCallback.Stub {
+
+        private final CallerIdentity mIdentity;
+        private final PowerManager.WakeLock mWakeLock;
+
+        ExternalWakeLockReleaser(CallerIdentity identity, PowerManager.WakeLock wakeLock) {
+            mIdentity = identity;
+            mWakeLock = Objects.requireNonNull(wakeLock);
+        }
+
+        @Override
+        public void sendResult(Bundle data) {
+            try {
+                mWakeLock.release();
+            } catch (RuntimeException e) {
+                Log.e(TAG, "wakelock over-released by " + mIdentity, e);
+            }
+        }
+    }
 }