Issue #7867: Move EngineSession from SessionManager to BrowserState.

Co-authored-by: Christian Sadilek <christian.sadilek@gmail.com>

Issue #8121: Replace MigrationStore with MigrationContext and prevent usage outside of a Middleware.

* Before executing the reducer chain we now verify that we are on the store thread and throw if we are on a different thread.
* MigrationContext now provides a store property that returns the underlying Store instance. This can be used to pass it to
  other components and threads that are not part of the Middleware.
* Fixed existing Middleware implementations to use the MiddlewareContext or pass the actual Store instance around.

components/browser/engine-gecko-beta/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngineSession.kt

@@ -232,7 +232,15 @@ class GeckoEngineSession(
             policy.contains(TrackingProtectionPolicy.TrackingCategory.SCRIPTS_AND_SUB_RESOURCES)
 
         geckoSession.settings.useTrackingProtection = shouldBlockContent && enabled
-        notifyAtLeastOneObserver { onTrackerBlockingEnabledChange(enabled) }
+        notifyAtLeastOneObserver {
+            // We now register engine observers in a middleware using a dedicated
+            // store thread. Since this notification can be delayed until an observer
+            // is registered we switch to the main scope to make sure we're not notifying
+            // on the store thread.
+            MainScope().launch {
+                onTrackerBlockingEnabledChange(enabled)
+            }
+        }
     }
 
     /**

components/browser/engine-gecko-nightly/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngineSession.kt

@@ -232,7 +232,15 @@ class GeckoEngineSession(
             policy.contains(TrackingProtectionPolicy.TrackingCategory.SCRIPTS_AND_SUB_RESOURCES)
 
         geckoSession.settings.useTrackingProtection = shouldBlockContent && enabled
-        notifyAtLeastOneObserver { onTrackerBlockingEnabledChange(enabled) }
+        notifyAtLeastOneObserver {
+            // We now register engine observers in a middleware using a dedicated
+            // store thread. Since this notification can be delayed until an observer
+            // is registered we switch to the main scope to make sure we're not notifying
+            // on the store thread.
+            MainScope().launch {
+                onTrackerBlockingEnabledChange(enabled)
+            }
+        }
     }
 
     /**

components/browser/engine-gecko/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngineSession.kt

@@ -206,7 +206,15 @@ class GeckoEngineSession(
             policy.contains(TrackingProtectionPolicy.TrackingCategory.SCRIPTS_AND_SUB_RESOURCES)
 
         geckoSession.settings.useTrackingProtection = shouldBlockContent && enabled
-        notifyAtLeastOneObserver { onTrackerBlockingEnabledChange(enabled) }
+        notifyAtLeastOneObserver {
+            // We now register engine observers in a middleware using a dedicated
+            // store thread. Since this notification can be delayed until an observer
+            // is registered we switch to the main scope to make sure we're not notifying
+            // on the store thread.
+            MainScope().launch {
+                onTrackerBlockingEnabledChange(enabled)
+            }
+        }
     }
 
     /**

components/browser/session/build.gradle

@@ -21,6 +21,12 @@ android {
     }
 }
 
+tasks.withType(org.jetbrains.kotlin.gradle.tasks.KotlinCompile).configureEach() {
+    kotlinOptions.freeCompilerArgs += [
+            "-Xuse-experimental=kotlinx.coroutines.ExperimentalCoroutinesApi"
+    ]
+}
+
 dependencies {
     api project(':browser-state')
 
@@ -47,6 +53,7 @@ dependencies {
     testImplementation Dependencies.androidx_test_junit
     testImplementation Dependencies.testing_robolectric
     testImplementation Dependencies.testing_mockito
+    testImplementation Dependencies.testing_coroutines
 }
 
 apply from: '../../../publish.gradle'

components/browser/session/src/main/java/mozilla/components/browser/session/LegacySessionManager.kt

@@ -6,8 +6,6 @@ package mozilla.components.browser.session
 
 import androidx.annotation.GuardedBy
 import mozilla.components.concept.engine.Engine
-import mozilla.components.concept.engine.EngineSession
-import mozilla.components.concept.engine.EngineSessionState
 import mozilla.components.support.base.observer.Observable
 import mozilla.components.support.base.observer.ObserverRegistry
 import kotlin.math.max
@@ -19,7 +17,6 @@ import kotlin.math.min
 @Suppress("TooManyFunctions", "LargeClass")
 class LegacySessionManager(
     val engine: Engine,
-    private val engineSessionLinker: SessionManager.EngineSessionLinker,
     delegate: Observable<SessionManager.Observer> = ObserverRegistry()
 ) : Observable<SessionManager.Observer> by delegate {
     // It's important that any access to `values` is synchronized;
@@ -35,60 +32,6 @@ class LegacySessionManager(
     val size: Int
         get() = synchronized(values) { values.size }
 
-    /**
-     * Produces a snapshot of this manager's state, suitable for restoring via [SessionManager.restore].
-     * Only regular sessions are included in the snapshot. Private and Custom Tab sessions are omitted.
-     *
-     * @return [SessionManager.Snapshot] of the current session state.
-     */
-    fun createSnapshot(): SessionManager.Snapshot = synchronized(values) {
-        if (values.isEmpty()) {
-            return SessionManager.Snapshot.empty()
-        }
-
-        // Filter out CustomTab and private sessions.
-        // We're using 'values' directly instead of 'sessions' to get benefits of a sequence.
-        val sessionStateTuples = values.asSequence()
-                .filter { !it.isCustomTabSession() }
-                .filter { !it.private }
-                .map { session -> createSessionSnapshot(session) }
-                .toList()
-
-        // We might have some sessions (private, custom tab) but none we'd include in the snapshot.
-        if (sessionStateTuples.isEmpty()) {
-            return SessionManager.Snapshot.empty()
-        }
-
-        // We need to find out the index of our selected session in the filtered list. If we have a
-        // mix of private, custom tab and regular sessions, global selectedIndex isn't good enough.
-        // We must have a selectedSession if there is at least one "regular" (non-CustomTabs) session
-        // present. Selected session might be private, in which case we reset our selection index to 0.
-        var selectedIndexAfterFiltering = 0
-        selectedSession?.takeIf { !it.private }?.let { selected ->
-            sessionStateTuples.find { it.session.id == selected.id }?.let { selectedTuple ->
-                selectedIndexAfterFiltering = sessionStateTuples.indexOf(selectedTuple)
-            }
-        }
-
-        // Sanity check to guard against producing invalid snapshots.
-        checkNotNull(sessionStateTuples.getOrNull(selectedIndexAfterFiltering)) {
-            "Selection index after filtering session must be valid"
-        }
-
-        SessionManager.Snapshot(
-            sessions = sessionStateTuples,
-            selectedSessionIndex = selectedIndexAfterFiltering
-        )
-    }
-
-    fun createSessionSnapshot(session: Session): SessionManager.Snapshot.Item {
-        return SessionManager.Snapshot.Item(
-            session,
-            session.engineSessionHolder.engineSession,
-            session.engineSessionHolder.engineSessionState
-        )
-    }
-
     /**
      * Gets the currently selected session if there is one.
      *
@@ -136,19 +79,15 @@ class LegacySessionManager(
     fun add(
         session: Session,
         selected: Boolean = false,
-        engineSession: EngineSession? = null,
-        engineSessionState: EngineSessionState? = null,
         parent: Session? = null
     ) = synchronized(values) {
-        addInternal(session, selected, engineSession, engineSessionState, parent = parent, viaRestore = false)
+        addInternal(session, selected, parent = parent, viaRestore = false)
     }
 
     @Suppress("LongParameterList", "ComplexMethod")
     private fun addInternal(
         session: Session,
         selected: Boolean = false,
-        engineSession: EngineSession? = null,
-        engineSessionState: EngineSessionState? = null,
         parent: Session? = null,
         viaRestore: Boolean = false,
         notifyObservers: Boolean = true
@@ -173,12 +112,6 @@ class LegacySessionManager(
             }
         }
 
-        if (engineSession != null) {
-            link(session, engineSession)
-        } else if (engineSessionState != null) {
-            session.engineSessionHolder.engineSessionState = engineSessionState
-        }
-
         // If session is being added via restore, skip notification and auto-selection.
         // Restore will handle these actions as appropriate.
         if (viaRestore || !notifyObservers) {
@@ -241,8 +174,6 @@ class LegacySessionManager(
         snapshot.sessions.asReversed().forEach {
             addInternal(
                 it.session,
-                engineSession = it.engineSession,
-                engineSessionState = it.engineSessionState,
                 parent = null,
                 viaRestore = true
             )
@@ -262,52 +193,6 @@ class LegacySessionManager(
         notifyObservers { onSessionsRestored() }
     }
 
-    /**
-     * Gets the linked engine session for the provided session (if it exists).
-     */
-    fun getEngineSession(session: Session = selectedSessionOrThrow) = session.engineSessionHolder.engineSession
-
-    /**
-     * Gets the linked engine session for the provided session and creates it if needed.
-     */
-    fun getOrCreateEngineSession(
-        session: Session = selectedSessionOrThrow,
-        skipLoading: Boolean = false
-    ): EngineSession {
-        getEngineSession(session)?.let { return it }
-
-        return engine.createSession(session.private, session.contextId).apply {
-            var restored = false
-            session.engineSessionHolder.engineSessionState?.let { state ->
-                restored = restoreState(state)
-                session.engineSessionHolder.engineSessionState = null
-            }
-
-            link(session, this, restored, skipLoading)
-        }
-    }
-
-    private fun link(
-        session: Session,
-        engineSession: EngineSession,
-        restored: Boolean = false,
-        skipLoading: Boolean = false
-    ) {
-        val parent = values.find { it.id == session.parentId }?.let {
-            this.getEngineSession(it)
-        }
-        engineSessionLinker.link(session, engineSession, parent, restored, skipLoading)
-
-        if (session == selectedSession) {
-            engineSession.markActiveForWebExtensions(true)
-        }
-    }
-
-    private fun unlink(session: Session) {
-        getEngineSession(session)?.markActiveForWebExtensions(false)
-        engineSessionLinker.unlink(session)
-    }
-
     /**
      * Removes the provided session. If no session is provided then the selected session is removed.
      */
