[fenix] Closes https://github.com/mozilla-mobile/fenix/issues/22484 - Track direct loads per tab; fix search term lookup

This patch fixes two problems:
1) We were treating "direct tab load" as an event which applies
   uniformally to all tabs, even though it's actually an event which
   happens for a specific tab. This lead to background tabs (pages opened as new tab)
   setting the direct load flag, and then a simultaneously loading
   parent tab would incorrectly interpret that flag for itself.

   The patch switches this tracking from a simple boolean (are we direct
   loading?) to a set of tab IDs that are currently direct loading.

2) In a case when a background tab was loading with a parent who's
   search terms were cleared by a direct load, we were not trying to
   lookup search terms on the background tab's historyMetadata key,
   which exists to capture search terms for this exact scenario.

   The patch adds an additional fallback lookup for that path.

app/src/main/java/org/mozilla/fenix/historymetadata/HistoryMetadataMiddleware.kt

@@ -31,9 +31,9 @@ class HistoryMetadataMiddleware(
 
     private val logger = Logger("HistoryMetadataMiddleware")
 
-    // Tracks whether a page load is in progress that was triggered directly by the app
+    // Tracks whether a load is in progress for a tab/session ID that was triggered directly by the app
     // e.g. via the toolbar as opposed to via web content.
-    private var directLoadTriggered: Boolean = false
+    private var directLoadTriggeredSet = mutableSetOf<String>()
 
     @Suppress("ComplexMethod")
     override fun invoke(
@@ -90,11 +90,13 @@ class HistoryMetadataMiddleware(
                     }
                 }
             }
-            is EngineAction.LoadUrlAction,
-            is EngineAction.OptimizedLoadUrlTriggeredAction -> {
+            is EngineAction.LoadUrlAction -> {
                 // This isn't an ideal fix as we shouldn't have to hold any state in the middleware:
                 // https://github.com/mozilla-mobile/android-components/issues/11034
-                directLoadTriggered = true
+                directLoadTriggeredSet.add(action.tabId)
+            }
+            is EngineAction.OptimizedLoadUrlTriggeredAction -> {
+                directLoadTriggeredSet.add(action.tabId)
             }
         }
 
@@ -116,7 +118,7 @@ class HistoryMetadataMiddleware(
                 }
 
                 // Once we get a history update let's reset the flag for future loads.
-                directLoadTriggered = false
+                directLoadTriggeredSet.remove(action.sessionId)
             }
             // NB: this could be called bunch of times in quick succession.
             is MediaSessionAction.UpdateMediaMetadataAction -> {
@@ -150,6 +152,7 @@ class HistoryMetadataMiddleware(
         val tabParent = tab.getParent(context.store)
         val previousUrlIndex = tab.content.history.currentIndex - 1
         val tabMetadataHasSearchTerms = !tab.historyMetadata?.searchTerm.isNullOrBlank()
+        val directLoadTriggered = directLoadTriggeredSet.contains(tab.id)
 
         // Obtain search terms and referrer url either from tab parent, from the history stack, or
         // from the tab itself.
@@ -200,7 +203,9 @@ class HistoryMetadataMiddleware(
                 tab.historyMetadata?.searchTerm to tab.historyMetadata?.referrerUrl
             }
             // In all other cases (e.g. direct load) find search terms by checking if page is a SERP
-            else -> findSearchTerms(tab, context.state.search) to null
+            else -> {
+                findSearchTerms(tab, context.state.search) to null
+            }
         }
 
         // Sanity check to make sure we don't record a metadata record referring to itself.
@@ -226,6 +231,18 @@ class HistoryMetadataMiddleware(
     }
 
     private fun findSearchTerms(tab: TabSessionState, searchState: SearchState): String? {
-        return tab.content.searchTerms.takeUnless { it.isEmpty() } ?: searchState.parseSearchTerms(tab.content.url)
+        // Only check for search terms in metadata if we're not direct loading this tab. If we are,
+        // we don't retain previous search terms.
+        // `tab.content.searchTerms` are cleared as a side-effect of performing a direct load.
+        val metadataSearchTerms: () -> String? = {
+            if (!directLoadTriggeredSet.contains(tab.id)) {
+                tab.historyMetadata?.searchTerm.takeUnless { it.isNullOrEmpty() }
+            } else {
+                null
+            }
+        }
+        return tab.content.searchTerms.takeUnless { it.isEmpty() }
+            ?: metadataSearchTerms()
+            ?: searchState.parseSearchTerms(tab.content.url)
     }
 }

app/src/test/java/org/mozilla/fenix/historymetadata/HistoryMetadataMiddlewareTest.kt

@@ -14,6 +14,7 @@ import mozilla.components.browser.state.action.EngineAction
 import mozilla.components.browser.state.action.MediaSessionAction
 import mozilla.components.browser.state.action.SearchAction
 import mozilla.components.browser.state.action.TabListAction
