Merge pull request #812 from ably/memory_leak_issue

Fix memory leak issue

common/src/main/java/com/ably/tracking/common/Ably.kt

@@ -546,6 +546,8 @@ constructor(
         }
         channelToRemove.unsubscribe()
         channelToRemove.presence.unsubscribe()
+        // TODO: Can possibly be removed once https://github.com/ably/ably-java/issues/919 is resolved
+        channelToRemove.off()
         ably.channels.release(channelToRemove.name)
     }
 
@@ -1049,12 +1051,15 @@ constructor(
             withTimeout(STOP_CONNECTION_MAXIMUM_DURATION_IN_MILLISECONDS) {
                 closeSuspending()
             }
+            ably.connection.off()
             Result.success(Unit)
         } catch (connectionException: ConnectionException) {
             logHandler?.w("$TAG Failed to stop Ably connection", connectionException)
+            ably.connection.off()
             Result.failure(connectionException)
         } catch (exception: TimeoutCancellationException) {
             logHandler?.w("$TAG Stop Ably connection timed out", exception)
+            ably.connection.off()
             Result.failure(exception)
         }
     }

common/src/main/java/com/ably/tracking/common/AblySdk.kt

@@ -110,5 +110,6 @@ interface AblySdkRealtime<ChannelStateListenerType : AblySdkChannelStateListener
 
         fun on(listener: ConnectionStateListener)
         fun off(listener: ConnectionStateListener)
+        fun off()
     }
 }

common/src/main/java/com/ably/tracking/common/DefaultAblySdk.kt

@@ -91,6 +91,10 @@ constructor(clientOptions: ClientOptions) : AblySdkRealtime<DefaultAblySdkChanne
         override fun off(listener: ConnectionStateListener) {
             connection.off(listener)
         }
+
+        override fun off() {
+            connection.off()
+        }
     }
 
     class Channels

common/src/test/java/com/ably/tracking/common/DefaultAblyTests.kt

