Fix and re-enable tab discarding

Requires brave/muon#504
Ensure that tab/WebContents objects are added to memory as soon as they are created.
Use muon's new `tab-replaced-at` event (instead of <webview>.tab-id-changed) to ensure state gets updated correctly when a tab is discarded, as well as communicating temporary contents replacement to a window's frame (such as when detaching a tab to a new window, the tab in the old window gets a temporary WebContents that then gets destroyed).

In the case of tabs which get discarded, they are replaced with a fresh, clean WebContents. We were not receiving this new object, so when the renderer asked for this new object's tabId to be made active, the cache did not have that object.
We must also handle updating references to the old TabId to the new TabId in state, in the module which remembers opener tab IDs and the module which handles historical tab 'active' history for a window.

The guestInstanceId for a tab also changes when a discarded tab is reloaded, so make sure we pass that new value in the frame options when a discarded tab is detached to a new window.

Adds more relevant log entries under the `--debug-tab-events` flag.

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: