Fix memory leak on render destroy (#426)

sdk/src/main/java/com/mapbox/maps/MapView.kt

@@ -19,7 +19,6 @@ import com.mapbox.maps.renderer.MapboxSurfaceHolderRenderer
 import com.mapbox.maps.renderer.MapboxTextureViewRenderer
 import com.mapbox.maps.renderer.OnFpsChangedListener
 import com.mapbox.maps.renderer.egl.EGLCore
-import java.lang.ref.WeakReference
 
 /**
  * A [MapView] provides an embeddable map interface.
@@ -92,7 +91,7 @@ open class MapView : FrameLayout, MapPluginProviderDelegate, MapControllable {
     mapController = MapController(
       when (view) {
         is SurfaceView -> MapboxSurfaceHolderRenderer(view.holder)
-        is TextureView -> MapboxTextureViewRenderer(WeakReference(view))
+        is TextureView -> MapboxTextureViewRenderer(view)
         else -> throw IllegalArgumentException("Provided view has to be a texture or a surface.")
       },
       resolvedMapInitOptions

sdk/src/main/java/com/mapbox/maps/renderer/MapboxRenderThread.kt

@@ -256,6 +256,9 @@ internal class MapboxRenderThread : Choreographer.FrameCallback {
         }
       }
       destroyCondition.await()
+      if (mapboxRenderer.needDestroy) {
+        destroy()
+      }
     }
   }
 
@@ -346,7 +349,7 @@ internal class MapboxRenderThread : Choreographer.FrameCallback {
   }
 
   @UiThread
-  fun destroy() {
+  internal fun destroy() {
     handlerThread.apply {
       clearMessageQueue()
       stop()

sdk/src/main/java/com/mapbox/maps/renderer/MapboxRenderer.kt

@@ -28,10 +28,12 @@ internal abstract class MapboxRenderer : MapClient() {
   @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
   internal var pixelReader: PixelReader? = null
 
+  internal var needDestroy = false
+
   @UiThread
-  @CallSuper
-  open fun onDestroy() {
-    // no-op
+  fun onDestroy() {
+    // we destroy and stop thread after surface or texture is destroyed
+    needDestroy = true
   }
 
   @AnyThread

sdk/src/main/java/com/mapbox/maps/renderer/MapboxSurfaceHolderRenderer.kt

@@ -2,21 +2,15 @@ package com.mapbox.maps.renderer
 
 import android.view.SurfaceHolder
 import androidx.annotation.VisibleForTesting
-import java.lang.ref.WeakReference
 
 internal class MapboxSurfaceHolderRenderer : MapboxSurfaceRenderer, SurfaceHolder.Callback {
 
-  private var surfaceHolder: WeakReference<SurfaceHolder>
-
-  constructor(surfaceHolder: SurfaceHolder?) : super() {
-    this.surfaceHolder = WeakReference(surfaceHolder)
-    surfaceHolder?.addCallback(this)
+  constructor(surfaceHolder: SurfaceHolder) : super() {
+    surfaceHolder.addCallback(this)
   }
 
   @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
-  internal constructor(surfaceHolder: SurfaceHolder?, renderThread: MapboxRenderThread) : super() {
-    this.surfaceHolder = WeakReference(surfaceHolder)
-  }
+  internal constructor(renderThread: MapboxRenderThread) : super(renderThread)
 
   override fun surfaceChanged(holder: SurfaceHolder?, format: Int, width: Int, height: Int) {
     holder?.let {
@@ -31,9 +25,4 @@ internal class MapboxSurfaceHolderRenderer : MapboxSurfaceRenderer, SurfaceHolde
   override fun surfaceCreated(holder: SurfaceHolder?) {
     super.surfaceCreated()
   }
-
-  override fun onDestroy() {
-    super.onDestroy()
-    surfaceHolder.get()?.removeCallback(this)
-  }
 }

sdk/src/main/java/com/mapbox/maps/renderer/MapboxSurfaceRenderer.kt

@@ -41,9 +41,4 @@ internal open class MapboxSurfaceRenderer : MapboxRenderer {
   fun surfaceCreated() {
     createSurface = true
   }
-
-  override fun onDestroy() {
-    super.onDestroy()
-    renderThread.destroy()
-  }
 }

sdk/src/main/java/com/mapbox/maps/renderer/MapboxTextureViewRenderer.kt

@@ -4,27 +4,22 @@ import android.graphics.SurfaceTexture
 import android.view.Surface
 import android.view.TextureView
 import androidx.annotation.VisibleForTesting
-import java.lang.ref.WeakReference
 
 internal class MapboxTextureViewRenderer : MapboxRenderer, TextureView.SurfaceTextureListener {
 
-  private val textureView: WeakReference<TextureView>
-
-  constructor(textureView: WeakReference<TextureView>) {
-    this.textureView = textureView
+  constructor(textureView: TextureView) {
     renderThread = MapboxRenderThread(
       mapboxRenderer = this,
       translucentSurface = true
     )
-    textureView.get()?.let {
+    textureView.let {
       it.isOpaque = false
       it.surfaceTextureListener = this
     }
   }
 
   @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
-  internal constructor(textureView: WeakReference<TextureView>, renderThread: MapboxRenderThread) {
-    this.textureView = textureView
+  internal constructor(renderThread: MapboxRenderThread) {
     this.renderThread = renderThread
   }
 
@@ -38,6 +33,7 @@ internal class MapboxTextureViewRenderer : MapboxRenderer, TextureView.SurfaceTe
 
   override fun onSurfaceTextureDestroyed(surface: SurfaceTexture?): Boolean {
     renderThread.onSurfaceDestroyed()
+    surface?.release()
     return true
   }
 
@@ -48,10 +44,4 @@ internal class MapboxTextureViewRenderer : MapboxRenderer, TextureView.SurfaceTe
       height = height
     )
   }
-
-  override fun onDestroy() {
-    super.onDestroy()
-    textureView.get()?.surfaceTextureListener = null
-    renderThread.destroy()
-  }
 }

sdk/src/test/java/com/mapbox/maps/renderer/MapboxRenderThreadTest.kt

@@ -125,28 +125,46 @@ class MapboxRenderThreadTest {
   }
 
   @Test
-  fun onSurfaceDestroyedTest() {
+  fun onSurfaceOnlyDestroyedTest() {
     val latch = CountDownLatch(1)
     every { mapboxRenderer.onSurfaceDestroyed() } answers { latch.countDown() }
+    every { mapboxRenderer.needDestroy } returns false
     val surface = mockk<Surface>(relaxUnitFun = true)
     every { surface.isValid } returns true
     every { eglCore.eglStatusSuccess } returns true
     every { eglCore.createWindowSurface(any()) } returns mockk(relaxed = true)
     mapboxRenderThread.onSurfaceCreated(surface, 1, 1)
     mapboxRenderThread.onSurfaceDestroyed()
-    Shadows.shadowOf(workerThread.handler?.looper).idle()
     if (!latch.await(waitTime, TimeUnit.MILLISECONDS)) {
       throw TimeoutException()
     }
-    verify {
-      mapboxRenderer.onSurfaceDestroyed()
+    verify { mapboxRenderer.onSurfaceDestroyed() }
+    assert(workerThread.handlerThread.isAlive)
+  }
+
+  @Test
+  fun onSurfaceWithActivityDestroyedTest() {
+    val latch = CountDownLatch(1)
+    every { mapboxRenderer.onSurfaceDestroyed() } answers { latch.countDown() }
+    every { mapboxRenderer.needDestroy } returns true
+    val surface = mockk<Surface>(relaxUnitFun = true)
+    every { surface.isValid } returns true
+    every { eglCore.eglStatusSuccess } returns true
+    every { eglCore.createWindowSurface(any()) } returns mockk(relaxed = true)
+    mapboxRenderThread.onSurfaceCreated(surface, 1, 1)
+    mapboxRenderThread.onSurfaceDestroyed()
+    if (!latch.await(waitTime, TimeUnit.MILLISECONDS)) {
+      throw TimeoutException()
     }
+    verify { mapboxRenderer.onSurfaceDestroyed() }
+    assert(!workerThread.handlerThread.isAlive)
   }
 
   @Test
   fun onSurfaceDestroyedWithRenderCallAfterTest() {
     val latch = CountDownLatch(1)
     every { mapboxRenderer.onSurfaceDestroyed() } answers { latch.countDown() }
+    every { mapboxRenderer.needDestroy } returns false
     val surface = mockk<Surface>(relaxUnitFun = true)
     every { surface.isValid } returns true
     every { eglCore.eglStatusSuccess } returns true

sdk/src/test/java/com/mapbox/maps/renderer/MapboxRendererTest.kt

@@ -14,6 +14,7 @@ import io.mockk.every
 import io.mockk.mockk
 import io.mockk.slot
 import io.mockk.verify
+import org.junit.Assert
 import org.junit.Before
 import org.junit.Test
 import org.junit.runner.RunWith
@@ -173,4 +174,10 @@ internal abstract class MapboxRendererTest {
     Shadows.shadowOf(handlerThread.looper).idle()
     Shadows.shadowOf(Looper.getMainLooper()).idle()
   }
+
+  @Test
+  fun onDestroyTest() {
+    mapboxRenderer.onDestroy()
+    Assert.assertEquals(true, mapboxRenderer.needDestroy)
+  }
 }

sdk/src/test/java/com/mapbox/maps/renderer/MapboxSurfaceHolderRendererTest.kt

@@ -0,0 +1,43 @@
+package com.mapbox.maps.renderer
+
+import android.view.SurfaceHolder
+import io.mockk.mockk
+import io.mockk.verify
+import org.junit.Before
+import org.junit.Test
+
+internal class MapboxSurfaceHolderRendererTest : MapboxRendererTest() {
+
+  private lateinit var surfaceHolderRenderer: MapboxSurfaceHolderRenderer
+  private lateinit var surfaceHolder: SurfaceHolder
+
+  @Before
+  override fun setUp() {
+    super.setUp()
+    surfaceHolder = mockk(relaxed = true)
+    mapboxRenderer = MapboxSurfaceHolderRenderer(renderThread)
+    surfaceHolderRenderer = MapboxSurfaceHolderRenderer(renderThread)
+  }
+
+  @Test
+  fun surfaceChangedTest() {
+    surfaceHolderRenderer.surfaceChanged(surfaceHolder, 0, 1, 1)
+    verify { renderThread.onSurfaceSizeChanged(1, 1) }
+  }
+
+  @Test
+  fun surfaceCreatedTest() {
+    surfaceHolderRenderer.surfaceCreated(surfaceHolder)
+    surfaceHolderRenderer.surfaceChanged(surfaceHolder, 0, 1, 1)
+    verify {
+      renderThread.onSurfaceCreated(surfaceHolder.surface, 1, 1)
+      renderThread.onSurfaceSizeChanged(1, 1)
+    }
+  }
+
+  @Test
+  fun surfaceDestroyedTest() {
+    surfaceHolderRenderer.surfaceDestroyed(surfaceHolder)
+    verify { renderThread.onSurfaceDestroyed() }
+  }
+}

sdk/src/test/java/com/mapbox/maps/renderer/MapboxSurfaceRendererTest.kt

@@ -40,10 +40,4 @@ internal class MapboxSurfaceRendererTest : MapboxRendererTest() {
     surfaceRenderer.surfaceDestroyed()
     verify { renderThread.onSurfaceDestroyed() }
   }
-
-  @Test
-  fun onDestroyTest() {
-    surfaceRenderer.onDestroy()
-    verify { renderThread.destroy() }
-  }
 }

sdk/src/test/java/com/mapbox/maps/renderer/MapboxTextureViewRendererTest.kt

@@ -1,11 +1,10 @@
 package com.mapbox.maps.renderer
 
-import android.view.TextureView
+import android.graphics.SurfaceTexture
 import io.mockk.mockk
 import io.mockk.verify
 import org.junit.Before
 import org.junit.Test
-import java.lang.ref.WeakReference
 
 internal class MapboxTextureViewRendererTest : MapboxRendererTest() {
 
@@ -14,9 +13,8 @@ internal class MapboxTextureViewRendererTest : MapboxRendererTest() {
   @Before
   override fun setUp() {
     super.setUp()
-    val weakView = WeakReference(mockk<TextureView>(relaxed = true))
-    mapboxRenderer = MapboxTextureViewRenderer(weakView, renderThread)
-    textureViewRenderer = MapboxTextureViewRenderer(weakView, renderThread)
+    mapboxRenderer = MapboxTextureViewRenderer(renderThread)
+    textureViewRenderer = MapboxTextureViewRenderer(renderThread)
   }
 
   @Test
@@ -32,13 +30,9 @@ internal class MapboxTextureViewRendererTest : MapboxRendererTest() {
 
   @Test
   fun onSurfaceTextureDestroyedTest() {
-    textureViewRenderer.onSurfaceTextureDestroyed(mockk())
+    val surfaceTexture = mockk<SurfaceTexture>(relaxUnitFun = true)
+    textureViewRenderer.onSurfaceTextureDestroyed(surfaceTexture)
     verify { renderThread.onSurfaceDestroyed() }
-  }
-
-  @Test
-  fun onDestroyTest() {
-    textureViewRenderer.onDestroy()
-    verify { renderThread.destroy() }
+    verify { surfaceTexture.release() }
   }
 }