SupplicantHal/HostapdHal: Wait for death

When wpa_supplicant/hostapd is being terminated, ensure that we block
for a small timeout (50 ms) to ensure that the daemon is indeed dead. This
prevents a potential race when wifi/tethering is quickly toggled & the
previous instance of the daemon is not fully terminated before the next
toggle on.

Bug: 134729435
Test: Manually toggled wifi/tethering multiple times in quick
succession.
Test: Will send for full regression testing.

Change-Id: I0cd14e22bba21a4f7fdebd8e68642bcae9739b25

2 files changed, 55 insertions, 8 deletions

diff --git a/service/java/com/android/server/wifi/HostapdHal.java b/service/java/com/android/server/wifi/HostapdHal.java
index 69c787f48..ee96fb269 100644
--- a/service/java/com/android/server/wifi/HostapdHal.java
+++ b/service/java/com/android/server/wifi/HostapdHal.java
@@ -39,6 +39,9 @@ import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.List;
 import java.util.NoSuchElementException;
+import java.util.Random;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
 
 import javax.annotation.concurrent.ThreadSafe;
 
@@ -51,6 +54,8 @@ public class HostapdHal {
     private static final String TAG = "HostapdHal";
     @VisibleForTesting
     public static final String HAL_INSTANCE_NAME = "default";
+    @VisibleForTesting
+    public static final long WAIT_FOR_DEATH_TIMEOUT_MS = 50L;
 
     private final Object mLock = new Object();
     private boolean mVerboseLoggingEnabled = false;
@@ -230,11 +235,11 @@ public class HostapdHal {
      * Link to death for IHostapd object.
      * @return true on success, false otherwise.
      */
-    private boolean linkToHostapdDeath() {
+    private boolean linkToHostapdDeath(HwRemoteBinder.DeathRecipient deathRecipient, long cookie) {
         synchronized (mLock) {
             if (mIHostapd == null) return false;
             try {
-                if (!mIHostapd.linkToDeath(mHostapdDeathRecipient, ++mDeathRecipientCookie)) {
+                if (!mIHostapd.linkToDeath(deathRecipient, cookie)) {
                     Log.wtf(TAG, "Error on linkToDeath on IHostapd");
                     hostapdServiceDiedHandler(mDeathRecipientCookie);
                     return false;
@@ -282,7 +287,7 @@ public class HostapdHal {
                 Log.e(TAG, "Got null IHostapd service. Stopping hostapd HIDL startup");
                 return false;
             }
-            if (!linkToHostapdDeath()) {
+            if (!linkToHostapdDeath(mHostapdDeathRecipient, ++mDeathRecipientCookie)) {
                 mIHostapd = null;
                 return false;
             }
@@ -486,10 +491,19 @@ public class HostapdHal {
     }
 
     /**
-     * Terminate the hostapd daemon.
+     * Terminate the hostapd daemon & wait for it's death.
      */
     public void terminate() {
         synchronized (mLock) {
+            // Register for a new death listener to block until hostapd is dead.
+            final long waitForDeathCookie = new Random().nextLong();
+            final CountDownLatch waitForDeathLatch = new CountDownLatch(1);
+            linkToHostapdDeath((cookie) -> {
+                Log.d(TAG, "IHostapd died: cookie=" + cookie);
+                if (cookie != waitForDeathCookie) return;
+                waitForDeathLatch.countDown();
+            }, waitForDeathCookie);
+
             final String methodStr = "terminate";
             if (!checkHostapdAndLogFailure(methodStr)) return;
             try {
@@ -497,6 +511,15 @@ public class HostapdHal {
             } catch (RemoteException e) {
                 handleRemoteException(e, methodStr);
             }
+
+            // Now wait for death listener callback to confirm that it's dead.
+            try {
+                if (!waitForDeathLatch.await(WAIT_FOR_DEATH_TIMEOUT_MS, TimeUnit.MILLISECONDS)) {
+                    Log.w(TAG, "Timed out waiting for confirmation of hostapd death");
+                }
+            } catch (InterruptedException e) {
+                Log.w(TAG, "Failed to wait for hostapd death");
+            }
         }
     }
 
diff --git a/service/java/com/android/server/wifi/SupplicantStaIfaceHal.java b/service/java/com/android/server/wifi/SupplicantStaIfaceHal.java
index bed66d926..86518c761 100644
--- a/service/java/com/android/server/wifi/SupplicantStaIfaceHal.java
+++ b/service/java/com/android/server/wifi/SupplicantStaIfaceHal.java
@@ -86,6 +86,9 @@ import java.util.List;
 import java.util.Map;
 import java.util.NoSuchElementException;
 import java.util.Objects;
+import java.util.Random;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 
@@ -108,6 +111,8 @@ public class SupplicantStaIfaceHal {
     public static final String INIT_STOP_PROPERTY = "ctl.stop";
     @VisibleForTesting
     public static final String INIT_SERVICE_NAME = "wpa_supplicant";
+    @VisibleForTesting
+    public static final long WAIT_FOR_DEATH_TIMEOUT_MS = 50L;
     /**
      * Regex pattern for extracting the wps device type bytes.
      * Matches a strings like the following: "<categ>-<OUI>-<subcateg>";
@@ -262,11 +267,12 @@ public class SupplicantStaIfaceHal {
         }
     }
 
-    private boolean linkToSupplicantDeath() {
+    private boolean linkToSupplicantDeath(
+            HwRemoteBinder.DeathRecipient deathRecipient, long cookie) {
         synchronized (mLock) {
             if (mISupplicant == null) return false;
             try {
-                if (!mISupplicant.linkToDeath(mSupplicantDeathRecipient, ++mDeathRecipientCookie)) {
+                if (!mISupplicant.linkToDeath(deathRecipient, cookie)) {
                     Log.wtf(TAG, "Error on linkToDeath on ISupplicant");
                     supplicantServiceDiedHandler(mDeathRecipientCookie);
                     return false;
@@ -294,7 +300,7 @@ public class SupplicantStaIfaceHal {
                 Log.e(TAG, "Got null ISupplicant service. Stopping supplicant HIDL startup");
                 return false;
             }
-            if (!linkToSupplicantDeath()) {
+            if (!linkToSupplicantDeath(mSupplicantDeathRecipient, ++mDeathRecipientCookie)) {
                 return false;
             }
         }
@@ -645,10 +651,19 @@ public class SupplicantStaIfaceHal {
     }
 
     /**
-     * Terminate the supplicant daemon.
+     * Terminate the supplicant daemon & wait for it's death.
      */
     public void terminate() {
         synchronized (mLock) {
+            // Register for a new death listener to block until supplicant is dead.
+            final long waitForDeathCookie = new Random().nextLong();
+            final CountDownLatch waitForDeathLatch = new CountDownLatch(1);
+            linkToSupplicantDeath((cookie) -> {
+                Log.d(TAG, "ISupplicant died: cookie=" + cookie);
+                if (cookie != waitForDeathCookie) return;
+                waitForDeathLatch.countDown();
+            }, waitForDeathCookie);
+
             if (isV1_1()) {
                 Log.i(TAG, "Terminating supplicant using HIDL");
                 terminate_V1_1();
@@ -656,6 +671,15 @@ public class SupplicantStaIfaceHal {
                 Log.i(TAG, "Terminating supplicant using init");
                 mPropertyService.set(INIT_STOP_PROPERTY, INIT_SERVICE_NAME);
             }
+
+            // Now wait for death listener callback to confirm that it's dead.
+            try {
+                if (!waitForDeathLatch.await(WAIT_FOR_DEATH_TIMEOUT_MS, TimeUnit.MILLISECONDS)) {
+                    Log.w(TAG, "Timed out waiting for confirmation of supplicant death");
+                }
+            } catch (InterruptedException e) {
+                Log.w(TAG, "Failed to wait for supplicant death");
+            }
         }
     }