From 032afbebabc2c918a254084e0b74a8d9db354635 Mon Sep 17 00:00:00 2001 From: Nan Date: Wed, 17 Jun 2026 11:15:30 -0700 Subject: [PATCH 1/4] fix: [SDK-4672] arm live location subscription on permission grant Enabling isShared before location permission was granted left the GMS listener subscribed via a requestLocationUpdates call that silently failed (SecurityException). A later start() short-circuited and only fired a one-shot last location, so live updates resumed only on the next app focus change. start() now re-arms the listener in the already-initialized branch, and the wrapper reports subscribe success so a failed request is retried rather than treated as active. Adds regression tests. Co-Authored-By: Cursor --- .../impl/FusedLocationApiWrapperImpl.kt | 8 ++- .../controller/impl/GmsLocationController.kt | 9 ++-- .../impl/IFusedLocationApiWrapper.kt | 2 +- .../controller/GmsLocationControllerTests.kt | 51 +++++++++++++++++++ .../mocks/FusedLocationApiWrapperMock.kt | 19 ++++++- 5 files changed, 81 insertions(+), 8 deletions(-) diff --git a/OneSignalSDK/onesignal/location/src/main/java/com/onesignal/location/internal/controller/impl/FusedLocationApiWrapperImpl.kt b/OneSignalSDK/onesignal/location/src/main/java/com/onesignal/location/internal/controller/impl/FusedLocationApiWrapperImpl.kt index b35cbfcb4c..69a9e3eba4 100644 --- a/OneSignalSDK/onesignal/location/src/main/java/com/onesignal/location/internal/controller/impl/FusedLocationApiWrapperImpl.kt +++ b/OneSignalSDK/onesignal/location/src/main/java/com/onesignal/location/internal/controller/impl/FusedLocationApiWrapperImpl.kt @@ -24,16 +24,20 @@ internal class FusedLocationApiWrapperImpl : IFusedLocationApiWrapper { googleApiClient: GoogleApiClient, locationRequest: LocationRequest, locationListener: LocationListener, - ) { - try { + ): Boolean { + return try { if (Looper.myLooper() == null) { Looper.prepare() } if (googleApiClient.isConnected) { LocationServices.FusedLocationApi.requestLocationUpdates(googleApiClient, locationRequest, locationListener) + true + } else { + false } } catch (t: Throwable) { Logging.warn("FusedLocationApi.requestLocationUpdates failed!", t) + false } } diff --git a/OneSignalSDK/onesignal/location/src/main/java/com/onesignal/location/internal/controller/impl/GmsLocationController.kt b/OneSignalSDK/onesignal/location/src/main/java/com/onesignal/location/internal/controller/impl/GmsLocationController.kt index 6c497ffc2f..6e7e3d362b 100644 --- a/OneSignalSDK/onesignal/location/src/main/java/com/onesignal/location/internal/controller/impl/GmsLocationController.kt +++ b/OneSignalSDK/onesignal/location/src/main/java/com/onesignal/location/internal/controller/impl/GmsLocationController.kt @@ -56,6 +56,10 @@ internal class GmsLocationController( setLocationAndFire(localLastLocation) } } + // Retry the subscription in case the first attempt failed because + // permission was not yet granted, otherwise it would only recover on + // the next app focus change. + locationUpdateListener?.refreshRequest() wasSuccessful = true } else { try { @@ -193,7 +197,7 @@ internal class GmsLocationController( } } - private fun refreshRequest() { + internal fun refreshRequest() { if (!googleApiClient.isConnected) { Logging.warn("Attempt to refresh location request but not currently connected!") return @@ -217,8 +221,7 @@ internal class GmsLocationController( .setMaxWaitTime((updateInterval * 1.5).toLong()) .setPriority(LocationRequest.PRIORITY_BALANCED_POWER_ACCURACY) Logging.debug("GMSLocationController GoogleApiClient requestLocationUpdates!") - _fusedLocationApiWrapper.requestLocationUpdates(googleApiClient, locationRequest, this) - hasExistingRequest = true + hasExistingRequest = _fusedLocationApiWrapper.requestLocationUpdates(googleApiClient, locationRequest, this) } override fun onLocationChanged(location: Location) { diff --git a/OneSignalSDK/onesignal/location/src/main/java/com/onesignal/location/internal/controller/impl/IFusedLocationApiWrapper.kt b/OneSignalSDK/onesignal/location/src/main/java/com/onesignal/location/internal/controller/impl/IFusedLocationApiWrapper.kt index 1854bac323..66184f96a5 100644 --- a/OneSignalSDK/onesignal/location/src/main/java/com/onesignal/location/internal/controller/impl/IFusedLocationApiWrapper.kt +++ b/OneSignalSDK/onesignal/location/src/main/java/com/onesignal/location/internal/controller/impl/IFusedLocationApiWrapper.kt @@ -15,7 +15,7 @@ internal interface IFusedLocationApiWrapper { googleApiClient: GoogleApiClient, locationRequest: LocationRequest, locationListener: LocationListener, - ) + ): Boolean fun getLastLocation(googleApiClient: GoogleApiClient): Location? } diff --git a/OneSignalSDK/onesignal/location/src/test/java/com/onesignal/location/internal/controller/GmsLocationControllerTests.kt b/OneSignalSDK/onesignal/location/src/test/java/com/onesignal/location/internal/controller/GmsLocationControllerTests.kt index 8ccc114419..9308daf630 100644 --- a/OneSignalSDK/onesignal/location/src/test/java/com/onesignal/location/internal/controller/GmsLocationControllerTests.kt +++ b/OneSignalSDK/onesignal/location/src/test/java/com/onesignal/location/internal/controller/GmsLocationControllerTests.kt @@ -173,4 +173,55 @@ class GmsLocationControllerTests : FunSpec({ ) } } + + // Regression: when isShared is enabled before location permission is granted, the + // GoogleApiClient connects (which does not require permission) but the first + // requestLocationUpdates fails. A later start() (e.g. triggered by the permission grant) + // previously short-circuited and only fired a one-shot last location, leaving the live + // subscription dead until the next app focus change. It must now re-arm on that start(). + test("start re-arms the live location subscription on a subsequent start") { + // Given + val location = Location("TEST_PROVIDER") + location.latitude = 123.45 + location.longitude = 678.91 + val fusedLocationApiWrapperMock = FusedLocationApiWrapperMock(listOf(location)) + + val applicationService = AndroidMockHelper.applicationService() + every { applicationService.isInForeground } returns true + val gmsLocationController = GmsLocationController(applicationService, fusedLocationApiWrapperMock) + + // When + val response1 = gmsLocationController.start() + val response2 = gmsLocationController.start() + + // Then + response1 shouldBe true + response2 shouldBe true + // Once on the initial init, and again when the second start re-arms the subscription. + fusedLocationApiWrapperMock.requestLocationUpdatesCallCount shouldBe 2 + } + + // Regression: a requestLocationUpdates that fails (e.g. a swallowed SecurityException + // when permission is missing) must not be recorded as an active request, otherwise + // close()/refresh would try to cancel a subscription that never existed. + test("a failed requestLocationUpdates is not treated as an active subscription") { + // Given + val location = Location("TEST_PROVIDER") + location.latitude = 123.45 + location.longitude = 678.91 + val fusedLocationApiWrapperMock = FusedLocationApiWrapperMock(listOf(location)) + fusedLocationApiWrapperMock.requestLocationUpdatesResult = false + + val applicationService = AndroidMockHelper.applicationService() + every { applicationService.isInForeground } returns true + val gmsLocationController = GmsLocationController(applicationService, fusedLocationApiWrapperMock) + + // When + gmsLocationController.start() + gmsLocationController.stop() + + // Then + fusedLocationApiWrapperMock.requestLocationUpdatesCallCount shouldBe 1 + fusedLocationApiWrapperMock.cancelLocationUpdatesCallCount shouldBe 0 + } }) diff --git a/OneSignalSDK/onesignal/location/src/test/java/com/onesignal/location/mocks/FusedLocationApiWrapperMock.kt b/OneSignalSDK/onesignal/location/src/test/java/com/onesignal/location/mocks/FusedLocationApiWrapperMock.kt index 89961b7a65..10bed5f7b6 100644 --- a/OneSignalSDK/onesignal/location/src/test/java/com/onesignal/location/mocks/FusedLocationApiWrapperMock.kt +++ b/OneSignalSDK/onesignal/location/src/test/java/com/onesignal/location/mocks/FusedLocationApiWrapperMock.kt @@ -11,6 +11,16 @@ import java.util.Queue internal class FusedLocationApiWrapperMock(locationsList: List) : IFusedLocationApiWrapper { private val locations: Queue + // Controls the value returned by [requestLocationUpdates] to simulate the subscription + // succeeding or failing (e.g. a swallowed SecurityException when permission is missing). + var requestLocationUpdatesResult: Boolean = true + + var requestLocationUpdatesCallCount: Int = 0 + private set + + var cancelLocationUpdatesCallCount: Int = 0 + private set + init { this.locations = LinkedList(locationsList) } @@ -18,13 +28,18 @@ internal class FusedLocationApiWrapperMock(locationsList: List) : IFus override fun cancelLocationUpdates( googleApiClient: GoogleApiClient, locationListener: LocationListener, - ) {} + ) { + cancelLocationUpdatesCallCount++ + } override fun requestLocationUpdates( googleApiClient: GoogleApiClient, locationRequest: LocationRequest, locationListener: LocationListener, - ) {} + ): Boolean { + requestLocationUpdatesCallCount++ + return requestLocationUpdatesResult + } override fun getLastLocation(googleApiClient: GoogleApiClient): Location? { return locations.remove() From 052e3d545a2a7b25646232cb8c9fc6936f36d33f Mon Sep 17 00:00:00 2001 From: Nan Date: Tue, 23 Jun 2026 13:46:04 -0700 Subject: [PATCH 2/4] test: cover failed-then-granted location re-registration Drive the real repro in one test: the first requestLocationUpdates fails (permission missing) and must not be recorded as active, then a later start() re-registers successfully and the now-active request is cancelled on stop. Asserts the state transition the call-count test only approximated. Co-authored-by: Cursor --- .../controller/GmsLocationControllerTests.kt | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/OneSignalSDK/onesignal/location/src/test/java/com/onesignal/location/internal/controller/GmsLocationControllerTests.kt b/OneSignalSDK/onesignal/location/src/test/java/com/onesignal/location/internal/controller/GmsLocationControllerTests.kt index 9308daf630..c611a4b477 100644 --- a/OneSignalSDK/onesignal/location/src/test/java/com/onesignal/location/internal/controller/GmsLocationControllerTests.kt +++ b/OneSignalSDK/onesignal/location/src/test/java/com/onesignal/location/internal/controller/GmsLocationControllerTests.kt @@ -224,4 +224,40 @@ class GmsLocationControllerTests : FunSpec({ fusedLocationApiWrapperMock.requestLocationUpdatesCallCount shouldBe 1 fusedLocationApiWrapperMock.cancelLocationUpdatesCallCount shouldBe 0 } + + // Regression for the full repro: the first requestLocationUpdates fails because permission + // is missing (swallowed SecurityException), then a later start() (triggered by the grant) + // must re-register. It must not cancel the never-active first attempt, and the now-active + // request must be cancelled on stop, proving the re-arm actually took effect. + test("start re-registers a previously failed request once it can succeed") { + // Given the first requestLocationUpdates fails (permission not yet granted) + val location = Location("TEST_PROVIDER") + location.latitude = 123.45 + location.longitude = 678.91 + val fusedLocationApiWrapperMock = FusedLocationApiWrapperMock(listOf(location)) + fusedLocationApiWrapperMock.requestLocationUpdatesResult = false + + val applicationService = AndroidMockHelper.applicationService() + every { applicationService.isInForeground } returns true + val gmsLocationController = GmsLocationController(applicationService, fusedLocationApiWrapperMock) + + // When the first start cannot register for updates + gmsLocationController.start() + + // Then the failed request is not recorded as active, so there is nothing to cancel + fusedLocationApiWrapperMock.requestLocationUpdatesCallCount shouldBe 1 + fusedLocationApiWrapperMock.cancelLocationUpdatesCallCount shouldBe 0 + + // When permission is granted and start() runs again + fusedLocationApiWrapperMock.requestLocationUpdatesResult = true + gmsLocationController.start() + + // Then it re-registers without cancelling the never-active first attempt + fusedLocationApiWrapperMock.requestLocationUpdatesCallCount shouldBe 2 + fusedLocationApiWrapperMock.cancelLocationUpdatesCallCount shouldBe 0 + + // And the now-active request is cancelled on stop, confirming the re-arm took effect + gmsLocationController.stop() + fusedLocationApiWrapperMock.cancelLocationUpdatesCallCount shouldBe 1 + } }) From 59df1838cdfe9baf63b64ab206856c5b99350e34 Mon Sep 17 00:00:00 2001 From: Nan Date: Tue, 23 Jun 2026 14:02:45 -0700 Subject: [PATCH 3/4] fix: serialize location-updates refresh to remove hasExistingRequest race refreshRequest() and close() run on both the start()/stop() coroutine path and the main-thread onFocus()/onUnfocused() lifecycle callbacks. Guard the cancel/register sequence and the hasExistingRequest flag with a dedicated lock so the check-then-act is atomic and visible across threads, instead of relying on an unsynchronized var. Co-authored-by: Cursor --- .../controller/impl/GmsLocationController.kt | 24 ++++++++++++------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/OneSignalSDK/onesignal/location/src/main/java/com/onesignal/location/internal/controller/impl/GmsLocationController.kt b/OneSignalSDK/onesignal/location/src/main/java/com/onesignal/location/internal/controller/impl/GmsLocationController.kt index 6e7e3d362b..70b4081b7b 100644 --- a/OneSignalSDK/onesignal/location/src/main/java/com/onesignal/location/internal/controller/impl/GmsLocationController.kt +++ b/OneSignalSDK/onesignal/location/src/main/java/com/onesignal/location/internal/controller/impl/GmsLocationController.kt @@ -168,6 +168,8 @@ internal class GmsLocationController( private val googleApiClient: GoogleApiClient, private val _fusedLocationApiWrapper: IFusedLocationApiWrapper, ) : LocationListener, IApplicationLifecycleHandler, Closeable { + // Guards hasExistingRequest and the cancel/register sequence against concurrent threads. + private val requestLock = Any() private var hasExistingRequest = false init { @@ -192,8 +194,11 @@ internal class GmsLocationController( override fun close() { _applicationService.removeApplicationLifecycleHandler(this) - if (hasExistingRequest) { - _fusedLocationApiWrapper.cancelLocationUpdates(googleApiClient, this) + synchronized(requestLock) { + if (hasExistingRequest) { + _fusedLocationApiWrapper.cancelLocationUpdates(googleApiClient, this) + hasExistingRequest = false + } } } @@ -203,10 +208,6 @@ internal class GmsLocationController( return } - if (hasExistingRequest) { - _fusedLocationApiWrapper.cancelLocationUpdates(googleApiClient, this) - } - val updateInterval = if (_applicationService.isInForeground) { LocationConstants.FOREGROUND_UPDATE_TIME_MS @@ -220,8 +221,15 @@ internal class GmsLocationController( .setInterval(updateInterval) .setMaxWaitTime((updateInterval * 1.5).toLong()) .setPriority(LocationRequest.PRIORITY_BALANCED_POWER_ACCURACY) - Logging.debug("GMSLocationController GoogleApiClient requestLocationUpdates!") - hasExistingRequest = _fusedLocationApiWrapper.requestLocationUpdates(googleApiClient, locationRequest, this) + + synchronized(requestLock) { + if (hasExistingRequest) { + _fusedLocationApiWrapper.cancelLocationUpdates(googleApiClient, this) + } + Logging.debug("GMSLocationController GoogleApiClient requestLocationUpdates!") + hasExistingRequest = + _fusedLocationApiWrapper.requestLocationUpdates(googleApiClient, locationRequest, this) + } } override fun onLocationChanged(location: Location) { From c395a43a43ce983471b54f2d9e650b164a99ef43 Mon Sep 17 00:00:00 2001 From: Nan Date: Tue, 23 Jun 2026 14:20:52 -0700 Subject: [PATCH 4/4] fix: only re-arm location updates when none is active to avoid churn The early start() path now re-registers for location updates only when there is no active request, so a successful registration is left in place on repeated start() calls (e.g. toggling isShared or permission callbacks) instead of being cancelled and re-registered (which also reset the interval timer). Recovery after a failed first attempt is preserved because the flag is false in that case, and the focus-driven refresh path still re-registers unconditionally to pick up the new interval. Co-Authored-By: Cursor --- .../controller/impl/GmsLocationController.kt | 10 +++++----- .../controller/GmsLocationControllerTests.kt | 14 +++++--------- 2 files changed, 10 insertions(+), 14 deletions(-) diff --git a/OneSignalSDK/onesignal/location/src/main/java/com/onesignal/location/internal/controller/impl/GmsLocationController.kt b/OneSignalSDK/onesignal/location/src/main/java/com/onesignal/location/internal/controller/impl/GmsLocationController.kt index 70b4081b7b..137449cc1e 100644 --- a/OneSignalSDK/onesignal/location/src/main/java/com/onesignal/location/internal/controller/impl/GmsLocationController.kt +++ b/OneSignalSDK/onesignal/location/src/main/java/com/onesignal/location/internal/controller/impl/GmsLocationController.kt @@ -56,10 +56,8 @@ internal class GmsLocationController( setLocationAndFire(localLastLocation) } } - // Retry the subscription in case the first attempt failed because - // permission was not yet granted, otherwise it would only recover on - // the next app focus change. - locationUpdateListener?.refreshRequest() + // Re-register here if the first attempt failed + locationUpdateListener?.refreshRequest(onlyIfInactive = true) wasSuccessful = true } else { try { @@ -202,7 +200,7 @@ internal class GmsLocationController( } } - internal fun refreshRequest() { + internal fun refreshRequest(onlyIfInactive: Boolean = false) { if (!googleApiClient.isConnected) { Logging.warn("Attempt to refresh location request but not currently connected!") return @@ -224,6 +222,8 @@ internal class GmsLocationController( synchronized(requestLock) { if (hasExistingRequest) { + // Don't tear down an already-active request when only recovering a failed one. + if (onlyIfInactive) return@synchronized _fusedLocationApiWrapper.cancelLocationUpdates(googleApiClient, this) } Logging.debug("GMSLocationController GoogleApiClient requestLocationUpdates!") diff --git a/OneSignalSDK/onesignal/location/src/test/java/com/onesignal/location/internal/controller/GmsLocationControllerTests.kt b/OneSignalSDK/onesignal/location/src/test/java/com/onesignal/location/internal/controller/GmsLocationControllerTests.kt index c611a4b477..f60066c4f8 100644 --- a/OneSignalSDK/onesignal/location/src/test/java/com/onesignal/location/internal/controller/GmsLocationControllerTests.kt +++ b/OneSignalSDK/onesignal/location/src/test/java/com/onesignal/location/internal/controller/GmsLocationControllerTests.kt @@ -174,12 +174,8 @@ class GmsLocationControllerTests : FunSpec({ } } - // Regression: when isShared is enabled before location permission is granted, the - // GoogleApiClient connects (which does not require permission) but the first - // requestLocationUpdates fails. A later start() (e.g. triggered by the permission grant) - // previously short-circuited and only fired a one-shot last location, leaving the live - // subscription dead until the next app focus change. It must now re-arm on that start(). - test("start re-arms the live location subscription on a subsequent start") { + // Repeated start() on a healthy request must not cancel + re-register it (which resets the interval). + test("start does not re-register an already-active request") { // Given val location = Location("TEST_PROVIDER") location.latitude = 123.45 @@ -194,11 +190,11 @@ class GmsLocationControllerTests : FunSpec({ val response1 = gmsLocationController.start() val response2 = gmsLocationController.start() - // Then + // Then the active request is left alone: not re-registered, not cancelled response1 shouldBe true response2 shouldBe true - // Once on the initial init, and again when the second start re-arms the subscription. - fusedLocationApiWrapperMock.requestLocationUpdatesCallCount shouldBe 2 + fusedLocationApiWrapperMock.requestLocationUpdatesCallCount shouldBe 1 + fusedLocationApiWrapperMock.cancelLocationUpdatesCallCount shouldBe 0 } // Regression: a requestLocationUpdates that fails (e.g. a swallowed SecurityException