Merge "Don't send spurious onAvailable NetworkCallbacks when rematching" into mnc-dev

services/core/java/com/android/server/ConnectivityService.java

@@ -2185,7 +2185,9 @@ public class ConnectivityService extends IConnectivityManager.Stub
                     // Not setting bestNetwork here as a listening NetworkRequest may be
                     // satisfied by multiple Networks.  Instead the request is added to
                     // each satisfying Network and notified about each.
-                    network.addRequest(nri.request);
+                    if (!network.addRequest(nri.request)) {
+                        Slog.wtf(TAG, "BUG: " + network.name() + " already has " + nri.request);
+                    }
                     notifyNetworkCallback(network, nri);
                 } else if (bestNetwork == null ||
                         bestNetwork.getCurrentScore() < network.getCurrentScore()) {
@@ -2196,7 +2198,9 @@ public class ConnectivityService extends IConnectivityManager.Stub
         if (bestNetwork != null) {
             if (DBG) log("using " + bestNetwork.name());
             unlinger(bestNetwork);
-            bestNetwork.addRequest(nri.request);
+            if (!bestNetwork.addRequest(nri.request)) {
+                Slog.wtf(TAG, "BUG: " + bestNetwork.name() + " already has " + nri.request);
+            }
             mNetworkForRequestId.put(nri.request.requestId, bestNetwork);
             notifyNetworkCallback(bestNetwork, nri);
             if (nri.request.legacyType != TYPE_NONE) {
@@ -4197,6 +4201,7 @@ public class ConnectivityService extends IConnectivityManager.Stub
         // Find and migrate to this Network any NetworkRequests for
         // which this network is now the best.
         ArrayList<NetworkAgentInfo> affectedNetworks = new ArrayList<NetworkAgentInfo>();
+        ArrayList<NetworkRequestInfo> addedRequests = new ArrayList<NetworkRequestInfo>();
         if (VDBG) log(" network has: " + newNetwork.networkCapabilities);
         for (NetworkRequestInfo nri : mNetworkRequests.values()) {
             NetworkAgentInfo currentNetwork = mNetworkForRequestId.get(nri.request.requestId);
@@ -4215,7 +4220,7 @@ public class ConnectivityService extends IConnectivityManager.Stub
                 if (!nri.isRequest) {
                     // This is not a request, it's a callback listener.
                     // Add it to newNetwork regardless of score.
-                    newNetwork.addRequest(nri.request);
+                    if (newNetwork.addRequest(nri.request)) addedRequests.add(nri);
                     continue;
                 }
 
@@ -4238,7 +4243,10 @@ public class ConnectivityService extends IConnectivityManager.Stub
                     }
                     unlinger(newNetwork);
                     mNetworkForRequestId.put(nri.request.requestId, newNetwork);
-                    newNetwork.addRequest(nri.request);
+                    if (!newNetwork.addRequest(nri.request)) {
+                        Slog.wtf(TAG, "BUG: " + newNetwork.name() + " already has " + nri.request);
+                    }
+                    addedRequests.add(nri);
                     keep = true;
                     // Tell NetworkFactories about the new score, so they can stop
                     // trying to connect if they know they cannot match it.
@@ -4281,7 +4289,7 @@ public class ConnectivityService extends IConnectivityManager.Stub
 
             // do this after the default net is switched, but
             // before LegacyTypeTracker sends legacy broadcasts
-            notifyNetworkCallbacks(newNetwork, ConnectivityManager.CALLBACK_AVAILABLE);
+            for (NetworkRequestInfo nri : addedRequests) notifyNetworkCallback(newNetwork, nri);
 
             if (isNewDefault) {
                 // Maintain the illusion: since the legacy API only

services/core/java/com/android/server/connectivity/NetworkAgentInfo.java

@@ -106,8 +106,16 @@ public class NetworkAgentInfo {
         networkMisc = misc;
     }
 
-    public void addRequest(NetworkRequest networkRequest) {
+    /**
+     * Add {@code networkRequest} to this network as it's satisfied by this network.
+     * NOTE: This function must only be called on ConnectivityService's main thread.
+     * @return true if {@code networkRequest} was added or false if {@code networkRequest} was
+     *         already present.
+     */
+    public boolean addRequest(NetworkRequest networkRequest) {
+        if (networkRequests.get(networkRequest.requestId) == networkRequest) return false;
         networkRequests.put(networkRequest.requestId, networkRequest);
+        return true;
     }
 
     // Does this network satisfy request?

services/tests/servicestests/src/com/android/server/ConnectivityServiceTest.java

@@ -134,6 +134,7 @@ public class ConnectivityServiceTest extends AndroidTestCase {
         private final NetworkInfo mNetworkInfo;
         private final NetworkCapabilities mNetworkCapabilities;
         private final Thread mThread;
+        private int mScore;
         private NetworkAgent mNetworkAgent;
 
         MockNetworkAgent(int transport) {
@@ -142,13 +143,12 @@ public class ConnectivityServiceTest extends AndroidTestCase {
             mNetworkInfo = new NetworkInfo(type, 0, typeName, "Mock");
             mNetworkCapabilities = new NetworkCapabilities();
             mNetworkCapabilities.addTransportType(transport);
-            final int score;
             switch (transport) {
                 case TRANSPORT_WIFI:
-                    score = 60;
+                    mScore = 60;
                     break;
                 case TRANSPORT_CELLULAR:
-                    score = 50;
+                    mScore = 50;
                     break;
                 default:
                     throw new UnsupportedOperationException("unimplemented network type");
@@ -159,7 +159,7 @@ public class ConnectivityServiceTest extends AndroidTestCase {
                     Looper.prepare();
                     mNetworkAgent = new NetworkAgent(Looper.myLooper(), mServiceContext,
                             "Mock" + typeName, mNetworkInfo, mNetworkCapabilities,
-                            new LinkProperties(), score, new NetworkMisc()) {
+                            new LinkProperties(), mScore, new NetworkMisc()) {
                         public void unwanted() {}
                     };
                     initComplete.open();
@@ -167,7 +167,12 @@ public class ConnectivityServiceTest extends AndroidTestCase {
                 }
             };
             mThread.start();
-            initComplete.block();
+            waitFor(initComplete);
+        }
+
+        public void adjustScore(int change) {
+            mScore += change;
+            mNetworkAgent.sendNetworkScore(mScore);
         }
 
         /**
@@ -209,7 +214,7 @@ public class ConnectivityServiceTest extends AndroidTestCase {
 
             if (validated) {
                 // Wait for network to validate.
-                validatedCv.block();
+                waitFor(validatedCv);
                 mNetworkCapabilities.addCapability(NET_CAPABILITY_INTERNET);
                 mNetworkAgent.sendNetworkCapabilities(mNetworkCapabilities);
             }
@@ -330,6 +335,10 @@ public class ConnectivityServiceTest extends AndroidTestCase {
         public boolean get();
     }
 
+    /**
+     * Wait up to 500ms for {@code criteria.get()} to become true, polling.
+     * Fails if 500ms goes by before {@code criteria.get()} to become true.
+     */
     static private void waitFor(Criteria criteria) {
         int delays = 0;
         while (!criteria.get()) {
@@ -341,6 +350,26 @@ public class ConnectivityServiceTest extends AndroidTestCase {
         }
     }
 
+    /**
+     * Wait up to 500ms for {@code conditonVariable} to open.
+     * Fails if 500ms goes by before {@code conditionVariable} opens.
+     */
+    static private void waitFor(ConditionVariable conditionVariable) {
+        assertTrue(conditionVariable.block(500));
+    }
+
+    /**
+     * This should only be used to verify that nothing happens, in other words that no unexpected
+     * changes occur.  It should never be used to wait for a specific positive signal to occur.
+     */
+    private void shortSleep() {
+        // TODO: Instead of sleeping, instead wait for all message loops to idle.
+        try {
+            Thread.sleep(500);
+        } catch (InterruptedException e) {
+        }
+    }
+
     @Override
     public void setUp() throws Exception {
         super.setUp();
@@ -431,7 +460,7 @@ public class ConnectivityServiceTest extends AndroidTestCase {
         // Test bringing up validated cellular.
         ConditionVariable cv = waitForConnectivityBroadcasts(1);
         mCellNetworkAgent.connect(true);
-        cv.block();
+        waitFor(cv);
         verifyActiveNetwork(TRANSPORT_CELLULAR);
         assertEquals(2, mCm.getAllNetworks().length);
         assertTrue(mCm.getAllNetworks()[0].equals(mCm.getActiveNetwork()) ||
@@ -441,7 +470,7 @@ public class ConnectivityServiceTest extends AndroidTestCase {
         // Test bringing up validated WiFi.
         cv = waitForConnectivityBroadcasts(2);
         mWiFiNetworkAgent.connect(true);
-        cv.block();
+        waitFor(cv);
         verifyActiveNetwork(TRANSPORT_WIFI);
         assertEquals(2, mCm.getAllNetworks().length);
         assertTrue(mCm.getAllNetworks()[0].equals(mCm.getActiveNetwork()) ||
@@ -459,7 +488,7 @@ public class ConnectivityServiceTest extends AndroidTestCase {
         // Test WiFi disconnect.
         cv = waitForConnectivityBroadcasts(1);
         mWiFiNetworkAgent.disconnect();
-        cv.block();
+        waitFor(cv);
         verifyNoNetwork();
     }
 
@@ -469,38 +498,32 @@ public class ConnectivityServiceTest extends AndroidTestCase {
         mWiFiNetworkAgent = new MockNetworkAgent(TRANSPORT_WIFI);
         ConditionVariable cv = waitForConnectivityBroadcasts(1);
         mWiFiNetworkAgent.connect(false);
-        cv.block();
+        waitFor(cv);
         verifyActiveNetwork(TRANSPORT_WIFI);
         // Test bringing up unvalidated cellular
         mCellNetworkAgent = new MockNetworkAgent(TRANSPORT_CELLULAR);
         mCellNetworkAgent.connect(false);
-        try {
+        shortSleep();
-            Thread.sleep(1000);
-        } catch (InterruptedException e) {
-        }
         verifyActiveNetwork(TRANSPORT_WIFI);
         // Test cellular disconnect.
         mCellNetworkAgent.disconnect();
-        try {
+        shortSleep();
-            Thread.sleep(1000);
-        } catch (InterruptedException e) {
-        }
         verifyActiveNetwork(TRANSPORT_WIFI);
         // Test bringing up validated cellular
         mCellNetworkAgent = new MockNetworkAgent(TRANSPORT_CELLULAR);
         cv = waitForConnectivityBroadcasts(2);
         mCellNetworkAgent.connect(true);
-        cv.block();
+        waitFor(cv);
         verifyActiveNetwork(TRANSPORT_CELLULAR);
         // Test cellular disconnect.
         cv = waitForConnectivityBroadcasts(2);
         mCellNetworkAgent.disconnect();
-        cv.block();
+        waitFor(cv);
         verifyActiveNetwork(TRANSPORT_WIFI);
         // Test WiFi disconnect.
         cv = waitForConnectivityBroadcasts(1);
         mWiFiNetworkAgent.disconnect();
-        cv.block();
+        waitFor(cv);
         verifyNoNetwork();
     }
 
@@ -510,26 +533,208 @@ public class ConnectivityServiceTest extends AndroidTestCase {
         mCellNetworkAgent = new MockNetworkAgent(TRANSPORT_CELLULAR);
         ConditionVariable cv = waitForConnectivityBroadcasts(1);
         mCellNetworkAgent.connect(false);
-        cv.block();
+        waitFor(cv);
         verifyActiveNetwork(TRANSPORT_CELLULAR);
         // Test bringing up unvalidated WiFi.
         mWiFiNetworkAgent = new MockNetworkAgent(TRANSPORT_WIFI);
         cv = waitForConnectivityBroadcasts(2);
         mWiFiNetworkAgent.connect(false);
-        cv.block();
+        waitFor(cv);
         verifyActiveNetwork(TRANSPORT_WIFI);
         // Test WiFi disconnect.
         cv = waitForConnectivityBroadcasts(2);
         mWiFiNetworkAgent.disconnect();
-        cv.block();
+        waitFor(cv);
         verifyActiveNetwork(TRANSPORT_CELLULAR);
         // Test cellular disconnect.
         cv = waitForConnectivityBroadcasts(1);
         mCellNetworkAgent.disconnect();
-        cv.block();
+        waitFor(cv);
         verifyNoNetwork();
     }
 
+    @LargeTest
+    public void testCellularOutscoresWeakWifi() throws Exception {
+        // Test bringing up validated cellular.
+        mCellNetworkAgent = new MockNetworkAgent(TRANSPORT_CELLULAR);
+        ConditionVariable cv = waitForConnectivityBroadcasts(1);
+        mCellNetworkAgent.connect(true);
+        waitFor(cv);
+        verifyActiveNetwork(TRANSPORT_CELLULAR);
+        // Test bringing up validated WiFi.
+        mWiFiNetworkAgent = new MockNetworkAgent(TRANSPORT_WIFI);
+        cv = waitForConnectivityBroadcasts(2);
+        mWiFiNetworkAgent.connect(true);
+        waitFor(cv);
+        verifyActiveNetwork(TRANSPORT_WIFI);
+        // Test WiFi getting really weak.
+        cv = waitForConnectivityBroadcasts(2);
+        mWiFiNetworkAgent.adjustScore(-11);
+        waitFor(cv);
+        verifyActiveNetwork(TRANSPORT_CELLULAR);
+        // Test WiFi restoring signal strength.
+        cv = waitForConnectivityBroadcasts(2);
+        mWiFiNetworkAgent.adjustScore(11);
+        waitFor(cv);
+        verifyActiveNetwork(TRANSPORT_WIFI);
+        mCellNetworkAgent.disconnect();
+        mWiFiNetworkAgent.disconnect();
+    }
+
+    enum CallbackState {
+        NONE,
+        AVAILABLE,
+        LOSING,
+        LOST
+    }
+
+    private class TestNetworkCallback extends NetworkCallback {
+        private final ConditionVariable mConditionVariable = new ConditionVariable();
+        private CallbackState mLastCallback = CallbackState.NONE;
+
+        public void onAvailable(Network network) {
+            assertEquals(CallbackState.NONE, mLastCallback);
+            mLastCallback = CallbackState.AVAILABLE;
+            mConditionVariable.open();
+        }
+
+        public void onLosing(Network network, int maxMsToLive) {
+            assertEquals(CallbackState.NONE, mLastCallback);
+            mLastCallback = CallbackState.LOSING;
+            mConditionVariable.open();
+        }
+
+        public void onLost(Network network) {
+            assertEquals(CallbackState.NONE, mLastCallback);
+            mLastCallback = CallbackState.LOST;
+            mConditionVariable.open();
+        }
+
+        ConditionVariable getConditionVariable() {
+            mLastCallback = CallbackState.NONE;
+            mConditionVariable.close();
+            return mConditionVariable;
+        }
+
+        CallbackState getLastCallback() {
+            return mLastCallback;
+        }
+    }
+
+    @LargeTest
+    public void testStateChangeNetworkCallbacks() throws Exception {
+        final TestNetworkCallback wifiNetworkCallback = new TestNetworkCallback();
+        final TestNetworkCallback cellNetworkCallback = new TestNetworkCallback();
+        final NetworkRequest wifiRequest = new NetworkRequest.Builder()
+                .addTransportType(TRANSPORT_WIFI).build();
+        final NetworkRequest cellRequest = new NetworkRequest.Builder()
+                .addTransportType(TRANSPORT_CELLULAR).build();
+        mCm.registerNetworkCallback(wifiRequest, wifiNetworkCallback);
+        mCm.registerNetworkCallback(cellRequest, cellNetworkCallback);
+
+        // Test unvalidated networks
+        ConditionVariable cellCv = cellNetworkCallback.getConditionVariable();
+        ConditionVariable wifiCv = wifiNetworkCallback.getConditionVariable();
+        ConditionVariable cv = waitForConnectivityBroadcasts(1);
+        mCellNetworkAgent = new MockNetworkAgent(TRANSPORT_CELLULAR);
+        mCellNetworkAgent.connect(false);
+        waitFor(cellCv);
+        assertEquals(CallbackState.AVAILABLE, cellNetworkCallback.getLastCallback());
+        assertEquals(CallbackState.NONE, wifiNetworkCallback.getLastCallback());
+        assertEquals(mCellNetworkAgent.getNetwork(), mCm.getActiveNetwork());
+        waitFor(cv);
+
+        cellCv = cellNetworkCallback.getConditionVariable();
+        wifiCv = wifiNetworkCallback.getConditionVariable();
+        // This should not trigger spurious onAvailable() callbacks, b/21762680.
+        mCellNetworkAgent.adjustScore(-1);
+        shortSleep();
+        assertEquals(CallbackState.NONE, wifiNetworkCallback.getLastCallback());
+        assertEquals(CallbackState.NONE, cellNetworkCallback.getLastCallback());
+        assertEquals(mCellNetworkAgent.getNetwork(), mCm.getActiveNetwork());
+
+        cellCv = cellNetworkCallback.getConditionVariable();
+        wifiCv = wifiNetworkCallback.getConditionVariable();
+        cv = waitForConnectivityBroadcasts(2);
+        mWiFiNetworkAgent = new MockNetworkAgent(TRANSPORT_WIFI);
+        mWiFiNetworkAgent.connect(false);
+        waitFor(wifiCv);
+        assertEquals(CallbackState.AVAILABLE, wifiNetworkCallback.getLastCallback());
+        assertEquals(CallbackState.NONE, cellNetworkCallback.getLastCallback());
+        assertEquals(mWiFiNetworkAgent.getNetwork(), mCm.getActiveNetwork());
+        waitFor(cv);
+
+        cellCv = cellNetworkCallback.getConditionVariable();
+        wifiCv = wifiNetworkCallback.getConditionVariable();
+        cv = waitForConnectivityBroadcasts(2);
+        mWiFiNetworkAgent.disconnect();
+        waitFor(wifiCv);
+        assertEquals(CallbackState.LOST, wifiNetworkCallback.getLastCallback());
+        assertEquals(CallbackState.NONE, cellNetworkCallback.getLastCallback());
+        waitFor(cv);
+
+        cellCv = cellNetworkCallback.getConditionVariable();
+        wifiCv = wifiNetworkCallback.getConditionVariable();
+        cv = waitForConnectivityBroadcasts(1);
+        mCellNetworkAgent.disconnect();
+        waitFor(cellCv);
+        assertEquals(CallbackState.LOST, cellNetworkCallback.getLastCallback());
+        assertEquals(CallbackState.NONE, wifiNetworkCallback.getLastCallback());
+        waitFor(cv);
+
+        // Test validated networks
+
+        cellCv = cellNetworkCallback.getConditionVariable();
+        wifiCv = wifiNetworkCallback.getConditionVariable();
+        // Our method for faking successful validation generates an additional callback, so wait
+        // for broadcast instead.
+        cv = waitForConnectivityBroadcasts(1);
+        mCellNetworkAgent = new MockNetworkAgent(TRANSPORT_CELLULAR);
+        mCellNetworkAgent.connect(true);
+        waitFor(cv);
+        waitFor(cellCv);
+        assertEquals(CallbackState.AVAILABLE, cellNetworkCallback.getLastCallback());
+        assertEquals(CallbackState.NONE, wifiNetworkCallback.getLastCallback());
+        assertEquals(mCellNetworkAgent.getNetwork(), mCm.getActiveNetwork());
+
+        cellCv = cellNetworkCallback.getConditionVariable();
+        wifiCv = wifiNetworkCallback.getConditionVariable();
+        // This should not trigger spurious onAvailable() callbacks, b/21762680.
+        mCellNetworkAgent.adjustScore(-1);
+        shortSleep();
+        assertEquals(CallbackState.NONE, wifiNetworkCallback.getLastCallback());
+        assertEquals(CallbackState.NONE, cellNetworkCallback.getLastCallback());
+        assertEquals(mCellNetworkAgent.getNetwork(), mCm.getActiveNetwork());
+
+        cellCv = cellNetworkCallback.getConditionVariable();
+        wifiCv = wifiNetworkCallback.getConditionVariable();
+        // Our method for faking successful validation generates an additional callback, so wait
+        // for broadcast instead.
+        cv = waitForConnectivityBroadcasts(1);
+        mWiFiNetworkAgent = new MockNetworkAgent(TRANSPORT_WIFI);
+        mWiFiNetworkAgent.connect(true);
+        waitFor(cv);
+        waitFor(wifiCv);
+        assertEquals(CallbackState.AVAILABLE, wifiNetworkCallback.getLastCallback());
+        waitFor(cellCv);
+        assertEquals(CallbackState.LOSING, cellNetworkCallback.getLastCallback());
+        assertEquals(mWiFiNetworkAgent.getNetwork(), mCm.getActiveNetwork());
+
+        cellCv = cellNetworkCallback.getConditionVariable();
+        wifiCv = wifiNetworkCallback.getConditionVariable();
+        mWiFiNetworkAgent.disconnect();
+        waitFor(wifiCv);
+        assertEquals(CallbackState.LOST, wifiNetworkCallback.getLastCallback());
+        assertEquals(CallbackState.NONE, cellNetworkCallback.getLastCallback());
+
+        cellCv = cellNetworkCallback.getConditionVariable();
+        wifiCv = wifiNetworkCallback.getConditionVariable();
+        mCellNetworkAgent.disconnect();
+        waitFor(cellCv);
+        assertEquals(CallbackState.LOST, cellNetworkCallback.getLastCallback());
+        assertEquals(CallbackState.NONE, wifiNetworkCallback.getLastCallback());
+    }
+
     @LargeTest
     public void testNetworkFactoryRequests() throws Exception {
         NetworkCapabilities filter = new NetworkCapabilities();
@@ -541,7 +746,7 @@ public class ConnectivityServiceTest extends AndroidTestCase {
         testFactory.setScoreFilter(40);
         ConditionVariable cv = testFactory.getNetworkStartedCV();
         testFactory.register();
-        cv.block();
+        waitFor(cv);
         assertEquals(1, testFactory.getMyRequestCount());
         assertEquals(true, testFactory.getMyStartRequested());
 
@@ -550,10 +755,10 @@ public class ConnectivityServiceTest extends AndroidTestCase {
         cv = waitForConnectivityBroadcasts(1);
         ConditionVariable cvRelease = testFactory.getNetworkStoppedCV();
         testAgent.connect(true);
-        cv.block();
+        waitFor(cv);
         // part of the bringup makes another network request and then releases it
         // wait for the release
-        cvRelease.block();
+        waitFor(cvRelease);
         assertEquals(false, testFactory.getMyStartRequested());
         testFactory.waitForNetworkRequests(1);
 
@@ -579,7 +784,7 @@ public class ConnectivityServiceTest extends AndroidTestCase {
         // drop the higher scored network
         cv = waitForConnectivityBroadcasts(1);
         testAgent.disconnect();
-        cv.block();
+        waitFor(cv);
         assertEquals(1, testFactory.getMyRequestCount());
         assertEquals(true, testFactory.getMyStartRequested());
 
@@ -605,7 +810,7 @@ public class ConnectivityServiceTest extends AndroidTestCase {
 //
 //        cv = waitForConnectivityBroadcasts(1);
 //        mTrackerHandler.obtainMessage(EVENT_STATE_CHANGED, mMobile.info).sendToTarget();
-//        cv.block();
+//        waitFor(cv);
 //
 //        // verify that both routes were added
 //        int mobileNetId = mMobile.tracker.getNetwork().netId;
@@ -625,7 +830,7 @@ public class ConnectivityServiceTest extends AndroidTestCase {
 //
 //        cv = waitForConnectivityBroadcasts(1);
 //        mTrackerHandler.obtainMessage(EVENT_STATE_CHANGED, mMobile.info).sendToTarget();
-//        cv.block();
+//        waitFor(cv);
 //
 //        reset(mNetManager);
 //
@@ -641,7 +846,7 @@ public class ConnectivityServiceTest extends AndroidTestCase {
 //
 //        cv = waitForConnectivityBroadcasts(1);
 //        mTrackerHandler.obtainMessage(EVENT_STATE_CHANGED, mWifi.info).sendToTarget();
-//        cv.block();
+//        waitFor(cv);
 //
 //        // verify that wifi routes added, and teardown requested
 //        int wifiNetId = mWifi.tracker.getNetwork().netId;
@@ -660,7 +865,7 @@ public class ConnectivityServiceTest extends AndroidTestCase {
 //
 //        cv = waitForConnectivityBroadcasts(1);
 //        mTrackerHandler.obtainMessage(EVENT_STATE_CHANGED, mMobile.info).sendToTarget();
-//        cv.block();
+//        waitFor(cv);
 //
 //        verify(mNetManager).removeRoute(eq(mobileNetId), eq(MOBILE_ROUTE_V4));
 //        verify(mNetManager).removeRoute(eq(mobileNetId), eq(MOBILE_ROUTE_V6));