Merge "Cleanup shims usage in Tethering"
diff --git a/Tethering/common/TetheringLib/src/android/net/TetheringManager.java b/Tethering/common/TetheringLib/src/android/net/TetheringManager.java
index c137b49..9e6e34e 100644
--- a/Tethering/common/TetheringLib/src/android/net/TetheringManager.java
+++ b/Tethering/common/TetheringLib/src/android/net/TetheringManager.java
@@ -290,6 +290,23 @@
getConnector(c -> c.registerTetheringEventCallback(mCallback, pkgName));
}
+ /** @hide */
+ @Override
+ protected void finalize() throws Throwable {
+ final String pkgName = mContext.getOpPackageName();
+ Log.i(TAG, "unregisterTetheringEventCallback:" + pkgName);
+ // 1. It's generally not recommended to perform long operations in finalize, but while
+ // unregisterTetheringEventCallback does an IPC, it's a oneway IPC so should not block.
+ // 2. If the connector is not yet connected, TetheringManager is impossible to finalize
+ // because the connector polling thread strong reference the TetheringManager object. So
+ // it's guaranteed that registerTetheringEventCallback was already called before calling
+ // unregisterTetheringEventCallback in finalize.
+ if (mConnector == null) Log.wtf(TAG, "null connector in finalize!");
+ getConnector(c -> c.unregisterTetheringEventCallback(mCallback, pkgName));
+
+ super.finalize();
+ }
+
private void startPollingForConnector() {
new Thread(() -> {
while (true) {
diff --git a/Tethering/tests/unit/src/com/android/networkstack/tethering/TetheringServiceTest.java b/Tethering/tests/unit/src/com/android/networkstack/tethering/TetheringServiceTest.java
index 0052267..f664d5d 100644
--- a/Tethering/tests/unit/src/com/android/networkstack/tethering/TetheringServiceTest.java
+++ b/Tethering/tests/unit/src/com/android/networkstack/tethering/TetheringServiceTest.java
@@ -28,6 +28,7 @@
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertNull;
+import static org.junit.Assert.fail;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyBoolean;
import static org.mockito.ArgumentMatchers.eq;
@@ -46,6 +47,7 @@
import android.net.TetheringRequestParcel;
import android.net.ip.IpServer;
import android.os.Bundle;
+import android.os.ConditionVariable;
import android.os.Handler;
import android.os.IBinder;
import android.os.ResultReceiver;
@@ -63,7 +65,6 @@
import org.junit.Rule;
import org.junit.Test;
import org.junit.runner.RunWith;
-import org.mockito.Matchers;
import org.mockito.Mock;
import org.mockito.MockitoAnnotations;
@@ -503,38 +504,80 @@
});
}
+ private class ConnectorSupplier<T> implements Supplier<T> {
+ private T mResult = null;
+
+ public void set(T result) {
+ mResult = result;
+ }
+
+ @Override
+ public T get() {
+ return mResult;
+ }
+ }
+
+ private void forceGc() {
+ System.gc();
+ System.runFinalization();
+ System.gc();
+ }
+
@Test
public void testTetheringManagerLeak() throws Exception {
runAsAccessNetworkState((none) -> {
final ArrayList<ITetheringEventCallback> callbacks = new ArrayList<>();
+ final ConditionVariable registeredCv = new ConditionVariable(false);
doAnswer((invocation) -> {
final Object[] args = invocation.getArguments();
callbacks.add((ITetheringEventCallback) args[0]);
+ registeredCv.open();
return null;
- }).when(mTethering).registerTetheringEventCallback(Matchers.anyObject());
+ }).when(mTethering).registerTetheringEventCallback(any());
- final Supplier<IBinder> supplier = () -> mMockConnector.getIBinder();
+ doAnswer((invocation) -> {
+ final Object[] args = invocation.getArguments();
+ callbacks.remove((ITetheringEventCallback) args[0]);
+ return null;
+ }).when(mTethering).unregisterTetheringEventCallback(any());
+
+ final ConnectorSupplier<IBinder> supplier = new ConnectorSupplier<>();
TetheringManager tm = new TetheringManager(mMockConnector.getService(), supplier);
assertNotNull(tm);
- assertEquals("Internal callback is not registered", 1, callbacks.size());
+ assertEquals("Internal callback should not be registered", 0, callbacks.size());
- final WeakReference weakTm = new WeakReference(tm);
+ final WeakReference<TetheringManager> weakTm = new WeakReference(tm);
assertNotNull(weakTm.get());
+ // TetheringManager couldn't be GCed because pollingConnector thread implicitly
+ // reference TetheringManager object.
tm = null;
+ forceGc();
+ assertNotNull(weakTm.get());
+
+ // After getting connector, pollingConnector thread stops and internal callback is
+ // registered.
+ supplier.set(mMockConnector.getIBinder());
+ final long timeout = 500L;
+ if (!registeredCv.block(timeout)) {
+ fail("TetheringManager poll connector fail after " + timeout + " ms");
+ }
+ assertEquals("Internal callback is not registered", 1, callbacks.size());
+ assertNotNull(weakTm.get());
+
final int attempts = 100;
final long waitIntervalMs = 50;
for (int i = 0; i < attempts; i++) {
- System.gc();
- System.runFinalization();
- System.gc();
+ forceGc();
if (weakTm.get() == null) break;
Thread.sleep(waitIntervalMs);
}
assertNull("TetheringManager weak reference still not null after " + attempts
+ " attempts", weakTm.get());
+
+ assertEquals("Internal callback is not unregistered", 0, callbacks.size());
});
}
}
diff --git a/framework/src/android/net/NetworkInfo.java b/framework/src/android/net/NetworkInfo.java
index bb23494..433933f 100644
--- a/framework/src/android/net/NetworkInfo.java
+++ b/framework/src/android/net/NetworkInfo.java
@@ -179,21 +179,19 @@
/** {@hide} */
@UnsupportedAppUsage
- public NetworkInfo(NetworkInfo source) {
- if (source != null) {
- synchronized (source) {
- mNetworkType = source.mNetworkType;
- mSubtype = source.mSubtype;
- mTypeName = source.mTypeName;
- mSubtypeName = source.mSubtypeName;
- mState = source.mState;
- mDetailedState = source.mDetailedState;
- mReason = source.mReason;
- mExtraInfo = source.mExtraInfo;
- mIsFailover = source.mIsFailover;
- mIsAvailable = source.mIsAvailable;
- mIsRoaming = source.mIsRoaming;
- }
+ public NetworkInfo(@NonNull NetworkInfo source) {
+ synchronized (source) {
+ mNetworkType = source.mNetworkType;
+ mSubtype = source.mSubtype;
+ mTypeName = source.mTypeName;
+ mSubtypeName = source.mSubtypeName;
+ mState = source.mState;
+ mDetailedState = source.mDetailedState;
+ mReason = source.mReason;
+ mExtraInfo = source.mExtraInfo;
+ mIsFailover = source.mIsFailover;
+ mIsAvailable = source.mIsAvailable;
+ mIsRoaming = source.mIsRoaming;
}
}
@@ -479,7 +477,7 @@
* @param detailedState the {@link DetailedState}.
* @param reason a {@code String} indicating the reason for the state change,
* if one was supplied. May be {@code null}.
- * @param extraInfo an optional {@code String} providing addditional network state
+ * @param extraInfo an optional {@code String} providing additional network state
* information passed up from the lower networking layers.
* @deprecated Use {@link NetworkCapabilities} instead.
*/
@@ -491,6 +489,11 @@
this.mState = stateMap.get(detailedState);
this.mReason = reason;
this.mExtraInfo = extraInfo;
+ // Catch both the case where detailedState is null and the case where it's some
+ // unknown value
+ if (null == mState) {
+ throw new NullPointerException("Unknown DetailedState : " + detailedState);
+ }
}
}
diff --git a/tests/cts/net/src/android/net/cts/ConnectivityManagerTest.java b/tests/cts/net/src/android/net/cts/ConnectivityManagerTest.java
index 99cfdc2..9606960 100644
--- a/tests/cts/net/src/android/net/cts/ConnectivityManagerTest.java
+++ b/tests/cts/net/src/android/net/cts/ConnectivityManagerTest.java
@@ -2829,6 +2829,19 @@
});
}
+ /**
+ * The networks used in this test are real networks and as such they can see seemingly random
+ * updates of their capabilities or link properties as conditions change, e.g. the network
+ * loses validation or IPv4 shows up. Many tests should simply treat these callbacks as
+ * spurious.
+ */
+ private void assertNoCallbackExceptCapOrLpChange(
+ @NonNull final TestableNetworkCallback cb) {
+ cb.assertNoCallbackThat(NO_CALLBACK_TIMEOUT_MS,
+ c -> !(c instanceof CallbackEntry.CapabilitiesChanged
+ || c instanceof CallbackEntry.LinkPropertiesChanged));
+ }
+
@AppModeFull(reason = "Cannot get WifiManager in instant app mode")
@Test
public void testMobileDataPreferredUids() throws Exception {
@@ -2861,8 +2874,7 @@
// CtsNetTestCases uid is not listed in MOBILE_DATA_PREFERRED_UIDS setting, so the
// per-app default network should be same as system default network.
waitForAvailable(systemDefaultCb, wifiNetwork);
- defaultTrackingCb.eventuallyExpect(CallbackEntry.AVAILABLE, NETWORK_CALLBACK_TIMEOUT_MS,
- entry -> wifiNetwork.equals(entry.getNetwork()));
+ waitForAvailable(defaultTrackingCb, wifiNetwork);
// Active network for CtsNetTestCases uid should be wifi now.
assertEquals(wifiNetwork, mCm.getActiveNetwork());
@@ -2872,10 +2884,10 @@
newMobileDataPreferredUids.add(uid);
ConnectivitySettingsManager.setMobileDataPreferredUids(
mContext, newMobileDataPreferredUids);
- defaultTrackingCb.eventuallyExpect(CallbackEntry.AVAILABLE, NETWORK_CALLBACK_TIMEOUT_MS,
- entry -> cellNetwork.equals(entry.getNetwork()));
- // System default network doesn't change.
- systemDefaultCb.assertNoCallback();
+ waitForAvailable(defaultTrackingCb, cellNetwork);
+ // No change for system default network. Expect no callback except CapabilitiesChanged
+ // or LinkPropertiesChanged which may be triggered randomly from wifi network.
+ assertNoCallbackExceptCapOrLpChange(systemDefaultCb);
// Active network for CtsNetTestCases uid should change to cell, too.
assertEquals(cellNetwork, mCm.getActiveNetwork());
@@ -2884,10 +2896,10 @@
newMobileDataPreferredUids.remove(uid);
ConnectivitySettingsManager.setMobileDataPreferredUids(
mContext, newMobileDataPreferredUids);
- defaultTrackingCb.eventuallyExpect(CallbackEntry.AVAILABLE, NETWORK_CALLBACK_TIMEOUT_MS,
- entry -> wifiNetwork.equals(entry.getNetwork()));
- // System default network still doesn't change.
- systemDefaultCb.assertNoCallback();
+ waitForAvailable(defaultTrackingCb, wifiNetwork);
+ // No change for system default network. Expect no callback except CapabilitiesChanged
+ // or LinkPropertiesChanged which may be triggered randomly from wifi network.
+ assertNoCallbackExceptCapOrLpChange(systemDefaultCb);
// Active network for CtsNetTestCases uid should change back to wifi.
assertEquals(wifiNetwork, mCm.getActiveNetwork());
} finally {
diff --git a/tests/unit/java/com/android/server/connectivity/Nat464XlatTest.java b/tests/unit/java/com/android/server/connectivity/Nat464XlatTest.java
index f358726..aa4c4e3 100644
--- a/tests/unit/java/com/android/server/connectivity/Nat464XlatTest.java
+++ b/tests/unit/java/com/android/server/connectivity/Nat464XlatTest.java
@@ -109,8 +109,8 @@
mNai.linkProperties = new LinkProperties();
mNai.linkProperties.setInterfaceName(BASE_IFACE);
- mNai.networkInfo = new NetworkInfo(null);
- mNai.networkInfo.setType(ConnectivityManager.TYPE_WIFI);
+ mNai.networkInfo = new NetworkInfo(ConnectivityManager.TYPE_WIFI, 0 /* subtype */,
+ null /* typeName */, null /* subtypeName */);
mNai.networkCapabilities = new NetworkCapabilities();
markNetworkConnected();
when(mNai.connService()).thenReturn(mConnectivity);