Merge pull request #13120 from brave/fix/tabs-discard-dead-tab

Fix tab auto-discarding ability

app/browser/activeTabHistory.js

@@ -55,6 +55,18 @@ const api = {
    */
   clearTabbedWindow: function (windowId) {
     activeTabsByWindow.delete(windowId)
+  },
+
+  /**
+   * Replace all intances of `oldTabId` with `newTabId` in
+   * active tab history for each window
+   */
+  tabIdChanged: function (oldTabId, newTabId) {
+    for (const [windowId, windowActiveTabs] of activeTabsByWindow) {
+      if (windowActiveTabs && windowActiveTabs.length) {
+        activeTabsByWindow.set(windowId, windowActiveTabs.map(previousTabId => (previousTabId === oldTabId) ? newTabId : previousTabId))
+      }
+    }
   }
 }
 

app/browser/reducers/tabsReducer.js

@@ -24,6 +24,7 @@ const tabActionConsts = require('../../common/constants/tabAction')
 const flash = require('../../../js/flash')
 const {frameOptsFromFrame} = require('../../../js/state/frameStateUtil')
 const {isSourceAboutUrl, isTargetAboutUrl, isNavigatableAboutPage} = require('../../../js/lib/appUrlUtil')
+const {shouldDebugTabEvents} = require('../../cmdLine')
 
 const WEBRTC_DEFAULT = 'default'
 const WEBRTC_DISABLE_NON_PROXY = 'disable_non_proxied_udp'
