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..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,6 +56,8 @@ internal class GmsLocationController( setLocationAndFire(localLastLocation) } } + // Re-register here if the first attempt failed + locationUpdateListener?.refreshRequest(onlyIfInactive = true) wasSuccessful = true } else { try { @@ -164,6 +166,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 { @@ -188,21 +192,20 @@ 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 + } } } - private fun refreshRequest() { + internal fun refreshRequest(onlyIfInactive: Boolean = false) { if (!googleApiClient.isConnected) { Logging.warn("Attempt to refresh location request but not currently connected!") return } - if (hasExistingRequest) { - _fusedLocationApiWrapper.cancelLocationUpdates(googleApiClient, this) - } - val updateInterval = if (_applicationService.isInForeground) { LocationConstants.FOREGROUND_UPDATE_TIME_MS @@ -216,9 +219,17 @@ internal class GmsLocationController( .setInterval(updateInterval) .setMaxWaitTime((updateInterval * 1.5).toLong()) .setPriority(LocationRequest.PRIORITY_BALANCED_POWER_ACCURACY) - Logging.debug("GMSLocationController GoogleApiClient requestLocationUpdates!") - _fusedLocationApiWrapper.requestLocationUpdates(googleApiClient, locationRequest, this) - hasExistingRequest = true + + 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!") + 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..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 @@ -173,4 +173,87 @@ class GmsLocationControllerTests : FunSpec({ ) } } + + // 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 + 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 the active request is left alone: not re-registered, not cancelled + response1 shouldBe true + response2 shouldBe true + fusedLocationApiWrapperMock.requestLocationUpdatesCallCount shouldBe 1 + fusedLocationApiWrapperMock.cancelLocationUpdatesCallCount shouldBe 0 + } + + // 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 + } + + // 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 + } }) 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()