diff --git a/Tethering/common/TetheringLib/src/android/net/TetheringManager.java b/Tethering/common/TetheringLib/src/android/net/TetheringManager.java index 69bb71f894..c137b4924d 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()); + }); + } }