From 1110b725a07684f549d26b2c38a07eae2d9456f9 Mon Sep 17 00:00:00 2001 From: markchien Date: Wed, 23 Jun 2021 21:26:07 +0800 Subject: [PATCH] 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 --- .../src/android/net/TetheringManager.java | 42 +++++++++++++---- .../tethering/MockTetheringService.java | 5 +- .../tethering/TetheringServiceTest.java | 46 ++++++++++++++++++- 3 files changed, 79 insertions(+), 14 deletions(-) diff --git a/Tethering/common/TetheringLib/src/android/net/TetheringManager.java b/Tethering/common/TetheringLib/src/android/net/TetheringManager.java index 73ab908af3..3086da2cee 100644 --- a/Tethering/common/TetheringLib/src/android/net/TetheringManager.java +++ b/Tethering/common/TetheringLib/src/android/net/TetheringManager.java @@ -37,6 +37,7 @@ import com.android.internal.annotations.GuardedBy; 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 class TetheringManager { public TetheringManager(@NonNull final Context context, @NonNull Supplier connectorSupplier) { mContext = context; - mCallback = new TetheringCallbackInternal(); + mCallback = new TetheringCallbackInternal(this); mConnectorSupplier = connectorSupplier; final String pkgName = mContext.getOpPackageName(); @@ -415,7 +416,7 @@ public class TetheringManager { } } - 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 @@ public class TetheringManager { } } - 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 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 @@ public class TetheringManager { @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 4865e03c21..3c07580762 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 static org.mockito.Mockito.mock; 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 @@ public class MockTetheringService extends TetheringService { 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 1b52f6efeb..00522677ed 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_CHANGE_TETHERING_PERM 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.content.Intent; 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.Before; 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 @@ public final class TetheringServiceTest { 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 @@ public final class TetheringServiceTest { verifyNoMoreInteractionsForTethering(); }); } + + @Test + public void testTetheringManagerLeak() throws Exception { + runAsAccessNetworkState((none) -> { + final ArrayList callbacks = new ArrayList<>(); + doAnswer((invocation) -> { + final Object[] args = invocation.getArguments(); + callbacks.add((ITetheringEventCallback) args[0]); + return null; + }).when(mTethering).registerTetheringEventCallback(Matchers.anyObject()); + + final Supplier 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()); + }); + } }