From 9296f7fc3754f9394b3f3080623fb3ff42edc8fa Mon Sep 17 00:00:00 2001 From: "Brian R. Bondy" Date: Sat, 8 Jul 2017 00:54:33 -0400 Subject: [PATCH] Fix chance of having a lost window Fix #9806 --- app/browser/menu.js | 12 +- app/browser/reducers/windowsReducer.js | 2 + app/index.js | 186 +---------- app/renderer/reducers/debugReducer.js | 19 ++ app/sessionStoreShutdown.js | 242 ++++++++++++++ js/actions/appActions.js | 9 + js/constants/appConfig.js | 4 +- js/constants/appConstants.js | 3 +- js/stores/appStore.js | 2 + js/stores/windowStore.js | 5 +- test/app/sessionStoreTest.js | 88 ++++- test/lib/brave.js | 28 +- test/unit/app/sessionStoreShutdownTest.js | 374 ++++++++++++++++++++++ test/unit/lib/fakeElectron.js | 26 +- test/unit/lib/fakeWindow.js | 19 ++ 15 files changed, 808 insertions(+), 211 deletions(-) create mode 100644 app/renderer/reducers/debugReducer.js create mode 100644 app/sessionStoreShutdown.js create mode 100644 test/unit/app/sessionStoreShutdownTest.js create mode 100644 test/unit/lib/fakeWindow.js diff --git a/app/browser/menu.js b/app/browser/menu.js index c76f9b7b6ef..fd695b04353 100644 --- a/app/browser/menu.js +++ b/app/browser/menu.js @@ -482,7 +482,7 @@ const createDebugSubmenu = () => { // The console will print a message like: [84790:0710/201431:ERROR:child_process.cc(136)] Renderer (84790) paused waiting for debugger to attach. Send SIGUSR1 to unpause. // And this means you should attach Xcode or whatever your debugger is to PID 84790 to unpause. // To debug all renderer processes then add the appendSwitch call to app/index.js - label: 'append wait renderer switch', + label: 'Append wait renderer switch', click: function () { app.commandLine.appendSwitch('renderer-startup-dialog') } @@ -512,11 +512,17 @@ const createDebugSubmenu = () => { } } }, { - label: 'Toggle React Profiling', + label: 'Toggle React profiling', accelerator: 'Alt+P', click: function (item, focusedWindow) { CommonMenu.sendToFocusedWindow(focusedWindow, [messages.DEBUG_REACT_PROFILE]) } + }, { + label: 'Stop reporting window state', + click: function () { + const win = BrowserWindow.getActiveWindow() + appActions.noReportStateModeClicked(win.id) + } } ] } @@ -553,7 +559,7 @@ const createMenu = (state) => { { label: locale.translation('help'), submenu: createHelpSubmenu(), role: 'help' } ] - if (process.env.NODE_ENV === 'development') { + if (process.env.NODE_ENV === 'development' || process.env.BRAVE_ENABLE_DEBUG_MENU !== undefined) { template.push({ label: 'Debug', submenu: createDebugSubmenu() }) } diff --git a/app/browser/reducers/windowsReducer.js b/app/browser/reducers/windowsReducer.js index e0591042b58..358ca04b709 100644 --- a/app/browser/reducers/windowsReducer.js +++ b/app/browser/reducers/windowsReducer.js @@ -8,6 +8,7 @@ const appConstants = require('../../../js/constants/appConstants') const windowConstants = require('../../../js/constants/windowConstants') const windowState = require('../../common/state/windowState') const windows = require('../windows') +const sessionStoreShutdown = require('../../sessionStoreShutdown') const {makeImmutable} = require('../../common/state/immutableUtil') const windowsReducer = (state, action, immutableAction) => { @@ -31,6 +32,7 @@ const windowsReducer = (state, action, immutableAction) => { break case appConstants.APP_WINDOW_CLOSED: state = windowState.removeWindow(state, action) + sessionStoreShutdown.removeWindowFromCache(action.getIn(['windowValue', 'windowId'])) break case appConstants.APP_WINDOW_CREATED: state = windowState.maybeCreateWindow(state, action) diff --git a/app/index.js b/app/index.js index 3f0a6b35fe0..5c71816d3a0 100644 --- a/app/index.js +++ b/app/index.js @@ -48,16 +48,15 @@ if (process.platform === 'win32') { const electron = require('electron') const app = electron.app -const BrowserWindow = electron.BrowserWindow const ipcMain = electron.ipcMain const Immutable = require('immutable') -const Updater = require('./updater') +const updater = require('./updater') const Importer = require('./importer') const messages = require('../js/constants/messages') -const appConfig = require('../js/constants/appConfig') const appActions = require('../js/actions/appActions') const SessionStore = require('./sessionStore') -const AppStore = require('../js/stores/appStore') +const {startSessionSaveInterval} = require('./sessionStoreShutdown') +const appStore = require('../js/stores/appStore') const Autofill = require('./autofill') const Extensions = require('./extensions') const TrackingProtection = require('./trackingProtection') @@ -67,29 +66,17 @@ const HttpsEverywhere = require('./httpsEverywhere') const PDFJS = require('./pdfJS') const SiteHacks = require('./siteHacks') const CmdLine = require('./cmdLine') -const UpdateStatus = require('../js/constants/updateStatus') const urlParse = require('./common/urlParse') const spellCheck = require('./spellCheck') const locale = require('./locale') const contentSettings = require('../js/state/contentSettings') const privacy = require('../js/state/privacy') -const async = require('async') const settings = require('../js/constants/settings') const BookmarksExporter = require('./browser/bookmarksExporter') const siteUtil = require('../js/state/siteUtil') app.commandLine.appendSwitch('enable-features', 'BlockSmallPluginContent,PreferHtmlOverPlugins') -// Used to collect the per window state when shutting down the application -let perWindowState = [] -let sessionStateStoreComplete = false -let sessionStateStoreCompleteCallback = null -let saveAppStateTimeout = null -let requestId = 0 -let shuttingDown = false -let lastWindowState -let lastWindowClosed = false - // Domains to accept bad certs for. TODO: Save the accepted cert fingerprints. let acceptCertDomains = {} let errorCerts = {} @@ -99,105 +86,8 @@ const prefsRestartLastValue = {} const defaultProtocols = ['http', 'https'] -const sessionStoreQueue = async.queue((task, callback) => { - task(callback) -}, 1) - -const logSaveAppStateError = (e) => { - console.error('Error saving app state: ', e) -} - -const saveAppState = (forceSave = false) => { - if (!sessionStateStoreCompleteCallback) { - return - } - - // If we're shutting down early and can't access the state, it's better - // to not try to save anything at all and just quit. - if (shuttingDown && !AppStore.getState()) { - app.exit(0) - } - - const appState = AppStore.getState().toJS() - appState.perWindowState = perWindowState - - const receivedAllWindows = perWindowState.length === BrowserWindow.getAllWindows().length - if (receivedAllWindows) { - clearTimeout(saveAppStateTimeout) - } - - if (!forceSave && !receivedAllWindows) { - return - } - - return SessionStore.saveAppState(appState, shuttingDown).catch((e) => { - logSaveAppStateError(e) - }).then(() => { - if (receivedAllWindows || forceSave) { - sessionStateStoreComplete = true - } - - if (sessionStateStoreComplete) { - if (shuttingDown) { - // If the status is still UPDATE_AVAILABLE then the user wants to quit - // and not restart - if (appState.updates && (appState.updates.status === UpdateStatus.UPDATE_AVAILABLE || - appState.updates.status === UpdateStatus.UPDATE_AVAILABLE_DEFERRED)) { - // In this case on win32, the process doesn't try to auto restart, so avoid the user - // having to open the app twice. Maybe squirrel detects the app is already shutting down. - if (process.platform === 'win32') { - appState.updates.status = UpdateStatus.UPDATE_APPLYING_RESTART - } else { - appState.updates.status = UpdateStatus.UPDATE_APPLYING_NO_RESTART - } - } - - // If there's an update to apply, then do it here. - // Otherwise just quit. - if (appState.updates && (appState.updates.status === UpdateStatus.UPDATE_APPLYING_NO_RESTART || - appState.updates.status === UpdateStatus.UPDATE_APPLYING_RESTART)) { - Updater.quitAndInstall() - } else { - app.quit() - } - } else { - const cb = sessionStateStoreCompleteCallback - sessionStateStoreCompleteCallback = null - cb() - } - } - }) -} - -/** - * Saves the session storage for all windows - */ -const initiateSessionStateSave = () => { - sessionStoreQueue.push((cb) => { - sessionStateStoreComplete = false - sessionStateStoreCompleteCallback = cb - - perWindowState.length = 0 - // quit triggered by window-all-closed should save last window state - if (lastWindowClosed && lastWindowState) { - perWindowState.push(lastWindowState) - saveAppState(true) - } else if (BrowserWindow.getAllWindows().length > 0) { - ++requestId - BrowserWindow.getAllWindows().forEach((win) => win.webContents.send(messages.REQUEST_WINDOW_STATE, requestId)) - // Just in case a window is not responsive, we don't want to wait forever. - // In this case just save session store for the windows that we have already. - saveAppStateTimeout = setTimeout(() => { - saveAppState(true) - }, appConfig.quitTimeout) - } else { - saveAppState() - } - }) -} - // exit cleanly on signals -['SIGTERM', 'SIGHUP', 'SIGINT', 'SIGBREAK'].forEach((signal) => { +;['SIGTERM', 'SIGHUP', 'SIGINT', 'SIGBREAK'].forEach((signal) => { process.on(signal, () => { app.quit() }) @@ -245,7 +135,6 @@ const notifyCertError = (webContents, url, error, cert) => { } app.on('ready', () => { - let sessionStateSaveInterval = null app.on('certificate-error', (e, webContents, url, error, cert, resourceType, overridable, strictEnforcement, expiredPreviousDecision, cb) => { let host = urlParse(url).host if (host && acceptCertDomains[host] === true) { @@ -263,34 +152,6 @@ app.on('ready', () => { notifyCertError(webContents, url, error, cert) }) - app.on('window-all-closed', () => { - // On macOS it is common for applications and their menu bar - // to stay active until the user quits explicitly with Cmd + Q - if (process.platform !== 'darwin') { - lastWindowClosed = true - app.quit() - } - }) - - app.on('before-quit', (e) => { - if (shuttingDown && sessionStateStoreComplete) { - return - } - - e.preventDefault() - - // before-quit can be triggered multiple times because of the preventDefault call - if (shuttingDown) { - return - } else { - shuttingDown = true - } - - appActions.shuttingDown() - clearInterval(sessionStateSaveInterval) - initiateSessionStateSave() - }) - app.on('network-connected', () => { appActions.networkConnected() }) @@ -299,32 +160,6 @@ app.on('ready', () => { appActions.networkDisconnected() }) - // User initiated exit using File->Quit - ipcMain.on(messages.RESPONSE_WINDOW_STATE, (evt, data, id) => { - if (id !== requestId) { - return - } - - if (data) { - perWindowState.push(data) - } - saveAppState() - }) - - ipcMain.on(messages.LAST_WINDOW_STATE, (evt, data) => { - if (data) { - lastWindowState = data - } - }) - - process.on(messages.UNDO_CLOSED_WINDOW, () => { - if (lastWindowState) { - SessionStore.cleanPerWindowData(lastWindowState) - appActions.newWindow(undefined, undefined, lastWindowState) - lastWindowState = undefined - } - }) - loadAppStatePromise.then((initialState) => { // Do this after loading the state // For tests we always want to load default app state @@ -455,8 +290,7 @@ app.on('ready', () => { } }) - // save app state every 5 minutes regardless of update frequency - sessionStateSaveInterval = setInterval(initiateSessionStateSave, 1000 * 60 * 5) + startSessionSaveInterval() ipcMain.on(messages.NOTIFICATION_RESPONSE, (e, message, buttonIndex, persist) => { if (prefsRestartCallbacks[message]) { @@ -469,13 +303,13 @@ app.on('ready', () => { }) ipcMain.on(messages.EXPORT_BOOKMARKS, () => { - BookmarksExporter.showDialog(AppStore.getState().get('sites')) + BookmarksExporter.showDialog(appStore.getState().get('sites')) }) // DO NOT TO THIS LIST - see above // We need the initial state to read the UPDATE_TO_PREVIEW_RELEASES preference loadAppStatePromise.then((initialState) => { - Updater.init( + updater.init( process.platform, process.arch, process.env.BRAVE_UPDATE_VERSION || app.getVersion(), @@ -483,8 +317,8 @@ app.on('ready', () => { ) // This is fired by a menu entry - process.on(messages.CHECK_FOR_UPDATE, () => Updater.checkForUpdate(true)) - ipcMain.on(messages.CHECK_FOR_UPDATE, () => Updater.checkForUpdate(true)) + process.on(messages.CHECK_FOR_UPDATE, () => updater.checkForUpdate(true)) + ipcMain.on(messages.CHECK_FOR_UPDATE, () => updater.checkForUpdate(true)) // This is fired from a auto-update metadata call process.on(messages.UPDATE_META_DATA_RETRIEVED, (metadata) => { @@ -498,7 +332,7 @@ app.on('ready', () => { }) process.on(messages.EXPORT_BOOKMARKS, () => { - BookmarksExporter.showDialog(AppStore.getState().get('sites')) + BookmarksExporter.showDialog(appStore.getState().get('sites')) }) ready = true diff --git a/app/renderer/reducers/debugReducer.js b/app/renderer/reducers/debugReducer.js new file mode 100644 index 00000000000..34c2baff5ff --- /dev/null +++ b/app/renderer/reducers/debugReducer.js @@ -0,0 +1,19 @@ +/* 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 appConstants = require('../../../js/constants/appConstants') +const messages = require('../../../js/constants/messages') +const {ipcRenderer} = require('electron') + +const debugReducer = (windowState, action) => { + switch (action.actionType) { + case appConstants.APP_DEBUG_NO_REPORT_STATE_MODE_CLICKED: + ipcRenderer.removeAllListeners(messages.REQUEST_WINDOW_STATE) + console.error('Disabled window state updates') + break + } + return windowState +} + +module.exports = debugReducer diff --git a/app/sessionStoreShutdown.js b/app/sessionStoreShutdown.js new file mode 100644 index 00000000000..70c93aca668 --- /dev/null +++ b/app/sessionStoreShutdown.js @@ -0,0 +1,242 @@ +/* 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/. */ + +'use strict' + +const {BrowserWindow, app, ipcMain} = require('electron') +const sessionStore = require('./sessionStore') +const updateStatus = require('../js/constants/updateStatus') +const updater = require('./updater') +const appConfig = require('../js/constants/appConfig') +const async = require('async') +const messages = require('../js/constants/messages') +const appActions = require('../js/actions/appActions') + +// Used to collect the per window state when shutting down the application +let perWindowState +let sessionStateStoreComplete +let sessionStateStoreCompleteCallback +let saveAppStateTimeout +let windowCloseRequestId +let shuttingDown +let lastWindowThatWasClosedState +let isAllWindowsClosed +let sessionStateSaveInterval +// Stores the last window state for each requested window in case a hung window happens, +// we'll at least have the last known window state. +let windowStateCache +let sessionStoreQueue +let appStore + +// Useful for automated tests +const reset = () => { + perWindowState = [] + sessionStateStoreComplete = false + if (saveAppStateTimeout) { + clearTimeout(saveAppStateTimeout) + } + saveAppStateTimeout = null + windowCloseRequestId = 0 + shuttingDown = false + lastWindowThatWasClosedState = undefined + isAllWindowsClosed = false + sessionStateSaveInterval = null + windowStateCache = {} + if (sessionStateStoreCompleteCallback) { + sessionStateStoreCompleteCallback() + } + sessionStateStoreCompleteCallback = null + sessionStoreQueue = async.queue((task, callback) => { + task(callback) + }, 1) +} +reset() + +const logSaveAppStateError = (e) => { + console.error('Error saving app state: ', e) +} + +const saveAppState = (forceSave = false) => { + if (!sessionStateStoreCompleteCallback) { + return + } + + if (!appStore) { + appStore = require('../js/stores/appStore') + } + + // If we're shutting down early and can't access the state, it's better + // to not try to save anything at all and just quit. + if (shuttingDown && !appStore.getState()) { + app.exit(0) + } + + const appState = appStore.getState().toJS() + appState.perWindowState = perWindowState + + const receivedAllWindows = perWindowState.length === BrowserWindow.getAllWindows().length + if (receivedAllWindows) { + clearTimeout(saveAppStateTimeout) + } + + if (!forceSave && !receivedAllWindows) { + return + } + + return sessionStore.saveAppState(appState, shuttingDown).catch((e) => { + logSaveAppStateError(e) + }).then(() => { + if (receivedAllWindows || forceSave) { + sessionStateStoreComplete = true + } + + if (sessionStateStoreComplete) { + if (shuttingDown) { + // If the status is still UPDATE_AVAILABLE then the user wants to quit + // and not restart + if (appState.updates && (appState.updates.status === updateStatus.UPDATE_AVAILABLE || + appState.updates.status === updateStatus.UPDATE_AVAILABLE_DEFERRED)) { + // In this case on win32, the process doesn't try to auto restart, so avoid the user + // having to open the app twice. Maybe squirrel detects the app is already shutting down. + if (process.platform === 'win32') { + appState.updates.status = updateStatus.UPDATE_APPLYING_RESTART + } else { + appState.updates.status = updateStatus.UPDATE_APPLYING_NO_RESTART + } + } + + // If there's an update to apply, then do it here. + // Otherwise just quit. + if (appState.updates && (appState.updates.status === updateStatus.UPDATE_APPLYING_NO_RESTART || + appState.updates.status === updateStatus.UPDATE_APPLYING_RESTART)) { + updater.quitAndInstall() + } else { + app.quit() + } + } else { + const cb = sessionStateStoreCompleteCallback + sessionStateStoreCompleteCallback = null + sessionStateStoreComplete = false + cb() + } + } + }) +} + +/** + * Saves the session storage for all windows + */ +const initiateSessionStateSave = () => { + sessionStoreQueue.push((cb) => { + sessionStateStoreComplete = false + sessionStateStoreCompleteCallback = cb + perWindowState.length = 0 + + // quit triggered by window-all-closed should save last window state + if (isAllWindowsClosed && lastWindowThatWasClosedState) { + perWindowState.push(lastWindowThatWasClosedState) + saveAppState(true) + } else if (BrowserWindow.getAllWindows().length > 0) { + ++windowCloseRequestId + const windowIds = BrowserWindow.getAllWindows().map((win) => { + return win.id + }) + BrowserWindow.getAllWindows().forEach((win) => win.webContents.send(messages.REQUEST_WINDOW_STATE, windowCloseRequestId)) + // Just in case a window is not responsive, we don't want to wait forever. + // In this case just save session store for the windows that we have already. + saveAppStateTimeout = setTimeout(() => { + // Rewrite perwindowstate here + perWindowState = windowIds + .filter((windowId) => windowStateCache[windowId]) + .map((windowId) => windowStateCache[windowId]) + saveAppState(true) + }, appConfig.quitTimeout) + } else { + saveAppState() + } + }) +} + +const removeWindowFromCache = (windowId) => { + if (shuttingDown) { + return + } + delete windowStateCache[windowId] +} + +const initWindowCacheState = (windowId, windowState) => { + windowStateCache[windowId] = Object.assign({}, windowState) +} + +app.on('before-quit', (e) => { + if (shuttingDown && sessionStateStoreComplete) { + return + } + + e.preventDefault() + + // before-quit can be triggered multiple times because of the preventDefault call + if (shuttingDown) { + return + } else { + shuttingDown = true + } + + appActions.shuttingDown() + if (sessionStateSaveInterval !== undefined) { + clearInterval(sessionStateSaveInterval) + } + initiateSessionStateSave() +}) + +const startSessionSaveInterval = () => { + // save app state every 5 minutes regardless of update frequency + initiateSessionStateSave() + sessionStateSaveInterval = setInterval(initiateSessionStateSave, appConfig.sessionSaveInterval) +} + +// User initiated exit using File->Quit +ipcMain.on(messages.RESPONSE_WINDOW_STATE, (evt, data, id) => { + const senderWindowId = evt.sender.getOwnerBrowserWindow().id + if (id !== windowCloseRequestId) { + return + } + + if (data) { + perWindowState.push(data) + windowStateCache[senderWindowId] = data + } + saveAppState() +}) + +ipcMain.on(messages.LAST_WINDOW_STATE, (evt, data) => { + if (data) { + lastWindowThatWasClosedState = data + } +}) + +process.on(messages.UNDO_CLOSED_WINDOW, () => { + if (lastWindowThatWasClosedState) { + sessionStore.cleanPerWindowData(lastWindowThatWasClosedState) + appActions.newWindow(undefined, undefined, lastWindowThatWasClosedState) + lastWindowThatWasClosedState = undefined + } +}) + +app.on('window-all-closed', () => { + // On macOS it is common for applications and their menu bar + // to stay active until the user quits explicitly with Cmd + Q + if (process.platform !== 'darwin') { + isAllWindowsClosed = true + app.quit() + } +}) + +module.exports = { + startSessionSaveInterval, + initiateSessionStateSave, + reset, + removeWindowFromCache, + initWindowCacheState +} diff --git a/js/actions/appActions.js b/js/actions/appActions.js index 951c72600e8..bb79c6a1725 100644 --- a/js/actions/appActions.js +++ b/js/actions/appActions.js @@ -1506,6 +1506,15 @@ const appActions = { tag, editKey }) + }, + + noReportStateModeClicked: function (windowId) { + dispatch({ + actionType: appConstants.APP_DEBUG_NO_REPORT_STATE_MODE_CLICKED, + queryInfo: { + windowId + } + }) } } diff --git a/js/constants/appConfig.js b/js/constants/appConfig.js index e014e868a06..7af06da816e 100644 --- a/js/constants/appConfig.js +++ b/js/constants/appConfig.js @@ -8,6 +8,7 @@ const updateHost = process.env.BRAVE_UPDATE_HOST || 'https://brave-laptop-update const winUpdateHost = process.env.BRAVE_WIN_UPDATE_HOST || 'https://brave-download.global.ssl.fastly.net' const crashURL = process.env.BRAVE_CRASH_URL || 'https://brave-laptop-updates.herokuapp.com/1/crashes' const adHost = process.env.AD_HOST || 'https://oip.brave.com' +const isTest = process.env.NODE_ENV === 'test' const buildConfig = require('./buildConfig') const isProduction = buildConfig.nodeEnv === 'production' @@ -16,7 +17,8 @@ const {fullscreenOption, autoplayOption} = require('../../app/common/constants/s module.exports = { name: 'Brave', contactUrl: 'mailto:support+laptop@brave.com', - quitTimeout: 10 * 1000, + quitTimeout: isTest ? 3 * 1000 : 10 * 1000, + sessionSaveInterval: 1000 * 60 * 5, resourceNames: { ADBLOCK: 'adblock', SAFE_BROWSING: 'safeBrowsing', diff --git a/js/constants/appConstants.js b/js/constants/appConstants.js index 4cf12f884bc..f41334fdf1e 100644 --- a/js/constants/appConstants.js +++ b/js/constants/appConstants.js @@ -141,7 +141,8 @@ const appConstants = { APP_SWIPE_LEFT: _, APP_SWIPE_RIGHT: _, APP_ADD_BOOKMARK: _, - APP_EDIT_BOOKMARK: _ + APP_EDIT_BOOKMARK: _, + APP_DEBUG_NO_REPORT_STATE_MODE_CLICKED: _ } module.exports = mapValuesByKeys(appConstants) diff --git a/js/stores/appStore.js b/js/stores/appStore.js index f34ddf79c13..240273c4182 100644 --- a/js/stores/appStore.js +++ b/js/stores/appStore.js @@ -35,6 +35,7 @@ const webtorrent = require('../../app/browser/webtorrent') const assert = require('assert') const profiles = require('../../app/browser/profiles') const {zoomLevel} = require('../../app/common/constants/toolbarUserInterfaceScale') +const {initWindowCacheState} = require('../../app/sessionStoreShutdown') // state helpers const {makeImmutable} = require('../../app/common/state/immutableUtil') @@ -197,6 +198,7 @@ const createWindow = (action) => { setImmediate(() => { let mainWindow = new BrowserWindow(Object.assign(windowProps, browserOpts, {disposition: frameOpts.disposition})) + initWindowCacheState(mainWindow.id, action.restoredState) // initialize frames state let frames = [] diff --git a/js/stores/windowStore.js b/js/stores/windowStore.js index 79d42a45dbd..4e09138b53f 100644 --- a/js/stores/windowStore.js +++ b/js/stores/windowStore.js @@ -248,7 +248,10 @@ const emitChanges = debounce(windowStore.emitChanges.bind(windowStore), 5) const applyReducers = (state, action, immutableAction) => [ require('../../app/renderer/reducers/urlBarReducer'), require('../../app/renderer/reducers/frameReducer'), - require('../../app/renderer/reducers/contextMenuReducer') + require('../../app/renderer/reducers/contextMenuReducer'), + // This should be included even in production builds since you can use + // an environment variable to show the Debug menu + require('../../app/renderer/reducers/debugReducer') ].reduce( (windowState, reducer) => { const newState = reducer(windowState, action, immutableAction) diff --git a/test/app/sessionStoreTest.js b/test/app/sessionStoreTest.js index 40bb2ad2a0d..e3a384529b4 100644 --- a/test/app/sessionStoreTest.js +++ b/test/app/sessionStoreTest.js @@ -1,17 +1,16 @@ /* global describe, it, before, after */ const Brave = require('../lib/brave') -const Immutable = require('immutable') const {urlInput, navigatorBookmarked, navigatorNotBookmarked, pinnedTabsTabs, tabsTabs} = require('../lib/selectors') const siteTags = require('../../js/constants/siteTags') -const siteUtil = require('../../js/state/siteUtil') +const settings = require('../../js/constants/settings') -describe('sessionStore', function () { +describe('sessionStore test', function () { function * setup (client) { Brave.addCommands() } - describe('state is preserved', function () { + describe('state is preserved with a normal shutdown', function () { Brave.beforeAllServerSetup(this) before(function * () { const page1Url = Brave.server.url('page1.html') @@ -19,24 +18,18 @@ describe('sessionStore', function () { location: page1Url, title: 'some page' } - const key = siteUtil.getSiteKey(Immutable.fromJS(site)) yield Brave.startApp() yield setup(Brave.app.client) yield Brave.app.client .waitForBrowserWindow() - .onClearBrowsingData('browserHistory', true) + .changeSetting(settings.DISABLE_TITLE_MODE, false) .waitForUrl(Brave.newTabUrl) .loadUrl(page1Url) .windowParentByUrl(page1Url) .activateURLMode() .waitForExist(navigatorNotBookmarked) - yield Brave.app.client.addSite(site, siteTags.BOOKMARK) - .waitUntil(function () { - return this.getAppState().then((val) => { - let state = val.value - return siteUtil.getBookmarks(Immutable.fromJS(state.sites)).size === 1 && state.sites[key].location === page1Url - }) - }) + .addBookmark(site, siteTags.BOOKMARK) + .waitForExist(navigatorBookmarked) yield Brave.stopApp(false) yield Brave.startApp() yield setup(Brave.app.client) @@ -51,14 +44,81 @@ describe('sessionStore', function () { yield Brave.app.client .waitForUrl(page1Url) .waitForBrowserWindow() - .moveToObject(urlInput) + .activateURLMode() .waitForInputText(urlInput, page1Url) }) it('appstate by preserving a bookmark', function * () { yield Brave.app.client .waitForBrowserWindow() + .activateURLMode() + .waitForExist(navigatorBookmarked) + }) + }) + + describe('state is preserved with a hung window', function () { + Brave.beforeAllServerSetup(this) + before(function * () { + const page1Url = Brave.server.url('page1.html') + const site = { + location: page1Url, + title: 'some page' + } + yield Brave.startApp() + yield setup(Brave.app.client) + yield Brave.app.client + .waitForBrowserWindow() + .changeSetting(settings.DISABLE_TITLE_MODE, false) + .waitForUrl(Brave.newTabUrl) + .loadUrl(page1Url) + .windowParentByUrl(page1Url) + .activateURLMode() + .waitForExist(navigatorNotBookmarked) + .addBookmark(site, siteTags.BOOKMARK) .waitForExist(navigatorBookmarked) + + yield Brave.stopApp(false) + yield Brave.startApp() + yield setup(Brave.app.client) + + yield Brave.app.client + .windowByUrl(Brave.browserWindowUrl) + .waitForBrowserWindow() + .activateURLMode() + .waitForInputText(urlInput, page1Url) + .stopReportingStateUpdates() + .newTab({active: false}) + .waitForTabCount(2) + .waitForBrowserWindow() + + yield Brave.stopApp(false, 10000) + yield Brave.startApp() + yield setup(Brave.app.client) + }) + + after(function * () { + yield Brave.stopApp() + }) + + it('windowState by preserving open page', function * () { + const page1Url = Brave.server.url('page1.html') + yield Brave.app.client + .waitForUrl(page1Url) + .waitForBrowserWindow() + .activateURLMode() + .waitForInputText(urlInput, page1Url) + }) + + it('appstate by preserving a bookmark', function * () { + yield Brave.app.client + .waitForBrowserWindow() + .activateURLMode() + .waitForExist(navigatorBookmarked) + }) + + it('has only 1 tab on restore', function * () { + yield Brave.app.client + .waitForTabCount(1) }) }) diff --git a/test/lib/brave.js b/test/lib/brave.js index f8634e661f8..eed1b4719ef 100644 --- a/test/lib/brave.js +++ b/test/lib/brave.js @@ -602,6 +602,12 @@ var exports = { }) }) + this.app.client.addCommand('stopReportingStateUpdates', function () { + return this.execute(function () { + devTools('electron').ipcRenderer.removeAllListeners('request-window-state') + }) + }) + this.app.client.addCommand('detachTabByIndex', function (index, windowId = -1) { return this.waitForTab({index}).getWindowState().then((val) => { const frame = val.value.frames[index] @@ -683,6 +689,24 @@ var exports = { .waitForSiteEntry(waitUrl, false) }) + /** + * Adds a bookmark to the bookmarks list. + * + * @param {object} siteDetail - Properties for the siteDetail to add + * @param {string} tag - A site tag from js/constants/siteTags.js + */ + this.app.client.addCommand('addBookmark', function (siteDetail, tag) { + logVerbose('addBookmark("' + siteDetail + '", "' + tag + '")') + let waitUrl = siteDetail.location + if (isSourceAboutUrl(waitUrl)) { + waitUrl = getTargetAboutUrl(waitUrl) + } + return this.execute(function (siteDetail, tag) { + return devTools('appActions').addBookmark(siteDetail, tag) + }, siteDetail, tag).then((response) => response.value) + .waitForSiteEntry(waitUrl, false) + }) + /** * Adds a list sites to the sites list, including bookmark and foler. * @@ -1020,7 +1044,7 @@ var exports = { return this.app.start() }, - stopApp: function (cleanSessionStore = true) { + stopApp: function (cleanSessionStore = true, timeout = 100) { const promises = [] if (process.env.BRAVE_TEST_ALL_LOGS || process.env.BRAVE_TEST_BROWSER_LOGS) { @@ -1056,7 +1080,7 @@ var exports = { } promises.push((callback) => { - callback = setTimeout(cleanup.bind(this, callback), 100) + callback = setTimeout(cleanup.bind(this, callback), timeout) this.app.client.waitForBrowserWindow().quit() .then(callback) .catch((err) => { diff --git a/test/unit/app/sessionStoreShutdownTest.js b/test/unit/app/sessionStoreShutdownTest.js new file mode 100644 index 00000000000..0894e45b34d --- /dev/null +++ b/test/unit/app/sessionStoreShutdownTest.js @@ -0,0 +1,374 @@ +/* global describe, before, beforeEach, after, it, afterEach */ +const mockery = require('mockery') +const assert = require('assert') +const sinon = require('sinon') +const Immutable = require('immutable') +const messages = require('../../../js/constants/messages') + +require('../braveUnit') + +const makeSender = (fakeWindow) => ({ + sender: { + getOwnerBrowserWindow: function () { + return { + id: fakeWindow.id + } + } + } +}) + +describe('sessionStoreShutdown unit tests', function () { + let sessionStore + let sessionStoreShutdown + let appActions + let appStore + const fakeElectron = Object.assign({}, require('../lib/fakeElectron')) + const FakeWindow = require('../lib/fakeWindow') + const fakeAdBlock = require('../lib/fakeAdBlock') + + before(function () { + this.clock = sinon.useFakeTimers() + mockery.enable({ + warnOnReplace: false, + warnOnUnregistered: false, + useCleanCache: true + }) + global.muon = { + file: { + writeImportant: (path, data, cb) => { + // simulate running on another thread + setTimeout(() => { + cb(true) + }, 0) + } + } + } + mockery.registerMock('electron', fakeElectron) + mockery.registerMock('ad-block', fakeAdBlock) + mockery.registerMock('leveldown', {}) + mockery.registerMock('keytar', {}) + sessionStore = require('../../../app/sessionStore') + sessionStoreShutdown = require('../../../app/sessionStoreShutdown') + appActions = require('../../../js/actions/appActions') + appStore = require('../../../js/stores/appStore') + }) + + after(function () { + this.clock.restore() + mockery.disable() + }) + + afterEach(function () { + sessionStoreShutdown.reset() + }) + + describe('windows all closed', function () { + before(function () { + this.appQuitSpy = sinon.spy(fakeElectron.app, 'quit') + this.oldPlatform = process.platform + sessionStoreShutdown.reset() + }) + afterEach(function () { + this.appQuitSpy.reset() + }) + after(function () { + Object.defineProperty(process, 'platform', { + value: this.oldPlatform + }) + this.appQuitSpy.restore() + }) + it('does not quit on macOS', function () { + Object.defineProperty(process, 'platform', { + value: 'darwin' + }) + fakeElectron.app.emit('window-all-closed') + assert.equal(this.appQuitSpy.notCalled, true) + }) + it('quits on windows', function () { + Object.defineProperty(process, 'platform', { + value: 'win32' + }) + fakeElectron.app.emit('window-all-closed') + assert.equal(this.appQuitSpy.calledOnce, true) + }) + it('quits on linux', function () { + Object.defineProperty(process, 'platform', { + value: 'linux' + }) + fakeElectron.app.emit('window-all-closed') + assert.equal(this.appQuitSpy.calledOnce, true) + }) + }) + + describe('undo close window', function () { + before(function () { + this.newWindowStub = sinon.stub(appActions, 'newWindow') + sessionStoreShutdown.reset() + }) + afterEach(function () { + this.newWindowStub.reset() + }) + after(function () { + this.newWindowStub.restore() + }) + + it('works for first closed window', function () { + const windowState = { a: 1 } + fakeElectron.ipcMain.send(messages.LAST_WINDOW_STATE, {}, windowState) + process.emit(messages.UNDO_CLOSED_WINDOW) + assert(this.newWindowStub.calledOnce) + assert.deepEqual(this.newWindowStub.getCall(0).args[2], windowState) + }) + it('works for subsequent windows', function () { + const windowState1 = { b: 1 } + const windowState2 = { x: 2 } + fakeElectron.ipcMain.send(messages.LAST_WINDOW_STATE, {}, windowState1) + fakeElectron.ipcMain.send(messages.LAST_WINDOW_STATE, {}, windowState2) + process.emit(messages.UNDO_CLOSED_WINDOW) + assert(this.newWindowStub.calledOnce) + assert.deepEqual(this.newWindowStub.getCall(0).args[2], windowState2) + }) + }) + + describe('app before-quit event', function () { + before(function () { + this.appStoreGetStateStub = sinon.stub(appStore, 'getState', () => { + return Immutable.fromJS({}) + }) + this.shuttingDownStub = sinon.stub(appActions, 'shuttingDown') + this.appQuitSpy = sinon.spy(fakeElectron.app, 'quit') + sessionStoreShutdown.reset() + }) + afterEach(function () { + this.shuttingDownStub.reset() + this.appStoreGetStateStub.reset() + this.appQuitSpy.reset() + }) + after(function () { + this.shuttingDownStub.restore() + this.appStoreGetStateStub.restore() + this.appQuitSpy.restore() + }) + + describe('with no windows', function () { + before(function () { + this.requestId = 1 + this.getAllWindowsStub = sinon.stub(fakeElectron.BrowserWindow, 'getAllWindows', () => { + return [] + }) + }) + after(function () { + this.getAllWindowsStub.restore() + }) + it('saves when no windows', function (cb) { + const saveAppStateStub = sinon.stub(sessionStore, 'saveAppState', (state) => { + assert.equal(saveAppStateStub.calledOnce, true) + saveAppStateStub.restore() + assert.equal(state.perWindowState.length, 0) + cb() + return Promise.resolve() + }) + fakeElectron.app.emit('before-quit', { preventDefault: () => {} }) + assert.equal(saveAppStateStub.notCalled, true) + this.clock.tick(1) + }) + it('remembers last closed window with no windows (Win32)', function (cb) { + const oldPlatform = process.platform + Object.defineProperty(process, 'platform', { + value: 'win32' + }) + const windowState = { a: 1 } + fakeElectron.ipcMain.send(messages.LAST_WINDOW_STATE, {}, windowState) + fakeElectron.app.emit('window-all-closed') + const saveAppStateStub = sinon.stub(sessionStore, 'saveAppState', (state) => { + Object.defineProperty(process, 'platform', { + value: oldPlatform + }) + assert.equal(saveAppStateStub.calledOnce, true) + saveAppStateStub.restore() + assert.equal(state.perWindowState.length, 1) + cb() + return Promise.resolve() + }) + fakeElectron.app.emit('before-quit', { preventDefault: () => {} }) + assert.equal(saveAppStateStub.notCalled, true) + this.clock.tick(1) + }) + it('remembers last closed window with no windows (Linux)', function (cb) { + const oldPlatform = process.platform + Object.defineProperty(process, 'platform', { + value: 'linux' + }) + const windowState = { a: 1 } + fakeElectron.ipcMain.send(messages.LAST_WINDOW_STATE, {}, windowState) + fakeElectron.app.emit('window-all-closed') + const saveAppStateStub = sinon.stub(sessionStore, 'saveAppState', (state) => { + Object.defineProperty(process, 'platform', { + value: oldPlatform + }) + assert.equal(saveAppStateStub.calledOnce, true) + saveAppStateStub.restore() + assert.equal(state.perWindowState.length, 1) + cb() + return Promise.resolve() + }) + fakeElectron.app.emit('before-quit', { preventDefault: () => {} }) + assert.equal(saveAppStateStub.notCalled, true) + this.clock.tick(1) + }) + it('remembers last closed window with no windows (macOS)', function (cb) { + const oldPlatform = process.platform + Object.defineProperty(process, 'platform', { + value: 'darwin' + }) + const windowState = { a: 1 } + fakeElectron.ipcMain.send(messages.LAST_WINDOW_STATE, {}, windowState) + fakeElectron.app.emit('window-all-closed') + const saveAppStateStub = sinon.stub(sessionStore, 'saveAppState', (state) => { + Object.defineProperty(process, 'platform', { + value: oldPlatform + }) + assert.equal(saveAppStateStub.calledOnce, true) + saveAppStateStub.restore() + assert.equal(state.perWindowState.length, 0) + cb() + return Promise.resolve() + }) + fakeElectron.app.emit('before-quit', { preventDefault: () => {} }) + assert.equal(saveAppStateStub.notCalled, true) + this.clock.tick(1) + }) + }) + + describe('with one window', function () { + before(function () { + this.fakeWindow1 = new FakeWindow(1) + this.requestId = 1 + this.getAllWindowsStub = sinon.stub(fakeElectron.BrowserWindow, 'getAllWindows', () => { + return [this.fakeWindow1] + }) + }) + beforeEach(function () { + this.fakeWindow1.webContents.removeAllListeners(messages.REQUEST_WINDOW_STATE) + }) + after(function () { + this.getAllWindowsStub.restore() + }) + it('saves when all windows responds without the clock moving forward', function (cb) { + this.fakeWindow1.webContents.on(messages.REQUEST_WINDOW_STATE, () => { + setImmediate(() => { + fakeElectron.ipcMain.send(messages.RESPONSE_WINDOW_STATE, makeSender(this.fakeWindow1), this.fakeWindow1, this.requestId) + }) + }) + const saveAppStateStub = sinon.stub(sessionStore, 'saveAppState', (state) => { + assert.equal(saveAppStateStub.calledOnce, true) + saveAppStateStub.restore() + assert.equal(state.perWindowState.length, 1) + cb() + return Promise.resolve() + }) + fakeElectron.app.emit('before-quit', { preventDefault: () => {} }) + assert.equal(saveAppStateStub.notCalled, true) + this.clock.tick(1) + }) + it('times out and saves app state anyway', function (cb) { + const saveAppStateStub = sinon.stub(sessionStore, 'saveAppState', (state) => { + assert.equal(saveAppStateStub.calledOnce, true) + saveAppStateStub.restore() + assert.deepEqual(state.perWindowState, [{a: 1}]) + cb() + return Promise.resolve() + }) + + fakeElectron.ipcMain.send(messages.RESPONSE_WINDOW_STATE, makeSender(this.fakeWindow1), {a: 1}, 0) + fakeElectron.app.emit('before-quit', { preventDefault: () => {} }) + assert.equal(saveAppStateStub.notCalled, true) + this.clock.tick(100000) + }) + }) + describe('with multiple windows', function () { + before(function () { + this.fakeWindow1 = new FakeWindow(1) + this.fakeWindow2 = new FakeWindow(2) + this.fakeWindow3 = new FakeWindow(3) + this.requestId = 1 + this.getAllWindowsStub = sinon.stub(fakeElectron.BrowserWindow, 'getAllWindows', () => { + return [this.fakeWindow1, this.fakeWindow2, this.fakeWindow3] + }) + }) + beforeEach(function () { + this.fakeWindow1.webContents.removeAllListeners(messages.REQUEST_WINDOW_STATE) + this.fakeWindow2.webContents.removeAllListeners(messages.REQUEST_WINDOW_STATE) + this.fakeWindow3.webContents.removeAllListeners(messages.REQUEST_WINDOW_STATE) + }) + after(function () { + this.getAllWindowsStub.restore() + }) + it('saves when all windows responds without the clock moving forward', function (cb) { + this.fakeWindow1.webContents.on(messages.REQUEST_WINDOW_STATE, () => { + setImmediate(() => { + fakeElectron.ipcMain.send(messages.RESPONSE_WINDOW_STATE, makeSender(this.fakeWindow1), this.fakeWindow1, this.requestId) + }) + }) + this.fakeWindow2.webContents.on(messages.REQUEST_WINDOW_STATE, () => { + setImmediate(() => { + fakeElectron.ipcMain.send(messages.RESPONSE_WINDOW_STATE, makeSender(this.fakeWindow2), this.fakeWindow2, this.requestId) + }) + }) + this.fakeWindow3.webContents.on(messages.REQUEST_WINDOW_STATE, () => { + setImmediate(() => { + fakeElectron.ipcMain.send(messages.RESPONSE_WINDOW_STATE, makeSender(this.fakeWindow3), this.fakeWindow3, this.requestId) + }) + }) + const saveAppStateStub = sinon.stub(sessionStore, 'saveAppState', (state) => { + assert.equal(saveAppStateStub.calledOnce, true) + saveAppStateStub.restore() + assert.equal(state.perWindowState.length, 3) + cb() + return Promise.resolve() + }) + fakeElectron.app.emit('before-quit', { preventDefault: () => {} }) + assert.equal(saveAppStateStub.notCalled, true) + this.clock.tick(1) + }) + it('times out and saves app state anyway', function (cb) { + const saveAppStateStub = sinon.stub(sessionStore, 'saveAppState', (state) => { + assert.equal(saveAppStateStub.calledOnce, true) + saveAppStateStub.restore() + assert.deepEqual(state.perWindowState, [{a: 1}, {b: 2}, {c: 3}]) + cb() + return Promise.resolve() + }) + fakeElectron.ipcMain.send(messages.RESPONSE_WINDOW_STATE, makeSender(this.fakeWindow1), {a: 1}, 0) + fakeElectron.ipcMain.send(messages.RESPONSE_WINDOW_STATE, makeSender(this.fakeWindow2), {b: 2}, 0) + fakeElectron.ipcMain.send(messages.RESPONSE_WINDOW_STATE, makeSender(this.fakeWindow3), {c: 3}, 0) + fakeElectron.app.emit('before-quit', { preventDefault: () => {} }) + assert.equal(saveAppStateStub.notCalled, true) + this.clock.tick(100000) + }) + it('saves when only one of multiple windows respond and timeout happens', function (cb) { + const saveAppStateStub = sinon.stub(sessionStore, 'saveAppState', (state) => { + assert.equal(saveAppStateStub.called, true) + saveAppStateStub.restore() + assert.deepEqual(state.perWindowState, [{a: 5}, {b: 2}]) + cb() + return Promise.resolve() + }) + + fakeElectron.ipcMain.send(messages.RESPONSE_WINDOW_STATE, makeSender(this.fakeWindow1), {a: 1}, 0) + fakeElectron.ipcMain.send(messages.RESPONSE_WINDOW_STATE, makeSender(this.fakeWindow2), {b: 2}, 0) + const requestId = 1 + + this.fakeWindow1.webContents.on(messages.REQUEST_WINDOW_STATE, () => { + setImmediate(() => { + fakeElectron.ipcMain.send(messages.RESPONSE_WINDOW_STATE, makeSender(this.fakeWindow1), {a: 5}, requestId) + }) + }) + + fakeElectron.app.emit('before-quit', { preventDefault: () => {} }) + assert.equal(saveAppStateStub.notCalled, true) + this.clock.tick(1000000) + }) + }) + }) +}) diff --git a/test/unit/lib/fakeElectron.js b/test/unit/lib/fakeElectron.js index f77c09e3ed7..04edee9d845 100644 --- a/test/unit/lib/fakeElectron.js +++ b/test/unit/lib/fakeElectron.js @@ -1,3 +1,6 @@ +const {EventEmitter} = require('events') +const ipcMain = new EventEmitter() +ipcMain.send = ipcMain.emit const fakeElectron = { BrowserWindow: { getFocusedWindow: function () { @@ -9,6 +12,9 @@ const fakeElectron = { return { id: 1 } + }, + getAllWindows: function () { + return [{id: 1}] } }, MenuItem: class { @@ -16,20 +22,14 @@ const fakeElectron = { this.template = template } }, - ipcMain: { - on: function () { }, - send: function () { } - }, + ipcMain, ipcRenderer: { on: function () { }, send: function () { }, sendSync: function () { } }, remote: { - app: { - on: function () { - } - }, + app: new EventEmitter(), clipboard: { readText: function () { return '' } }, @@ -48,14 +48,13 @@ const fakeElectron = { } } }, - app: { - on: function () { - }, + app: Object.assign(new EventEmitter(), { getPath: (param) => `${process.cwd()}/${param}`, getVersion: () => '0.14.0', setLocale: (locale) => {}, + quit: () => {}, exit: () => {} - }, + }), clipboard: { writeText: function () { } @@ -82,7 +81,8 @@ const fakeElectron = { }, extensions: { createTab: function () {} - } + }, + autoUpdater: new EventEmitter() } module.exports = fakeElectron diff --git a/test/unit/lib/fakeWindow.js b/test/unit/lib/fakeWindow.js new file mode 100644 index 00000000000..be694a57749 --- /dev/null +++ b/test/unit/lib/fakeWindow.js @@ -0,0 +1,19 @@ +/* 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 EventEmitter = require('events') + +class FakeWindow extends EventEmitter { + constructor (id) { + super() + this.id = id + this.webContents = Object.assign(new EventEmitter()) + this.webContents.send = this.webContents.emit + } + getId () { + return this.id + } +} + +module.exports = FakeWindow