Merge "Fix ServiceConnection leak" into udc-dev

wifi/java/src/android/net/wifi/sharedconnectivity/app/SharedConnectivityManager.java

@@ -38,6 +38,7 @@ import android.os.RemoteException;
 import android.util.Log;
 
 import com.android.internal.R;
+import com.android.internal.annotations.GuardedBy;
 
 import java.util.HashMap;
 import java.util.List;
@@ -49,9 +50,14 @@ import java.util.concurrent.Executor;
  * This class is the library used by consumers of Shared Connectivity data to bind to the service,
  * receive callbacks from, and send user actions to the service.
  *
+ * A client must register at least one callback so that the manager will bind to the service. Once
+ * all callbacks are unregistered, the manager will unbind from the service. When the client no
+ * longer needs Shared Connectivity data, the client must unregister.
+ *
  * The methods {@link #connectHotspotNetwork}, {@link #disconnectHotspotNetwork},
  * {@link #connectKnownNetwork} and {@link #forgetKnownNetwork} are not valid and will return false
- * if not called between {@link SharedConnectivityClientCallback#onServiceConnected()}
+ * and getter methods will fail and return null if not called between
+ * {@link SharedConnectivityClientCallback#onServiceConnected()}
  * and {@link SharedConnectivityClientCallback#onServiceDisconnected()} or if
  * {@link SharedConnectivityClientCallback#onRegisterCallbackFailed} was called.
  *
@@ -139,19 +145,22 @@ public class SharedConnectivityManager {
     }
 
     private ISharedConnectivityService mService;
+    @GuardedBy("mProxyDataLock")
     private final Map<SharedConnectivityClientCallback, SharedConnectivityCallbackProxy>
             mProxyMap = new HashMap<>();
+    @GuardedBy("mProxyDataLock")
     private final Map<SharedConnectivityClientCallback, SharedConnectivityCallbackProxy>
             mCallbackProxyCache = new HashMap<>();
-    // Used for testing
+    // Makes sure mProxyMap and mCallbackProxyCache are locked together when one of them is used.
-    private final ServiceConnection mServiceConnection;
+    private final Object mProxyDataLock = new Object();
+    private final Context mContext;
+    private final String mServicePackageName;
+    private final String mIntentAction;
+    private ServiceConnection mServiceConnection;
 
     /**
      * Creates a new instance of {@link SharedConnectivityManager}.
      *
-     * Automatically binds to implementation of {@link SharedConnectivityService} specified in
-     * the device overlay.
-     *
      * @return An instance of {@link SharedConnectivityManager} or null if the shared connectivity
      * service is not found.
      * @hide
@@ -185,12 +194,18 @@ public class SharedConnectivityManager {
 
     private SharedConnectivityManager(@NonNull Context context, String servicePackageName,
             String serviceIntentAction) {
+        mContext = context;
+        mServicePackageName = servicePackageName;
+        mIntentAction = serviceIntentAction;
+    }
+
+    private void bind() {
         mServiceConnection = new ServiceConnection() {
             @Override
             public void onServiceConnected(ComponentName name, IBinder service) {
                 mService = ISharedConnectivityService.Stub.asInterface(service);
-                if (!mCallbackProxyCache.isEmpty()) {
+                synchronized (mProxyDataLock) {
-                    synchronized (mCallbackProxyCache) {
+                    if (!mCallbackProxyCache.isEmpty()) {
                         mCallbackProxyCache.keySet().forEach(callback ->
                                 registerCallbackInternal(
                                         callback, mCallbackProxyCache.get(callback)));
@@ -203,15 +218,13 @@ public class SharedConnectivityManager {
             public void onServiceDisconnected(ComponentName name) {
                 if (DEBUG) Log.i(TAG, "onServiceDisconnected");
                 mService = null;
-                if (!mCallbackProxyCache.isEmpty()) {
+                synchronized (mProxyDataLock) {
-                    synchronized (mCallbackProxyCache) {
+                    if (!mCallbackProxyCache.isEmpty()) {
                         mCallbackProxyCache.keySet().forEach(
                                 SharedConnectivityClientCallback::onServiceDisconnected);
                         mCallbackProxyCache.clear();
                     }
-                }
+                    if (!mProxyMap.isEmpty()) {
-                if (!mProxyMap.isEmpty()) {
-                    synchronized (mProxyMap) {
                         mProxyMap.keySet().forEach(
                                 SharedConnectivityClientCallback::onServiceDisconnected);
                         mProxyMap.clear();
@@ -220,8 +233,8 @@ public class SharedConnectivityManager {
             }
         };
 
-        context.bindService(
+        mContext.bindService(
-                new Intent().setPackage(servicePackageName).setAction(serviceIntentAction),
+                new Intent().setPackage(mServicePackageName).setAction(mIntentAction),
                 mServiceConnection, Context.BIND_AUTO_CREATE);
     }
 
@@ -229,7 +242,7 @@ public class SharedConnectivityManager {
             SharedConnectivityCallbackProxy proxy) {
         try {
             mService.registerCallback(proxy);
-            synchronized (mProxyMap) {
+            synchronized (mProxyDataLock) {
                 mProxyMap.put(callback, proxy);
             }
             callback.onServiceConnected();
@@ -256,10 +269,19 @@ public class SharedConnectivityManager {
         return mServiceConnection;
     }
 
+    private void unbind() {
+        if (mServiceConnection != null) {
+            mContext.unbindService(mServiceConnection);
+            mServiceConnection = null;
+        }
+    }
+
     /**
      * Registers a callback for receiving updates to the list of Hotspot Networks, Known Networks,
      * shared connectivity settings state, hotspot network connection status and known network
      * connection status.
+     * Automatically binds to implementation of {@link SharedConnectivityService} specified in
+     * the device overlay when the first callback is registered.
      * The {@link SharedConnectivityClientCallback#onRegisterCallbackFailed} will be called if the
      * registration failed.
      *
@@ -284,9 +306,16 @@ public class SharedConnectivityManager {
         SharedConnectivityCallbackProxy proxy =
                 new SharedConnectivityCallbackProxy(executor, callback);
         if (mService == null) {
-            synchronized (mCallbackProxyCache) {
+            boolean shouldBind;
+            synchronized (mProxyDataLock) {
+                // Size can be 1 in different cases of register/unregister sequences. If size is 0
+                // Bind never happened or unbind was called.
+                shouldBind = mCallbackProxyCache.size() == 0;
                 mCallbackProxyCache.put(callback, proxy);
             }
+            if (shouldBind) {
+                bind();
+            }
             return;
         }
         registerCallbackInternal(callback, proxy);
@@ -294,6 +323,7 @@ public class SharedConnectivityManager {
 
     /**
      * Unregisters a callback.
+     * Unbinds from {@link SharedConnectivityService} when no more callbacks are registered.
      *
      * @return Returns true if the callback was successfully unregistered, false otherwise.
      */
@@ -309,16 +339,27 @@ public class SharedConnectivityManager {
         }
 
         if (mService == null) {
-            synchronized (mCallbackProxyCache) {
+            boolean shouldUnbind;
+            synchronized (mProxyDataLock) {
                 mCallbackProxyCache.remove(callback);
+                // Connection was never established, so all registered callbacks are in the cache.
+                shouldUnbind = mCallbackProxyCache.isEmpty();
+            }
+            if (shouldUnbind) {
+                unbind();
             }
             return true;
         }
 
         try {
-            mService.unregisterCallback(mProxyMap.get(callback));
+            boolean shouldUnbind;
-            synchronized (mProxyMap) {
+            synchronized (mProxyDataLock) {
+                mService.unregisterCallback(mProxyMap.get(callback));
                 mProxyMap.remove(callback);
+                shouldUnbind = mProxyMap.isEmpty();
+            }
+            if (shouldUnbind) {
+                unbind();
             }
         } catch (RemoteException e) {
             Log.e(TAG, "Exception in unregisterCallback", e);

wifi/tests/src/android/net/wifi/sharedconnectivity/app/SharedConnectivityManagerTest.java

@@ -28,15 +28,17 @@ import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.anyInt;
 import static org.mockito.Mockito.doThrow;
 import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
 import android.content.ComponentName;
 import android.content.Context;
+import android.content.Intent;
+import android.content.ServiceConnection;
 import android.content.res.Resources;
 import android.net.wifi.sharedconnectivity.service.ISharedConnectivityService;
 import android.os.Bundle;
-import android.os.Parcel;
 import android.os.RemoteException;
 
 import androidx.test.filters.SmallTest;
@@ -80,7 +82,7 @@ public class SharedConnectivityManagerTest {
     @Mock
     Executor mExecutor;
     @Mock
-    SharedConnectivityClientCallback mClientCallback;
+    SharedConnectivityClientCallback mClientCallback, mClientCallback2;
     @Mock
     Resources mResources;
     @Mock
@@ -95,47 +97,52 @@ public class SharedConnectivityManagerTest {
         setResources(mContext);
     }
 
-    /**
-     * Verifies constructor is binding to service.
-     */
     @Test
-    public void bindingToService() {
+    public void resourcesNotDefined_createShouldReturnNull() {
-        SharedConnectivityManager.create(mContext);
-
-        verify(mContext).bindService(any(), any(), anyInt());
-    }
-
-    /**
-     * Verifies create method returns null when resources are not specified
-     */
-    @Test
-    public void resourcesNotDefined() {
         when(mResources.getString(anyInt())).thenThrow(new Resources.NotFoundException());
 
         assertThat(SharedConnectivityManager.create(mContext)).isNull();
     }
 
-    /**
-     * Verifies registerCallback behavior.
-     */
     @Test
-    public void registerCallback_serviceNotConnected_registrationCachedThenConnected()
+    public void bindingToServiceOnFirstCallbackRegistration() {
-            throws Exception {
+        SharedConnectivityManager manager = SharedConnectivityManager.create(mContext);
+        manager.registerCallback(mExecutor, mClientCallback);
+
+        verify(mContext).bindService(any(Intent.class), any(ServiceConnection.class), anyInt());
+    }
+
+    @Test
+    public void bindIsCalledOnceOnMultipleCallbackRegistrations() throws Exception {
         SharedConnectivityManager manager = SharedConnectivityManager.create(mContext);
-        manager.setService(null);
 
         manager.registerCallback(mExecutor, mClientCallback);
-        manager.getServiceConnection().onServiceConnected(COMPONENT_NAME, mIBinder);
+        verify(mContext, times(1)).bindService(any(Intent.class), any(ServiceConnection.class),
+                anyInt());
 
-        // Since the binder is embedded in a proxy class, the call to registerCallback is done on
+        manager.registerCallback(mExecutor, mClientCallback2);
-        // the proxy. So instead verifying that the proxy is calling the binder.
+        verify(mContext, times(1)).bindService(any(Intent.class), any(ServiceConnection.class),
-        verify(mIBinder).transact(anyInt(), any(Parcel.class), any(Parcel.class), anyInt());
+                anyInt());
+    }
+
+    @Test
+    public void unbindIsCalledOnLastCallbackUnregistrations() throws Exception {
+        SharedConnectivityManager manager = SharedConnectivityManager.create(mContext);
+
+        manager.registerCallback(mExecutor, mClientCallback);
+        manager.registerCallback(mExecutor, mClientCallback2);
+        manager.unregisterCallback(mClientCallback);
+        verify(mContext, never()).unbindService(
+                any(ServiceConnection.class));
+
+        manager.unregisterCallback(mClientCallback2);
+        verify(mContext, times(1)).unbindService(
+                any(ServiceConnection.class));
     }
 
     @Test
     public void registerCallback_serviceNotConnected_canUnregisterAndReregister() {
         SharedConnectivityManager manager = SharedConnectivityManager.create(mContext);
-        manager.setService(null);
 
         manager.registerCallback(mExecutor, mClientCallback);
         manager.unregisterCallback(mClientCallback);
@@ -177,9 +184,6 @@ public class SharedConnectivityManagerTest {
         verify(mClientCallback).onRegisterCallbackFailed(any(RemoteException.class));
     }
 
-    /**
-     * Verifies unregisterCallback behavior.
-     */
     @Test
     public void unregisterCallback_withoutRegisteringFirst_serviceNotConnected_shouldFail() {
         SharedConnectivityManager manager = SharedConnectivityManager.create(mContext);
@@ -239,11 +243,8 @@ public class SharedConnectivityManagerTest {
         assertThat(manager.unregisterCallback(mClientCallback)).isFalse();
     }
 
-    /**
-     * Verifies callback is called when service is connected
-     */
     @Test
-    public void onServiceConnected_registerCallbackBeforeConnection() {
+    public void onServiceConnected() {
         SharedConnectivityManager manager = SharedConnectivityManager.create(mContext);
 
         manager.registerCallback(mExecutor, mClientCallback);
@@ -253,20 +254,7 @@ public class SharedConnectivityManagerTest {
     }
 
     @Test
-    public void onServiceConnected_registerCallbackAfterConnection() {
+    public void onServiceDisconnected() {
-        SharedConnectivityManager manager = SharedConnectivityManager.create(mContext);
-
-        manager.getServiceConnection().onServiceConnected(COMPONENT_NAME, mIBinder);
-        manager.registerCallback(mExecutor, mClientCallback);
-
-        verify(mClientCallback).onServiceConnected();
-    }
-
-    /**
-     * Verifies callback is called when service is disconnected
-     */
-    @Test
-    public void onServiceDisconnected_registerCallbackBeforeConnection() {
         SharedConnectivityManager manager = SharedConnectivityManager.create(mContext);
 
         manager.registerCallback(mExecutor, mClientCallback);
@@ -276,20 +264,7 @@ public class SharedConnectivityManagerTest {
         verify(mClientCallback).onServiceDisconnected();
     }
 
-    @Test
-    public void onServiceDisconnected_registerCallbackAfterConnection() {
-        SharedConnectivityManager manager = SharedConnectivityManager.create(mContext);
 
-        manager.getServiceConnection().onServiceConnected(COMPONENT_NAME, mIBinder);
-        manager.registerCallback(mExecutor, mClientCallback);
-        manager.getServiceConnection().onServiceDisconnected(COMPONENT_NAME);
-
-        verify(mClientCallback).onServiceDisconnected();
-    }
-
-    /**
-     * Verifies connectHotspotNetwork behavior.
-     */
     @Test
     public void connectHotspotNetwork_serviceNotConnected_shouldFail() {
         HotspotNetwork network = buildHotspotNetwork();
@@ -320,9 +295,6 @@ public class SharedConnectivityManagerTest {
         assertThat(manager.connectHotspotNetwork(network)).isFalse();
     }
 
-    /**
-     * Verifies disconnectHotspotNetwork behavior.
-     */
     @Test
     public void disconnectHotspotNetwork_serviceNotConnected_shouldFail() {
         HotspotNetwork network = buildHotspotNetwork();
@@ -353,9 +325,6 @@ public class SharedConnectivityManagerTest {
         assertThat(manager.disconnectHotspotNetwork(network)).isFalse();
     }
 
-    /**
-     * Verifies connectKnownNetwork behavior.
-     */
     @Test
     public void connectKnownNetwork_serviceNotConnected_shouldFail() throws RemoteException {
         KnownNetwork network = buildKnownNetwork();
@@ -386,9 +355,6 @@ public class SharedConnectivityManagerTest {
         assertThat(manager.connectKnownNetwork(network)).isFalse();
     }
 
-    /**
-     * Verifies forgetKnownNetwork behavior.
-     */
     @Test
     public void forgetKnownNetwork_serviceNotConnected_shouldFail() {
         KnownNetwork network = buildKnownNetwork();
@@ -419,9 +385,6 @@ public class SharedConnectivityManagerTest {
         assertThat(manager.forgetKnownNetwork(network)).isFalse();
     }
 
-    /**
-     * Verify getters.
-     */
     @Test
     public void getHotspotNetworks_serviceNotConnected_shouldReturnNull() {
         SharedConnectivityManager manager = SharedConnectivityManager.create(mContext);