18 Unbelievable ways to get rid of dead tabs on tab close

Fix #11028
Fix #10696
Fix #10905
Fix #10749

app/browser/reducers/tabsReducer.js

@@ -98,6 +98,27 @@ const tabsReducer = (state, action, immutableAction) => {
       })
       break
     }
+    case appConstants.APP_TAB_PAGE_CLOSE_MENU_ITEM_CLICKED: {
+      const windowId = action.get('windowId')
+      const tabPageIndex = action.get('tabPageIndex')
+      state = tabs.closeTabPage(state, windowId, tabPageIndex)
+      break
+    }
+    case appConstants.APP_CLOSE_TABS_TO_LEFT_MENU_ITEM_CLICKED: {
+      const tabId = action.get('tabId')
+      state = tabs.closeTabsToLeft(state, tabId)
+      break
+    }
+    case appConstants.APP_CLOSE_TABS_TO_RIGHT_MENU_ITEM_CLICKED: {
+      const tabId = action.get('tabId')
+      state = tabs.closeTabsToRight(state, tabId)
+      break
+    }
+    case appConstants.APP_CLOSE_OTHER_TABS_MENU_ITEM_CLICKED: {
+      const tabId = action.get('tabId')
+      state = tabs.closeOtherTabs(state, tabId)
+      break
+    }
     case appConstants.APP_CREATE_TAB_REQUESTED:
       if (action.getIn(['createProperties', 'windowId']) == null) {
         const senderWindowId = action.getIn(['senderWindowId'])
@@ -147,7 +168,7 @@ const tabsReducer = (state, action, immutableAction) => {
               tabs.toggleDevTools(tabId)
             })
           } else {
-            // This check is only needed because sometimes front end can try to close
+            // This check is only needed in case front end tries to close
             // a tabId it thinks exists but doesn't actually exist anymore.
             const tabValue = tabState.getByTabId(state, tabId)
             if (!tabValue) {
@@ -190,6 +211,7 @@ const tabsReducer = (state, action, immutableAction) => {
             tabs.setActive(nextActiveTabId)
           }
         })
+        tabs.forgetTab(tabId)
       }
       break
     case appConstants.APP_ALLOW_FLASH_ONCE:

app/browser/tabs.js

@@ -1,7 +1,6 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this file,
  * You can obtain one at http://mozilla.org/MPL/2.0/. */
-
 const appActions = require('../../js/actions/appActions')
 const windowActions = require('../../js/actions/windowActions')
 const tabActions = require('../common/actions/tabActions')
@@ -430,11 +429,6 @@ const api = {
       updateTab(tabId, changeInfo)
     })
 
