Merge "Fix the logic for handling network status change" am: f9a72a813d am: 74be4cd836 am: 6a53d96ea2 am: fd128b6cf1

Original change: https://android-review.googlesource.com/c/platform/packages/services/Mms/+/1739413

Change-Id: I1c7a0edd02d702345bdfe2cbb4374f41449aa9cb

2 files changed, 300 insertions, 26 deletions

diff --git a/src/com/android/mms/service/MmsNetworkManager.java b/src/com/android/mms/service/MmsNetworkManager.java
index af4d8a4..670ed81 100644
--- a/src/com/android/mms/service/MmsNetworkManager.java
+++ b/src/com/android/mms/service/MmsNetworkManager.java
@@ -32,8 +32,8 @@ import android.provider.DeviceConfig;
 import android.telephony.SubscriptionManager;
 import android.telephony.TelephonyManager;
 
+import com.android.internal.annotations.VisibleForTesting;
 import com.android.internal.telephony.PhoneConstants;
-
 import com.android.mms.service.exception.MmsNetworkException;
 
 /**
@@ -90,6 +90,8 @@ public class MmsNetworkManager {
     // If ACTION_SIM_CARD_STATE_CHANGED intent receiver is registered
     private boolean mReceiverRegistered;
 
+    private final Dependencies mDeps;
+
     /**
      * This receiver listens to ACTION_SIM_CARD_STATE_CHANGED after starting a new NetworkRequest.
      * If ACTION_SIM_CARD_STATE_CHANGED with SIM_STATE_ABSENT for a SIM card corresponding to the
@@ -142,22 +144,12 @@ public class MmsNetworkManager {
      */
     private class NetworkRequestCallback extends ConnectivityManager.NetworkCallback {
         @Override
-        public void onAvailable(Network network) {
-            super.onAvailable(network);
-            LogUtil.i("NetworkCallbackListener.onAvailable: network=" + network);
-            synchronized (MmsNetworkManager.this) {
-                mNetwork = network;
-                MmsNetworkManager.this.notifyAll();
-            }
-        }
-
-        @Override
         public void onLost(Network network) {
             super.onLost(network);
             LogUtil.w("NetworkCallbackListener.onLost: network=" + network);
             synchronized (MmsNetworkManager.this) {
-                releaseRequestLocked(this);
-                MmsNetworkManager.this.notifyAll();
+                // Wait for other available network. Not notify.
+                if (network.equals(mNetwork)) mNetwork = null;
             }
         }
 
@@ -170,10 +162,61 @@ public class MmsNetworkManager {
                 MmsNetworkManager.this.notifyAll();
             }
         }
+
+        @Override
+        public void onCapabilitiesChanged(Network network, NetworkCapabilities nc) {
+            // onAvailable will always immediately be followed by a onCapabilitiesChanged. Check
+            // network status here is enough.
+            super.onCapabilitiesChanged(network, nc);
+            LogUtil.w("NetworkCallbackListener.onCapabilitiesChanged: network="
+                    + network + ", nc=" + nc);
+            synchronized (MmsNetworkManager.this) {
+                final boolean isAvailable =
+                        nc.hasCapability(NetworkCapabilities.NET_CAPABILITY_NOT_SUSPENDED);
+                if (network.equals(mNetwork) && !isAvailable) {
+                    // Current network becomes suspended.
+                    mNetwork = null;
+                    // Not notify. Either wait for other available network or current network to
+                    // become available again.
+                    return;
+                }
+
+                // New available network
+                if (mNetwork == null && isAvailable) {
+                    mNetwork = network;
+                    MmsNetworkManager.this.notifyAll();
+                }
+            }
+        }
     }
 
