From 33f4643e65db1a655b9908755b62b94e98e811e8 Mon Sep 17 00:00:00 2001 From: Kiryl Dzehtsiarenka Date: Tue, 15 Jun 2021 18:05:54 +0300 Subject: [PATCH] Fix memory leak on render destroy (#426) --- sdk/src/main/java/com/mapbox/maps/MapView.kt | 3 +- .../maps/renderer/MapboxRenderThread.kt | 5 ++- .../mapbox/maps/renderer/MapboxRenderer.kt | 8 ++-- .../renderer/MapboxSurfaceHolderRenderer.kt | 17 ++------ .../maps/renderer/MapboxSurfaceRenderer.kt | 5 --- .../renderer/MapboxTextureViewRenderer.kt | 18 ++------ .../maps/renderer/MapboxRenderThreadTest.kt | 26 +++++++++-- .../maps/renderer/MapboxRendererTest.kt | 7 +++ .../MapboxSurfaceHolderRendererTest.kt | 43 +++++++++++++++++++ .../renderer/MapboxSurfaceRendererTest.kt | 6 --- .../renderer/MapboxTextureViewRendererTest.kt | 18 +++----- 11 files changed, 95 insertions(+), 61 deletions(-) create mode 100644 sdk/src/test/java/com/mapbox/maps/renderer/MapboxSurfaceHolderRendererTest.kt diff --git a/sdk/src/main/java/com/mapbox/maps/MapView.kt b/sdk/src/main/java/com/mapbox/maps/MapView.kt index f321068d4e..02bf4c1ead 100644 --- a/sdk/src/main/java/com/mapbox/maps/MapView.kt +++ b/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 diff --git a/sdk/src/main/java/com/mapbox/maps/renderer/MapboxRenderThread.kt b/sdk/src/main/java/com/mapbox/maps/renderer/MapboxRenderThread.kt index 8b68801462..eb62911c18 100644 --- a/sdk/src/main/java/com/mapbox/maps/renderer/MapboxRenderThread.kt +++ b/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() diff --git a/sdk/src/main/java/com/mapbox/maps/renderer/MapboxRenderer.kt b/sdk/src/main/java/com/mapbox/maps/renderer/MapboxRenderer.kt index 7add039836..80615c3d8f 100644 --- a/sdk/src/main/java/com/mapbox/maps/renderer/MapboxRenderer.kt +++ b/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 diff --git a/sdk/src/main/java/com/mapbox/maps/renderer/MapboxSurfaceHolderRenderer.kt b/sdk/src/main/java/com/mapbox/maps/renderer/MapboxSurfaceHolderRenderer.kt index f1a4b96de1..5df2719a96 100644 --- a/sdk/src/main/java/com/mapbox/maps/renderer/MapboxSurfaceHolderRenderer.kt +++ b/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 - - 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) - } } \ No newline at end of file diff --git a/sdk/src/main/java/com/mapbox/maps/renderer/MapboxSurfaceRenderer.kt b/sdk/src/main/java/com/mapbox/maps/renderer/MapboxSurfaceRenderer.kt index 50da7e799e..16454154a2 100644 --- a/sdk/src/main/java/com/mapbox/maps/renderer/MapboxSurfaceRenderer.kt +++ b/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() - } } \ No newline at end of file diff --git a/sdk/src/main/java/com/mapbox/maps/renderer/MapboxTextureViewRenderer.kt b/sdk/src/main/java/com/mapbox/maps/renderer/MapboxTextureViewRenderer.kt index 041803a175..550db0a5d3 100644 --- a/sdk/src/main/java/com/mapbox/maps/renderer/MapboxTextureViewRenderer.kt +++ b/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 - - constructor(textureView: WeakReference) { - 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, 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() - } } \ No newline at end of file diff --git a/sdk/src/test/java/com/mapbox/maps/renderer/MapboxRenderThreadTest.kt b/sdk/src/test/java/com/mapbox/maps/renderer/MapboxRenderThreadTest.kt index 4b0477f8c0..e72da69350 100644 --- a/sdk/src/test/java/com/mapbox/maps/renderer/MapboxRenderThreadTest.kt +++ b/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(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(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(relaxUnitFun = true) every { surface.isValid } returns true every { eglCore.eglStatusSuccess } returns true diff --git a/sdk/src/test/java/com/mapbox/maps/renderer/MapboxRendererTest.kt b/sdk/src/test/java/com/mapbox/maps/renderer/MapboxRendererTest.kt index 87f5d35489..eb026265a1 100644 --- a/sdk/src/test/java/com/mapbox/maps/renderer/MapboxRendererTest.kt +++ b/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) + } } \ No newline at end of file diff --git a/sdk/src/test/java/com/mapbox/maps/renderer/MapboxSurfaceHolderRendererTest.kt b/sdk/src/test/java/com/mapbox/maps/renderer/MapboxSurfaceHolderRendererTest.kt new file mode 100644 index 0000000000..907831c5b4 --- /dev/null +++ b/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() } + } +} \ No newline at end of file diff --git a/sdk/src/test/java/com/mapbox/maps/renderer/MapboxSurfaceRendererTest.kt b/sdk/src/test/java/com/mapbox/maps/renderer/MapboxSurfaceRendererTest.kt index ca4031466e..c8a28fd514 100644 --- a/sdk/src/test/java/com/mapbox/maps/renderer/MapboxSurfaceRendererTest.kt +++ b/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() } - } } \ No newline at end of file diff --git a/sdk/src/test/java/com/mapbox/maps/renderer/MapboxTextureViewRendererTest.kt b/sdk/src/test/java/com/mapbox/maps/renderer/MapboxTextureViewRendererTest.kt index c500e233fe..4c19f44185 100644 --- a/sdk/src/test/java/com/mapbox/maps/renderer/MapboxTextureViewRendererTest.kt +++ b/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(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(relaxUnitFun = true) + textureViewRenderer.onSurfaceTextureDestroyed(surfaceTexture) verify { renderThread.onSurfaceDestroyed() } - } - - @Test - fun onDestroyTest() { - textureViewRenderer.onDestroy() - verify { renderThread.destroy() } + verify { surfaceTexture.release() } } } \ No newline at end of file