Merge "Fix TetheringManager memory leak" am: d7d41a73e7 am: 059afb99bb

Original change: https://android-review.googlesource.com/c/platform/packages/modules/Connectivity/+/1744513

Change-Id: I93d4c5bb23cb45e10dbcd8b19792d9b79baa3ff4

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<IBinder> 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,34 +427,55 @@ 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<TetheringManager> mTetheringMgrRef;
+
+        TetheringCallbackInternal(final TetheringManager tm) {
+            mTetheringMgrRef = new WeakReference<>(tm);
+        }
 
         @Override
         public void onCallbackStarted(TetheringCallbackStartedParcel parcel) {
-            mTetheringConfiguration = parcel.config;
+            TetheringManager tetheringMgr = mTetheringMgrRef.get();
-            mTetherStatesParcel = parcel.states;
+            if (tetheringMgr != null) {
+                tetheringMgr.mTetheringConfiguration = parcel.config;
+                tetheringMgr.mTetherStatesParcel = parcel.states;
                 mWaitForCallback.open();
             }
+        }
 
         @Override
         public void onCallbackStopped(int errorCode) {
+            TetheringManager tetheringMgr = mTetheringMgrRef.get();
+            if (tetheringMgr != null) {
                 mError = errorCode;
                 mWaitForCallback.open();
             }
+        }
 
         @Override
         public void onUpstreamChanged(Network network) { }
 
         @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

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() {
+        public IBinder getIBinder() {
-            return ITetheringConnector.Stub.asInterface(mBase);
+            return mBase;
         }
 
         public MockTetheringService getService() {

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<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());
+        });
+    }
 }