After tab close, set next active tab immediately, and only if differe…

…nt from the tab muon will set.

Avoids race condition that can occur after will-destory, before appState has been fully updated with any tab details that change from muon as a result of a tab closing.

Fix #11981

app/browser/activeTabHistory.js

@@ -0,0 +1,58 @@
+// static
+const activeTabsByWindow = new Map()
+
+const api = {
+
+  /**
+   * Inform this store that a tab is active for a window.
+   * This information will be added to index 0 in the
+   * active-tab history for the window.
+   */
+  setActiveTabForWindow: function (windowId, tabId) {
+    const existing = activeTabsByWindow.get(windowId)
+    if (existing) {
+      existing.unshift(tabId)
+    } else {
+      activeTabsByWindow.set(windowId, [ tabId ])
+    }
+  },
+
+  /**
+   * Retrieve the tabId that was active at the specified history index,
+   * 0 for most recent (default)
+   */
+  getActiveTabForWindow: function (windowId, historyIndex = 0) {
+    // get history of active tabs for specified window
+    const windowActiveTabs = activeTabsByWindow.get(windowId)
+    // handle no history for specified window
+    if (!windowActiveTabs || !windowActiveTabs.length) {
+      return null
+    }
+    // verify specified index in active-tab history exists
+    const lastIndex = windowActiveTabs.length - 1
+    if (lastIndex < historyIndex) {
+      return null
+    }
+    // get tabId at specified index in active-tab history
+    return windowActiveTabs[historyIndex]
+  },
+
+  /**
+   * Removes specified tab from active-tab history in specified window
+   */
+  clearTabFromWindow: function (windowId, tabId) {
+    const windowActiveTabs = activeTabsByWindow.get(windowId)
+    if (windowActiveTabs && windowActiveTabs.length) {
+      activeTabsByWindow.set(windowId, windowActiveTabs.filter(previousTabId => previousTabId !== tabId))
+    }
+  },
+
+  /**
+   * Forget history of active tabs for specified window
+   */
+  clearTabbedWindow: function (windowId) {
+    activeTabsByWindow.delete(windowId)
+  }
+}
+
+module.exports = api

app/browser/reducers/tabsReducer.js

@@ -201,8 +201,6 @@ const tabsReducer = (state, action, immutableAction) => {
         if (tabId === tabState.TAB_ID_NONE) {
           break
         }
-        const nextActiveTabId = tabs.getNextActiveTab(state, tabId)
-
         // Must be called before tab is removed
         // But still check for no tabId because on tab detach there's a dummy tabId
         const tabValue = tabState.getByTabId(state, tabId)
@@ -211,11 +209,6 @@ const tabsReducer = (state, action, immutableAction) => {
           state = tabs.updateTabsStateForWindow(state, windowIdOfTabBeingRemoved)
         }
         state = tabState.removeTabByTabId(state, tabId)
-        setImmediate(() => {
-          if (nextActiveTabId !== tabState.TAB_ID_NONE) {
-            tabs.setActive(nextActiveTabId)
-          }
-        })
         tabs.forgetTab(tabId)
       }
       break

app/browser/tabs.js

@@ -8,7 +8,6 @@ const tabActions = require('../common/actions/tabActions')
 const config = require('../../js/constants/config')
 const Immutable = require('immutable')
 const tabState = require('../common/state/tabState')
-const windowState = require('../common/state/windowState')
 const {app, BrowserWindow, extensions, session, ipcMain} = require('electron')
 const {makeImmutable, makeJS} = require('../common/state/immutableUtil')
 const {getTargetAboutUrl, getSourceAboutUrl, isSourceAboutUrl, newFrameUrl, isTargetAboutUrl, isIntermediateAboutPage, isTargetMagnetUrl, getSourceMagnetUrl} = require('../../js/lib/appUrlUtil')
@@ -27,7 +26,7 @@ const appStore = require('../../js/stores/appStore')
 const appConfig = require('../../js/constants/appConfig')
 const {newTabMode} = require('../common/constants/settingsEnums')
 const {tabCloseAction} = require('../common/constants/settingsEnums')
-const {cleanupWebContents, currentWebContents, getWebContents, updateWebContents} = require('./webContentsCache')
+const {cleanupWebContents, currentWebContents, getWebContents, getOpenerTabId, updateWebContents} = require('./webContentsCache')
 const {FilterOptions} = require('ad-block')
 const {isResourceEnabled} = require('../filtering')
 const autofill = require('../autofill')
@@ -38,6 +37,7 @@ const siteSettingsState = require('../common/state/siteSettingsState')
 const bookmarkOrderCache = require('../common/cache/bookmarkOrderCache')
 const ledgerState = require('../common/state/ledgerState')
 const {getWindow} = require('./windows')
+const activeTabHistory = require('./activeTabHistory')
 
 let adBlockRegions
 let currentPartitionNumber = 0
@@ -462,7 +462,7 @@ const api = {
       }
 
       const tabId = newTab.getId()
-      updateWebContents(tabId, newTab)
+      updateWebContents(tabId, newTab, openerTabId)
       let newTabValue = getTabValue(newTab.getId())
 
       let windowId
@@ -569,10 +569,25 @@ const api = {
         appActions.tabMoved(tabId)
       })
 
-      tab.on('will-attach', () => {
+      tab.on('will-attach', (e, windowWebContents) => {
         appActions.tabWillAttach(tab.getId())
       })
 
+      tab.on('set-active', (sender, isActive) => {
+        if (isActive) {
+          const tabValue = getTabValue(tabId)
+          if (tabValue) {
+            const windowId = tabValue.get('windowId')
+            // set-active could be called multiple times even when the index does not change
+            // so make sure we only add this to the active-tab trail for the window
+            // once
+            if (activeTabHistory.getActiveTabForWindow(windowId, 0) !== tabId) {
+              activeTabHistory.setActiveTabForWindow(windowId, tabId)
+            }
+          }
+        }
+      })
+
       tab.on('tab-strip-empty', () => {
         // It's only safe to close a window when the last web-contents tab has been
         // re-attached.  A detach which already happens by this point is not enough.
@@ -586,7 +601,17 @@ const api = {
         })
       })
 
-      tab.on('did-attach', () => {
+      tab.on('did-detach', (e, oldTabId) => {
+        // forget last active trail in window tab
+        // is detaching from
+        const oldTab = getTabValue(oldTabId)
+        const detachedFromWindowId = oldTab.get('windowId')
+        if (detachedFromWindowId != null) {
+          activeTabHistory.clearTabFromWindow(detachedFromWindowId, oldTabId)
+        }
+      })
+
+      tab.on('did-attach', (e, tabId) => {
         appActions.tabAttached(tab.getId())
       })
 
@@ -604,10 +629,24 @@ const api = {
         }
       })
 
-      tab.once('will-destroy', () => {
+      tab.once('will-destroy', (e) => {
         const tabValue = getTabValue(tabId)
         if (tabValue) {
           const windowId = tabValue.get('windowId')
+          // forget about this tab in the history of active tabs
+          activeTabHistory.clearTabFromWindow(windowId, tabId)
+          // handle closed tab being the current active tab for window
+          if (tabValue.get('active')) {
+            // set the next active tab, if different from what muon will have set to
+            // Muon sets it to the next index (immediately above or below)
+            // But this app can be configured to select the parent tab,
+            // or the last active tab
+            let nextTabId = api.getNextActiveTabId(windowId, tabId)
+            if (nextTabId != null) {
+              api.setActive(nextTabId)
+            }
+          }
+          // let the state know
           appActions.tabClosed(tabId, windowId)
         }
       })
@@ -632,10 +671,10 @@ const api = {
 
   sendToAll: (...args) => {
     for (let tabId in currentWebContents) {
-      const tab = currentWebContents[tabId]
+      const tabData = currentWebContents[tabId]
       try {
-        if (tab && !tab.isDestroyed()) {
-          tab.send(...args)
+        if (tabData && tabData.tab && !tabData.tab.isDestroyed()) {
+          tabData.tab.send(...args)
         }
       } catch (e) {
         // ignore exceptions
@@ -915,22 +954,30 @@ const api = {
         // If the current tab is pinned, then don't allow to drag out
         return
       }
-      const nextActiveTabIdForOldWindow = api.getNextActiveTab(state, tabId)
+
+      // detach from current window
       tab.detach(() => {
+        // handle tab has detached from window
+        // handle tab was the active tab of the window
+        if (tabValue.get('active')) {
+          // decide and set next-active-tab muon override
+          const nextActiveTabIdForOldWindow = api.getNextActiveTabId(currentWindowId, tabId)
+          if (nextActiveTabIdForOldWindow !== null) {
+            api.setActive(nextActiveTabIdForOldWindow)
+          }
+        }
         if (toWindowId == null || toWindowId === -1) {
+          // move tab to a new window
           frameOpts = frameOpts.set('index', 0)
           appActions.newWindow(frameOpts, browserOpts)
         } else {
+          // ask for tab to be attached (via frame state and webview) to
+          // specified window
           appActions.newWebContentsAdded(toWindowId, frameOpts, tabValue)
         }
-
-        // Setting the next active tab for the old window must happen after re-attach of the new tab.
-        // This is because muon's tab_strip index for the tab would not be consistent with browser-laptop's
-        // expectation and it would try to set an invalid index as active, possibly leaivng nothing active.
+        // handle tab has made it to the new window
         tab.once('did-attach', () => {
-          if (nextActiveTabIdForOldWindow !== tabState.TAB_ID_NONE) {
-            api.setActive(nextActiveTabIdForOldWindow)
-          }
+          // put the tab in the desired index position
           const index = frameOpts.get('index')
           if (index !== undefined) {
             api.setTabIndex(tabId, frameOpts.get('index'))
@@ -1036,51 +1083,24 @@ const api = {
     return null
   },
 
-  getNextActiveTab: (state, closeTabId) => {
-    if (!tabState.getByTabId(state, closeTabId)) {
-      return
-    }
-
-    const index = tabState.getIndex(state, closeTabId)
-    if (index === -1) {
-      return
-    }
-
-    const windowId = tabState.getWindowId(state, closeTabId)
-    if (windowId === windowState.WINDOW_ID_NONE) {
-      return
-    }
+  getNextActiveTabId: (windowId, closedTabId) => {
+    const nextTabActionType = getSetting(settings.TAB_CLOSE_ACTION)
 
-    const lastActiveTabId = tabState.getTabsByLastActivated(state, windowId).last()
-    if (lastActiveTabId !== closeTabId && !tabState.isActive(state, closeTabId)) {
-      return
+    if (nextTabActionType === tabCloseAction.LAST_ACTIVE) {
+      return activeTabHistory.getActiveTabForWindow(windowId)
     }
 
-    let nextTabId = tabState.TAB_ID_NONE
-    switch (getSetting(settings.TAB_CLOSE_ACTION)) {
-      case tabCloseAction.LAST_ACTIVE:
-        nextTabId = tabState.getLastActiveTabId(state, windowId)
-        break
-      case tabCloseAction.PARENT:
-        {
-          const openerTabId = tabState.getOpenerTabId(state, closeTabId)
-          if (openerTabId !== tabState.TAB_ID_NONE) {
-            nextTabId = openerTabId
-          }
-          break
+    if (nextTabActionType === tabCloseAction.PARENT) {
+      const parentTabId = getOpenerTabId(closedTabId)
+      if (parentTabId != null) {
+        const parentTab = getWebContents(parentTabId)
+        if (parentTab && !parentTab.isDestroyed()) {
+          return parentTabId
         }
-    }
-
-    // DEFAULT: always fall back to NEXT
-    if (nextTabId === tabState.TAB_ID_NONE) {
-      nextTabId = tabState.getNextTabIdByIndex(state, windowId, index)
-      if (nextTabId === tabState.TAB_ID_NONE) {
-        // no unpinned tabs so find the next pinned tab
-        nextTabId = tabState.getNextTabIdByIndex(state, windowId, index, true)
       }
     }
 
-    return nextTabId
+    return null
   },
 
   closeTabPage: (state, windowId, tabPageIndex) => {

app/browser/webContentsCache.js

@@ -11,16 +11,23 @@ const cleanupWebContents = (tabId) => {
 }
 
 const getWebContents = (tabId) => {
-  return currentWebContents[tabId]
+  const tabData = currentWebContents[tabId]
+  return tabData ? tabData.tab : null
 }
 
-const updateWebContents = (tabId, tab) => {
-  currentWebContents[tabId] = tab
+const getOpenerTabId = (tabId) => {
+  const tabData = currentWebContents[tabId]
+  return tabData ? tabData.openerTabId : null
+}
+
+const updateWebContents = (tabId, tab, openerTabId) => {
+  currentWebContents[tabId] = { tab, openerTabId }
 }
 
 module.exports = {
   cleanupWebContents,
   getWebContents,
+  getOpenerTabId,
   updateWebContents,
   currentWebContents
 }

app/browser/windows.js

@@ -22,6 +22,7 @@ const platformUtil = require('../common/lib/platformUtil')
 const windowState = require('../common/state/windowState')
 const pinnedSitesState = require('../common/state/pinnedSitesState')
 const {zoomLevel} = require('../common/constants/toolbarUserInterfaceScale')
+const activeTabHistory = require('./activeTabHistory')
 
 const isDarwin = platformUtil.isDarwin()
 const {app, BrowserWindow, ipcMain} = electron
@@ -43,6 +44,7 @@ const getWindowState = (win) => {
 
 const cleanupWindow = (windowId) => {
   delete currentWindows[windowId]
+  activeTabHistory.clearTabbedWindow(windowId)
 }
 
 const getWindowValue = (windowId) => {