fix: [SDK-4672] start location updates when permission is granted

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
         }
     }
 

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) {

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?
 }

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
+    }
 })

OneSignalSDK/onesignal/location/src/test/java/com/onesignal/location/mocks/FusedLocationApiWrapperMock.kt

@@ -11,20 +11,35 @@ import java.util.Queue
 internal class FusedLocationApiWrapperMock(locationsList: List<Location>) : IFusedLocationApiWrapper {
     private val locations: Queue<Location>
 
+    // 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)
     }
 
     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()