-    process.on('chrome-tabs-removed', (tabId, windowId) => {
-      appActions.tabClosed(tabId, windowId)
-      cleanupWebContents(tabId)
-    })
-
     app.on('web-contents-created', function (event, tab) {
       if (tab.isBackgroundPage() || !tab.isGuest()) {
         return
@@ -526,6 +520,14 @@ const api = {
           windowActions.gotResponseDetails(tabId, {status, newURL, originalURL, httpResponseCode, requestMethod, referrer, headers, resourceType})
         }
       })
+
+      tab.once('will-destroy', () => {
+        const tabValue = getTabValue(tabId)
+        if (tabValue) {
+          const windowId = tabValue.get('windowId')
+          appActions.tabClosed(tabId, windowId)
+        }
+      })
     })
 
     process.on('on-tab-created', (tab, options) => {
@@ -985,6 +987,86 @@ const api = {
 
     return nextTabId
   },
+
+  closeTabPage: (state, windowId, tabPageIndex) => {
+    const tabsPerPage = Number(getSetting(settings.TABS_PER_PAGE))
+    const startTabIndex = tabPageIndex * tabsPerPage
+    const pinnedTabsCount = tabState.getPinnedTabsByWindowId(state, windowId).size
+    tabState.getTabsByWindowId(state, windowId)
+      .sort((tab1, tab2) => tab1.get('index') - tab2.get('index'))
+      .filter((tabValue) => !tabValue.get('pinned'))
+      .slice(startTabIndex + pinnedTabsCount, startTabIndex + tabsPerPage + pinnedTabsCount)
+      .forEach((tabValue) => {
+        const tab = getWebContents(tabValue.get('tabId'))
+        if (tab && !tab.isDestroyed()) {
+          tab.forceClose()
+        }
+      })
+    state = api.updateTabsStateForWindow(state, windowId)
+    return state
+  },
+
+  closeTabsToLeft: (state, tabId) => {
+    const tabValue = tabState.getByTabId(state, tabId)
+    if (!tabValue) {
+      return state
+    }
+    const index = tabValue.get('index')
+    const windowId = tabValue.get('windowId')
+    const pinnedTabsCount = tabState.getPinnedTabsByWindowId(state, windowId).size
+    tabState.getTabsByWindowId(state, windowId)
+      .sort((tab1, tab2) => tab1.get('index') - tab2.get('index'))
+      .filter((tabValue) => !tabValue.get('pinned'))
+      .slice(0, index - pinnedTabsCount)
+      .forEach((tabValue) => {
+        const tab = getWebContents(tabValue.get('tabId'))
+        if (tab && !tab.isDestroyed()) {
+          tab.forceClose()
+        }
+      })
+    state = api.updateTabsStateForWindow(state, windowId)
+    return state
+  },
+
+  closeTabsToRight: (state, tabId) => {
+    const tabValue = tabState.getByTabId(state, tabId)
+    if (!tabValue) {
+      return state
+    }
+    const index = tabValue.get('index')
+    const windowId = tabValue.get('windowId')
+    const pinnedTabsCount = tabState.getPinnedTabsByWindowId(state, windowId).size
+    tabState.getTabsByWindowId(state, windowId)
+      .sort((tab1, tab2) => tab1.get('index') - tab2.get('index'))
+      .filter((tabValue) => !tabValue.get('pinned'))
+      .slice(index + 1 - pinnedTabsCount)
+      .forEach((tabValue) => {
+        const tab = getWebContents(tabValue.get('tabId'))
+        if (tab && !tab.isDestroyed()) {
+          tab.forceClose()
+        }
+      })
+    state = api.updateTabsStateForWindow(state, windowId)
+    return state
+  },
+
+  closeOtherTabs: (state, tabId) => {
+    const tabValue = tabState.getByTabId(state, tabId)
+    if (!tabValue) {
+      return state
+    }
+    const windowId = tabValue.get('windowId')
+    tabState.getTabsByWindowId(state, windowId)
+      .forEach((tabValue) => {
+        const tab = getWebContents(tabValue.get('tabId'))
+        if (tab && !tab.isDestroyed() && tabValue.get('tabId') !== tabId && !tabValue.get('pinned')) {
+          tab.forceClose()
+        }
+      })
+    state = api.updateTabsStateForWindow(state, windowId)
+    return state
+  },
+
   debugTabs: (state) => {
     console.log(tabState.getTabs(state)
       .toJS()
@@ -1042,6 +1124,9 @@ const api = {
       }
     })
     return state
+  },
+  forgetTab: (tabId) => {
+    cleanupWebContents(tabId)
   }
 }
 

app/renderer/components/frame/frame.js

