Bug 1950991 - Serializes access to sessionsPendingDeletion r=android-reviewers,kaya

Differential Revision: https://phabricator.services.mozilla.com/D239962

mobile/android/android-components/components/browser/state/src/main/java/mozilla/components/browser/state/engine/middleware/TabsRemovedMiddleware.kt

@@ -7,6 +7,8 @@ package mozilla.components.browser.state.engine.middleware
 import androidx.annotation.VisibleForTesting
 import kotlinx.coroutines.CoroutineScope
 import kotlinx.coroutines.launch
+import kotlinx.coroutines.sync.Mutex
+import kotlinx.coroutines.sync.withLock
 import mozilla.components.browser.state.action.BrowserAction
 import mozilla.components.browser.state.action.CustomTabListAction
 import mozilla.components.browser.state.action.EngineAction
@@ -24,6 +26,29 @@ import mozilla.components.concept.engine.EngineSessionState
 import mozilla.components.lib.state.Middleware
 import mozilla.components.lib.state.MiddlewareContext
 
+@VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
+internal class SessionsPendingDeletion {
+    @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
+    var sessions: MutableMap<String, EngineSession> = mutableMapOf()
+    private var mutex = Mutex()
+
+    suspend fun add(id: String, session: EngineSession) = mutex.withLock {
+        sessions[id] = session
+    }
+
+    suspend fun remove(id: String) = mutex.withLock {
+        sessions.remove(id)
+    }
+
+    suspend fun removeAll(callback: (EngineSession) -> Unit) = mutex.withLock {
+        val keys = sessions.keys.toList()
+        for (key in keys) {
+            sessions[key]?.also(callback)
+            sessions.remove(key)
+        }
+    }
+}
+
 /**
  * [Middleware] responsible for closing and unlinking [EngineSession] instances whenever tabs get
  * removed.
@@ -32,7 +57,7 @@ internal class TabsRemovedMiddleware(
     private val scope: CoroutineScope,
 ) : Middleware<BrowserState, BrowserAction> {
     @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
-    var sessionsPendingDeletion: MutableMap<String, EngineSession> = mutableMapOf()
+    val sessionsPendingDeletion = SessionsPendingDeletion()
 
     @Suppress("ComplexMethod")
     override fun invoke(
@@ -97,22 +122,21 @@ internal class TabsRemovedMiddleware(
             it.register(object : EngineSession.Observer {
                 override fun onStateUpdated(state: EngineSessionState) {
                     context.store.dispatch(UndoAction.UpdateEngineStateForRecoverableTab(sessionState.id, state))
-                    sessionsPendingDeletion.remove(sessionState.id)
+                    scope.launch {
+                        sessionsPendingDeletion.remove(sessionState.id)
+                    }
                     it.close()
                 }
             })
-
-            sessionsPendingDeletion[sessionState.id] = it
+            scope.launch {
+                sessionsPendingDeletion.add(sessionState.id, it)
+            }
         }
     }
 
-    private fun clearSessionsPendingDeletion() {
-        sessionsPendingDeletion.keys.toList().forEach { id ->
-            scope.launch {
-                sessionsPendingDeletion[id]?.close()
-            }
-
-            sessionsPendingDeletion.remove(id)
+    private fun clearSessionsPendingDeletion() = scope.launch {
+        sessionsPendingDeletion.removeAll {
+            it.close()
         }
     }
 }

mobile/android/android-components/components/browser/state/src/test/java/mozilla/components/browser/state/engine/middleware/TabsRemovedMiddlewareTest.kt

@@ -56,7 +56,7 @@ class TabsRemovedMiddlewareTest {
         dispatcher.scheduler.advanceUntilIdle()
 
         assertNull(store.state.findTab(tab.id)?.engineState?.engineSession)
-        assertEquals(1, middleware.sessionsPendingDeletion.size)
+        assertEquals(1, middleware.sessionsPendingDeletion.sessions.size)
     }
 
     @Test
@@ -83,7 +83,7 @@ class TabsRemovedMiddlewareTest {
         assertNull(store.state.findTab(tab1.id)?.engineState?.engineSession)
         assertNull(store.state.findTab(tab2.id)?.engineState?.engineSession)
         assertNotNull(store.state.findTab(tab3.id)?.engineState?.engineSession)
-        assertEquals(2, middleware.sessionsPendingDeletion.size)
+        assertEquals(2, middleware.sessionsPendingDeletion.sessions.size)
     }
 
     @Test
@@ -109,7 +109,7 @@ class TabsRemovedMiddlewareTest {
         assertNull(store.state.findTab(tab1.id)?.engineState?.engineSession)
         assertNull(store.state.findTab(tab2.id)?.engineState?.engineSession)
         assertNotNull(store.state.findTab(tab3.id)?.engineState?.engineSession)
-        assertEquals(2, middleware.sessionsPendingDeletion.size)
+        assertEquals(2, middleware.sessionsPendingDeletion.sessions.size)
     }
 
     @Test
@@ -135,7 +135,7 @@ class TabsRemovedMiddlewareTest {
         assertNull(store.state.findTab(tab1.id)?.engineState?.engineSession)
         assertNull(store.state.findTab(tab2.id)?.engineState?.engineSession)
         assertNotNull(store.state.findTab(tab3.id)?.engineState?.engineSession)
-        assertEquals(2, middleware.sessionsPendingDeletion.size)
+        assertEquals(2, middleware.sessionsPendingDeletion.sessions.size)
     }
 
     @Test
@@ -161,7 +161,7 @@ class TabsRemovedMiddlewareTest {
         assertNull(store.state.findTab(tab1.id)?.engineState?.engineSession)
         assertNull(store.state.findTab(tab2.id)?.engineState?.engineSession)
         assertNotNull(store.state.findCustomTab(tab3.id)?.engineState?.engineSession)
-        assertEquals(2, middleware.sessionsPendingDeletion.size)
+        assertEquals(2, middleware.sessionsPendingDeletion.sessions.size)
     }
 
     @Test
@@ -230,7 +230,7 @@ class TabsRemovedMiddlewareTest {
 
         verify(engineSession1).register(observerCaptor.capture())
 
-        assertEquals(1, middleware.sessionsPendingDeletion.size)
+        assertEquals(1, middleware.sessionsPendingDeletion.sessions.size)
 
         observerCaptor.value.onStateUpdated(mock())
 
@@ -238,7 +238,7 @@ class TabsRemovedMiddlewareTest {
         dispatcher.scheduler.advanceUntilIdle()
 
         verify(engineSession1).close()
-        assertTrue(middleware.sessionsPendingDeletion.isEmpty())
+        assertTrue(middleware.sessionsPendingDeletion.sessions.isEmpty())
     }
 
     @Test
@@ -264,7 +264,7 @@ class TabsRemovedMiddlewareTest {
         assertNull(store.state.findTab(tab1.id)?.engineState?.engineSession)
         assertNull(store.state.findTab(tab2.id)?.engineState?.engineSession)
         assertNotNull(store.state.findTab(tab3.id)?.engineState?.engineSession)
-        assertEquals(2, middleware.sessionsPendingDeletion.size)
+        assertEquals(2, middleware.sessionsPendingDeletion.sessions.size)
 
         store.dispatch(UndoAction.ClearRecoverableTabs("noop")).joinBlocking()
         store.waitUntilIdle()
@@ -273,7 +273,7 @@ class TabsRemovedMiddlewareTest {
         verify(engineSession1).close()
         verify(engineSession2).close()
         verify(engineSession3, never()).close()
-        assertTrue(middleware.sessionsPendingDeletion.isEmpty())
+        assertTrue(middleware.sessionsPendingDeletion.sessions.isEmpty())
     }
 
     @Test
@@ -299,7 +299,7 @@ class TabsRemovedMiddlewareTest {
         assertNull(store.state.findTab(tab1.id)?.engineState?.engineSession)
         assertNull(store.state.findTab(tab2.id)?.engineState?.engineSession)
         assertNotNull(store.state.findTab(tab3.id)?.engineState?.engineSession)
-        assertEquals(2, middleware.sessionsPendingDeletion.size)
+        assertEquals(2, middleware.sessionsPendingDeletion.sessions.size)
 
         store.dispatch(UndoAction.RestoreRecoverableTabs).joinBlocking()
         store.waitUntilIdle()
@@ -308,7 +308,7 @@ class TabsRemovedMiddlewareTest {
         verify(engineSession1).close()
         verify(engineSession2).close()
         verify(engineSession3, never()).close()
-        assertTrue(middleware.sessionsPendingDeletion.isEmpty())
+        assertTrue(middleware.sessionsPendingDeletion.sessions.isEmpty())
     }
 
     private fun linkEngineSession(store: BrowserStore, tabId: String): EngineSession {