CaptivePortalLogin correctly unregisters callbacks

The NetworkCallback registered by the CaptivePortalLogin activity in
onCreate was unregistered in both onDestroy() and done(). In addition
done() can be called concurrently from different places (from the
webview, from the captive portal test probe, from the activity menu),
resulting in incorrectly unregistering the callback more than once.

This patch fixes the lifecycle management of the NetworkCallback
registered by the CaptivePortalLogin activity so that it is unregistered
once only in onDestroy.

In addition the done() method is made robust against multiple calls and
becomes a no-op after the first call. This avoids multiple calls
to CaptivePortal for the same captive portal.

Bug: 62497809
Test: tested manually with captive portal setup in the office
Merged-In: I77fbeb55cf91d3b44e91d2fecb800dae40279652

(cherry pick from commit 31c176d91068934f5b79125afa3b5ce0f44dbe8f)

Change-Id: I34706201395ed5ddba11d12cc75c357e371d1920

1 files changed, 8 insertions, 5 deletions

diff --git a/packages/CaptivePortalLogin/src/com/android/captiveportallogin/CaptivePortalLoginActivity.java b/packages/CaptivePortalLogin/src/com/android/captiveportallogin/CaptivePortalLoginActivity.java
index 22f53eabb99b..e38cca739253 100644
--- a/packages/CaptivePortalLogin/src/com/android/captiveportallogin/CaptivePortalLoginActivity.java
+++ b/packages/CaptivePortalLogin/src/com/android/captiveportallogin/CaptivePortalLoginActivity.java
@@ -57,6 +57,7 @@ import java.lang.InterruptedException;
 import java.lang.reflect.Field;
 import java.lang.reflect.Method;
 import java.util.Random;
+import java.util.concurrent.atomic.AtomicBoolean;
 
 public class CaptivePortalLoginActivity extends Activity {
     private static final String TAG = CaptivePortalLoginActivity.class.getSimpleName();
@@ -82,6 +83,8 @@ public class CaptivePortalLoginActivity extends Activity {
     private ConnectivityManager mCm;
     private boolean mLaunchBrowser = false;
     private MyWebViewClient mWebViewClient;
+    // Ensures that done() happens once exactly, handling concurrent callers with atomic operations.
+    private final AtomicBoolean isDone = new AtomicBoolean(false);
 
     @Override
     protected void onCreate(Bundle savedInstanceState) {
@@ -178,13 +181,13 @@ public class CaptivePortalLoginActivity extends Activity {
     }
 
     private void done(Result result) {
+        if (isDone.getAndSet(true)) {
+            // isDone was already true: done() already called
+            return;
+        }
         if (DBG) {
             Log.d(TAG, String.format("Result %s for %s", result.name(), mUrl.toString()));
         }
-        if (mNetworkCallback != null) {
-            mCm.unregisterNetworkCallback(mNetworkCallback);
-            mNetworkCallback = null;
-        }
         logMetricsEvent(result.metricsEvent);
         switch (result) {
             case DISMISSED:
@@ -244,8 +247,8 @@ public class CaptivePortalLoginActivity extends Activity {
     public void onDestroy() {
         super.onDestroy();
         if (mNetworkCallback != null) {
+            // mNetworkCallback is not null if mUrl is not null.
             mCm.unregisterNetworkCallback(mNetworkCallback);
-            mNetworkCallback = null;
         }
         if (mLaunchBrowser) {
             // Give time for this network to become default. After 500ms just proceed.