From 0ec832bba2ec5c5e45ff03184ecb95890447acb2 Mon Sep 17 00:00:00 2001
From: Soonil Nagarkar <sooniln@google.com>
Date: Wed, 22 Sep 2021 16:31:32 -0700
Subject: [PATCH] Only allow completion callback to be run once for PIs

PendingIntent.send() should either throw an exception OR run it's
completion callback, but not both. However, a bug that is not fixable in
the near term means that it's possible for both to happen. In order to
avoid the completion callback from being run even if send() fails, we
gate it.

Bug: 199464864
Test: manual + presubmits
Change-Id: I993436b2bd2ec5240be92e6d3815b3a9edef970f
---
 .../provider/LocationProviderManager.java     | 58 ++++++++++++++++++-
 1 file changed, 56 insertions(+), 2 deletions(-)

diff --git a/services/core/java/com/android/server/location/provider/LocationProviderManager.java b/services/core/java/com/android/server/location/provider/LocationProviderManager.java
index 05999ae13b8d6..345dc217110b3 100644
--- a/services/core/java/com/android/server/location/provider/LocationProviderManager.java
+++ b/services/core/java/com/android/server/location/provider/LocationProviderManager.java
@@ -243,15 +243,24 @@ public class LocationProviderManager extends
                 intent.putExtra(KEY_LOCATIONS, locationResult.asList().toArray(new Location[0]));
             }
 
+            // 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
+            // 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);
+
             mPendingIntent.send(
                     mContext,
                     0,
                     intent,
-                    onCompleteCallback != null ? (pI, i, rC, rD, rE) -> onCompleteCallback.run()
-                            : null,
+                    (pI, i, rC, rD, rE) -> gatedCallback.run(),
                     null,
                     null,
                     options.toBundle());
+            gatedCallback.allow();
         }
 
         @Override
@@ -2734,4 +2743,49 @@ public class LocationProviderManager extends
             }
         }
     }
+
+    private static class GatedCallback implements Runnable {
+
+        private @Nullable Runnable mCallback;
+
+        @GuardedBy("this")
+        private boolean mGate;
+        @GuardedBy("this")
+        private boolean mRun;
+
+        GatedCallback(Runnable callback) {
+            mCallback = callback;
+        }
+
+        public void allow() {
+            Runnable callback = null;
+            synchronized (this) {
+                mGate = true;
+                if (mRun && mCallback != null) {
+                    callback = mCallback;
+                    mCallback = null;
+                }
+            }
+
+            if (callback != null) {
+                callback.run();
+            }
+        }
+
+        @Override
+        public void run() {
+            Runnable callback = null;
+            synchronized (this) {
+                mRun = true;
+                if (mGate && mCallback != null) {
+                    callback = mCallback;
+                    mCallback = null;
+                }
+            }
+
+            if (callback != null) {
+                callback.run();
+            }
+        }
+    }
 }