@@ -484,6 +484,7 @@ class DefaultAblyTests {
          * ...and calls `get` on the Channels instance...
          * ...and tells the channel to leave presence...
          * ...and calls `unsubscribe` on the channel and on its Presence instance...
+         * ...and calls `off` on the channel to unregister all listeners...
          * ...and fetches the channel’s name and calls `release` on the Channels instance...
          * ...and the call to `disconnect` (on the object under test) succeeds.
          */
@@ -518,6 +519,7 @@ class DefaultAblyTests {
          * ...and calls `get` on the Channels instance...
          * ...and tells the channel to leave presence...
          * ...and calls `unsubscribe` on the channel and on its Presence instance...
+         * ...and calls `off` on the channel to unregister all listeners...
          * ...and fetches the channel’s name and calls `release` on the Channels instance...
          * ...and the call to `disconnect` (on the object under test) succeeds.
          */
@@ -560,6 +562,7 @@ class DefaultAblyTests {
          * ...and calls `get` on the Channels instance...
          * ...and tells the channel to leave presence...
          * ...and calls `unsubscribe` on the channel and on its Presence instance...
+         * ...and calls `off` on the channel to unregister all listeners...
          * ...and fetches the channel’s name and calls `release` on the Channels instance...
          * ...and the call to `disconnect` (on the object under test) succeeds.
          */
@@ -1646,6 +1649,7 @@ class DefaultAblyTests {
             configuredChannel.mockSuccessfulPresenceLeave()
             configuredChannel.stubUnsubscribe()
             configuredChannel.stubPresenceUnsubscribe()
+            configuredChannel.stubOffAll()
             testEnvironment.stubRelease(configuredChannel)
         }
 
@@ -1711,6 +1715,7 @@ class DefaultAblyTests {
             }
             configuredChannel.stubUnsubscribe()
             configuredChannel.stubPresenceUnsubscribe()
+            configuredChannel.stubOffAll()
             testEnvironment.stubRelease(configuredChannel)
         }
 

common/src/test/java/com/ably/tracking/common/helper/DefaultAblyTestEnvironment.kt

@@ -323,6 +323,13 @@ class DefaultAblyTestEnvironment private constructor(
             every { channelMock.off(any()) } returns Unit
         }
 
+        /**
+         * Stubs [channelMock]’s [AblySdkRealtime.Channel.off] method (the overload with no listeners).
+         */
+        fun stubOffAll() {
+            every { channelMock.off() } returns Unit
+        }
+
         /**
          * Mocks [channelMock]’s [AblySdkRealtime.Channel.on] method to capture the received [AblySdkChannelStateListener], and to then immediately call this listener with a [AblySdkChannelStateListener.ChannelStateChange] MockK mock constructed from the [current] argument.
          *
@@ -516,7 +523,10 @@ class DefaultAblyTestEnvironment private constructor(
      * Stubs [connectionMock]’s [AblySdkRealtime.Connection.off] method.
      */
     fun stubConnectionOff() {
-        every { connectionMock.off(any()) } returns Unit
+        every {
+            connectionMock.off(any())
+            connectionMock.off()
+        } returns Unit
     }
 
     /**

common/src/test/java/com/ably/tracking/common/helper/DefaultAblyTestScenarios.kt

@@ -932,6 +932,7 @@ class DefaultAblyTestScenarios {
          * ...and calls `get` on the Channels instance...
          * ...and tells the channel to leave presence...
          * ...and calls `unsubscribe` on the channel and on its Presence instance...
+         * ...and calls `off` on the channel to unregister all listeners...
          * ...and fetches the channel’s name and calls `release` on the Channels instance...
          * }
          *
@@ -989,6 +990,7 @@ class DefaultAblyTestScenarios {
 
                 configuredChannel.stubUnsubscribe()
                 configuredChannel.stubPresenceUnsubscribe()
+                configuredChannel.stubOffAll()
                 configuredChannel.mockName()
                 testEnvironment.stubRelease(configuredChannel)
 
@@ -1012,6 +1014,7 @@ class DefaultAblyTestScenarios {
                          * ...and calls `get` on the Channels instance...
                          * ...and tells the channel to leave presence...
                          * ...and calls `unsubscribe` on the channel and on its Presence instance...
+                         * ...and calls `off` on the channel to unregister all listeners...
                          * ...and fetches the channel’s name and calls `release` on the Channels instance...
                          * }
                          */
@@ -1021,6 +1024,7 @@ class DefaultAblyTestScenarios {
 
                         configuredChannel.channelMock.unsubscribe()
                         configuredChannel.presenceMock.unsubscribe()
+                        configuredChannel.channelMock.off()
                         configuredChannel.channelMock.name
                         testEnvironment.channelsMock.release(configuredChannel.channelName)
                     }
@@ -1369,6 +1373,8 @@ class DefaultAblyTestScenarios {
                          */
                         testEnvironment.connectionMock.off(any())
                     }
+
+                    testEnvironment.connectionMock.off()
                 }
 
                 /* when ${thenConfig.resultOfStopConnectionCallOnObjectUnderTest} is Terminates and ${thenConfig.resultOfStopConnectionCallOnObjectUnderTest.result} is Success {

publishing-example-app/build.gradle

@@ -27,4 +27,5 @@ dependencies {
     implementation 'pub.devrel:easypermissions:3.0.0'
     // This version needs to be compatible with the "com.mapbox.navigation:core" dependency version from publishing-sdk.
     implementation 'com.mapbox.maps:android:10.10.0'
+    debugImplementation 'com.squareup.leakcanary:leakcanary-android:2.10'
 }

publishing-example-app/src/main/java/com/ably/tracking/example/publisher/MainActivity.kt

@@ -95,6 +95,7 @@ class MainActivity : PublisherServiceActivity() {
             startAndBindPublisherService()
         } else {
             if (trackablesAdapter.trackables.isEmpty()) {
+                trackablesUpdateJob?.cancel()
                 stopPublisherService()
             } else {
                 showCannotStopServiceDialog()

publishing-example-app/src/main/java/com/ably/tracking/example/publisher/PublisherService.kt

@@ -20,11 +20,13 @@ import com.ably.tracking.publisher.Publisher
 import com.ably.tracking.publisher.PublisherNotificationProvider
 import kotlinx.coroutines.CoroutineScope
 import kotlinx.coroutines.Dispatchers
+import kotlinx.coroutines.Job
 import kotlinx.coroutines.SupervisorJob
 import kotlinx.coroutines.flow.launchIn
 import kotlinx.coroutines.flow.onEach
 import kotlinx.coroutines.launch
 import timber.log.Timber
+import java.lang.ref.WeakReference
 
 // The public token for the Mapbox SDK. For more details see the README.
 private const val MAPBOX_ACCESS_TOKEN = BuildConfig.MAPBOX_ACCESS_TOKEN
@@ -40,9 +42,10 @@ class PublisherService : Service() {
     private val scope = CoroutineScope(Dispatchers.Main + SupervisorJob())
     private val NOTIFICATION_ID = 5235
     private lateinit var notification: Notification
-    private val binder = Binder()
+    private val binder = Binder(WeakReference(this))
     var publisher: Publisher? = null
     private lateinit var appPreferences: AppPreferences
+    private var locationUpdateJob: Job? = null
 
     override fun onCreate() {
         super.onCreate()
@@ -61,16 +64,19 @@ class PublisherService : Service() {
         return START_NOT_STICKY
     }
 
-    inner class Binder : android.os.Binder() {
-        fun getService(): PublisherService = this@PublisherService
+    class Binder(private val weakService: WeakReference<PublisherService>) : android.os.Binder() {
+        fun getService(): PublisherService? = weakService.get()
     }
 
     override fun onBind(intent: Intent?): IBinder = binder
 
     override fun onDestroy() {
         // We want to be sure that after the service is stopped the publisher is stopped too.
         // Otherwise we could end up with multiple active publishers.
+        Timber.d("PublisherService onDestroy")
         scope.launch { publisher?.stop() }
+        locationUpdateJob?.cancel()
+        locationUpdateJob = null
         super.onDestroy()
     }
 
@@ -84,16 +90,23 @@ class PublisherService : Service() {
     fun startPublisher(locationSource: LocationSource? = null) {
         if (!isPublisherStarted) {
             publisher = createPublisher(locationSource).apply {
-                locationHistory
+                locationUpdateJob = locationHistory
                     .onEach { uploadLocationHistoryData(it) }
                     .launchIn(scope)
             }
         }
     }
 
+    /**
+     * In this method, we take a clone of the notification used by the service.
+     *
+     * This is to prevent a leaking issue, whereby the service would be kept alive
+     * by a synthetic lambda in mapbox if the [notification] member were used directly.
+     */
     @RequiresPermission(anyOf = [Manifest.permission.ACCESS_COARSE_LOCATION, Manifest.permission.ACCESS_FINE_LOCATION])
-    private fun createPublisher(locationSource: LocationSource?): Publisher =
-        Publisher.publishers()
+    private fun createPublisher(locationSource: LocationSource?): Publisher {
+        val providedNotification = notification.clone()
+        return Publisher.publishers()
             .connection(ConnectionConfiguration(Authentication.basic(CLIENT_ID, ABLY_API_KEY)))
             .map(MapConfiguration(MAPBOX_ACCESS_TOKEN))
             .locationSource(locationSource)
@@ -113,7 +126,7 @@ class PublisherService : Service() {
             })
             .backgroundTrackingNotificationProvider(
                 object : PublisherNotificationProvider {
-                    override fun getNotification(): Notification = notification
+                    override fun getNotification(): Notification = providedNotification
                 },
                 NOTIFICATION_ID
             )
@@ -122,6 +135,7 @@ class PublisherService : Service() {
             .constantLocationEngineResolution(createConstantLocationEngineResolution())
             .vehicleProfile(appPreferences.getVehicleProfile().toAssetTracking())
             .start()
+    }
 
     private fun createDefaultResolution(): Resolution =
         Resolution(

publishing-example-app/src/main/java/com/ably/tracking/example/publisher/PublisherServiceActivity.kt

@@ -14,7 +14,7 @@ abstract class PublisherServiceActivity : AppCompatActivity() {
     protected var publisherService: PublisherService? = null
     private val publisherServiceConnection = object : ServiceConnection {
         override fun onServiceConnected(className: ComponentName, serviceBinder: IBinder) {
-            (serviceBinder as PublisherService.Binder).getService().let { service ->
+            (serviceBinder as PublisherService.Binder).getService()?.let { service ->
                 publisherService = service
                 onPublisherServiceConnected(service)
             }
@@ -70,6 +70,7 @@ abstract class PublisherServiceActivity : AppCompatActivity() {
      */
     protected fun stopPublisherService() {
         stopService(createServiceIntent())
+        publisherService = null
     }
 
     private fun bindPublisherService() {

publishing-example-app/src/main/java/com/ably/tracking/example/publisher/TrackableDetailsActivity.kt

@@ -6,6 +6,7 @@ import android.os.Bundle
 import android.view.View
 import androidx.appcompat.app.AlertDialog
 import androidx.core.content.ContextCompat
+import androidx.lifecycle.lifecycleScope
 import com.ably.tracking.Location
 import com.ably.tracking.TrackableState
 import com.ably.tracking.publisher.Trackable
@@ -54,13 +55,13 @@ class TrackableDetailsActivity : PublisherServiceActivity() {
         publisherService?.publisher?.apply {
             getTrackableState(trackableId)
                 ?.onEach { updateAssetStateInfo(it) }
-                ?.launchIn(scope)
+                ?.launchIn(lifecycleScope)
             locations
                 .onEach { updateLocationInfo(it.location) }
-                .launchIn(scope)
+                .launchIn(lifecycleScope)
             trackables
                 .onEach { this@TrackableDetailsActivity.trackables = it }
-                .launchIn(scope)
+                .launchIn(lifecycleScope)
         }
     }
 

publishing-sdk/src/main/java/com/ably/tracking/publisher/BatteryDataProvider.kt

@@ -5,6 +5,7 @@ import android.content.Intent
 import android.content.IntentFilter
 import android.os.BatteryManager
 import androidx.core.math.MathUtils.clamp
+import java.lang.ref.WeakReference
 
 internal interface BatteryDataProvider {
 
@@ -20,12 +21,15 @@ internal interface BatteryDataProvider {
 /**
  * Based on https://developer.android.com/training/monitoring-device-state/battery-monitoring
  */
-internal class DefaultBatteryDataProvider(private val context: Context) : BatteryDataProvider {
+internal class DefaultBatteryDataProvider(context: Context) : BatteryDataProvider {
+    private val weakContext: WeakReference<Context>
+    init {
+        weakContext = WeakReference(context)
+    }
     private val MINIMUM_BATTERY_PERCENTAGE = 0.0f
     private val MAXIMUM_BATTERY_PERCENTAGE = 100.0f
 
-    override fun getCurrentBatteryPercentage(): Float? =
-        getCurrentBatteryPercentage(context)
+    override fun getCurrentBatteryPercentage(): Float? = weakContext.get()?.let { getCurrentBatteryPercentage(it) }
 
     private fun getCurrentBatteryPercentage(context: Context): Float? =
         getCurrentBatteryStatusIntent(context)?.let { intent ->

publishing-sdk/src/main/java/com/ably/tracking/publisher/DefaultResolutionPolicyFactory.kt

@@ -13,7 +13,10 @@ class DefaultResolutionPolicyFactory(
         hooks: ResolutionPolicy.Hooks,
         methods: ResolutionPolicy.Methods
     ): ResolutionPolicy {
-        return DefaultResolutionPolicy(hooks, methods, defaultResolution, DefaultBatteryDataProvider(context))
+        return DefaultResolutionPolicy(
+            hooks, methods, defaultResolution,
+            DefaultBatteryDataProvider(context)
+        )
     }
 }
 

publishing-sdk/src/main/java/com/ably/tracking/publisher/Mapbox.kt

@@ -414,6 +414,7 @@ internal class DefaultMapbox(
             }
 
             if (MapboxInstanceProvider.destroyIfPossible()) {
+                Mapbox_TripNotificationModuleConfiguration.moduleProvider = null
                 logHandler?.v("$TAG Destroyed the MapboxNavigation instance")
             }
         }

subscribing-example-app/build.gradle

@@ -21,4 +21,5 @@ dependencies {
     implementation project(':ui-sdk')
     implementation 'com.google.android.gms:play-services-maps:17.0.0'
     implementation 'androidx.fragment:fragment:1.4.1'
+    debugImplementation 'com.squareup.leakcanary:leakcanary-android:2.10'
 }