Fix TetheringManager memory leak
TetheringCallbackInteranl is inner class which explicitly reference
TetheringManager object. This causes TetheringManager can't be GC. Using
static nested class which has its own lifecycle and weak reference
TetheringManager object.
Still have a leak inside Tethering that TetheringCallbackInternal is
never unregistered. Currently it rely on binder died to remove the
reference, which usually happen in kill process. If process keep alive,
the TetheringCallbackInternal would not be freed even TetheringManager is
gone. Will have follow CL to fix this.
Bug: 177265744
Bug: 191798390
Bug: 187972579
Test: 1. lunch Settings with ON/OFF tethering, dump java heap.
2. close Settings and restart Settings again, dump java heap.
3. Compare java heap between step 1 and step 2.
Change-Id: I0e2a21b7988115098a033a581cd98da8bffe2791
diff --git a/Tethering/common/TetheringLib/src/android/net/TetheringManager.java b/Tethering/common/TetheringLib/src/android/net/TetheringManager.java
index 73ab908..3086da2 100644
--- a/Tethering/common/TetheringLib/src/android/net/TetheringManager.java
+++ b/Tethering/common/TetheringLib/src/android/net/TetheringManager.java
@@ -37,6 +37,7 @@
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
+import java.lang.ref.WeakReference;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
@@ -265,7 +266,7 @@
public TetheringManager(@NonNull final Context context,
@NonNull Supplier<IBinder> connectorSupplier) {
mContext = context;
- mCallback = new TetheringCallbackInternal();
+ mCallback = new TetheringCallbackInternal(this);
mConnectorSupplier = connectorSupplier;
final String pkgName = mContext.getOpPackageName();
@@ -415,7 +416,7 @@
}
}
- private void throwIfPermissionFailure(final int errorCode) {
+ private static void throwIfPermissionFailure(final int errorCode) {
switch (errorCode) {
case TETHER_ERROR_NO_CHANGE_TETHERING_PERMISSION:
throw new SecurityException("No android.permission.TETHER_PRIVILEGED"
@@ -426,21 +427,40 @@
}
}
- private class TetheringCallbackInternal extends ITetheringEventCallback.Stub {
+ private static class TetheringCallbackInternal extends ITetheringEventCallback.Stub {
private volatile int mError = TETHER_ERROR_NO_ERROR;
private final ConditionVariable mWaitForCallback = new ConditionVariable();
+ // This object is never garbage collected because the Tethering code running in
+ // the system server always maintains a reference to it for as long as
+ // mCallback is registered.
+ //
+ // Don't keep a strong reference to TetheringManager because otherwise
+ // TetheringManager cannot be garbage collected, and because TetheringManager
+ // stores the Context that it was created from, this will prevent the calling
+ // Activity from being garbage collected as well.
+ private final WeakReference<TetheringManager> mTetheringMgrRef;
+
+ TetheringCallbackInternal(final TetheringManager tm) {
+ mTetheringMgrRef = new WeakReference<>(tm);
+ }
@Override
public void onCallbackStarted(TetheringCallbackStartedParcel parcel) {
- mTetheringConfiguration = parcel.config;
- mTetherStatesParcel = parcel.states;
- mWaitForCallback.open();
+ TetheringManager tetheringMgr = mTetheringMgrRef.get();
+ if (tetheringMgr != null) {
+ tetheringMgr.mTetheringConfiguration = parcel.config;
+ tetheringMgr.mTetherStatesParcel = parcel.states;
+ mWaitForCallback.open();
+ }
}
@Override
public void onCallbackStopped(int errorCode) {
- mError = errorCode;
- mWaitForCallback.open();
+ TetheringManager tetheringMgr = mTetheringMgrRef.get();
+ if (tetheringMgr != null) {
+ mError = errorCode;
+ mWaitForCallback.open();
+ }
}
@Override
@@ -448,12 +468,14 @@
@Override
public void onConfigurationChanged(TetheringConfigurationParcel config) {
- mTetheringConfiguration = config;
+ TetheringManager tetheringMgr = mTetheringMgrRef.get();
+ if (tetheringMgr != null) tetheringMgr.mTetheringConfiguration = config;
}
@Override
public void onTetherStatesChanged(TetherStatesParcel states) {
- mTetherStatesParcel = states;
+ TetheringManager tetheringMgr = mTetheringMgrRef.get();
+ if (tetheringMgr != null) tetheringMgr.mTetherStatesParcel = states;
}
@Override
diff --git a/Tethering/tests/unit/src/com/android/networkstack/tethering/MockTetheringService.java b/Tethering/tests/unit/src/com/android/networkstack/tethering/MockTetheringService.java
index 4865e03..3c07580 100644
--- a/Tethering/tests/unit/src/com/android/networkstack/tethering/MockTetheringService.java
+++ b/Tethering/tests/unit/src/com/android/networkstack/tethering/MockTetheringService.java
@@ -22,7 +22,6 @@
import android.content.Context;
import android.content.Intent;
-import android.net.ITetheringConnector;
import android.os.Binder;
import android.os.IBinder;
import android.util.ArrayMap;
@@ -72,8 +71,8 @@
mBase = base;
}
- public ITetheringConnector getTetheringConnector() {
- return ITetheringConnector.Stub.asInterface(mBase);
+ public IBinder getIBinder() {
+ return mBase;
}
public MockTetheringService getService() {
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 1b52f6e..0052267 100644
--- a/Tethering/tests/unit/src/com/android/networkstack/tethering/TetheringServiceTest.java
+++ b/Tethering/tests/unit/src/com/android/networkstack/tethering/TetheringServiceTest.java
@@ -26,6 +26,8 @@
import static android.net.TetheringManager.TETHER_ERROR_NO_ERROR;
import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyBoolean;
import static org.mockito.ArgumentMatchers.eq;
@@ -40,10 +42,12 @@
import android.net.IIntResultListener;
import android.net.ITetheringConnector;
import android.net.ITetheringEventCallback;
+import android.net.TetheringManager;
import android.net.TetheringRequestParcel;
import android.net.ip.IpServer;
import android.os.Bundle;
import android.os.Handler;
+import android.os.IBinder;
import android.os.ResultReceiver;
import androidx.test.InstrumentationRegistry;
@@ -59,9 +63,14 @@
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;
+import java.lang.ref.WeakReference;
+import java.util.ArrayList;
+import java.util.function.Supplier;
+
@RunWith(AndroidJUnit4.class)
@SmallTest
public final class TetheringServiceTest {
@@ -113,7 +122,7 @@
InstrumentationRegistry.getTargetContext(),
MockTetheringService.class);
mMockConnector = (MockTetheringConnector) mServiceTestRule.bindService(mMockServiceIntent);
- mTetheringConnector = mMockConnector.getTetheringConnector();
+ mTetheringConnector = ITetheringConnector.Stub.asInterface(mMockConnector.getIBinder());
final MockTetheringService service = mMockConnector.getService();
mTethering = service.getTethering();
}
@@ -493,4 +502,39 @@
verifyNoMoreInteractionsForTethering();
});
}
+
+ @Test
+ public void testTetheringManagerLeak() throws Exception {
+ runAsAccessNetworkState((none) -> {
+ final ArrayList<ITetheringEventCallback> callbacks = new ArrayList<>();
+ doAnswer((invocation) -> {
+ final Object[] args = invocation.getArguments();
+ callbacks.add((ITetheringEventCallback) args[0]);
+ return null;
+ }).when(mTethering).registerTetheringEventCallback(Matchers.anyObject());
+
+ final Supplier<IBinder> supplier = () -> mMockConnector.getIBinder();
+
+ TetheringManager tm = new TetheringManager(mMockConnector.getService(), supplier);
+ assertNotNull(tm);
+ assertEquals("Internal callback is not registered", 1, callbacks.size());
+
+ final WeakReference weakTm = new WeakReference(tm);
+ assertNotNull(weakTm.get());
+
+ tm = null;
+ final int attempts = 100;
+ final long waitIntervalMs = 50;
+ for (int i = 0; i < attempts; i++) {
+ System.gc();
+ System.runFinalization();
+ System.gc();
+ if (weakTm.get() == null) break;
+
+ Thread.sleep(waitIntervalMs);
+ }
+ assertNull("TetheringManager weak reference still not null after " + attempts
+ + " attempts", weakTm.get());
+ });
+ }
}