From 3d911469a190437fe936103e861bfa171841fbd6 Mon Sep 17 00:00:00 2001 From: Paul Jensen Date: Fri, 12 Jun 2015 06:40:24 -0400 Subject: Don't send spurious onAvailable NetworkCallbacks when rematching Bug:21762680 Change-Id: Ia701045dffc666fe75fba0e1771872147e37179a --- .../com/android/server/ConnectivityService.java | 18 +- .../server/connectivity/NetworkAgentInfo.java | 10 +- .../android/server/ConnectivityServiceTest.java | 271 ++++++++++++++++++--- 3 files changed, 260 insertions(+), 39 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 9c6e16f..00d7a50 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -2229,7 +2229,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()) { @@ -2240,7 +2242,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) { @@ -4226,6 +4230,7 @@ public class ConnectivityService extends IConnectivityManager.Stub // Find and migrate to this Network any NetworkRequests for // which this network is now the best. ArrayList affectedNetworks = new ArrayList(); + ArrayList addedRequests = new ArrayList(); if (VDBG) log(" network has: " + newNetwork.networkCapabilities); for (NetworkRequestInfo nri : mNetworkRequests.values()) { NetworkAgentInfo currentNetwork = mNetworkForRequestId.get(nri.request.requestId); @@ -4244,7 +4249,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; } @@ -4267,7 +4272,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. @@ -4310,7 +4318,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 diff --git a/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java b/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java index eac748f..3bf1183 100644 --- a/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java +++ b/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? diff --git a/services/tests/servicestests/src/com/android/server/ConnectivityServiceTest.java b/services/tests/servicestests/src/com/android/server/ConnectivityServiceTest.java index b58c2e2..712db09 100644 --- a/services/tests/servicestests/src/com/android/server/ConnectivityServiceTest.java +++ b/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 { - Thread.sleep(1000); - } catch (InterruptedException e) { - } + shortSleep(); verifyActiveNetwork(TRANSPORT_WIFI); // Test cellular disconnect. mCellNetworkAgent.disconnect(); - try { - Thread.sleep(1000); - } catch (InterruptedException e) { - } + shortSleep(); 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,27 +533,209 @@ 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(); filter.addCapability(NET_CAPABILITY_INTERNET); @@ -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)); -- cgit v1.1