-    public MmsNetworkManager(Context context, int subId) {
+    /**
+     * Dependencies of MmsNetworkManager, for injection in tests.
+     */
+    @VisibleForTesting
+    public static class Dependencies {
+        /** Get phone Id from the given subId */
+        public int getPhoneId(int subId) {
+            return SubscriptionManager.getPhoneId(subId);
+        }
+
+        // Timeout used to call ConnectivityManager.requestNetwork. Given that the telephony layer
+        // will retry on failures, this timeout should be high enough.
+        public int getNetworkRequestTimeoutMillis() {
+            return DeviceConfig.getInt(
+                    DeviceConfig.NAMESPACE_TELEPHONY, MMS_SERVICE_NETWORK_REQUEST_TIMEOUT_MILLIS,
+                    DEFAULT_MMS_SERVICE_NETWORK_REQUEST_TIMEOUT_MILLIS);
+        }
+
+        public int getAdditionalNetworkAcquireTimeoutMillis() {
+            return ADDITIONAL_NETWORK_ACQUIRE_TIMEOUT_MILLIS;
+        }
+    }
+
+    @VisibleForTesting
+    protected MmsNetworkManager(Context context, int subId, Dependencies dependencies) {
         mContext = context;
+        mDeps = dependencies;
         mNetworkCallback = null;
         mNetwork = null;
         mMmsRequestCount = 0;
@@ -200,6 +243,10 @@ public class MmsNetworkManager {
         };
     }
 
+    public MmsNetworkManager(Context context, int subId) {
+        this(context, subId, new Dependencies());
+    }
+
     /**
      * Acquire the MMS network
      *
@@ -207,7 +254,7 @@ public class MmsNetworkManager {
      * @throws com.android.mms.service.exception.MmsNetworkException if we fail to acquire it
      */
     public void acquireNetwork(final String requestId) throws MmsNetworkException {
-        int networkRequestTimeoutMillis = getNetworkRequestTimeoutMillis();
+        int networkRequestTimeoutMillis = mDeps.getNetworkRequestTimeoutMillis();
 
         synchronized (this) {
             // Since we are acquiring the network, remove the network release task if exists.
@@ -220,7 +267,7 @@ public class MmsNetworkManager {
             }
             // Not available, so start a new request if not done yet
             if (mNetworkCallback == null) {
-                mPhoneId = SubscriptionManager.getPhoneId(mSubId);
+                mPhoneId = mDeps.getPhoneId(mSubId);
                 if (mPhoneId == SubscriptionManager.INVALID_PHONE_INDEX
                         || mPhoneId == SubscriptionManager.DEFAULT_PHONE_INDEX) {
                     throw new MmsNetworkException("Invalid Phone Id: " + mPhoneId);
@@ -236,7 +283,8 @@ public class MmsNetworkManager {
                 mReceiverRegistered = true;
             }
             try {
-                this.wait(networkRequestTimeoutMillis + ADDITIONAL_NETWORK_ACQUIRE_TIMEOUT_MILLIS);
+                this.wait(networkRequestTimeoutMillis
+                        + mDeps.getAdditionalNetworkAcquireTimeoutMillis());
             } catch (InterruptedException e) {
                 LogUtil.w(requestId, "MmsNetworkManager: acquire network wait interrupted");
             }
@@ -269,15 +317,6 @@ public class MmsNetworkManager {
         }
     }
 
-    // Timeout used to call ConnectivityManager.requestNetwork
-    // Given that the telephony layer will retry on failures, this timeout should be high enough.
-    private int getNetworkRequestTimeoutMillis() {
-        return DeviceConfig.getInt(
-                DeviceConfig.NAMESPACE_TELEPHONY, MMS_SERVICE_NETWORK_REQUEST_TIMEOUT_MILLIS,
-                DEFAULT_MMS_SERVICE_NETWORK_REQUEST_TIMEOUT_MILLIS);
-    }
-
-
     /**
      * Release the MMS network when nobody is holding on to it.
      *
diff --git a/tests/robotests/src/com/android/mms/service/MmsNetworkManagerTest.java b/tests/robotests/src/com/android/mms/service/MmsNetworkManagerTest.java
new file mode 100644
index 0000000..c2d3e05
--- /dev/null
+++ b/tests/robotests/src/com/android/mms/service/MmsNetworkManagerTest.java
@@ -0,0 +1,235 @@
+/*
+ * Copyright (C) 2021 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.android.mms.service;
+
+import static android.net.NetworkCapabilities.NET_CAPABILITY_NOT_SUSPENDED;
+
+import static junit.framework.Assert.assertEquals;
+import static junit.framework.Assert.assertFalse;
+import static junit.framework.Assert.fail;
+
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyInt;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.timeout;
+import static org.mockito.Mockito.verify;
+
+import android.content.Context;
+import android.net.ConnectivityManager;
+import android.net.ConnectivityManager.NetworkCallback;
+import android.net.Network;
+import android.net.NetworkCapabilities;
+import android.net.NetworkInfo;
+
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.ArgumentCaptor;
+import org.mockito.Mock;
+import org.mockito.MockitoAnnotations;
+import org.robolectric.RobolectricTestRunner;
+
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
+
+@RunWith(RobolectricTestRunner.class)
+public final class MmsNetworkManagerTest {
+    private static final int TEST_SUBID = 1234;
+    private static final int CALLBACK_TIMEOUT_MS = 3000;
+    private static final int NETWORK_ACQUIRE_TIMEOUT_MS = 5000;
+
+    private static final String MMS_APN = "mmsapn";
+    private static final String MMS_APN2 = "mmsapn2";
+    private static final NetworkCapabilities SUSPEND_NC = new NetworkCapabilities.Builder().build();
+    private static final NetworkCapabilities USABLE_NC =
+            new NetworkCapabilities.Builder().addCapability(NET_CAPABILITY_NOT_SUSPENDED).build();
+
+    @Mock Network mTestNetwork;
+    @Mock Network mTestNetwork2;
+    @Mock NetworkInfo mNetworkInfo;
+    @Mock NetworkInfo mNetworkInfo2;
+    @Mock Context mCtx;
+    @Mock ConnectivityManager mCm;
+    @Mock MmsNetworkManager.Dependencies mDeps;
+
+
+    private MmsNetworkManager mMnm;
+    private final ExecutorService mExecutor = Executors.newSingleThreadExecutor();
+    private final AtomicInteger mRequestId = new AtomicInteger(1);
+
+    @Before
+    public void setup() {
+        MockitoAnnotations.initMocks(this);
+
+        doReturn(mCm).when(mCtx).getSystemService(Context.CONNECTIVITY_SERVICE);
+        doReturn(mCm).when(mCtx).getSystemService(ConnectivityManager.class);
+        doReturn(1).when(mDeps).getPhoneId(anyInt());
+        doReturn(mNetworkInfo).when(mCm).getNetworkInfo(eq(mTestNetwork));
+        doReturn(MMS_APN).when(mNetworkInfo).getExtraInfo();
+        doReturn(mNetworkInfo2).when(mCm).getNetworkInfo(eq(mTestNetwork2));
+        doReturn(MMS_APN2).when(mNetworkInfo2).getExtraInfo();
+        doReturn(NETWORK_ACQUIRE_TIMEOUT_MS).when(mDeps).getNetworkRequestTimeoutMillis();
+        doReturn(NETWORK_ACQUIRE_TIMEOUT_MS).when(mDeps).getAdditionalNetworkAcquireTimeoutMillis();
+
+        mMnm = new MmsNetworkManager(mCtx, TEST_SUBID, mDeps);
+    }
+
+    @Test
+    public void testAvailableNetwork_newNetworkAvailable() throws Exception {
+        acquireAvailableNetworkAndGetCallback(
+                mTestNetwork /* expectNetwork */, MMS_APN /* expectApn */);
+    }
+
+    @Test
+    public void testAvailableNetwork_newNetworkIsSuspend() throws Exception {
+        final ArgumentCaptor<NetworkCallback> callbackCaptor =
+                ArgumentCaptor.forClass(NetworkCallback.class);
+        final CompletableFuture<String> future =
+                acquireNetwork(Integer.toString(mRequestId.getAndIncrement()));
+        verify(mCm, timeout(CALLBACK_TIMEOUT_MS).times(1))
+                .requestNetwork(any(), callbackCaptor.capture(), anyInt());
+        final NetworkCallback callback = callbackCaptor.getValue();
+
+        callback.onCapabilitiesChanged(mTestNetwork, SUSPEND_NC);
+        assertFalse(future.isDone());
+        // mNetwork should be null.
+        assertEquals(null, mMnm.getApnName());
+        verify(mCm, never()).unregisterNetworkCallback(eq(callback));
+    }
+
+    @Test
+    public void testAvailableNetwork_networkSuspendThenResume() throws Exception {
+        final NetworkCallback callback = acquireAvailableNetworkAndGetCallback(
+                mTestNetwork /* expectNetwork */, MMS_APN /* expectApn */);
+
+        // Network becomes suspended. mNetwork should be null.
+        callback.onCapabilitiesChanged(mTestNetwork, SUSPEND_NC);
+        assertEquals(null, mMnm.getApnName());
+        verify(mCm, never()).unregisterNetworkCallback(eq(callback));
+
+        // Network resume
+        callback.onCapabilitiesChanged(mTestNetwork, USABLE_NC);
+        assertEquals(MMS_APN, mMnm.getApnName());
+    }
+
+    @Test
+    public void testAvailableNetwork_networkReplaced() throws Exception {
+        final NetworkCallback callback = acquireAvailableNetworkAndGetCallback(
+                mTestNetwork /* expectNetwork */, MMS_APN /* expectApn */);
+
+        // Previous network lost. Callback will not release to wait for possible network.
+        callback.onLost(mTestNetwork);
+        verify(mCm, never()).unregisterNetworkCallback(eq(callback));
+        // New mTestNetwork2 available
+        callback.onCapabilitiesChanged(mTestNetwork2, USABLE_NC);
+        assertEquals(MMS_APN2, mMnm.getApnName());
+    }
+
+    @Test
+    public void testAvailableNetwork_networkBecomeSuspend() throws Exception {
+        final NetworkCallback callback = acquireAvailableNetworkAndGetCallback(
+                mTestNetwork /* expectNetwork */, MMS_APN /* expectApn */);
+
+        callback.onCapabilitiesChanged(mTestNetwork, SUSPEND_NC);
+        // mNetwork should be null.
+        assertEquals(null, mMnm.getApnName());
+        // Callback will not release to wait for possible network.
+        verify(mCm, never()).unregisterNetworkCallback(eq(callback));
+    }
+
+    @Test
+    public void testAvailableNetwork_networkUnavailable() throws Exception {
+        doReturn(100).when(mDeps).getNetworkRequestTimeoutMillis();
+        doReturn(100).when(mDeps).getAdditionalNetworkAcquireTimeoutMillis();
+        final ArgumentCaptor<NetworkCallback> callbackCaptor =
+                ArgumentCaptor.forClass(NetworkCallback.class);
+        final CompletableFuture<String> future =
+                acquireNetwork(Integer.toString(mRequestId.getAndIncrement()));
+        verify(mCm, timeout(CALLBACK_TIMEOUT_MS).times(1))
+                .requestNetwork(any(), callbackCaptor.capture(), anyInt());
+        final NetworkCallback callback = callbackCaptor.getValue();
+
+        assertFalse(future.isDone());
+        // No network available after 100+100 ms. Callback will be released.
+        verify(mCm, timeout(CALLBACK_TIMEOUT_MS).times(1))
+                .unregisterNetworkCallback(eq(callback));
+
+        // mNetwork should be null.
+        assertEquals(null, mMnm.getApnName());
+    }
+
+    @Test
+    public void testAvailableNetwork_newNetworkSuspendedEventuallyReleased() throws Exception {
+        doReturn(100).when(mDeps).getNetworkRequestTimeoutMillis();
+        doReturn(100).when(mDeps).getAdditionalNetworkAcquireTimeoutMillis();
+        final ArgumentCaptor<NetworkCallback> callbackCaptor =
+                ArgumentCaptor.forClass(NetworkCallback.class);
+        final CompletableFuture<String> future =
+                acquireNetwork(Integer.toString(mRequestId.getAndIncrement()));
+        verify(mCm, timeout(CALLBACK_TIMEOUT_MS).times(1))
+                .requestNetwork(any(), callbackCaptor.capture(), anyInt());
+        final NetworkCallback callback = callbackCaptor.getValue();
+
+        // New network but not a usable network.
+        callback.onCapabilitiesChanged(mTestNetwork, SUSPEND_NC);
+
+        assertFalse(future.isDone());
+        // No network available after 100+100 ms. Callback will be released.
+        verify(mCm, timeout(CALLBACK_TIMEOUT_MS).times(1))
+                .unregisterNetworkCallback(eq(callback));
+
+        // mNetwork should be null.
+        assertEquals(null, mMnm.getApnName());
+    }
+
+    private NetworkCallback acquireAvailableNetworkAndGetCallback(
+            Network expectNetwork, String expectApn) throws Exception {
+        final ArgumentCaptor<NetworkCallback> callbackCaptor =
+                ArgumentCaptor.forClass(NetworkCallback.class);
+        final CompletableFuture<String> future =
+                acquireNetwork(Integer.toString(mRequestId.getAndIncrement()));
+        verify(mCm, timeout(CALLBACK_TIMEOUT_MS).times(1))
+                .requestNetwork(any(), callbackCaptor.capture(), anyInt());
+        final NetworkCallback callback = callbackCaptor.getValue();
+
+        // Network available
+        callback.onCapabilitiesChanged(expectNetwork, USABLE_NC);
+        assertEquals(expectApn, future.get(CALLBACK_TIMEOUT_MS, TimeUnit.MILLISECONDS));
+        return callbackCaptor.getValue();
+    }
+
+    private CompletableFuture<String> acquireNetwork(String requestId) {
+        final CompletableFuture<String> future = new CompletableFuture();
+
+        mExecutor.execute(() -> {
+            try {
+                mMnm.acquireNetwork(requestId);
+                future.complete(mMnm.getApnName());
+                android.util.Log.d("MmsNetworkManagerTest", "acquireNetwork done");
+            } catch (Exception e) {
+                fail("Acquire network fail");
+            }
+        });
+
+        return future;
+    }
+}