From 048b1217c1890fed902580c00313b5eb0a4cc02f Mon Sep 17 00:00:00 2001 From: "Brian R. Bondy" Date: Tue, 12 Sep 2017 18:15:45 -0400 Subject: [PATCH] Fix dispatch issues for actions pre windowReady Fix #10866 Changes in this PR: - Browser process now registers the process right away for the dispatcher.. - Callbacks are registered from the window store. - Waits until the window is ready before dispatching, queues up messages sent before this. - No extensions.createTab will be issued before a window is ready. --- app/browser/tabs.js | 25 +++++++- app/browser/windows.js | 1 + app/common/actions/extensionActions.js | 20 +++---- js/actions/syncActions.js | 8 +-- js/constants/messages.js | 2 + js/dispatcher/appDispatcher.js | 79 +++++++++++++++++--------- js/state/contentSettings.js | 10 ++-- js/state/privacy.js | 6 +- js/stores/appStore.js | 27 ++++----- js/stores/eventStore.js | 6 +- js/stores/windowStore.js | 4 +- test/unit/js/stores/windowStoreTest.js | 3 + 12 files changed, 120 insertions(+), 71 deletions(-) diff --git a/app/browser/tabs.js b/app/browser/tabs.js index ca4dba67ec9..aa2149f4c70 100644 --- a/app/browser/tabs.js +++ b/app/browser/tabs.js @@ -30,6 +30,7 @@ const {cleanupWebContents, currentWebContents, getWebContents, updateWebContents const {FilterOptions} = require('ad-block') const {isResourceEnabled} = require('../filtering') const autofill = require('../autofill') +const {getWindow} = require('./windows') let currentPartitionNumber = 0 const incrementPartitionNumber = () => ++currentPartitionNumber @@ -744,9 +745,27 @@ const api = { createProperties.discarded = false createProperties.autoDiscardable = false } - extensions.createTab(createProperties, (tab) => { - cb && cb(tab) - }) + + const doCreate = () => { + extensions.createTab(createProperties, (tab) => { + cb && cb(tab) + }) + } + + if (createProperties.windowId) { + const win = getWindow(createProperties.windowId) + if (!win || win.isDestroyed()) { + console.error('Cannot create a tab for a window which does not exist or is destroyed') + return + } + if (!win.__ready) { + win.once(messages.WINDOW_RENDERER_READY, () => { + doCreate() + }) + return + } + } + doCreate() }) }, diff --git a/app/browser/windows.js b/app/browser/windows.js index 9335369adeb..8db8ac189fe 100644 --- a/app/browser/windows.js +++ b/app/browser/windows.js @@ -320,6 +320,7 @@ const api = { win.__alreadyPinnedSites = new Immutable.Set() updatePinnedTabs(win) win.__ready = true + win.emit(messages.WINDOW_RENDERER_READY) } }) }, diff --git a/app/common/actions/extensionActions.js b/app/common/actions/extensionActions.js index 1711c64689a..badf28a5ea7 100644 --- a/app/common/actions/extensionActions.js +++ b/app/common/actions/extensionActions.js @@ -3,7 +3,7 @@ * You can obtain one at http://mozilla.org/MPL/2.0/. */ 'use strict' -const AppDispatcher = require('../../../js/dispatcher/appDispatcher') +const appDispatcher = require('../../../js/dispatcher/appDispatcher') const ExtensionConstants = require('../constants/extensionConstants') const extensionActions = { @@ -14,7 +14,7 @@ const extensionActions = { * @param {object} browserAction - browser action details */ browserActionRegistered: function (extensionId, browserAction) { - AppDispatcher.dispatch({ + appDispatcher.dispatch({ actionType: ExtensionConstants.BROWSER_ACTION_REGISTERED, extensionId, browserAction @@ -22,7 +22,7 @@ const extensionActions = { }, browserActionUpdated: function (extensionId, browserAction, tabId) { - AppDispatcher.dispatch({ + appDispatcher.dispatch({ actionType: ExtensionConstants.BROWSER_ACTION_UPDATED, extensionId, browserAction, @@ -36,7 +36,7 @@ const extensionActions = { * @param {string} extensionId - the extension id */ extensionInstalled: function (extensionId, installInfo) { - AppDispatcher.dispatch({ + appDispatcher.dispatch({ actionType: ExtensionConstants.EXTENSION_INSTALLED, extensionId, installInfo @@ -49,7 +49,7 @@ const extensionActions = { * @param {string} extensionId - the extension id */ extensionUninstalled: function (extensionId) { - AppDispatcher.dispatch({ + appDispatcher.dispatch({ actionType: ExtensionConstants.EXTENSION_UNINSTALLED, extensionId }) @@ -61,7 +61,7 @@ const extensionActions = { * @param {string} extensionId - the extension id */ extensionEnabled: function (extensionId) { - AppDispatcher.dispatch({ + appDispatcher.dispatch({ actionType: ExtensionConstants.EXTENSION_ENABLED, extensionId }) @@ -73,7 +73,7 @@ const extensionActions = { * @param {string} extensionId - the extension id */ extensionDisabled: function (extensionId) { - AppDispatcher.dispatch({ + appDispatcher.dispatch({ actionType: ExtensionConstants.EXTENSION_DISABLED, extensionId }) @@ -88,7 +88,7 @@ const extensionActions = { * @param {string} icon - 16x16 extension icon */ contextMenuCreated: function (extensionId, menuItemId, properties, icon) { - AppDispatcher.dispatch({ + appDispatcher.dispatch({ actionType: ExtensionConstants.CONTEXT_MENU_CREATED, extensionId, menuItemId, @@ -103,7 +103,7 @@ const extensionActions = { * @param {string} extensionId - the extension id */ contextMenuAllRemoved: function (extensionId) { - AppDispatcher.dispatch({ + appDispatcher.dispatch({ actionType: ExtensionConstants.CONTEXT_MENU_ALL_REMOVED, extensionId }) @@ -117,7 +117,7 @@ const extensionActions = { * @param {object} info - the arg of onclick callback */ contextMenuClicked: function (extensionId, tabId, info) { - AppDispatcher.dispatch({ + appDispatcher.dispatch({ actionType: ExtensionConstants.CONTEXT_MENU_CLICKED, extensionId, tabId, diff --git a/js/actions/syncActions.js b/js/actions/syncActions.js index bc50ac2cec7..bdcedc3a216 100644 --- a/js/actions/syncActions.js +++ b/js/actions/syncActions.js @@ -3,25 +3,25 @@ * You can obtain one at http://mozilla.org/MPL/2.0/. */ 'use strict' -const AppDispatcher = require('../dispatcher/appDispatcher') +const appDispatcher = require('../dispatcher/appDispatcher') const syncConstants = require('../constants/syncConstants') const syncActions = { removeSite: function (item) { - AppDispatcher.dispatch({ + appDispatcher.dispatch({ actionType: syncConstants.SYNC_REMOVE_SITE, item }) }, clearHistory: function () { - AppDispatcher.dispatch({ + appDispatcher.dispatch({ actionType: syncConstants.SYNC_CLEAR_HISTORY }) }, clearSiteSettings: function () { - AppDispatcher.dispatch({ + appDispatcher.dispatch({ actionType: syncConstants.SYNC_CLEAR_SITE_SETTINGS }) } diff --git a/js/constants/messages.js b/js/constants/messages.js index 9d11a863735..21afa547b8a 100644 --- a/js/constants/messages.js +++ b/js/constants/messages.js @@ -118,6 +118,8 @@ const messages = { SET_CERT_ERROR_DETAIL: _, SET_SECURITY_STATE: _, /** @arg {number} key of frame, @arg {Object} security state */ HTTPSE_RULE_APPLIED: _, /** @arg {string} name of ruleset file, @arg {Object} details of rewritten request */ + // Dispatch related message + WINDOW_RENDERER_READY: _, // Extensions NEW_POPUP_WINDOW: _, // Localization diff --git a/js/dispatcher/appDispatcher.js b/js/dispatcher/appDispatcher.js index ec86225ead8..a7e19081369 100644 --- a/js/dispatcher/appDispatcher.js +++ b/js/dispatcher/appDispatcher.js @@ -49,6 +49,16 @@ class AppDispatcher { if (process.type === 'renderer') { ipc.send('app-dispatcher-register') } + return this.registerLocalCallback(callback) + } + + /** + * Same as above, but registers the specified callback + * locally only. This is used by the windowStore since + * the store process is registered as soon as the window + * is created. + */ + registerLocalCallback (callback) { this.callbacks.push(callback) return this.callbacks.length - 1 // index } @@ -131,33 +141,8 @@ class AppDispatcher { shutdown () { appDispatcher.dispatch = (payload) => {} } -} - -const appDispatcher = new AppDispatcher() - -const doneDispatching = () => { - if (dispatchCargo.idle()) { - appDispatcher.dispatching = false - } -} -const dispatchCargo = async.cargo((task, callback) => { - for (let i = 0; i < task.length; i++) { - appDispatcher.dispatchInternal(task[i], () => {}) - } - callback() - doneDispatching() -}, 200) - -const ipcCargo = async.cargo((tasks, callback) => { - ipc.send(messages.DISPATCH_ACTION, Serializer.serialize(tasks)) - callback() -}, 200) - -if (processType === 'browser') { - ipc.on('app-dispatcher-register', (event) => { - const registrant = event.sender - const hostWebContents = event.sender.hostWebContents || event.sender + registerWindow (registrant, hostWebContents) { const win = BrowserWindow.fromWebContents(hostWebContents) const windowId = win.id @@ -168,6 +153,15 @@ if (processType === 'browser') { callback() }, 20) + // If the window isn't ready yet then wait until it is ready before delivering + // messages to it. + if (!win.__ready) { + registrantCargo.pause() + win.on(messages.WINDOW_RENDERER_READY, () => { + registrantCargo.resume() + }) + } + const callback = function (payload) { try { if (registrant.isDestroyed()) { @@ -196,13 +190,42 @@ if (processType === 'browser') { appDispatcher.unregister(callback) } } - event.sender.on('crashed', () => { + registrant.on('crashed', () => { appDispatcher.unregister(callback) }) - event.sender.on('destroyed', () => { + registrant.on('destroyed', () => { appDispatcher.unregister(callback) }) appDispatcher.register(callback) + } +} + +const appDispatcher = new AppDispatcher() + +const doneDispatching = () => { + if (dispatchCargo.idle()) { + appDispatcher.dispatching = false + } +} + +const dispatchCargo = async.cargo((task, callback) => { + for (let i = 0; i < task.length; i++) { + appDispatcher.dispatchInternal(task[i], () => {}) + } + callback() + doneDispatching() +}, 200) + +const ipcCargo = async.cargo((tasks, callback) => { + ipc.send(messages.DISPATCH_ACTION, Serializer.serialize(tasks)) + callback() +}, 200) + +if (processType === 'browser') { + ipc.on('app-dispatcher-register', (event) => { + const registrant = event.sender + const hostWebContents = event.sender.hostWebContents || event.sender + appDispatcher.registerWindow(registrant, hostWebContents) }) const dispatchEventPayload = (event, payload) => { diff --git a/js/state/contentSettings.js b/js/state/contentSettings.js index 0c94ff98f53..48636fcce8a 100644 --- a/js/state/contentSettings.js +++ b/js/state/contentSettings.js @@ -2,7 +2,7 @@ * 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 AppDispatcher = require('../dispatcher/appDispatcher') +const appDispatcher = require('../dispatcher/appDispatcher') const AppStore = require('../stores/appStore') const appConstants = require('../constants/appConstants') const appConfig = require('../constants/appConfig') @@ -345,21 +345,21 @@ const doAction = (action) => { case appConstants.APP_REMOVE_SITE_SETTING: case appConstants.APP_CHANGE_SITE_SETTING: case appConstants.APP_ADD_NOSCRIPT_EXCEPTIONS: - AppDispatcher.waitFor([AppStore.dispatchToken], () => { + appDispatcher.waitFor([AppStore.dispatchToken], () => { userPrefsUpdateTrigger(action.temporary) contentSettingsUpdateTrigger(action.temporary) }) break case appConstants.APP_CHANGE_SETTING: case appConstants.APP_SET_RESOURCE_ENABLED: - AppDispatcher.waitFor([AppStore.dispatchToken], () => { + appDispatcher.waitFor([AppStore.dispatchToken], () => { userPrefsUpdateTrigger() contentSettingsUpdateTrigger() }) break case appConstants.APP_ALLOW_FLASH_ONCE: case appConstants.APP_ALLOW_FLASH_ALWAYS: - AppDispatcher.waitFor([AppStore.dispatchToken], () => { + appDispatcher.waitFor([AppStore.dispatchToken], () => { userPrefsUpdateTrigger(action.isPrivate) contentSettingsUpdateTrigger(action.isPrivate) }) @@ -377,5 +377,5 @@ module.exports.init = () => { updateContentSettings(AppStore.getState(), appConfig, incognito) ) - AppDispatcher.register(doAction) + appDispatcher.register(doAction) } diff --git a/js/state/privacy.js b/js/state/privacy.js index c499825f84d..416c8818de3 100644 --- a/js/state/privacy.js +++ b/js/state/privacy.js @@ -2,7 +2,7 @@ * 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 AppDispatcher = require('../dispatcher/appDispatcher') +const appDispatcher = require('../dispatcher/appDispatcher') const AppStore = require('../stores/appStore') const appConstants = require('../constants/appConstants') const {passwordManagers} = require('../constants/passwordManagers') @@ -24,7 +24,7 @@ let updateTrigger // Register callback to handle all updates const doAction = (action) => { if (action.actionType === appConstants.APP_CHANGE_SETTING) { - AppDispatcher.waitFor([AppStore.dispatchToken], () => { + appDispatcher.waitFor([AppStore.dispatchToken], () => { updateTrigger() }) } @@ -32,5 +32,5 @@ const doAction = (action) => { module.exports.init = () => { updateTrigger = registerUserPrefs(() => getPrivacySettings()) - AppDispatcher.register(doAction) + appDispatcher.register(doAction) } diff --git a/js/stores/appStore.js b/js/stores/appStore.js index 71ddb04f098..8c1b2b5e1b5 100644 --- a/js/stores/appStore.js +++ b/js/stores/appStore.js @@ -6,7 +6,7 @@ const appConstants = require('../constants/appConstants') const windowConstants = require('../constants/windowConstants') const ExtensionConstants = require('../../app/common/constants/extensionConstants') -const AppDispatcher = require('../dispatcher/appDispatcher') +const appDispatcher = require('../dispatcher/appDispatcher') const appConfig = require('../constants/appConfig') const settings = require('../constants/settings') const siteUtil = require('../state/siteUtil') @@ -200,9 +200,9 @@ const createWindow = (action) => { const toolbarUserInterfaceScale = getSetting(settings.TOOLBAR_UI_SCALE) setImmediate(() => { - let mainWindow = new BrowserWindow(Object.assign(windowProps, browserOpts, {disposition: frameOpts.disposition})) + const win = new BrowserWindow(Object.assign(windowProps, browserOpts, {disposition: frameOpts.disposition})) let restoredImmutableWindowState = action.restoredState - initWindowCacheState(mainWindow.id, restoredImmutableWindowState) + initWindowCacheState(win.id, restoredImmutableWindowState) // initialize frames state let frames = Immutable.List() @@ -231,21 +231,22 @@ const createWindow = (action) => { } if (immutableWindowState.getIn(['ui', 'isMaximized'])) { - mainWindow.maximize() + win.maximize() } if (immutableWindowState.getIn(['ui', 'isFullScreen'])) { - mainWindow.setFullScreen(true) + win.setFullScreen(true) } - mainWindow.webContents.on('did-finish-load', (e) => { + appDispatcher.registerWindow(win, win.webContents) + win.webContents.on('did-finish-load', (e) => { lastEmittedState = appState - mainWindow.webContents.setZoomLevel(zoomLevel[toolbarUserInterfaceScale] || 0.0) + win.webContents.setZoomLevel(zoomLevel[toolbarUserInterfaceScale] || 0.0) const mem = muon.shared_memory.create({ windowValue: { disposition: frameOpts.disposition, - id: mainWindow.id + id: win.id }, appState: appState.toJS(), frames: frames.toJS(), @@ -257,11 +258,11 @@ const createWindow = (action) => { } }) - mainWindow.on('ready-to-show', () => { - mainWindow.show() + win.on('ready-to-show', () => { + win.show() }) - mainWindow.loadURL(appUrlUtil.getBraveExtIndexHTML()) + win.loadURL(appUrlUtil.getBraveExtIndexHTML()) }) } @@ -438,7 +439,7 @@ const handleAppAction = (action) => { appState = require('../../app/sync').init(appState, action, appStore) break case appConstants.APP_SHUTTING_DOWN: - AppDispatcher.shutdown() + appDispatcher.shutdown() app.quit() break case appConstants.APP_NEW_WINDOW: @@ -915,6 +916,6 @@ const handleAppAction = (action) => { emitChanges() } -appStore.dispatchToken = AppDispatcher.register(handleAppAction) +appStore.dispatchToken = appDispatcher.register(handleAppAction) module.exports = appStore diff --git a/js/stores/eventStore.js b/js/stores/eventStore.js index d8d9ec7edb4..c33068cff38 100644 --- a/js/stores/eventStore.js +++ b/js/stores/eventStore.js @@ -3,7 +3,7 @@ * You can obtain one at http://mozilla.org/MPL/2.0/. */ const appConstants = require('../constants/appConstants') -const AppDispatcher = require('../dispatcher/appDispatcher') +const appDispatcher = require('../dispatcher/appDispatcher') const AppStore = require('./appStore') const EventEmitter = require('events').EventEmitter const Immutable = require('immutable') @@ -112,7 +112,7 @@ const doAction = (action) => { } break case appConstants.APP_CLOSE_WINDOW: - AppDispatcher.waitFor([AppStore.dispatchToken], () => { + appDispatcher.waitFor([AppStore.dispatchToken], () => { windowClosed(action.windowId) }) break @@ -151,6 +151,6 @@ const doAction = (action) => { emitChanges() } -AppDispatcher.register(doAction) +appDispatcher.register(doAction) module.exports = eventStore diff --git a/js/stores/windowStore.js b/js/stores/windowStore.js index 80e4f9b978b..83ec6000aec 100644 --- a/js/stores/windowStore.js +++ b/js/stores/windowStore.js @@ -2,7 +2,7 @@ * 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 AppDispatcher = require('../dispatcher/appDispatcher') +const appDispatcher = require('../dispatcher/appDispatcher') const EventEmitter = require('events').EventEmitter const appActions = require('../actions/appActions') const appConstants = require('../constants/appConstants') @@ -835,6 +835,6 @@ frameShortcuts.forEach((shortcut) => { } }) -AppDispatcher.register(doAction) +appDispatcher.registerLocalCallback(doAction) module.exports = windowStore diff --git a/test/unit/js/stores/windowStoreTest.js b/test/unit/js/stores/windowStoreTest.js index f3b258e0ee7..6f8e5a294e1 100644 --- a/test/unit/js/stores/windowStoreTest.js +++ b/test/unit/js/stores/windowStoreTest.js @@ -19,6 +19,9 @@ describe('Window store unit tests', function () { const fakeDispatcher = { register: (actionHandler) => { doAction = actionHandler + }, + registerLocalCallback: (actionHandler) => { + doAction = actionHandler } }