@@ -67,7 +67,6 @@ function isTorrentViewerURL (url) {
 class Frame extends React.Component {
   constructor (props) {
     super(props)
-    this.onCloseFrame = this.onCloseFrame.bind(this)
     this.onUpdateWheelZoom = debounce(this.onUpdateWheelZoom.bind(this), 20)
     this.onFocus = this.onFocus.bind(this)
     // Maps notification message to its callback
@@ -88,10 +87,6 @@ class Frame extends React.Component {
     return appStoreRenderer.state.get('tabs').find((tab) => tab.get('tabId') === frame.get('tabId'))
   }
 
-  onCloseFrame () {
-    windowActions.closeFrame(this.props.frameKey)
-  }
-
   isAboutPage () {
     return aboutUrls.get(getBaseUrl(this.props.location))
   }
@@ -484,9 +479,6 @@ class Frame extends React.Component {
     this.webview.addEventListener('mouseleave', (e) => {
       windowActions.onFrameMouseLeave(this.props.tabId)
     }, { passive: true })
-    this.webview.addEventListener('will-destroy', (e) => {
-      this.onCloseFrame()
-    }, { passive: true })
     this.webview.addEventListener('page-favicon-updated', (e) => {
       if (this.frame.isEmpty()) {
         return

app/renderer/components/main/main.js

@@ -373,10 +373,6 @@ class Main extends React.Component {
 
     ipc.on(messages.SHORTCUT_UNDO_CLOSED_FRAME, () => windowActions.undoClosedFrame())
 
-    ipc.on(messages.SHORTCUT_CLOSE_OTHER_FRAMES, (e, tabId, isCloseRight, isCloseLeft) => {
-      windowActions.closeOtherFrames(tabId, isCloseRight, isCloseLeft)
-    })
-
     ipc.on(messages.SHOW_DOWNLOADS_TOOLBAR, () => {
       windowActions.setDownloadsToolbarVisible(true)
     })

app/renderer/reducers/contextMenuReducer.js

@@ -38,7 +38,7 @@ const urlUtil = require('../../../js/lib/urlutil')
 const frameStateUtil = require('../../../js/state/frameStateUtil')
 const dndData = require('../../../js/dndData')
 const {makeImmutable, isMap} = require('../../common/state/immutableUtil')
-const {getCurrentWindow} = require('../../renderer/currentWindow')
+const {getCurrentWindow, getCurrentWindowId} = require('../../renderer/currentWindow')
 const {getSetting} = require('../../../js/settings')
 
 const validateAction = function (action) {
@@ -93,9 +93,7 @@ const onTabPageMenu = function (state, action) {
   }, {
     label: locale.translation('closeTabPage'),
     click: () => {
-      tabPageFrames
-        .map((frame) => frame.get('tabId'))
-        .forEach((tabId) => appActions.tabCloseRequested(tabId))
+      appActions.tabPageCloseMenuItemClicked(getCurrentWindowId(), index)
     }
   }]
 

app/renderer/reducers/frameReducer.js

@@ -79,22 +79,40 @@ const getLocation = (location) => {
 
 const frameReducer = (state, action, immutableAction) => {
   switch (action.actionType) {
+    case appConstants.APP_TAB_CLOSED: {
+      const tabId = immutableAction.get('tabId')
+      const frame = frameStateUtil.getFrameByTabId(state, tabId)
+      if (frame) {
+        action.frameKey = frame.get('key')
+        state = closeFrame(state, action)
+        const activeFrame = frameStateUtil.getActiveFrame(state)
+        if (activeFrame) {
+          state = frameStateUtil.updateTabPageIndex(state, activeFrame.get('tabId'))
+        }
+      }
+      break
+    }
     case appConstants.APP_TAB_UPDATED:
       // This case will be fired for both tab creation and tab update.
-      // being `tabValue` set for tab creation and `changeInfo` set for tab update
       const tab = immutableAction.get('tabValue')
-      const changeInfo = immutableAction.get('changeInfo')
       if (!tab) {
         break
       }
       const tabId = tab.get('tabId')
+      if (tabId === -1) {
+        break
+      }
       let frame = frameStateUtil.getFrameByTabId(state, tabId)
       if (!frame) {
         break
       }
       let frames = state.get('frames')
       const index = tab.get('index')
+      // If front end doesn't know about this tabId, then do nothing!
       let sourceFrameIndex = frameStateUtil.getIndexByTabId(state, tabId)
+      if (sourceFrameIndex == null) {
+        break
+      }
       if (index != null &&
           sourceFrameIndex !== index &&
           // Only update the index once the frame is known.
@@ -156,22 +174,10 @@ const frameReducer = (state, action, immutableAction) => {
       }
 
       const active = tab.get('active')
-      if (active != null) {
-        if (active) {
-          state = frameStateUtil.setActiveFrameKey(state, frame.get('key'))
-          state = frameStateUtil.setFrameLastAccessedTime(state, sourceFrameIndex)
-
-          // Handle tabPage updates and preview cancelation based on tab updated
-          // otherwise tabValue will fire those events each time a tab finish loading
-          // see bug #8429
-          const isNewTab = changeInfo.isEmpty()
-          const activeTabHasUpdated = changeInfo.get('active') != null
-
-          if (!isNewTab && activeTabHasUpdated) {
-            state = frameStateUtil.updateTabPageIndex(state, tabId)
-            state = state.set('previewFrameKey', null)
-          }
-        }
+      if (active && state.get('activeFrameKey') !== frame.get('key')) {
+        state = frameStateUtil.setActiveFrameKey(state, frame.get('key'))
+        state = frameStateUtil.setFrameLastAccessedTime(state, sourceFrameIndex)
+        state = state.set('previewFrameKey', null)
       }
       break
     case windowConstants.WINDOW_SET_NAVIGATED:
@@ -224,34 +230,6 @@ const frameReducer = (state, action, immutableAction) => {
         appActions.tabCloseRequested(frame.get('tabId'))
       })
       break
-    case windowConstants.WINDOW_CLOSE_OTHER_FRAMES:
-      {
-        const currentIndex = frameStateUtil.getIndexByTabId(state, action.tabId)
-        if (currentIndex === -1) {
-          break
-        }
-
-        let tabs = []
-
-        state.get('frames').forEach((frame, i) => {
-          if (!frame.get('pinnedLocation') &&
-            ((i < currentIndex && action.isCloseLeft) || (i > currentIndex && action.isCloseRight))) {
-            if (frame) {
-              tabs.push(frame.get('tabId'))
-              appActions.tabCloseRequested(frame.get('tabId'))
-            }
-          }
-        })
-
-        // TODO(nejc) this can be simplified when states are merged
-        const newFrames = state.get('frames').filter(frame => !tabs.includes(frame.get('tabId')))
-        let newState = state.set('frames', newFrames)
-        newState = frameStateUtil.updateTabPageIndex(newState, action.tabId)
-        const index = newState.getIn(['ui', 'tabs', 'tabPageIndex'], 0)
-        state = state.setIn(['ui', 'tabs', 'tabPageIndex'], index)
-      }
-      break
-
     case windowConstants.WINDOW_CLOSE_FRAME:
       state = closeFrame(state, action)
       const activeFrame = frameStateUtil.getActiveFrame(state)

js/actions/appActions.js

@@ -155,6 +155,51 @@ const appActions = {
     })
   },
 
+  /**
+   * Menu item for closing a tab page has been clicked.
+   * @param {Number} tabPageIndex The index of the tab page to close
+   */
+  tabPageCloseMenuItemClicked: function (windowId, tabPageIndex) {
+    dispatch({
+      actionType: appConstants.APP_TAB_PAGE_CLOSE_MENU_ITEM_CLICKED,
+      tabPageIndex,
+      windowId
+    })
+  },
+
+  /**
+   * Menu item for closing tabs to the left has been clicked.
+   * @param {Number} tabId The tabId woh's tabs to the left should be closed.
+   */
+  closeTabsToLeftMenuItemClicked: function (tabId) {
+    dispatch({
+      actionType: appConstants.APP_CLOSE_TABS_TO_LEFT_MENU_ITEM_CLICKED,
+      tabId
+    })
+  },
+
+  /**
+   * Menu item for closing tabs to the right has been clicked.
+   * @param {Number} tabId The tabId woh's tabs to the right should be closed.
+   */
+  closeTabsToRightMenuItemClicked: function (tabId) {
+    dispatch({
+      actionType: appConstants.APP_CLOSE_TABS_TO_RIGHT_MENU_ITEM_CLICKED,
+      tabId
+    })
+  },
+
+  /**
+   * Menu item for closing other tabs than the specified tab.
+   * @param {Number} tabId The tabId woh's tabs to the left should be closed.
+   */
+  closeOtherTabsMenuItemClicked: function (tabId) {
+    dispatch({
+      actionType: appConstants.APP_CLOSE_OTHER_TABS_MENU_ITEM_CLICKED,
+      tabId
+    })
+  },
+
   /**
    * A request for a new tab has been made with the specified createProperties
    * @param {Object} createProperties