From 26fe7b14b6b6fa2162ef7c6f7854cffebc3d76eb Mon Sep 17 00:00:00 2001 From: MiMoHo <37556964+MiMoHo@users.noreply.github.com> Date: Wed, 8 Jul 2026 02:50:07 +0200 Subject: [PATCH] fix(api): don't crash when the service connects after the API was closed NextcloudAPI.close() nulls the ApiConnectedListener while the binding to the AccountManagerService may still be in flight. unbindService() only unbound established connections, so a pending ServiceConnection stayed registered and later invoked onConnected() on the nulled callback, crashing the host app on the main thread with a NullPointerException in AidlNetworkRequest$1.onServiceConnected(). Unbind the ServiceConnection whenever a binding has been requested, not only once it is established, and ignore stale onServiceConnected() dispatches that race with close(). Fixes the crash reported in nextcloud/notes-android#2622. Assisted-by: Claude Code:claude-fable-5 Co-Authored-By: Claude Fable 5 Signed-off-by: MiMoHo <37556964+MiMoHo@users.noreply.github.com> --- lib/build.gradle | 1 + .../android/sso/api/AidlNetworkRequest.java | 23 ++++-- .../sso/api/AidlNetworkRequestTest.java | 80 +++++++++++++++++++ 3 files changed, 98 insertions(+), 6 deletions(-) create mode 100644 lib/src/test/java/com/nextcloud/android/sso/api/AidlNetworkRequestTest.java diff --git a/lib/build.gradle b/lib/build.gradle index 834388e9..fc51fcd2 100644 --- a/lib/build.gradle +++ b/lib/build.gradle @@ -92,6 +92,7 @@ android { } testOptions { targetSdk 36 + unitTests.returnDefaultValues = true } } diff --git a/lib/src/main/java/com/nextcloud/android/sso/api/AidlNetworkRequest.java b/lib/src/main/java/com/nextcloud/android/sso/api/AidlNetworkRequest.java index bc79b59a..3f80f789 100644 --- a/lib/src/main/java/com/nextcloud/android/sso/api/AidlNetworkRequest.java +++ b/lib/src/main/java/com/nextcloud/android/sso/api/AidlNetworkRequest.java @@ -48,7 +48,8 @@ public class AidlNetworkRequest extends NetworkRequest { private static final String TAG = AidlNetworkRequest.class.getCanonicalName(); private IInputStreamService mService = null; - private final AtomicBoolean mBound = new AtomicBoolean(false); // Flag indicating whether we have called bind on the service + private final AtomicBoolean mBound = new AtomicBoolean(false); // Flag indicating whether the service connection has been established + private final AtomicBoolean mBindingRequested = new AtomicBoolean(false); // Flag indicating whether we have called bind on the service AidlNetworkRequest(@NonNull Context context, @NonNull SingleSignOnAccount account, @NonNull NextcloudAPI.ApiConnectedListener callback) { super(context, account, callback); @@ -61,12 +62,19 @@ public class AidlNetworkRequest extends NetworkRequest { public void onServiceConnected(ComponentName className, IBinder service) { Log.d(TAG, "[onServiceConnected] called from Thread: [" + Thread.currentThread().getName() + "] with IBinder [" + className.toString() + "]: " + service); + final NextcloudAPI.ApiConnectedListener callback = mCallback; + if (mDestroyed || callback == null) { + // API has been closed while the connection was still being established + Log.w(TAG, "[onServiceConnected] API already closed - ignoring connection to [" + className + "]"); + return; + } + mService = IInputStreamService.Stub.asInterface(service); mBound.set(true); synchronized (mBound) { mBound.notifyAll(); } - mCallback.onConnected(); + callback.onConnected(); } public void onServiceDisconnected(ComponentName className) { @@ -105,6 +113,7 @@ public void connect(String type) { throw new IllegalStateException("Binding to AccountManagerService returned false"); } else { Log.d(TAG, "[connect] Bound to AccountManagerService successfully"); + mBindingRequested.set(true); } } catch (SecurityException e) { Log.e(TAG, "[connect] can't bind to AccountManagerService, check permission in Manifest"); @@ -127,17 +136,19 @@ public void close() { } private void unbindService() { - // Unbind from the service - if (mBound.get()) { + // Unbind whenever a binding has been requested: the ServiceConnection stays registered + // even if the connection has not been established yet and would otherwise still receive + // onServiceConnected() after this API has been closed + if (mBindingRequested.getAndSet(false)) { if (mContext != null) { Log.d(TAG, "[unbindService] Unbinding AccountManagerService"); mContext.unbindService(mConnection); } else { Log.e(TAG, "[unbindService] Context was null, cannot unbind nextcloud single sign-on service connection!"); } - mBound.set(false); - mService = null; } + mBound.set(false); + mService = null; } private void waitForApi() throws NextcloudApiNotRespondingException { diff --git a/lib/src/test/java/com/nextcloud/android/sso/api/AidlNetworkRequestTest.java b/lib/src/test/java/com/nextcloud/android/sso/api/AidlNetworkRequestTest.java new file mode 100644 index 00000000..a1c20c62 --- /dev/null +++ b/lib/src/test/java/com/nextcloud/android/sso/api/AidlNetworkRequestTest.java @@ -0,0 +1,80 @@ +/* + * Nextcloud Android SingleSignOn Library + * + * SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors + * SPDX-License-Identifier: GPL-3.0-or-later + */ +package com.nextcloud.android.sso.api; + +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyInt; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +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.os.IBinder; + +import com.nextcloud.android.sso.model.SingleSignOnAccount; + +import org.junit.Before; +import org.junit.Test; +import org.mockito.ArgumentCaptor; + +public class AidlNetworkRequestTest { + + private static final String ACCOUNT_TYPE = "nextcloud"; + + private Context context; + private NextcloudAPI.ApiConnectedListener callback; + private AidlNetworkRequest request; + + @Before + public void setUp() { + context = mock(Context.class); + callback = mock(NextcloudAPI.ApiConnectedListener.class); + final var account = new SingleSignOnAccount("name", "userId", "token", "https://example.com", ACCOUNT_TYPE); + request = new AidlNetworkRequest(context, account, callback); + } + + private ServiceConnection connectAndCaptureServiceConnection() { + when(context.bindService(any(Intent.class), any(ServiceConnection.class), anyInt())).thenReturn(true); + request.connect(ACCOUNT_TYPE); + + final var connectionCaptor = ArgumentCaptor.forClass(ServiceConnection.class); + verify(context).bindService(any(Intent.class), connectionCaptor.capture(), anyInt()); + return connectionCaptor.getValue(); + } + + @Test + public void onServiceConnectedBeforeCloseInvokesCallback() { + final var connection = connectAndCaptureServiceConnection(); + + connection.onServiceConnected(new ComponentName("com.nextcloud.client", "AccountManagerService"), mock(IBinder.class)); + + verify(callback).onConnected(); + } + + @Test + public void onServiceConnectedAfterCloseDoesNotThrowAndDoesNotInvokeCallback() { + final var connection = connectAndCaptureServiceConnection(); + + request.close(); + connection.onServiceConnected(new ComponentName("com.nextcloud.client", "AccountManagerService"), mock(IBinder.class)); + + verify(callback, never()).onConnected(); + } + + @Test + public void closeBeforeServiceConnectedUnbindsServiceConnection() { + final var connection = connectAndCaptureServiceConnection(); + + request.close(); + + verify(context).unbindService(connection); + } +}