Fix up service worker tab navigation issues (#1551)

* Fix up service worker tab navigation issues * Fix up targeting code * Clean up
duckduckgo · Nov 23, 2022 · b2daa68 · b2daa68
1 parent b2ff783
commit b2daa68
Show file tree

Hide file tree

Showing 2 changed files with 26 additions and 27 deletions.
diff --git a/shared/js/background/before-request.es6.js b/shared/js/background/before-request.es6.js
@@ -84,7 +84,7 @@ function handleRequest (requestData) {
     // Skip requests to background tabs
     if (tabId === -1) { return }
 
-    let thisTab = tabManager.get(requestData)
+    const thisTab = tabManager.get(requestData)
 
     // control access to web accessible resources
     if (requestData.url.startsWith(browserWrapper.getExtensionURL('/web_accessible_resources'))) {
@@ -93,19 +93,7 @@ function handleRequest (requestData) {
         }
     }
 
-    // For main_frame requests: create a new tab instance whenever we either
-    // don't have a tab instance for this tabId or this is a new requestId.
     if (requestData.type === 'main_frame') {
-        if (!thisTab || thisTab.requestId !== requestData.requestId) {
-            const newTab = tabManager.create(requestData)
-
-            // persist the last URL the tab was trying to upgrade to HTTPS
-            if (thisTab && thisTab.httpsRedirects) {
-                newTab.httpsRedirects.persistMainFrameRedirect(thisTab.httpsRedirects.getMainFrameRedirect())
-            }
-            thisTab = newTab
-        }
-
         let mainFrameRequestURL = new URL(requestData.url)
 
         // AMP protection

diff --git a/shared/js/background/events.es6.js b/shared/js/background/events.es6.js
@@ -250,35 +250,46 @@ startup.ready().then(() => {
     )
 })
 
+// Store the created tab id for when onBeforeNavigate is called so data can be copied across from the source tab
+const createdTargets = new Map()
 browser.webNavigation.onCreatedNavigationTarget.addListener(details => {
-    const currentTab = tabManager.get({ tabId: details.sourceTabId })
-    if (currentTab && currentTab.adClick) {
-        const newTab = tabManager.createOrUpdateTab(details.tabId, { url: details.url })
-        if (currentTab.adClick.shouldPropagateAdClickForNewTab(newTab)) {
-            newTab.adClick = currentTab.adClick.propagate(newTab.id)
-        }
-    }
+    createdTargets.set(details.tabId, details.sourceTabId)
 })
 
 /**
  * Web Navigation
  */
 // keep track of URLs that the browser navigates to.
 //
-// this is currently meant to supplement tabManager.updateTabUrl() above:
+// this is supplemented by tabManager.updateTabUrl() on headersReceived:
 // tabManager.updateTabUrl only fires when a tab has finished loading with a 200,
 // which misses a couple of edge cases like browser special pages
 // and Gmail's weird redirect which returns a 200 via a service worker
-browser.webNavigation.onCommitted.addListener(details => {
+browser.webNavigation.onBeforeNavigate.addListener(details => {
     // ignore navigation on iframes
     if (details.frameId !== 0) return
 
-    const tab = tabManager.get({ tabId: details.tabId })
-
-    if (!tab) return
+    const currentTab = tabManager.get({ tabId: details.tabId })
+    const newTab = tabManager.create({ tabId: details.tabId, url: details.url })
+    // persist the last URL the tab was trying to upgrade to HTTPS
+    if (currentTab && currentTab.httpsRedirects) {
+        newTab.httpsRedirects.persistMainFrameRedirect(currentTab.httpsRedirects.getMainFrameRedirect())
+    }
+    if (createdTargets.has(details.tabId)) {
+        const sourceTabId = createdTargets.get(details.tabId)
+        createdTargets.delete(details.tabId)
+
+        const sourceTab = tabManager.get({ tabId: sourceTabId })
+        if (sourceTab && sourceTab.adClick) {
+            createdTargets.set(details.tabId, sourceTabId)
+            if (sourceTab.adClick.shouldPropagateAdClickForNewTab(newTab)) {
+                newTab.adClick = sourceTab.adClick.propagate(newTab.id)
+            }
+        }
+    }
 
-    tab.updateSite(details.url)
-    devtools.postMessage(details.tabId, 'tabChange', devtools.serializeTab(tab))
+    newTab.updateSite(details.url)
+    devtools.postMessage(details.tabId, 'tabChange', devtools.serializeTab(newTab))
 })
 
 /**