@@ -324,8 +209,6 @@ class LegacySessionManager(
 
         values.removeAt(indexToRemove)
 
-        unlink(session)
-
         recalculateSelectionIndex(
             indexToRemove,
             selectParentIfExists,
@@ -339,7 +222,6 @@ class LegacySessionManager(
         notifyObservers { onSessionRemoved(session) }
 
         if (selectedBeforeRemove != selectedSession && selectedIndex != NO_SELECTION) {
-            getEngineSession(selectedSessionOrThrow)?.markActiveForWebExtensions(true)
             notifyObservers { onSessionSelected(selectedSessionOrThrow) }
         }
     }
@@ -472,7 +354,6 @@ class LegacySessionManager(
      */
     fun removeSessions() = synchronized(values) {
         sessions.forEach {
-            unlink(it)
             values.remove(it)
         }
 
@@ -487,7 +368,6 @@ class LegacySessionManager(
      * Removes all sessions including CustomTab sessions.
      */
     fun removeAll() = synchronized(values) {
-        values.forEach { unlink(it) }
         values.clear()
 
         selectedIndex = NO_SELECTION
@@ -507,13 +387,8 @@ class LegacySessionManager(
             "Value to select is not in list"
         }
 
-        selectedSession?.let {
-            getEngineSession(it)?.markActiveForWebExtensions(false)
-        }
-
         selectedIndex = index
 
-        getEngineSession(session)?.markActiveForWebExtensions(true)
         notifyObservers { onSessionSelected(session) }
     }
 

components/browser/session/src/main/java/mozilla/components/browser/session/Session.kt

@@ -6,14 +6,11 @@ package mozilla.components.browser.session
 
 import android.content.Intent
 import android.graphics.Bitmap
-import mozilla.components.browser.session.engine.EngineSessionHolder
 import mozilla.components.browser.session.engine.request.LaunchIntentMetadata
 import mozilla.components.browser.session.engine.request.LoadRequestMetadata
 import mozilla.components.browser.session.engine.request.LoadRequestOption
 import mozilla.components.browser.session.ext.syncDispatch
 import mozilla.components.browser.session.ext.toSecurityInfoState
-import mozilla.components.browser.session.ext.toTabSessionState
-import mozilla.components.browser.state.action.ContentAction.RemoveThumbnailAction
 import mozilla.components.browser.state.action.ContentAction.RemoveWebAppManifestAction
 import mozilla.components.browser.state.action.ContentAction.UpdateBackNavigationStateAction
 import mozilla.components.browser.state.action.ContentAction.UpdateForwardNavigationStateAction
@@ -21,13 +18,10 @@ import mozilla.components.browser.state.action.ContentAction.UpdateLoadingStateA
 import mozilla.components.browser.state.action.ContentAction.UpdateProgressAction
 import mozilla.components.browser.state.action.ContentAction.UpdateSearchTermsAction
 import mozilla.components.browser.state.action.ContentAction.UpdateSecurityInfoAction
-import mozilla.components.browser.state.action.ContentAction.UpdateThumbnailAction
 import mozilla.components.browser.state.action.ContentAction.UpdateTitleAction
 import mozilla.components.browser.state.action.ContentAction.UpdateUrlAction
 import mozilla.components.browser.state.action.ContentAction.UpdateWebAppManifestAction
-import mozilla.components.browser.state.action.CustomTabListAction.RemoveCustomTabAction
-import mozilla.components.browser.state.action.EngineAction
-import mozilla.components.browser.state.action.TabListAction.AddTabAction
+import mozilla.components.browser.state.action.CustomTabListAction
 import mozilla.components.browser.state.action.TrackingProtectionAction
 import mozilla.components.browser.state.selector.findTabOrCustomTab
 import mozilla.components.browser.state.state.CustomTabConfig
@@ -55,12 +49,6 @@ class Session(
     val contextId: String? = null,
     delegate: Observable<Observer> = ObserverRegistry()
 ) : Observable<Session.Observer> by delegate {
-    /**
-     * Holder for keeping a reference to an engine session and its observer to update this session
-     * object.
-     */
-    internal val engineSessionHolder = EngineSessionHolder()
-
     // For migration purposes every `Session` has a reference to the `BrowserStore` (if used) in order to dispatch
     // actions to it when the `Session` changes.
     internal var store: BrowserStore? = null
@@ -94,10 +82,8 @@ class Session(
         fun onTrackerBlocked(session: Session, tracker: Tracker, all: List<Tracker>) = Unit
         fun onTrackerLoaded(session: Session, tracker: Tracker, all: List<Tracker>) = Unit
         fun onDesktopModeChanged(session: Session, enabled: Boolean) = Unit
-        fun onThumbnailChanged(session: Session, bitmap: Bitmap?) = Unit
         fun onContentPermissionRequested(session: Session, permissionRequest: PermissionRequest): Boolean = false
         fun onAppPermissionRequested(session: Session, permissionRequest: PermissionRequest): Boolean = false
-        fun onCrashStateChanged(session: Session, crashed: Boolean) = Unit
         fun onRecordingDevicesChanged(session: Session, devices: List<RecordingDevice>) = Unit
         fun onLaunchIntentRequest(session: Session, url: String, appIntent: Intent?) = Unit
     }
@@ -218,11 +204,9 @@ class Session(
         // tabs to regular tabs, so we have to dispatch the corresponding
         // browser actions to keep the store in sync.
         if (old != new && new == null) {
-            store?.syncDispatch(RemoveCustomTabAction(id))
-            store?.syncDispatch(AddTabAction(toTabSessionState()))
-            engineSessionHolder.engineSession?.let { engineSession ->
-                store?.syncDispatch(EngineAction.LinkEngineSessionAction(id, engineSession))
-            }
+            store?.syncDispatch(
+                CustomTabListAction.TurnCustomTabIntoNormalTabAction(id)
+            )
         }
     }
 
@@ -293,16 +277,6 @@ class Session(
         }
     }
 
-    /**
-     * The target of the latest thumbnail.
-     */
-    var thumbnail: Bitmap? by Delegates.observable<Bitmap?>(null) { _, _, new ->
-        notifyObservers { onThumbnailChanged(this@Session, new) }
-
-        val action = if (new != null) UpdateThumbnailAction(id, new) else RemoveThumbnailAction(id)
-        store?.syncDispatch(action)
-    }
-
     /**
      * Desktop Mode state, true if the desktop mode is requested, otherwise false.
      */
@@ -341,19 +315,6 @@ class Session(
             !request.consumeBy(consumers)
     }
 
-    /**
-     * Whether this [Session] has crashed.
-     *
-     * In conjunction with a `concept-engine` implementation that uses a multi-process architecture, single sessions
-     * can crash without crashing the whole app.
-     *
-     * A crashed session may still be operational (since the underlying engine implementation has recovered its content
-     * process), but further action may be needed to restore the last state before the session has crashed (if desired).
-     */
-    var crashed: Boolean by Delegates.observable(false) { _, old, new ->
-        notifyObservers(old, new) { onCrashStateChanged(this@Session, new) }
-    }
-
     /**
      * List of recording devices (e.g. camera or microphone) currently in use by web content.
      */

components/browser/session/src/main/java/mozilla/components/browser/session/SessionManager.kt

@@ -4,17 +4,13 @@
 
 package mozilla.components.browser.session
 
-import android.content.ComponentCallbacks2
-import mozilla.components.browser.session.engine.EngineObserver
 import mozilla.components.browser.session.ext.syncDispatch
 import mozilla.components.browser.session.ext.toCustomTabSessionState
 import mozilla.components.browser.session.ext.toTabSessionState
 import mozilla.components.browser.state.action.CustomTabListAction
 import mozilla.components.browser.state.action.EngineAction.LinkEngineSessionAction
 import mozilla.components.browser.state.action.EngineAction.UpdateEngineSessionStateAction
-import mozilla.components.browser.state.action.EngineAction.UnlinkEngineSessionAction
 import mozilla.components.browser.state.action.ReaderAction
-import mozilla.components.browser.state.action.SystemAction
 import mozilla.components.browser.state.action.TabListAction
 import mozilla.components.browser.state.selector.findTab
 import mozilla.components.browser.state.state.ReaderState
@@ -22,11 +18,7 @@ import mozilla.components.browser.state.store.BrowserStore
 import mozilla.components.concept.engine.Engine
 import mozilla.components.concept.engine.EngineSession
 import mozilla.components.concept.engine.EngineSessionState
-import mozilla.components.support.base.log.logger.Logger
-import mozilla.components.support.base.memory.MemoryConsumer
 import mozilla.components.support.base.observer.Observable
-import mozilla.components.support.ktx.kotlin.isExtensionUrl
-import java.lang.IllegalArgumentException
 
 /**
  * This class provides access to a centralized registry of all active sessions.
@@ -35,117 +27,25 @@ import java.lang.IllegalArgumentException
 class SessionManager(
     val engine: Engine,
     private val store: BrowserStore? = null,
-    private val linker: EngineSessionLinker = EngineSessionLinker(store),
-    private val delegate: LegacySessionManager = LegacySessionManager(engine, linker)
-) : Observable<SessionManager.Observer> by delegate, MemoryConsumer {
-    private val logger = Logger("SessionManager")
-
-    /**
-     * This class only exists for migrating from browser-session
-     * to browser-state. We need a way to dispatch the corresponding browser
-     * actions when an engine session is linked and unlinked.
-     */
-    class EngineSessionLinker(private val store: BrowserStore?) {
-        /**
-         * Links the provided [Session] and [EngineSession].
-         */
-        fun link(
-            session: Session,
-            engineSession: EngineSession,
-            parentEngineSession: EngineSession?,
-            sessionRestored: Boolean = false,
-            skipLoading: Boolean = false
-        ) {
-            unlink(session)
-
-            session.engineSessionHolder.apply {
-                this.engineSession = engineSession
-                this.engineObserver = EngineObserver(session, store).also { observer ->
-                    engineSession.register(observer)
-                    if (!sessionRestored && !skipLoading) {
-                        if (session.url.isExtensionUrl()) {
-                            // The parent tab/session is used as a referrer which is not accurate
-                            // for extension pages. The extension page is not loaded by the parent
-                            // tab, but opened by an extension e.g. via browser.tabs.update.
-                            engineSession.loadUrl(session.url)
-                        } else {
-                            engineSession.loadUrl(session.url, parentEngineSession)
-                        }
-                    }
-                }
-            }
-
-            store?.syncDispatch(LinkEngineSessionAction(session.id, engineSession))
-        }
-
-        fun unlink(session: Session) {
-            val observer = session.engineSessionHolder.engineObserver
-            val engineSession = session.engineSessionHolder.engineSession
-
-            if (observer != null && engineSession != null) {
-                engineSession.unregister(observer)
-            }
-
-            session.engineSessionHolder.engineSession = null
-            session.engineSessionHolder.engineObserver = null
-
-            // Note: We could consider not clearing the engine session state here. Instead we could
-            // try to actively set it here by calling save() on the engine session (if we have one).
-            // That way adding the same Session again could restore the previous state automatically
-            // (although we would need to make sure we do not override it with null in link()).
-            // Clearing the engine session state would be left to the garbage collector whenever the
-            // session itself gets collected.
-            session.engineSessionHolder.engineSessionState = null
-
-            store?.syncDispatch(UnlinkEngineSessionAction(session.id))
-
-            // Now, that neither the session manager nor the store keep a reference to the engine
-            // session, we can close it.
-            engineSession?.close()
-        }
-    }
-
+    private val delegate: LegacySessionManager = LegacySessionManager(engine)
+) : Observable<SessionManager.Observer> by delegate {
     /**
      * Returns the number of session including CustomTab sessions.
      */
     val size: Int
         get() = delegate.size
 
-    /**
-     * Produces a snapshot of this manager's state, suitable for restoring via [SessionManager.restore].
-     * Only regular sessions are included in the snapshot. Private and Custom Tab sessions are omitted.
-     *
-     * @return [Snapshot] of the current session state.
-     */
-    fun createSnapshot(): Snapshot {
-        val snapshot = delegate.createSnapshot()
-
-        // The reader state is no longer part of the browser session so we copy
-        // it from the store if it exists. This can be removed once browser-state
-        // migration is complete and the session storage uses the store instead
-        // of the session manager.
-        return snapshot.copy(
-            sessions = snapshot.sessions.map { item ->
-                store?.state?.findTab(item.session.id)?.let {
-                    item.copy(readerState = it.readerState)
-                } ?: item
-            }
-        )
-    }
-
     /**
      * Produces a [Snapshot.Item] of a single [Session], suitable for restoring via [SessionManager.restore].
      */
     fun createSessionSnapshot(session: Session): Snapshot.Item {
-        val item = delegate.createSessionSnapshot(session)
-
-        // The reader state is no longer part of the browser session so we copy
-        // it from the store if it exists. This can be removed once browser-state
-        // migration is complete and the session storage uses the store instead
-        // of the session manager.
-        return store?.state?.findTab(item.session.id)?.let {
-            item.copy(readerState = it.readerState)
-        } ?: item
+        val tab = store?.state?.findTab(session.id)
+
+        return Snapshot.Item(
+            session,
+            tab?.engineState?.engineSession,
+            tab?.engineState?.engineSessionState,
+            tab?.readerState)
     }
 
     /**
@@ -216,6 +116,16 @@ class SessionManager(
             )
         }
 
+        delegate.add(session, selected, parent)
+
+        if (engineSession != null) {
+            store?.syncDispatch(LinkEngineSessionAction(
+                session.id,
+                engineSession,
+                skipLoading = true
+            ))
+        }
+
         if (engineSessionState != null && engineSession == null) {
             // If the caller passed us an engine session state then also notify the store. We only
             // do this if there is no engine session, because this mirrors the behavior in
@@ -225,8 +135,6 @@ class SessionManager(
                 engineSessionState