Ensure that ScanInfo is cached before callbacks fire

Fixes a regression that was introduced in aosp/1312940.

There is a critical section in requestNetworkScan after the scanId
has been generated and before the ScanInfo is added to the cache.
If during this time a callback fires on a separate thread, it will
not yet find the ScanInfo record, and due to strict checking, this
causes a RuntimeException. This CL re-expands the synchronized block
to cover the critical section, and clarifies a little bounds checking.

Note: this change is not ideal. This means that all callbacks for all
scans will be blocked while a request for a new scan is being made,
which was the motivation behind the change that introduced this
regression. But, a narrow performance hit is better than a crash, and
it's not worth the complexity of a bigger fix.

Bug: 200634560
Test: compilation
Merged-In: I4670da109256170121ceb6d8fbad0efda310335f
Change-Id: I4670da109256170121ceb6d8fbad0efda310335f

1 files changed, 18 insertions, 17 deletions

diff --git a/telephony/java/android/telephony/TelephonyScanManager.java b/telephony/java/android/telephony/TelephonyScanManager.java
index e890acb36b48..9572154ec773 100644
--- a/telephony/java/android/telephony/TelephonyScanManager.java
+++ b/telephony/java/android/telephony/TelephonyScanManager.java
@@ -36,6 +36,7 @@ import com.android.telephony.Rlog;
 
 import java.util.Arrays;
 import java.util.List;
+import java.util.Objects;
 import java.util.concurrent.Executor;
 
 /**
@@ -152,16 +153,9 @@ public final class TelephonyScanManager {
                     throw new RuntimeException(
                         "Failed to find NetworkScanInfo with id " + message.arg2);
                 }
-                NetworkScanCallback callback = nsi.mCallback;
-                Executor executor = nsi.mExecutor;
-                if (callback == null) {
-                    throw new RuntimeException(
-                        "Failed to find NetworkScanCallback with id " + message.arg2);
-                }
-                if (executor == null) {
-                    throw new RuntimeException(
-                        "Failed to find Executor with id " + message.arg2);
-                }
+
+                final NetworkScanCallback callback = nsi.mCallback;
+                final Executor executor = nsi.mExecutor;
 
                 switch (message.what) {
                     case CALLBACK_RESTRICTED_SCAN_RESULTS:
@@ -246,17 +240,24 @@ public final class TelephonyScanManager {
             NetworkScanRequest request, Executor executor, NetworkScanCallback callback,
             String callingPackage, @Nullable String callingFeatureId) {
         try {
+            Objects.requireNonNull(request, "Request was null");
+            Objects.requireNonNull(callback, "Callback was null");
+            Objects.requireNonNull(executor, "Executor was null");
             final ITelephony telephony = getITelephony();
             if (telephony == null) return null;
 
-            int scanId = telephony.requestNetworkScan(
-                    subId, request, mMessenger, new Binder(), callingPackage,
-                    callingFeatureId);
-            if (scanId == INVALID_SCAN_ID) {
-                Rlog.e(TAG, "Failed to initiate network scan");
-                return null;
-            }
+            // The lock must be taken before calling requestNetworkScan because the resulting
+            // scanId can be invoked asynchronously on another thread at any time after
+            // requestNetworkScan invoked, leaving a critical section between that call and adding
+            // the record to the ScanInfo cache.
             synchronized (mScanInfo) {
+                int scanId = telephony.requestNetworkScan(
+                        subId, request, mMessenger, new Binder(), callingPackage,
+                        callingFeatureId);
+                if (scanId == INVALID_SCAN_ID) {
+                    Rlog.e(TAG, "Failed to initiate network scan");
+                    return null;
+                }
                 // We link to death whenever a scan is started to ensure that we are linked
                 // at the point that phone process death might matter.
                 // We never unlink because: