Merge #6151

6151: Closes #6018: Do not load url during link if session is restored r=pocmo,csadilek a=psymoon

The potential pitfall of this patch is we are assuming `geckoSession.restoreState()` would always be successful. Perhaps we could have a follow-up for GV to provide us a callback for when `restoreState()` is completed with a status? 🤔

Co-authored-by: Simon Chae <chaesmn@gmail.com>

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

@@ -152,16 +152,17 @@ class GeckoEngineSession(
     /**
      * See [EngineSession.restoreState]
      */
-    override fun restoreState(state: EngineSessionState) {
+    override fun restoreState(state: EngineSessionState): Boolean {
         if (state !is GeckoEngineSessionState) {
             throw IllegalStateException("Can only restore from GeckoEngineSessionState")
         }
 
         if (state.actualState == null) {
-            return
+            return false
         }
 
         geckoSession.restoreState(state.actualState)
+        return true
     }
 
     /**

components/browser/engine-gecko-beta/src/test/java/mozilla/components/browser/engine/gecko/GeckoEngineSessionTest.kt

@@ -474,18 +474,18 @@ class GeckoEngineSessionTest {
         val actualState: GeckoSession.SessionState = mock()
         val state = GeckoEngineSessionState(actualState)
 
-        engineSession.restoreState(state)
+        assertTrue(engineSession.restoreState(state))
         verify(geckoSession).restoreState(any())
     }
 
     @Test
-    fun `restoreState does nothing for null state`() {
+    fun `restoreState returns false for null state`() {
         val engineSession = GeckoEngineSession(mock(),
             geckoSessionProvider = geckoSessionProvider)
 
         val state = GeckoEngineSessionState(null)
 
-        engineSession.restoreState(state)
+        assertFalse(engineSession.restoreState(state))
         verify(geckoSession, never()).restoreState(any())
     }
 

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

@@ -152,16 +152,17 @@ class GeckoEngineSession(
     /**
      * See [EngineSession.restoreState]
      */
-    override fun restoreState(state: EngineSessionState) {
+    override fun restoreState(state: EngineSessionState): Boolean {
         if (state !is GeckoEngineSessionState) {
             throw IllegalStateException("Can only restore from GeckoEngineSessionState")
         }
 
         if (state.actualState == null) {
-            return
+            return false
         }
 
         geckoSession.restoreState(state.actualState)
+        return true
     }
 
     /**

components/browser/engine-gecko-nightly/src/test/java/mozilla/components/browser/engine/gecko/GeckoEngineSessionTest.kt

@@ -475,18 +475,18 @@ class GeckoEngineSessionTest {
         val actualState: GeckoSession.SessionState = mock()
         val state = GeckoEngineSessionState(actualState)
 
-        engineSession.restoreState(state)
+        assertTrue(engineSession.restoreState(state))
         verify(geckoSession).restoreState(any())
     }
 
     @Test
-    fun `restoreState does nothing for null state`() {
+    fun `restoreState returns false for null state`() {
         val engineSession = GeckoEngineSession(mock(),
             geckoSessionProvider = geckoSessionProvider)
 
         val state = GeckoEngineSessionState(null)
 
-        engineSession.restoreState(state)
+        assertFalse(engineSession.restoreState(state))
         verify(geckoSession, never()).restoreState(any())
     }
 

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

@@ -152,16 +152,17 @@ class GeckoEngineSession(
     /**
      * See [EngineSession.restoreState]
      */
-    override fun restoreState(state: EngineSessionState) {
+    override fun restoreState(state: EngineSessionState): Boolean {
         if (state !is GeckoEngineSessionState) {
             throw IllegalStateException("Can only restore from GeckoEngineSessionState")
         }
 
         if (state.actualState == null) {
-            return
+            return false
         }
 
         geckoSession.restoreState(state.actualState)
+        return true
     }
 
     /**

components/browser/engine-gecko/src/test/java/mozilla/components/browser/engine/gecko/GeckoEngineSessionTest.kt

@@ -474,18 +474,18 @@ class GeckoEngineSessionTest {
         val actualState: GeckoSession.SessionState = mock()
         val state = GeckoEngineSessionState(actualState)
 
-        engineSession.restoreState(state)
+        assertTrue(engineSession.restoreState(state))
         verify(geckoSession).restoreState(any())
     }
 
     @Test
-    fun `restoreState does nothing for null state`() {
+    fun `restoreState returns false for null state`() {
         val engineSession = GeckoEngineSession(mock(),
             geckoSessionProvider = geckoSessionProvider)
 
         val state = GeckoEngineSessionState(null)
 
-        engineSession.restoreState(state)
+        assertFalse(engineSession.restoreState(state))
         verify(geckoSession, never()).restoreState(any())
     }
 

components/browser/engine-system/src/main/java/mozilla/components/browser/engine/system/SystemEngineSession.kt

@@ -125,12 +125,12 @@ class SystemEngineSession(
     /**
      * See [EngineSession.restoreState]
      */
-    override fun restoreState(state: EngineSessionState) {
+    override fun restoreState(state: EngineSessionState): Boolean {
         if (state !is SystemEngineSessionState) {
             throw IllegalArgumentException("Can only restore from SystemEngineSessionState")
         }
 
-        webView.restoreState(state.bundle)
+        return webView.restoreState(state.bundle) != null
     }
 
     /**

components/browser/engine-system/src/test/java/mozilla/components/browser/engine/system/SystemEngineSessionTest.kt

@@ -206,18 +206,20 @@ class SystemEngineSessionTest {
     @Test
     fun restoreState() {
         val engineSession = spy(SystemEngineSession(testContext))
-        val webView = mock<WebView>()
+        val webView = spy(WebView(testContext))
 
         try {
             engineSession.restoreState(mock())
             fail("Expected IllegalArgumentException")
         } catch (e: IllegalArgumentException) {}
-        engineSession.restoreState(SystemEngineSessionState(Bundle()))
+        assertFalse(engineSession.restoreState(SystemEngineSessionState(Bundle())))
         verify(webView, never()).restoreState(any(Bundle::class.java))
 
         engineSession.webView = webView
+        engineSession.webView.loadUrl("http://example.com")
+        val state = engineSession.saveState()
 
-        engineSession.restoreState(SystemEngineSessionState(Bundle()))
+        assertTrue(engineSession.restoreState(state))
         verify(webView).restoreState(any(Bundle::class.java))
     }
 

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

@@ -273,20 +273,21 @@ class LegacySessionManager(
         getEngineSession(session)?.let { return it }
 
         return engine.createSession(session.private).apply {
+            var restored = false
             session.engineSessionHolder.engineSessionState?.let { state ->
-                restoreState(state)
+                restored = restoreState(state)
                 session.engineSessionHolder.engineSessionState = null
             }
 
-            link(session, this)
+            link(session, this, restored)
         }
     }
 
-    fun link(session: Session, engineSession: EngineSession) {
+    private fun link(session: Session, engineSession: EngineSession, restored: Boolean = false) {
         val parent = values.find { it.id == session.parentId }?.let {
             this.getEngineSession(it)
         }
-        engineSessionLinker.link(session, engineSession, parent)
+        engineSessionLinker.link(session, engineSession, parent, restored)
 
         if (session == selectedSession) {
             engineSession.markActiveForWebExtensions(true)

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

@@ -40,14 +40,21 @@ class SessionManager(
         /**
          * Links the provided [Session] and [EngineSession].
          */
-        fun link(session: Session, engineSession: EngineSession, parentEngineSession: EngineSession?) {
+        fun link(
+            session: Session,
+            engineSession: EngineSession,
+            parentEngineSession: EngineSession?,
+            sessionRestored: Boolean = false
+        ) {
             unlink(session)
 
             session.engineSessionHolder.apply {
                 this.engineSession = engineSession
                 this.engineObserver = EngineObserver(session, store).also { observer ->
                     engineSession.register(observer)
-                    engineSession.loadUrl(session.url, parentEngineSession)
+                    if (!sessionRestored) {
+                        engineSession.loadUrl(session.url, parentEngineSession)
+                    }
                 }
             }
 

components/browser/session/src/test/java/mozilla/components/browser/session/engine/EngineObserverTest.kt

@@ -49,7 +49,7 @@ class EngineObserverTest {
             override fun goForward() {}
             override fun reload() {}
             override fun stopLoading() {}
-            override fun restoreState(state: EngineSessionState) {}
+            override fun restoreState(state: EngineSessionState): Boolean { return false }
             override fun enableTrackingProtection(policy: TrackingProtectionPolicy) {}
             override fun disableTrackingProtection() {}
             override fun toggleDesktopMode(enable: Boolean, reload: Boolean) {
@@ -99,7 +99,7 @@ class EngineObserverTest {
             override fun goForward() {}
             override fun stopLoading() {}
             override fun reload() {}
-            override fun restoreState(state: EngineSessionState) {}
+            override fun restoreState(state: EngineSessionState): Boolean { return false }
             override fun enableTrackingProtection(policy: TrackingProtectionPolicy) {}
             override fun disableTrackingProtection() {}
             override fun toggleDesktopMode(enable: Boolean, reload: Boolean) {}
@@ -138,7 +138,7 @@ class EngineObserverTest {
             override fun goForward() {}
             override fun stopLoading() {}
             override fun reload() {}
-            override fun restoreState(state: EngineSessionState) {}
+            override fun restoreState(state: EngineSessionState): Boolean { return false }
             override fun enableTrackingProtection(policy: TrackingProtectionPolicy) {
                 notifyObservers { onTrackerBlockingEnabledChange(true) }
             }

components/concept/engine/src/main/java/mozilla/components/concept/engine/EngineSession.kt

@@ -453,8 +453,10 @@ abstract class EngineSession(
      * Restores the engine state as provided by [saveState].
      *
      * @param state state retrieved from [saveState]
+     * @return true if the engine session has successfully been restored with the provided state,
+     * false otherwise.
      */
-    abstract fun restoreState(state: EngineSessionState)
+    abstract fun restoreState(state: EngineSessionState): Boolean
 
     /**
      * Enables tracking protection for this engine session.

components/concept/engine/src/test/java/mozilla/components/concept/engine/EngineSessionTest.kt

@@ -697,7 +697,7 @@ open class DummyEngineSession : EngineSession() {
     override val settings: Settings
         get() = mock(Settings::class.java)
 
-    override fun restoreState(state: EngineSessionState) {}
+    override fun restoreState(state: EngineSessionState): Boolean { return false }
 
     override fun saveState(): EngineSessionState { return mock() }