Merge "Refine TetherSettings to avoid activity leaks"

src/com/android/settings/network/tether/TetherSettings.java

@@ -77,7 +77,7 @@ import java.util.concurrent.atomic.AtomicReference;
  */
 @SearchIndexable
 public class TetherSettings extends RestrictedSettingsFragment
-        implements DataSaverBackend.Listener {
+        implements DataSaverBackend.Listener, TetheringManager.TetheringEventCallback {
 
     @VisibleForTesting
     static final String KEY_TETHER_PREFS_SCREEN = "tether_prefs_screen";
@@ -111,7 +111,6 @@ public class TetherSettings extends RestrictedSettingsFragment
     private OnStartTetheringCallback mStartTetheringCallback;
     private ConnectivityManager mCm;
     private EthernetManager mEm;
-    private TetheringEventCallback mTetheringEventCallback;
     private EthernetListener mEthernetListener;
     private final HashSet<String> mAvailableInterfaces = new HashSet<>();
 
@@ -133,6 +132,7 @@ public class TetherSettings extends RestrictedSettingsFragment
     Context mContext;
     @VisibleForTesting
     TetheringManager mTm;
+    private TetheringHelper mTetheringHelper;
 
     @Override
     public int getMetricsCategory() {
@@ -148,6 +148,8 @@ public class TetherSettings extends RestrictedSettingsFragment
         super.onAttach(context);
         mWifiTetherPreferenceController =
                 new WifiTetherPreferenceController(context, getSettingsLifecycle());
+        mTetheringHelper = TetheringHelper.getInstance(context, this /* TetheringEventCallback */,
+                getSettingsLifecycle());
     }
 
     @Override
@@ -186,7 +188,7 @@ public class TetherSettings extends RestrictedSettingsFragment
         mDataSaverBackend.addListener(this);
 
         mCm = (ConnectivityManager) getSystemService(Context.CONNECTIVITY_SERVICE);
-        mTm = (TetheringManager) getSystemService(Context.TETHERING_SERVICE);
+        mTm = mTetheringHelper.getTetheringManager();
         // Some devices do not have available EthernetManager. In that case getSystemService will
         // return null.
         mEm = mContext.getSystemService(EthernetManager.class);
@@ -364,15 +366,14 @@ public class TetherSettings extends RestrictedSettingsFragment
 
 
         mStartTetheringCallback = new OnStartTetheringCallback(this);
-        mTetheringEventCallback = new TetheringEventCallback(this);
-        mTm.registerTetheringEventCallback(r -> mHandler.post(r), mTetheringEventCallback);
 
         mMassStorageActive = Environment.MEDIA_SHARED.equals(Environment.getExternalStorageState());
         registerReceiver();
 
         mEthernetListener = new EthernetListener(this);
         if (mEm != null) {
-            mEm.addInterfaceStateListener(r -> mHandler.post(r), mEthernetListener);
+            mEm.addInterfaceStateListener(mContext.getApplicationContext().getMainExecutor(),
+                    mEthernetListener);
         }
 
         updateUsbState();
@@ -387,13 +388,11 @@ public class TetherSettings extends RestrictedSettingsFragment
             return;
         }
         getActivity().unregisterReceiver(mTetherChangeReceiver);
-        mTm.unregisterTetheringEventCallback(mTetheringEventCallback);
         if (mEm != null) {
             mEm.removeInterfaceStateListener(mEthernetListener);
         }
         mTetherChangeReceiver = null;
         mStartTetheringCallback = null;
-        mTetheringEventCallback = null;
     }
 
     @VisibleForTesting
@@ -688,25 +687,8 @@ public class TetherSettings extends RestrictedSettingsFragment
         }
     }
 
-    private static final class TetheringEventCallback implements
-            TetheringManager.TetheringEventCallback {
-        final WeakReference<TetherSettings> mTetherSettings;
-
-        TetheringEventCallback(TetherSettings settings) {
-            mTetherSettings = new WeakReference<>(settings);
-        }
-
     @Override
     public void onTetheredInterfacesChanged(List<String> interfaces) {
-            final TetherSettings tetherSettings = mTetherSettings.get();
-            if (tetherSettings == null) {
-                return;
-            }
-            tetherSettings.onTetheredInterfacesChanged(interfaces);
-        }
-    }
-
-    void onTetheredInterfacesChanged(List<String> interfaces) {
         Log.d(TAG, "onTetheredInterfacesChanged() interfaces : " + interfaces.toString());
         final String[] tethered = interfaces.toArray(new String[interfaces.size()]);
         updateUsbState(tethered);

src/com/android/settings/network/tether/TetheringHelper.java

@@ -0,0 +1,157 @@
+/*
+ * Copyright (C) 2022 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.android.settings.network.tether;
+
+import android.annotation.NonNull;
+import android.annotation.TestApi;
+import android.content.Context;
+import android.net.TetheringManager;
+import android.util.Log;
+
+import androidx.annotation.VisibleForTesting;
+import androidx.lifecycle.DefaultLifecycleObserver;
+import androidx.lifecycle.Lifecycle;
+import androidx.lifecycle.LifecycleOwner;
+
+import com.android.internal.annotations.GuardedBy;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+
+/**
+ * Helper class for TetheringManager.
+ */
+public class TetheringHelper implements DefaultLifecycleObserver {
+    private static final String TAG = "TetheringHelper";
+
+    private static final Object sInstanceLock = new Object();
+    @TestApi
+    @GuardedBy("sInstanceLock")
+    private static Map<Context, TetheringHelper> sTestInstances;
+
+    protected static Context sAppContext;
+    protected static TetheringManager sTetheringManager;
+    protected static TetheringEventCallback sTetheringEventCallback = new TetheringEventCallback();
+    protected static ArrayList<TetheringManager.TetheringEventCallback> sEventCallbacks =
+            new ArrayList<>();
+
+    protected TetheringManager.TetheringEventCallback mCallback;
+
+    /**
+     * Static method to create a singleton class for TetheringHelper.
+     *
+     * @param context The Context this is associated with.
+     * @return an instance of {@link TetheringHelper} object.
+     */
+    @NonNull
+    public static TetheringHelper getInstance(@NonNull Context context,
+            @NonNull TetheringManager.TetheringEventCallback callback,
+            @NonNull Lifecycle lifecycle) {
+        synchronized (sInstanceLock) {
+            if (sTestInstances != null && sTestInstances.containsKey(context)) {
+                TetheringHelper testInstance = sTestInstances.get(context);
+                Log.w(TAG, "The context owner use a test instance:" + testInstance);
+                return testInstance;
+            }
+
+            if (sAppContext == null) {
+                sAppContext = context.getApplicationContext();
+                sTetheringManager = sAppContext.getSystemService(TetheringManager.class);
+            }
+            TetheringHelper helper = new TetheringHelper();
+            helper.mCallback = callback;
+            lifecycle.addObserver(helper);
+            return helper;
+        }
+    }
+
+    /**
+     * A convenience method to set pre-prepared instance or mock class for testing.
+     *
+     * @param context  The Context this is associated with.
+     * @param instance of {@link TetheringHelper} object.
+     * @hide
+     */
+    @TestApi
+    @VisibleForTesting
+    public static void setTestInstance(@NonNull Context context, TetheringHelper instance) {
+        synchronized (sInstanceLock) {
+            if (sTestInstances == null) sTestInstances = new ConcurrentHashMap<>();
+            Log.w(TAG, "Set a test instance by context:" + context);
+            sTestInstances.put(context, instance);
+        }
+    }
+
+    /**
+     * The constructor can only be accessed from static method inside the class itself, this is
+     * to avoid creating a class by adding a private constructor.
+     */
+    private TetheringHelper() {
+        // Do nothing.
+    }
+
+    /**
+     * Returns the TetheringManager If the system service is successfully obtained.
+     */
+    public TetheringManager getTetheringManager() {
+        return sTetheringManager;
+    }
+
+    @Override
+    public void onStart(@NonNull LifecycleOwner owner) {
+        if (sEventCallbacks.contains(mCallback)) {
+            Log.w(TAG, "The callback already contains, don't register callback:" + mCallback);
+            return;
+        }
+        sEventCallbacks.add(mCallback);
+        if (sEventCallbacks.size() == 1) {
+            sTetheringManager.registerTetheringEventCallback(sAppContext.getMainExecutor(),
+                    sTetheringEventCallback);
+        }
+    }
+
+    @Override
+    public void onStop(@NonNull LifecycleOwner owner) {
+        if (!sEventCallbacks.remove(mCallback)) {
+            Log.w(TAG, "The callback does not contain, don't unregister callback:" + mCallback);
+            return;
+        }
+        if (sEventCallbacks.size() == 0) {
+            sTetheringManager.unregisterTetheringEventCallback(sTetheringEventCallback);
+        }
+    }
+
+    @Override
+    public void onDestroy(@NonNull LifecycleOwner owner) {
+        mCallback = null;
+    }
+
+    protected static final class TetheringEventCallback implements
+            TetheringManager.TetheringEventCallback {
+        @Override
+        public void onTetheredInterfacesChanged(List<String> interfaces) {
+            for (TetheringManager.TetheringEventCallback callback : sEventCallbacks) {
+                if (callback == null) continue;
+                callback.onTetheredInterfacesChanged(interfaces);
+            }
+        }
+    }
+}
+
+

tests/robotests/src/com/android/settings/network/tether/TetheringHelperTest.java

@@ -0,0 +1,150 @@
+/*
+ * Copyright (C) 2022 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.android.settings.network.tether;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.verify;
+
+import android.content.Context;
+import android.net.TetheringManager;
+
+import androidx.lifecycle.Lifecycle;
+import androidx.lifecycle.LifecycleOwner;
+import androidx.test.core.app.ApplicationProvider;
+import androidx.test.ext.junit.runners.AndroidJUnit4;
+
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.Spy;
+import org.mockito.junit.MockitoJUnit;
+import org.mockito.junit.MockitoRule;
+
+import java.util.List;
+import java.util.concurrent.Executor;
+
+@RunWith(AndroidJUnit4.class)
+public class TetheringHelperTest {
+    @Rule
+    public final MockitoRule mMockitoRule = MockitoJUnit.rule();
+    @Spy
+    Context mContext = ApplicationProvider.getApplicationContext();
+    @Mock
+    Lifecycle mLifecycle;
+    @Mock
+    LifecycleOwner mLifecycleOwner;
+    @Mock
+    TetheringManager mTetheringManager;
+    @Mock
+    TetheringManager.TetheringEventCallback mTetheringEventCallback;
+    @Mock
+    List<String> mInterfaces;
+
+    TetheringHelper mHelper;
+
+    @Before
+    public void setUp() {
+        mHelper = TetheringHelper.getInstance(mContext, mTetheringEventCallback, mLifecycle);
+        mHelper.sTetheringManager = mTetheringManager;
+    }
+
+    @Test
+    public void constructor_propertiesShouldBeReady() {
+        assertThat(mHelper.sAppContext).isNotNull();
+        assertThat(mHelper.sTetheringManager).isNotNull();
+        assertThat(mHelper.mCallback).isNotNull();
+        verify(mLifecycle).addObserver(any());
+    }
+
+    @Test
+    public void getTetheringManager_isNotNull() {
+        assertThat(mHelper.getTetheringManager()).isNotNull();
+    }
+
+    @Test
+    public void onStart_eventCallbacksClear_addAndRegisterCallback() {
+        mHelper.sEventCallbacks.clear();
+
+        mHelper.onStart(mLifecycleOwner);
+
+        assertThat(mHelper.sEventCallbacks.contains(mTetheringEventCallback)).isTrue();
+        assertThat(mHelper.sEventCallbacks.size()).isEqualTo(1);
+        verify(mTetheringManager).registerTetheringEventCallback(any(Executor.class),
+                eq(TetheringHelper.sTetheringEventCallback));
+    }
+
+    @Test
+    public void onStart_eventCallbacksContains_doNotRegisterCallback() {
+        mHelper.sEventCallbacks.clear();
+        mHelper.sEventCallbacks.add(mTetheringEventCallback);
+
+        mHelper.onStart(mLifecycleOwner);
+
+        verify(mTetheringManager, never()).registerTetheringEventCallback(any(Executor.class),
+                eq(TetheringHelper.sTetheringEventCallback));
+    }
+
+    @Test
+    public void onStop_eventCallbacksContains_removeAndUnregisterCallback() {
+        mHelper.sEventCallbacks.clear();
+        mHelper.sEventCallbacks.add(mTetheringEventCallback);
+
+        mHelper.onStop(mLifecycleOwner);
+
+        assertThat(mHelper.sEventCallbacks.contains(mTetheringEventCallback)).isFalse();
+        assertThat(mHelper.sEventCallbacks.size()).isEqualTo(0);
+        verify(mTetheringManager).unregisterTetheringEventCallback(
+                eq(TetheringHelper.sTetheringEventCallback));
+    }
+
+    @Test
+    public void onStop_eventCallbacksClear_doNotUnregisterCallback() {
+        mHelper.sEventCallbacks.clear();
+
+        mHelper.onStop(mLifecycleOwner);
+
+        assertThat(mHelper.sEventCallbacks.contains(mTetheringEventCallback)).isFalse();
+        assertThat(mHelper.sEventCallbacks.size()).isEqualTo(0);
+        verify(mTetheringManager, never()).unregisterTetheringEventCallback(
+                eq(TetheringHelper.sTetheringEventCallback));
+    }
+
+    @Test
+    public void onTetheredInterfacesChanged_eventCallbacksContains_doCallback() {
+        mHelper.sEventCallbacks.clear();
+        mHelper.sEventCallbacks.add(mTetheringEventCallback);
+
+        mHelper.sTetheringEventCallback.onTetheredInterfacesChanged(mInterfaces);
+
+        verify(mTetheringEventCallback).onTetheredInterfacesChanged(eq(mInterfaces));
+    }
+
+    @Test
+    public void onTetheredInterfacesChanged_eventCallbacksClear_doNotCallback() {
+        mHelper.sEventCallbacks.clear();
+
+        mHelper.sTetheringEventCallback.onTetheredInterfacesChanged(mInterfaces);
+
+        verify(mTetheringEventCallback, never()).onTetheredInterfacesChanged(eq(mInterfaces));
+    }
+}