@@ -150,6 +151,19 @@ const tabsReducer = (state, action, immutableAction) => {
       state = tabState.maybeCreateTab(state, action)
       // tabs.debugTabs(state)
       break
+    case appConstants.APP_TAB_REPLACED:
+      if (action.get('isPermanent')) {
+        if (shouldDebugTabEvents) {
+          console.log('APP_TAB_REPLACED before')
+          tabs.debugTabs(state)
+        }
+        state = tabState.replaceTabValue(state, action.get('oldTabId'), action.get('newTabValue'))
+        if (shouldDebugTabEvents) {
+          console.log('APP_TAB_REPLACED after')
+          tabs.debugTabs(state)
+        }
+      }
+      break
     case appConstants.APP_TAB_CLOSE_REQUESTED:
       {
         let tabId = action.get('tabId')
@@ -335,7 +349,7 @@ const tabsReducer = (state, action, immutableAction) => {
         break
       }
     case appConstants.APP_FRAME_CHANGED:
-      state = tabState.updateFrame(state, action)
+      state = tabState.updateFrame(state, action, shouldDebugTabEvents)
       break
     case windowConstants.WINDOW_SET_FRAME_ERROR:
       {

app/browser/tabs.js

@@ -467,6 +467,9 @@ const api = {
     })
 
     process.on('add-new-contents', (e, source, newTab, disposition, size, userGesture) => {
+      if (shouldDebugTabEvents) {
+        console.log(`Contents [${newTab.getId()}] process:add-new-contents`, {userGesture, isBackground: newTab.isBackgroundPage(), disposition})
+      }
       if (userGesture === false) {
         e.preventDefault()
         return
@@ -491,7 +494,10 @@ const api = {
       }
 
       const tabId = newTab.getId()
+
+      // update our webContents Map with the openerTabId
       webContentsCache.updateWebContents(tabId, newTab, openerTabId)
+
       let newTabValue = getTabValue(newTab.getId())
 
       let windowId
@@ -559,6 +565,10 @@ const api = {
       }
       const tabId = tab.getId()
 
+      // This is the first and most consistent event for WebContents so cache the item in the Map.
+      // Not all contents will get the 'add-new-contents' event (e.g. replaced contents during tab discard).
+      webContentsCache.updateWebContents(tabId, tab, null)
+
       // command-line flag --debug-tab-events
       if (shouldDebugTabEvents) {
         console.log(`Tab [${tabId}] created in window ${tab.tabValue().windowId}`)
@@ -642,6 +652,29 @@ const api = {
         }
       })
 
+      tab.on('tab-replaced-at', (e, windowId, tabIndex, newContents) => {
+        // if not a placeholder, new contents is permanent replacement, e.g. tab has been discarded
+        // if is a placeholder, new contents is temporary, and should not be used for tab ID
+        const isPlaceholder = newContents.isPlaceholder()
+        const newTabId = newContents.getId()
+
+        if (shouldDebugTabEvents) {
+          if (isPlaceholder) {
+            console.log(`Tab [${tabId}] got a new placeholder (${newTabId}), not updating state.`)
+          } else {
+            console.log(`Tab [${tabId}] permanently changed to tabId ${newTabId}. Updating state references...`)
+          }
+        }
+
+        // update state
+        appActions.tabReplaced(tabId, getTabValue(newTabId), getTabValue(tabId).get('windowId'), !isPlaceholder)
+        if (!isPlaceholder) {
+          // update in-memory caches
+          webContentsCache.tabIdChanged(tabId, newTabId)
+          activeTabHistory.tabIdChanged(tabId, newTabId)
+        }
+      })
+
       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.
@@ -914,6 +947,9 @@ const api = {
   },
 
   closeTab: (tabId, forceClosePinned = false) => {
+    if (shouldDebugTabEvents) {
+      console.log(`[${tabId}] tabs.closeTab(forceClosePinned: ${forceClosePinned})`)
+    }
     const tabValue = getTabValue(tabId)
     if (!tabValue) {
       return false
@@ -979,12 +1015,6 @@ const api = {
       const preventAutoDiscard = createProperties.pinned || !isRegularContent
       if (preventAutoDiscard) {
         createProperties.autoDiscardable = false
-      } else {
-        // (temporarily) forced autoDiscardable to ALWAYS false due to
-        // inability to switch to auto-discarded tabs.
-        // See https://github.com/brave/browser-laptop/issues/10673
-        // remove this forced 'else' condition when #10673 is resolved
-        createProperties.autoDiscardable = false
       }
 
       const doCreate = () => {
@@ -1042,6 +1072,9 @@ const api = {
   },
 
   moveTo: (state, tabId, frameOpts, browserOpts, toWindowId) => {
+    if (shouldDebugTabEvents) {
+      console.log(`Tab ${tabId}] tabs.moveTo(window: ${toWindowId})`)
+    }
     frameOpts = makeImmutable(frameOpts)
     browserOpts = makeImmutable(browserOpts)
     const tab = webContentsCache.getWebContents(tabId)
@@ -1071,8 +1104,24 @@ const api = {
         return
       }
 
-      // detach from current window
+      // perform detach from current window
+      // and handle at `did-detach`
       tab.detach(() => {
+        if (shouldDebugTabEvents) {
+          console.log(`Tab [${tabId}] detached tab guestinstance id`, tab.guestInstanceId)
+        }
+        frameOpts = frameOpts.set('guestInstanceId', tab.guestInstanceId)
+        // handle tab has made it to the new window
+        tab.once('did-attach', () => {
+          if (shouldDebugTabEvents) {
+            console.log(`Tab [${tabId}] attached to a new window, so setting the desired index`)
+          }
+          // put the tab in the desired index position
+          const index = frameOpts.get('index')
+          if (index !== undefined) {
+            api.setTabIndex(tabId, frameOpts.get('index'))
+          }
+        })
         // handle tab has detached from window
         // handle tab was the active tab of the window
         if (tabValue.get('active')) {
@@ -1082,7 +1131,12 @@ const api = {
             api.setActive(nextActiveTabIdForOldWindow)
           }
         }
+        // tell another window to add the tab, this will cause the tab to render a webview and
+        // then attach to that window
         if (toWindowId == null || toWindowId === -1) {
+          if (shouldDebugTabEvents) {
+            console.log('creating new window for detached tab')
+          }
           // move tab to a new window
           frameOpts = frameOpts.set('index', 0)
           appActions.newWindow(frameOpts, browserOpts)
@@ -1091,14 +1145,6 @@ const api = {
           // specified window
           appActions.newWebContentsAdded(toWindowId, frameOpts, tabValue)
         }
-        // handle tab has made it to the new window
-        tab.once('did-attach', () => {
-          // put the tab in the desired index position
-          const index = frameOpts.get('index')
-          if (index !== undefined) {
-            api.setTabIndex(tabId, frameOpts.get('index'))
-          }
-        })
       })
     }
   },

app/browser/webContentsCache.js

@@ -31,11 +31,29 @@ const forgetOpenerForTabId = (tabId) => {
   }
 }
 
+const tabIdChanged = (oldTabId, newTabId) => {
+  // any tabs referencing the old contents Id as the opener,
+  // should now reference the new contents Id
+  for (const tabId in currentWebContents) {
+    const tabData = currentWebContents[tabId]
+    if (tabData && tabData.openerTabId != null && tabData.openerTabId === oldTabId) {
+      tabData.openerTabId = newTabId
+    }
+  }
+  // we should also give the replacement tab the opener for the old tab
+  const newTabData = currentWebContents[newTabId]
+  const oldTabData = currentWebContents[oldTabId]
+  if (newTabData && oldTabData && oldTabData.openerTabId != null) {
+    newTabData.openerTabId = oldTabData.openerTabId
+  }
+}
+
 module.exports = {
   cleanupWebContents,
   getWebContents,
   getOpenerTabId,
   forgetOpenerForTabId,
   updateWebContents,
+  tabIdChanged,
   currentWebContents
 }

app/common/state/tabState.js

@@ -404,6 +404,31 @@ const tabState = {
     return state.set('tabs', tabs.delete(index).insert(index, tabValue))
   },
 
+  replaceTabValue: (state, tabId, newTabValue) => {
+    state = validateState(state)
+    newTabValue = makeImmutable(newTabValue)
+    // update tab
+    const index = getTabInternalIndexByTabId(state, tabId)
+    const oldTabValue = state.getIn(['tabs', index])
+    if (index == null || index === -1) {
+      console.error(`tabState: cannot replace tab ${tabId} as tab's index did not exist in state`, { index })
+      return state
+    }
+    let mergedTabValue = oldTabValue.mergeDeep(newTabValue)
+    if (mergedTabValue.has('frame')) {
+      mergedTabValue = mergedTabValue.mergeIn(['frame'], {
+        tabId: newTabValue.get('tabId'),
+        guestInstanceId: newTabValue.get('guestInstanceId')
+      })
+    }
+    mergedTabValue = mergedTabValue.set('windowId', oldTabValue.get('windowId'))
+    state = state.set('tabs', state.get('tabs').delete(index).insert(index, mergedTabValue))
+    // update tabId at tabsInternal index
+    state = deleteTabsInternalIndex(state, oldTabValue)
+    state = updateTabsInternalIndex(state, 0)
+    return state
+  },
+
   removeTabField: (state, field) => {
     state = makeImmutable(state)
 
@@ -417,19 +442,30 @@ const tabState = {
     return state.set('tabs', tabs)
   },
 
-  updateFrame: (state, action) => {
+  updateFrame: (state, action, shouldDebugTabEvents = false) => {
     state = validateState(state)
     action = validateAction(action)
     const tabId = action.getIn(['frame', 'tabId'])
+
     if (!tabId) {
+      if (shouldDebugTabEvents) {
+        console.log(`Tab [${tabId}] frame changed for tab - no tabId provided!`)
+      }
       return state
     }
 
     let tabValue = tabState.getByTabId(state, tabId)
     if (!tabValue) {
+      if (shouldDebugTabEvents) {
+        console.log(`Tab [${tabId}] frame changed for tab - tab not found in state, probably a temporary frame`)
+      }
       return state
     }
 
+    if (shouldDebugTabEvents) {
+      console.log(`Tab [${tabId}] frame changed for tab`)
+    }
+
     const bookmarkUtil = require('../lib/bookmarkUtil')
     const frameLocation = action.getIn(['frame', 'location'])
     const frameBookmarked = bookmarkUtil.isLocationBookmarked(state, frameLocation)

app/renderer/components/frame/frame.js

@@ -433,13 +433,9 @@ class Frame extends React.Component {
   }
 
   addEventListeners () {
-    this.webview.addEventListener('tab-id-changed', (e) => {
-      if (this.frame.isEmpty()) {
-        return
-      }
-
-      windowActions.frameTabIdChanged(this.frame, this.props.tabId, e.tabID)
-    }, { passive: true })
+    // Webview also exposes the 'tab-id-changed' event, with e.tabID as the new tabId.
+    // We don't handle that event anymore, in favor of tab-replaced-at in the browser process.
+    // Keeping this comment here as it is not documented - petemill.
     this.webview.addEventListener('guest-ready', (e) => {
       if (this.frame.isEmpty()) {
         return
@@ -938,6 +934,9 @@ class Frame extends React.Component {
     const transitionClassName = this.getTransitionStateClassName(this.props.transitionState)
     return <div
       data-partition={this.props.partition}
+      data-tab-id={this.props.tabId}
+      data-frame-key={this.props.frameKey}
+      data-guest-id={this.props.guestInstanceId}
       data-test-id='frameWrapper'
       data-test2-id={this.props.isActive ? 'activeFrame' : null}
       data-test3-id={this.props.isPreview ? 'previewFrame' : null}

js/actions/appActions.js

@@ -305,6 +305,18 @@ const appActions = {
     })
   },
 
+  tabReplaced: function (oldTabId, newTabValue, windowId, isPermanent) {
+    dispatch({
+      actionType: appConstants.APP_TAB_REPLACED,
+      oldTabId,
+      newTabValue,
+      isPermanent,
+      queryInfo: {
+        windowId
+      }
+    })
+  },
+
   /**
    * Dispatches a message to the store to set a new frame as the active frame.
    *

js/constants/appConstants.js

@@ -15,6 +15,7 @@ const appConstants = {
   APP_TAB_STRIP_EMPTY: _,
   APP_TAB_CLOSED: _,
   APP_TAB_UPDATED: _,
+  APP_TAB_REPLACED: _,
   APP_ADD_HISTORY_SITE: _,
   APP_SET_STATE: _,
   APP_REMOVE_HISTORY_SITE: _,

js/stores/windowStore.js

@@ -175,14 +175,21 @@ const newFrame = (state, frameOpts) => {
 const frameTabIdChanged = (state, action) => {
   action = makeImmutable(action)
   const oldTabId = action.get('oldTabId')
-  const newTabId = action.get('newTabId')
+  const newTabValue = action.get('newTabValue')
+  const newTabId = newTabValue.get('tabId')
   if (newTabId == null || oldTabId === newTabId) {
+    console.error('Invalid action arguments for frameTabIdChanged')
     return state
   }
 
   let newFrameProps = new Immutable.Map()
   newFrameProps = newFrameProps.set('tabId', newTabId)
-  const index = frameStateUtil.getFrameIndex(state, action.getIn(['frameProps', 'key']))
+  const frame = frameStateUtil.getFrameByTabId(state, oldTabId)
+  if (!frame) {
+    console.error(`Could not find frame with tabId ${oldTabId} in order to replace with new tabId ${newTabId}`)
+    return state
+  }
+  const index = frameStateUtil.getFrameIndex(state, frame.get('key'))
   state = state.mergeIn(['frames', index], newFrameProps)
   state = frameStateUtil.deleteTabInternalIndex(state, oldTabId)
   state = frameStateUtil.updateFramesInternalIndex(state, index)
@@ -261,7 +268,7 @@ const doAction = (action) => {
       }
       // We should not emit here because the Window already know about the change on startup.
       return
-    case windowConstants.WINDOW_FRAME_TAB_ID_CHANGED:
+    case appConstants.APP_TAB_REPLACED:
       windowState = frameTabIdChanged(windowState, action)
       break
     case windowConstants.WINDOW_FRAME_GUEST_INSTANCE_ID_CHANGED: