From bf5c2979dd3ba80cc6cc6edb1709681abc40817b Mon Sep 17 00:00:00 2001 From: Lorenzo Colitti Date: Tue, 9 Feb 2021 22:45:25 +0900 Subject: [PATCH 1/2] Remove buggy ConnectivityManagerTest#ensureWifiConnected. This method does not behave correctly when wifi is connected but the last CONNECTIVITY_ACTION broadcast was not for wifi. This could happen due to another network connecting or disconnecting, such as VPN. Bug: 179774433 Test: test-only change Change-Id: Ib2010b3871133c38b6d508bf508134dd9b814ce2 --- .../net/cts/ConnectivityManagerTest.java | 47 +++++++++---------- .../android/net/cts/util/CtsNetUtils.java | 6 +-- 2 files changed, 24 insertions(+), 29 deletions(-) diff --git a/tests/cts/net/src/android/net/cts/ConnectivityManagerTest.java b/tests/cts/net/src/android/net/cts/ConnectivityManagerTest.java index 3145d7eaba..0a948a09d2 100644 --- a/tests/cts/net/src/android/net/cts/ConnectivityManagerTest.java +++ b/tests/cts/net/src/android/net/cts/ConnectivityManagerTest.java @@ -195,7 +195,6 @@ public class ConnectivityManagerTest { private PackageManager mPackageManager; private final HashMap mNetworks = new HashMap(); - boolean mWifiWasDisabled; private UiAutomation mUiAutomation; private CtsNetUtils mCtsNetUtils; @@ -207,7 +206,6 @@ public class ConnectivityManagerTest { mWifiManager = (WifiManager) mContext.getSystemService(Context.WIFI_SERVICE); mPackageManager = mContext.getPackageManager(); mCtsNetUtils = new CtsNetUtils(mContext); - mWifiWasDisabled = false; // Get com.android.internal.R.array.networkAttributes int resId = mContext.getResources().getIdentifier("networkAttributes", "array", "android"); @@ -230,10 +228,7 @@ public class ConnectivityManagerTest { @After public void tearDown() throws Exception { - // Return WiFi to its original disabled state after tests that explicitly connect. - if (mWifiWasDisabled) { - mCtsNetUtils.disconnectFromWifi(null); - } + // Release any NetworkRequests filed to connect mobile data. if (mCtsNetUtils.cellConnectAttempted()) { mCtsNetUtils.disconnectFromCell(); } @@ -249,17 +244,6 @@ public class ConnectivityManagerTest { } } - /** - * Make sure WiFi is connected to an access point if it is not already. If - * WiFi is enabled as a result of this function, it will be disabled - * automatically in tearDown(). - */ - private Network ensureWifiConnected() { - mWifiWasDisabled = !mWifiManager.isWifiEnabled(); - // Even if wifi is enabled, the network may not be connected or ready yet - return mCtsNetUtils.connectToWifi(); - } - @Test public void testIsNetworkTypeValid() { assertTrue(ConnectivityManager.isNetworkTypeValid(ConnectivityManager.TYPE_MOBILE)); @@ -543,7 +527,7 @@ public class ConnectivityManagerTest { Network wifiNetwork = null; try { - ensureWifiConnected(); + mCtsNetUtils.ensureWifiConnected(); // Now we should expect to get a network callback about availability of the wifi // network even if it was already connected as a state-based action when the callback @@ -596,7 +580,7 @@ public class ConnectivityManagerTest { mCm.registerNetworkCallback(makeWifiNetworkRequest(), pendingIntent); try { - ensureWifiConnected(); + mCtsNetUtils.ensureWifiConnected(); // Now we expect to get the Intent delivered notifying of the availability of the wifi // network even if it was already connected as a state-based action when the callback @@ -677,6 +661,17 @@ public class ConnectivityManagerTest { return null; } + /** + * Checks that enabling/disabling wifi causes CONNECTIVITY_ACTION broadcasts. + */ + @AppModeFull(reason = "Cannot get WifiManager in instant app mode") + @Test + public void testToggleWifiConnectivityAction() { + // toggleWifi calls connectToWifi and disconnectFromWifi, which both wait for + // CONNECTIVITY_ACTION broadcasts. + mCtsNetUtils.toggleWifi(); + } + /** Verify restricted networks cannot be requested. */ @AppModeFull(reason = "CHANGE_NETWORK_STATE permission can't be granted to instant apps") @Test @@ -804,7 +799,7 @@ public class ConnectivityManagerTest { @Test public void testGetMultipathPreference() throws Exception { final ContentResolver resolver = mContext.getContentResolver(); - ensureWifiConnected(); + mCtsNetUtils.ensureWifiConnected(); final String ssid = unquoteSSID(mWifiManager.getConnectionInfo().getSSID()); final String oldMeteredSetting = getWifiMeteredStatus(ssid); final String oldMeteredMultipathPreference = Settings.Global.getString( @@ -818,7 +813,7 @@ public class ConnectivityManagerTest { waitForActiveNetworkMetered(TRANSPORT_WIFI, true); // Wifi meterness changes from unmetered to metered will disconnect and reconnect since // R. - final Network network = ensureWifiConnected(); + final Network network = mCtsNetUtils.ensureWifiConnected(); assertEquals(ssid, unquoteSSID(mWifiManager.getConnectionInfo().getSSID())); assertEquals(mCm.getNetworkCapabilities(network).hasCapability( NET_CAPABILITY_NOT_METERED), false); @@ -1032,7 +1027,7 @@ public class ConnectivityManagerTest { return; } - final Network network = ensureWifiConnected(); + final Network network = mCtsNetUtils.ensureWifiConnected(); if (getSupportedKeepalivesForNet(network) != 0) return; final InetAddress srcAddr = getFirstV4Address(network); assumeTrue("This test requires native IPv4", srcAddr != null); @@ -1052,7 +1047,7 @@ public class ConnectivityManagerTest { return; } - final Network network = ensureWifiConnected(); + final Network network = mCtsNetUtils.ensureWifiConnected(); if (getSupportedKeepalivesForNet(network) == 0) return; final InetAddress srcAddr = getFirstV4Address(network); assumeTrue("This test requires native IPv4", srcAddr != null); @@ -1263,7 +1258,7 @@ public class ConnectivityManagerTest { return; } - final Network network = ensureWifiConnected(); + final Network network = mCtsNetUtils.ensureWifiConnected(); final int supported = getSupportedKeepalivesForNet(network); if (supported == 0) { return; @@ -1360,7 +1355,7 @@ public class ConnectivityManagerTest { return; } - final Network network = ensureWifiConnected(); + final Network network = mCtsNetUtils.ensureWifiConnected(); final int supported = getSupportedKeepalivesForNet(network); if (supported == 0) { return; @@ -1408,7 +1403,7 @@ public class ConnectivityManagerTest { // Ensure that NetworkUtils.queryUserAccess always returns false since this package should // not have netd system permission to call this function. - final Network wifiNetwork = ensureWifiConnected(); + final Network wifiNetwork = mCtsNetUtils.ensureWifiConnected(); assertFalse(NetworkUtils.queryUserAccess(Binder.getCallingUid(), wifiNetwork.netId)); // Ensure that this package cannot bind to any restricted network that's currently diff --git a/tests/cts/net/util/java/android/net/cts/util/CtsNetUtils.java b/tests/cts/net/util/java/android/net/cts/util/CtsNetUtils.java index 05270115b1..88172d7540 100644 --- a/tests/cts/net/util/java/android/net/cts/util/CtsNetUtils.java +++ b/tests/cts/net/util/java/android/net/cts/util/CtsNetUtils.java @@ -212,7 +212,7 @@ public final class CtsNetUtils { mContext.registerReceiver(receiver, filter); boolean connected = false; - final String err = "Wifi must be configured to connect to an access point for this test."; + final String err = "Wifi must be configured to connect to an access point for this test"; try { clearWifiBlacklist(); SystemUtil.runShellCommand("svc wifi enable"); @@ -235,7 +235,7 @@ public final class CtsNetUtils { } // Ensure we get an onAvailable callback and possibly a CONNECTIVITY_ACTION. wifiNetwork = callback.waitForAvailable(); - assertNotNull(err, wifiNetwork); + assertNotNull(err + ": onAvailable callback not received", wifiNetwork); connected = !expectLegacyBroadcast || receiver.waitForState(); } catch (InterruptedException ex) { fail("connectToWifi was interrupted"); @@ -244,7 +244,7 @@ public final class CtsNetUtils { mContext.unregisterReceiver(receiver); } - assertTrue(err, connected); + assertTrue(err + ": CONNECTIVITY_ACTION not received", connected); return wifiNetwork; } From baea70055458d30214cc84d5f93ad75fc8483815 Mon Sep 17 00:00:00 2001 From: Lorenzo Colitti Date: Wed, 10 Feb 2021 22:32:24 +0900 Subject: [PATCH 2/2] Improve testing of registerSystemDefaultNetworkCallback. Addresses comments on aosp/1570840. Bug: 179774433 Test: test-only change Change-Id: I71a376631503e5b50ada3f7bb3dca6dbae9ebc27 --- .../com/android/cts/net/hostside/VpnTest.java | 39 +++++++++++++------ .../net/cts/ConnectivityManagerTest.java | 7 +++- 2 files changed, 32 insertions(+), 14 deletions(-) diff --git a/tests/cts/hostside/app/src/com/android/cts/net/hostside/VpnTest.java b/tests/cts/hostside/app/src/com/android/cts/net/hostside/VpnTest.java index 4668ba3670..a663cd6f73 100755 --- a/tests/cts/hostside/app/src/com/android/cts/net/hostside/VpnTest.java +++ b/tests/cts/hostside/app/src/com/android/cts/net/hostside/VpnTest.java @@ -698,16 +698,30 @@ public class VpnTest extends InstrumentationTestCase { } private class NeverChangeNetworkCallback extends NetworkCallback { - private volatile Network mLastNetwork; + private CountDownLatch mLatch = new CountDownLatch(1); + private volatile Network mFirstNetwork; + private volatile Network mOtherNetwork; public void onAvailable(Network n) { - assertNull("Callback got onAvailable more than once: " + mLastNetwork + ", " + n, - mLastNetwork); - mLastNetwork = n; + // Don't assert here, as it crashes the test with a hard to debug message. + if (mFirstNetwork == null) { + mFirstNetwork = n; + mLatch.countDown(); + } else if (mOtherNetwork == null) { + mOtherNetwork = n; + } } - public Network getLastNetwork() { - return mLastNetwork; + public Network getFirstNetwork() throws Exception { + assertTrue( + "System default callback got no network after " + TIMEOUT_MS + "ms. " + + "Please ensure the device has a working Internet connection.", + mLatch.await(TIMEOUT_MS, TimeUnit.MILLISECONDS)); + return mFirstNetwork; + } + + public void assertNeverChanged() { + assertNull(mOtherNetwork); } } @@ -753,13 +767,14 @@ public class VpnTest extends InstrumentationTestCase { expectVpnTransportInfo(mCM.getActiveNetwork()); - // Check that system default network callback has not seen any network changes, but the app - // default network callback has. This needs to be done before testing private DNS because - // checkStrictModePrivateDns will set the private DNS server to a nonexistent name, which - // will cause validation to fail could cause the default network to switch (e.g., from wifi - // to cellular). - assertEquals(defaultNetwork, neverChangeCallback.getLastNetwork()); + // Check that system default network callback has not seen any network changes, even though + // the app's default network changed. This needs to be done before testing private + // DNS because checkStrictModePrivateDns will set the private DNS server to a nonexistent + // name, which will cause validation to fail and cause the default network to switch (e.g., + // from wifi to cellular). + assertEquals(defaultNetwork, neverChangeCallback.getFirstNetwork()); assertNotEqual(defaultNetwork, mCM.getActiveNetwork()); + neverChangeCallback.assertNeverChanged(); runWithShellPermissionIdentity( () -> mCM.unregisterNetworkCallback(neverChangeCallback), NETWORK_SETTINGS); diff --git a/tests/cts/net/src/android/net/cts/ConnectivityManagerTest.java b/tests/cts/net/src/android/net/cts/ConnectivityManagerTest.java index 0a948a09d2..831810c42a 100644 --- a/tests/cts/net/src/android/net/cts/ConnectivityManagerTest.java +++ b/tests/cts/net/src/android/net/cts/ConnectivityManagerTest.java @@ -533,11 +533,14 @@ public class ConnectivityManagerTest { // network even if it was already connected as a state-based action when the callback // is registered. wifiNetwork = callback.waitForAvailable(); - assertNotNull("Did not receive NetworkCallback.onAvailable for TRANSPORT_WIFI", + assertNotNull("Did not receive onAvailable for TRANSPORT_WIFI request", wifiNetwork); - assertNotNull("Did not receive NetworkCallback.onAvailable for any default network", + assertNotNull("Did not receive onAvailable on default network callback", defaultTrackingCallback.waitForAvailable()); + + assertNotNull("Did not receive onAvailable on system default network callback", + systemDefaultTrackingCallback.waitForAvailable()); } catch (InterruptedException e) { fail("Broadcast receiver or NetworkCallback wait was interrupted."); } finally {