MAPSAND-366 Fix ANR while open and close MapView quickly with texture…

…View set to true, fix potential crash on RenderHandlerThread creation. (#1567) MAPSAND-366 Fix possible ANR when destroying renderer.
mapbox · Aug 10, 2022 · 7ad2031 · 7ad2031
1 parent c880f9f
commit 7ad2031
Show file tree

Hide file tree

Showing 5 changed files with 69 additions and 13 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -16,6 +16,7 @@ Mapbox welcomes participation and contributions from everyone.
 * Fix a bug in cameraForGeometry returning incorrect camera options when pitch > 0. ([1568](https://github.com/mapbox/mapbox-maps-android/pull/1568))
 * Fix Android memory leak when destroying platform view annotation manager. ([1568](https://github.com/mapbox/mapbox-maps-android/pull/1568))
 * Fix style getters for terrain, light and atmosphere resetting properties. ([1573](https://github.com/mapbox/mapbox-maps-android/pull/1573))
+* Fix possible ANR when destroying renderer. ([1567](https://github.com/mapbox/mapbox-maps-android/pull/1567))
 
 ## Dependencies
 Bump gl-native to v10.8.0-beta.1, common to v23.0.0-beta.1. ([1568](https://github.com/mapbox/mapbox-maps-android/pull/1568))

diff --git a/sdk/src/main/java/com/mapbox/maps/renderer/MapboxRenderThread.kt b/sdk/src/main/java/com/mapbox/maps/renderer/MapboxRenderThread.kt
@@ -403,7 +403,7 @@ internal class MapboxRenderThread : Choreographer.FrameCallback {
     logI(TAG, "RenderThread : surface destroyed")
     lock.withLock {
       // in some situations `destroy` is called earlier than onSurfaceDestroyed - in that case no need to clean up
-      if (renderHandlerThread.started) {
+      if (renderHandlerThread.isRunning) {
         renderHandlerThread.post {
           awaitingNextVsync = false
           Choreographer.getInstance().removeFrameCallback(this)
@@ -545,7 +545,7 @@ internal class MapboxRenderThread : Choreographer.FrameCallback {
   internal fun destroy() {
     lock.withLock {
       // do nothing if destroy for some reason called more than once to avoid deadlock
-      if (renderHandlerThread.started) {
+      if (renderHandlerThread.isRunning) {
         renderHandlerThread.post {
           lock.withLock {
             if (renderCreated) {

diff --git a/sdk/src/main/java/com/mapbox/maps/renderer/RenderHandlerThread.kt b/sdk/src/main/java/com/mapbox/maps/renderer/RenderHandlerThread.kt
@@ -10,11 +10,14 @@ import com.mapbox.maps.logW
 internal class RenderHandlerThread {
 
   @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
-  internal lateinit var handlerThread: HandlerThread
+  internal val handlerThread: HandlerThread =
+    HandlerThread(HANDLE_THREAD_NAME, THREAD_PRIORITY_DISPLAY)
+
+  @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
   internal var handler: Handler? = null
 
-  internal val started
-    get() = handlerThread.isAlive
+  internal val isRunning
+    get() = handler != null && handlerThread.isAlive
 
   fun post(task: () -> Unit) {
     postDelayed(task, 0, EventType.DEFAULT)
@@ -29,7 +32,6 @@ internal class RenderHandlerThread {
   }
 
   fun start(): Handler {
-    handlerThread = HandlerThread(HANDLE_THREAD_NAME, THREAD_PRIORITY_DISPLAY)
     handlerThread.start()
     return Handler(handlerThread.looper).also {
       handler = it

diff --git a/sdk/src/test/java/com/mapbox/maps/renderer/MapboxRenderThreadTest.kt b/sdk/src/test/java/com/mapbox/maps/renderer/MapboxRenderThreadTest.kt
@@ -105,9 +105,10 @@ class MapboxRenderThreadTest {
     idleHandler()
   }
 
-  private fun mockCountdownRunnable(latch: CountDownLatch) = mockk<Runnable>(relaxUnitFun = true).also {
-    every { it.run() } answers { latch.countDown() }
-  }
+  private fun mockCountdownRunnable(latch: CountDownLatch) =
+    mockk<Runnable>(relaxUnitFun = true).also {
+      every { it.run() } answers { latch.countDown() }
+    }
 
   @After
   fun cleanup() {
@@ -212,7 +213,7 @@ class MapboxRenderThreadTest {
     verifyOnce { eglCore.release() }
     verifyOnce { mapboxRenderer.destroyRenderer() }
     verifyOnce { fpsManager.destroy() }
-    assertFalse(renderHandlerThread.started)
+    assertFalse(renderHandlerThread.isRunning)
   }
 
   @Test
@@ -227,7 +228,7 @@ class MapboxRenderThreadTest {
     verifyOnce { eglCore.release() }
     verifyOnce { mapboxRenderer.destroyRenderer() }
     verifyOnce { fpsManager.destroy() }
-    assertFalse(renderHandlerThread.started)
+    assertFalse(renderHandlerThread.isRunning)
   }
 
   @Test
@@ -390,7 +391,7 @@ class MapboxRenderThreadTest {
   fun destroyTest() {
     initRenderThread()
     mapboxRenderThread.destroy()
-    assertFalse(renderHandlerThread.started)
+    assertFalse(renderHandlerThread.isRunning)
   }
 
   @Test
@@ -876,4 +877,26 @@ class MapboxRenderThreadTest {
       mapboxRenderer.createRenderer()
     }
   }
+
+  @Test(timeout = 10000) // Added timeout to ensure that if test fails, test does not hang forever.
+  fun onSurfaceWithActivityDestroyedBeforeSurfaceWithDestroyTaskInQueueTest() {
+    initRenderThread()
+    provideValidSurface()
+    waitZeroCounter {
+      val latch = this
+      // simulate real situation that `destroyRenderer` schedules some task in queue
+      // which was leading to a deadlock in `onSurfaceDestroyed`
+      every { mapboxRenderer.destroyRenderer() } answers {
+        renderHandlerThread.handler?.post {
+          Thread.sleep(500)
+          latch.countDown()
+        }
+      }
+      mapboxRenderThread.destroy()
+      mapboxRenderThread.onSurfaceDestroyed()
+    }
+    verifyOnce { eglCore.release() }
+    verifyOnce { mapboxRenderer.destroyRenderer() }
+    assertFalse(renderHandlerThread.isRunning)
+  }
 }
diff --git a/sdk/src/test/java/com/mapbox/maps/renderer/RenderHandlerThreadTest.kt b/sdk/src/test/java/com/mapbox/maps/renderer/RenderHandlerThreadTest.kt
@@ -5,6 +5,7 @@ import com.mapbox.maps.logW
 import com.mapbox.verifyNo
 import io.mockk.*
 import org.junit.After
+import org.junit.Assert.assertThrows
 import org.junit.Before
 import org.junit.Test
 import org.junit.runner.RunWith
@@ -36,15 +37,44 @@ class RenderHandlerThreadTest {
     renderHandlerThread.start()
     assert(renderHandlerThread.handler != null)
     assert(renderHandlerThread.handlerThread.isAlive)
+    assert(renderHandlerThread.isRunning)
   }
 
   @Test
-  fun stopTest() {
+  fun startStartTest() {
+    renderHandlerThread.start()
+    assertThrows(IllegalThreadStateException::class.java) {
+      renderHandlerThread.start()
+    }
+  }
+
+  @Test
+  fun startStopTest() {
     renderHandlerThread.start()
     renderHandlerThread.stop()
     assert(renderHandlerThread.handler == null)
   }
 
+  @Test
+  fun startStopStartTest() {
+    renderHandlerThread.start()
+    renderHandlerThread.stop()
+    assertThrows(IllegalThreadStateException::class.java) {
+      renderHandlerThread.start()
+    }
+  }
+
+  @Test
+  fun isRunningTest() {
+    with(renderHandlerThread) {
+      assert(!isRunning)
+      start()
+      assert(isRunning)
+      stop()
+      assert(!isRunning)
+    }
+  }
+
   @Test
   fun clearDefaultMessagesTest() {
     renderHandlerThread.start()