Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Fix chance of having a lost window #9912

Merged
merged 1 commit into from
Jul 14, 2017
Merged

Fix chance of having a lost window #9912

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions app/browser/menu.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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')
}
Expand Down Expand Up @@ -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)
}
}
]
}
Expand Down Expand Up @@ -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() })
}

Expand Down
2 changes: 2 additions & 0 deletions app/browser/reducers/windowsReducer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) => {
Expand All @@ -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)
Expand Down
186 changes: 10 additions & 176 deletions app/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand All @@ -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 = {}
Expand All @@ -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) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this semi-colon intentional?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think it is. Because we don't use ; and you have } in the last line and this line starts with [, so this could lead into errors

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep it's a good practice to start lines like that with a semicolon so you don't need to worry about moved code and the preceding line of code.

process.on(signal, () => {
app.quit()
})
Expand Down Expand Up @@ -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) {
Expand All @@ -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()
})
Expand All @@ -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
Expand Down Expand Up @@ -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]) {
Expand All @@ -469,22 +303,22 @@ 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(),
initialState.settings[settings.UPDATE_TO_PREVIEW_RELEASES]
)

// 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) => {
Expand All @@ -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
Expand Down
19 changes: 19 additions & 0 deletions app/renderer/reducers/debugReducer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Loading