From 1e2e461e5e6a0814863b6eb698ed75732720af6d Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Thu, 26 Jul 2018 10:20:01 -0700 Subject: [PATCH 1/3] Avoid excessive numbers of IPC event listeners in ApplicationDelegate --- spec/auto-update-manager-spec.js | 7 +--- src/application-delegate.js | 71 ++++++++------------------------ 2 files changed, 20 insertions(+), 58 deletions(-) diff --git a/spec/auto-update-manager-spec.js b/spec/auto-update-manager-spec.js index 780c9816b..ab9e0ed70 100644 --- a/spec/auto-update-manager-spec.js +++ b/spec/auto-update-manager-spec.js @@ -1,11 +1,8 @@ -'use babel' - -import AutoUpdateManager from '../src/auto-update-manager' -import {remote} from 'electron' +const AutoUpdateManager = require('../src/auto-update-manager') +const {remote} = require('electron') const electronAutoUpdater = remote.require('electron').autoUpdater describe('AutoUpdateManager (renderer)', () => { - if (process.platform !== 'darwin') return // Tests are tied to electron autoUpdater, we use something else on Linux and Win32 let autoUpdateManager diff --git a/src/application-delegate.js b/src/application-delegate.js index ec6a37454..b78c62aae 100644 --- a/src/application-delegate.js +++ b/src/application-delegate.js @@ -1,12 +1,16 @@ const {ipcRenderer, remote, shell} = require('electron') const ipcHelpers = require('./ipc-helpers') -const {Disposable} = require('event-kit') +const {Emitter, Disposable} = require('event-kit') const getWindowLoadSettings = require('./get-window-load-settings') module.exports = class ApplicationDelegate { constructor () { this.pendingSettingsUpdateCount = 0 + this.ipcMessageEmitter = new Emitter() + ipcRenderer.on('message', (event, message, detail) => { + this.ipcMessageEmitter.emit(message, detail) + }) } getWindowLoadSettings () { return getWindowLoadSettings() } @@ -189,21 +193,13 @@ class ApplicationDelegate { } onDidChangeUserSettings (callback) { - const outerCallback = (event, message, detail) => { - if (message === 'did-change-user-settings') { - if (this.pendingSettingsUpdateCount === 0) callback(detail) - } - } - ipcRenderer.on('message', outerCallback) - return new Disposable(() => ipcRenderer.removeListener('message', outerCallback)) + return this.ipcMessageEmitter.on('did-change-user-settings', detail => { + if (this.pendingSettingsUpdateCount === 0) callback(detail) + }) } onDidFailToReadUserSettings (callback) { - const outerCallback = (event, message, detail) => { - if (message === 'did-fail-to-read-user-settings') callback(detail) - } - ipcRenderer.on('message', outerCallback) - return new Disposable(() => ipcRenderer.removeListener('message', outerCallback)) + return this.ipcMessageEmitter.on('did-fail-to-read-user-setting', callback) } confirm (options, callback) { @@ -261,24 +257,14 @@ class ApplicationDelegate { } onDidOpenLocations (callback) { - const outerCallback = (event, message, detail) => { - if (message === 'open-locations') callback(detail) - } - - ipcRenderer.on('message', outerCallback) - return new Disposable(() => ipcRenderer.removeListener('message', outerCallback)) + return this.ipcMessageEmitter.on('open-locations', callback) } onUpdateAvailable (callback) { - const outerCallback = (event, message, detail) => { - // TODO: Yes, this is strange that `onUpdateAvailable` is listening for - // `did-begin-downloading-update`. We currently have no mechanism to know - // if there is an update, so begin of downloading is a good proxy. - if (message === 'did-begin-downloading-update') callback(detail) - } - - ipcRenderer.on('message', outerCallback) - return new Disposable(() => ipcRenderer.removeListener('message', outerCallback)) + // TODO: Yes, this is strange that `onUpdateAvailable` is listening for + // `did-begin-downloading-update`. We currently have no mechanism to know + // if there is an update, so begin of downloading is a good proxy. + return this.ipcMessageEmitter.on('did-begin-downloading-update', callback) } onDidBeginDownloadingUpdate (callback) { @@ -286,40 +272,19 @@ class ApplicationDelegate { } onDidBeginCheckingForUpdate (callback) { - const outerCallback = (event, message, detail) => { - if (message === 'checking-for-update') callback(detail) - } - - ipcRenderer.on('message', outerCallback) - return new Disposable(() => ipcRenderer.removeListener('message', outerCallback)) + return this.ipcMessageEmitter.on('checking-for-update', callback) } onDidCompleteDownloadingUpdate (callback) { - const outerCallback = (event, message, detail) => { - // TODO: We could rename this event to `did-complete-downloading-update` - if (message === 'update-available') callback(detail) - } - - ipcRenderer.on('message', outerCallback) - return new Disposable(() => ipcRenderer.removeListener('message', outerCallback)) + return this.ipcMessageEmitter.on('update-available', callback) } onUpdateNotAvailable (callback) { - const outerCallback = (event, message, detail) => { - if (message === 'update-not-available') callback(detail) - } - - ipcRenderer.on('message', outerCallback) - return new Disposable(() => ipcRenderer.removeListener('message', outerCallback)) + return this.ipcMessageEmitter.on('update-not-available', callback) } onUpdateError (callback) { - const outerCallback = (event, message, detail) => { - if (message === 'update-error') callback(detail) - } - - ipcRenderer.on('message', outerCallback) - return new Disposable(() => ipcRenderer.removeListener('message', outerCallback)) + return this.ipcMessageEmitter.on('update-error', callback) } onApplicationMenuCommand (handler) { From c8c4e4fa3d31ca692e81fd26dcc4cae5bc91c81a Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Thu, 26 Jul 2018 10:23:51 -0700 Subject: [PATCH 2/3] Delete redundant test for on atom.onUpdateAvailable This test emitted an event in the main process, which caused exceptions in all *other* Atom windows :scream_cat:. --- spec/atom-environment-spec.js | 23 ----------------------- 1 file changed, 23 deletions(-) diff --git a/spec/atom-environment-spec.js b/spec/atom-environment-spec.js index 5574e9663..37f7de72c 100644 --- a/spec/atom-environment-spec.js +++ b/spec/atom-environment-spec.js @@ -746,29 +746,6 @@ describe('AtomEnvironment', () => { }) }) - describe('::updateAvailable(info) (called via IPC from browser process)', () => { - let subscription - - afterEach(() => { - if (subscription) subscription.dispose() - }) - - it('invokes onUpdateAvailable listeners', async () => { - if (process.platform !== 'darwin') return // Test tied to electron autoUpdater, we use something else on Linux and Win32 - - const updateAvailablePromise = new Promise(resolve => { - subscription = atom.onUpdateAvailable(resolve) - }) - - atom.listenForUpdates() - const {autoUpdater} = require('electron').remote - autoUpdater.emit('update-downloaded', null, 'notes', 'version') - - const {releaseVersion} = await updateAvailablePromise - expect(releaseVersion).toBe('version') - }) - }) - describe('::getReleaseChannel()', () => { let version From 5f0231b3988b5c292c6cb8c9e961524a695b2a72 Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Thu, 26 Jul 2018 10:39:36 -0700 Subject: [PATCH 3/3] Listen for IPC messages lazily in application delegate --- src/application-delegate.js | 31 +++++++++++++++++++------------ 1 file changed, 19 insertions(+), 12 deletions(-) diff --git a/src/application-delegate.js b/src/application-delegate.js index b78c62aae..8d7981edb 100644 --- a/src/application-delegate.js +++ b/src/application-delegate.js @@ -7,10 +7,17 @@ module.exports = class ApplicationDelegate { constructor () { this.pendingSettingsUpdateCount = 0 - this.ipcMessageEmitter = new Emitter() - ipcRenderer.on('message', (event, message, detail) => { - this.ipcMessageEmitter.emit(message, detail) - }) + this._ipcMessageEmitter = null + } + + ipcMessageEmitter () { + if (!this._ipcMessageEmitter) { + this._ipcMessageEmitter = new Emitter() + ipcRenderer.on('message', (event, message, detail) => { + this._ipcMessageEmitter.emit(message, detail) + }) + } + return this._ipcMessageEmitter } getWindowLoadSettings () { return getWindowLoadSettings() } @@ -193,13 +200,13 @@ class ApplicationDelegate { } onDidChangeUserSettings (callback) { - return this.ipcMessageEmitter.on('did-change-user-settings', detail => { + return this.ipcMessageEmitter().on('did-change-user-settings', detail => { if (this.pendingSettingsUpdateCount === 0) callback(detail) }) } onDidFailToReadUserSettings (callback) { - return this.ipcMessageEmitter.on('did-fail-to-read-user-setting', callback) + return this.ipcMessageEmitter().on('did-fail-to-read-user-setting', callback) } confirm (options, callback) { @@ -257,14 +264,14 @@ class ApplicationDelegate { } onDidOpenLocations (callback) { - return this.ipcMessageEmitter.on('open-locations', callback) + return this.ipcMessageEmitter().on('open-locations', callback) } onUpdateAvailable (callback) { // TODO: Yes, this is strange that `onUpdateAvailable` is listening for // `did-begin-downloading-update`. We currently have no mechanism to know // if there is an update, so begin of downloading is a good proxy. - return this.ipcMessageEmitter.on('did-begin-downloading-update', callback) + return this.ipcMessageEmitter().on('did-begin-downloading-update', callback) } onDidBeginDownloadingUpdate (callback) { @@ -272,19 +279,19 @@ class ApplicationDelegate { } onDidBeginCheckingForUpdate (callback) { - return this.ipcMessageEmitter.on('checking-for-update', callback) + return this.ipcMessageEmitter().on('checking-for-update', callback) } onDidCompleteDownloadingUpdate (callback) { - return this.ipcMessageEmitter.on('update-available', callback) + return this.ipcMessageEmitter().on('update-available', callback) } onUpdateNotAvailable (callback) { - return this.ipcMessageEmitter.on('update-not-available', callback) + return this.ipcMessageEmitter().on('update-not-available', callback) } onUpdateError (callback) { - return this.ipcMessageEmitter.on('update-error', callback) + return this.ipcMessageEmitter().on('update-error', callback) } onApplicationMenuCommand (handler) {