Android/frameworks_base
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Fix ServiceConnection leak

Browse Source 
Calling bind in the first callback register and unbind when all
callbackswere unregistered.

Bug: 268743334
Test: atest SharedConnectiviyManagerTest
Change-Id: Ib37c5d26a575813588d930026acfc48fef68091f
This commit is contained in:
Isaac Katzenelson
2023-03-18 00:09:03 +00:00
committed by Isaac Katzenelson
parent 59efad38ed
commit a13226cd45
2 changed files with 97 additions and 93 deletions
Download Patch File Download Diff File Expand all files Collapse all files

81
wifi/java/src/android/net/wifi/sharedconnectivity/app/SharedConnectivityManager.java
View File

@@ -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
private final ServiceConnection mServiceConnection;
// Makes sure mProxyMap and mCallbackProxyCache are locked together when one of them is used.
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 (mCallbackProxyCache) {
synchronized (mProxyDataLock) {
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 (mCallbackProxyCache) {
synchronized (mProxyDataLock) {
if (!mCallbackProxyCache.isEmpty()) {
mCallbackProxyCache.keySet().forEach(
SharedConnectivityClientCallback::onServiceDisconnected);
mCallbackProxyCache.clear();
}
}
if (!mProxyMap.isEmpty()) {
synchronized (mProxyMap) {
if (!mProxyMap.isEmpty()) {
mProxyMap.keySet().forEach(
SharedConnectivityClientCallback::onServiceDisconnected);
mProxyMap.clear();
@@ -220,8 +233,8 @@ public class SharedConnectivityManager {
}
};
context.bindService(
new Intent().setPackage(servicePackageName).setAction(serviceIntentAction),
mContext.bindService(
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));
synchronized (mProxyMap) {
boolean shouldUnbind;
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);

109
wifi/tests/src/android/net/wifi/sharedconnectivity/app/SharedConnectivityManagerTest.java
View File

@@ -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() {
SharedConnectivityManager.create(mContext);
verify(mContext).bindService(any(), any(), anyInt());
}
/**
* Verifies create method returns null when resources are not specified
*/
@Test
public void resourcesNotDefined() {
public void resourcesNotDefined_createShouldReturnNull() {
when(mResources.getString(anyInt())).thenThrow(new Resources.NotFoundException());
assertThat(SharedConnectivityManager.create(mContext)).isNull();
}
/**
* Verifies registerCallback behavior.
*/
@Test
public void registerCallback_serviceNotConnected_registrationCachedThenConnected()
throws Exception {
public void bindingToServiceOnFirstCallbackRegistration() {
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
// the proxy. So instead verifying that the proxy is calling the binder.
verify(mIBinder).transact(anyInt(), any(Parcel.class), any(Parcel.class), anyInt());
manager.registerCallback(mExecutor, mClientCallback2);
verify(mContext, times(1)).bindService(any(Intent.class), any(ServiceConnection.class),
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() {
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() {
public void onServiceDisconnected() {
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);