+import mozilla.components.browser.state.action.HistoryMetadataAction
 import mozilla.components.browser.state.engine.EngineMiddleware
 import mozilla.components.browser.state.search.SearchEngine
 import mozilla.components.browser.state.selector.findTab
@@ -233,6 +234,68 @@ class HistoryMetadataMiddlewareTest {
         }
     }
 
+    @Test
+    fun `GIVEN tab opened as new tab from a search page WHEN it loads while parent navigates to a result THEN parent will retain its search terms`() {
+        service = TestingMetadataService()
+        middleware = HistoryMetadataMiddleware(service)
+        store = BrowserStore(
+            middleware = listOf(middleware) + EngineMiddleware.create(engine = mockk()),
+            initialState = BrowserState()
+        )
+        setupGoogleSearchEngine()
+
+        val parentTab = createTab("https://google.com?q=mozilla+website", searchTerms = "mozilla website")
+        val tab = createTab("https://google.com?url=https://mozilla.org", parent = parentTab)
+        store.dispatch(TabListAction.AddTabAction(parentTab, select = true)).joinBlocking()
+        store.dispatch(TabListAction.AddTabAction(tab)).joinBlocking()
+
+        with((service as TestingMetadataService).createdMetadata) {
+            assertEquals(2, this.count())
+            assertEquals("https://google.com?q=mozilla+website", this[0].url)
+            assertEquals("mozilla website", this[0].searchTerm)
+            assertNull(this[0].referrerUrl)
+
+            assertEquals("https://google.com?url=https://mozilla.org", this[1].url)
+            assertEquals("mozilla website", this[1].searchTerm)
+            assertEquals("https://google.com?q=mozilla+website", this[1].referrerUrl)
+        }
+
+        // Parent tab loads.
+        store.dispatch(ContentAction.UpdateHistoryStateAction(parentTab.id, listOf(HistoryItem("Google - mozilla website", "https://google.com?q=mozilla+website")), 0)).joinBlocking()
+        with((service as TestingMetadataService).createdMetadata) {
+            assertEquals(2, this.count())
+        }
+
+        // Simulate a state where search metadata is missing for the child tab.
+        store.dispatch(HistoryMetadataAction.SetHistoryMetadataKeyAction(tab.id, HistoryMetadataKey("https://google.com?url=https://mozilla.org", null, null))).joinBlocking()
+
+        // Parent navigates away, while the child starts loading. A mostly realistic sequence of events...
+        store.dispatch(ContentAction.UpdateUrlAction(parentTab.id, "https://firefox.com")).joinBlocking()
+        store.dispatch(ContentAction.UpdateSearchTermsAction(parentTab.id, "")).joinBlocking()
+        store.dispatch(EngineAction.LoadUrlAction(tab.id, "https://google.com?url=https://mozilla.org")).joinBlocking()
+        store.dispatch(ContentAction.UpdateHistoryStateAction(tab.id, listOf(HistoryItem("", "https://google.com?url=https://mozilla.org")), currentIndex = 0)).joinBlocking()
+        store.dispatch(ContentAction.UpdateUrlAction(tab.id, "https://mozilla.org")).joinBlocking()
+        store.dispatch(ContentAction.UpdateHistoryStateAction(tab.id, listOf(HistoryItem("Mozilla", "https://mozilla.org")), currentIndex = 0)).joinBlocking()
+        store.dispatch(ContentAction.UpdateHistoryStateAction(parentTab.id, listOf(HistoryItem("Google - mozilla website", "https://google.com?q=mozilla+website"), HistoryItem("Firefox", "https://firefox.com")), 1)).joinBlocking()
+
+        with((service as TestingMetadataService).createdMetadata) {
+            assertEquals(4, this.count())
+            assertEquals("https://firefox.com", this[3].url)
+            assertEquals("mozilla website", this[3].searchTerm)
+            assertEquals("https://google.com?q=mozilla+website", this[3].referrerUrl)
+
+            assertEquals("https://google.com?url=https://mozilla.org", this[1].url)
+            assertEquals("mozilla website", this[1].searchTerm)
+            assertEquals("https://google.com?q=mozilla+website", this[1].referrerUrl)
+
+            assertEquals("https://mozilla.org", this[2].url)
+            assertEquals("mozilla website", this[2].searchTerm)
+            // This is suspect. The parent tab switched away right before the child loaded, so the
+            // referrer here is potentially bogus.
+            assertEquals("https://firefox.com", this[2].referrerUrl)
+        }
+    }
+
     @Test
     fun `GIVEN tab with search terms WHEN subsequent direct load occurs THEN search terms are not retained`() {
         service = TestingMetadataService()