From 4f80c90a30e198e06e33b7a2452cc058c5333d6b Mon Sep 17 00:00:00 2001 From: Pete Miller Date: Mon, 12 Feb 2018 20:30:36 -0800 Subject: [PATCH] Fix and re-enable tab discarding Requires https://github.com/brave/muon/pull/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 .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 | 12 ++++ app/browser/reducers/tabsReducer.js | 16 +++++- app/browser/tabs.js | 76 +++++++++++++++++++++----- app/browser/webContentsCache.js | 12 ++++ app/common/state/tabState.js | 38 ++++++++++++- app/renderer/components/frame/frame.js | 13 ++--- js/actions/appActions.js | 12 ++++ js/constants/appConstants.js | 1 + js/stores/windowStore.js | 13 ++++- 9 files changed, 166 insertions(+), 27 deletions(-) diff --git a/app/browser/activeTabHistory.js b/app/browser/activeTabHistory.js index a3650face01..9569c9a5197 100644 --- a/app/browser/activeTabHistory.js +++ b/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)) + } + } } } diff --git a/app/browser/reducers/tabsReducer.js b/app/browser/reducers/tabsReducer.js index b2935dbffba..fd6fa21ca0d 100644 --- a/app/browser/reducers/tabsReducer.js +++ b/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: { diff --git a/app/browser/tabs.js b/app/browser/tabs.js index b5e0da8b6c4..93bb4bbed01 100644 --- a/app/browser/tabs.js +++ b/app/browser/tabs.js @@ -434,6 +434,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 @@ -458,7 +461,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 @@ -526,6 +532,10 @@ const api = { } const tabId = tab.getId() + // This is the first event for WebContents so cache the item in the Map + // This is not ideal since we do not know the old tabId here, we cannot + webContentsCache.updateWebContents(tabId, tab, null) + // command-line flag --debug-tab-events if (shouldDebugTabEvents) { console.log(`Tab [${tabId}] created in window ${tab.tabValue().windowId}`) @@ -609,6 +619,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. @@ -873,6 +906,9 @@ const api = { }, closeTab: (tabId, forceClosePinned = false) => { + if (shouldDebugTabEvents) { + console.log(`[${tabId}] tabs.closeTab(forceClosePinned: ${forceClosePinned})`) + } const tabValue = getTabValue(tabId) if (!tabValue) { return false @@ -938,12 +974,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 = () => { @@ -998,6 +1028,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) @@ -1027,8 +1060,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')) { @@ -1038,7 +1087,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) @@ -1047,14 +1101,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')) - } - }) }) } }, diff --git a/app/browser/webContentsCache.js b/app/browser/webContentsCache.js index e483ffb3bf6..0c8c580b85b 100644 --- a/app/browser/webContentsCache.js +++ b/app/browser/webContentsCache.js @@ -31,11 +31,23 @@ 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 + } + } +} + module.exports = { cleanupWebContents, getWebContents, getOpenerTabId, forgetOpenerForTabId, updateWebContents, + tabIdChanged, currentWebContents } diff --git a/app/common/state/tabState.js b/app/common/state/tabState.js index 74cb2cf1222..268c3e22982 100644 --- a/app/common/state/tabState.js +++ b/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) diff --git a/app/renderer/components/frame/frame.js b/app/renderer/components/frame/frame.js index 603a717171f..9b4f92bca23 100644 --- a/app/renderer/components/frame/frame.js +++ b/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 al 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 @@ -929,6 +925,9 @@ class Frame extends React.Component { const transitionClassName = this.getTransitionStateClassName(this.props.transitionState) return
{ 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) @@ -263,7 +270,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: