Only ask Renderer Windows for saved state, on shutdown

Create a new windows API method to retrieve renderer windows.
Avoid circular dependency in sessionStoreShutdown / windows.js by exposing a 'new-window-state' event that the session store can subscribe to in order to set state.

Fix #12488 since state was being asked from all windows, even non-renderer windows, which causes a timeout waiting for a state response.

app/browser/windows.js

@@ -4,6 +4,7 @@
 
 const electron = require('electron')
 const Immutable = require('immutable')
+const { EventEmitter } = require('events')
 const appActions = require('../../js/actions/appActions')
 const appStore = require('../../js/stores/appStore')
 const appUrlUtil = require('../../js/lib/appUrlUtil')
@@ -12,7 +13,6 @@ const debounce = require('../../js/lib/debounce')
 const {getSetting} = require('../../js/settings')
 const locale = require('../locale')
 const LocalShortcuts = require('../localShortcuts')
-const {initWindowCacheState} = require('../sessionStoreShutdown')
 const {makeImmutable} = require('../common/state/immutableUtil')
 const {getPinnedTabsByWindowId} = require('../common/state/tabState')
 const messages = require('../../js/constants/messages')
@@ -31,6 +31,8 @@ const {app, BrowserWindow, ipcMain} = electron
 // TODO(bridiver) - set window uuid
 let currentWindows = {}
 const windowPinnedTabStateMemoize = new WeakMap()
+const publicEvents = new EventEmitter()
+let lastCreatedWindowIsRendererWindow = false
 
 const getWindowState = (win) => {
   if (win.isFullScreen()) {
@@ -154,6 +156,12 @@ function showDeferredShowWindow (win) {
 const api = {
   init: (state, action) => {
     app.on('browser-window-created', function (event, win) {
+      // handle non-renderer windows
+      if (!lastCreatedWindowIsRendererWindow) {
+        // nothing to do as yet
+        return
+      }
+      lastCreatedWindowIsRendererWindow = false
       let windowId = -1
       const updateWindowMove = debounce(updateWindow, 100)
       const updateWindowDebounce = debounce(updateWindow, 5)
@@ -423,10 +431,16 @@ const api = {
       fullscreenWhenRendered = true
     }
     // create window with Url to renderer
+    // new-window action handler is sync, so won't be added to any 'renderer collection' we create here
+    // by the time the action handler runs.
+    // Instead, add a flag to indicate the next-created window is an actual
+    // tabbed renderer window
+    lastCreatedWindowIsRendererWindow = true
     const win = new electron.BrowserWindow(windowOptions)
     win.loadURL(appUrlUtil.getBraveExtIndexHTML())
+
     // TODO: pass UUID
-    initWindowCacheState(win.id, immutableState)
+    publicEvents.emit('new-window-state', win.id, immutableState)
     // let the windowReady handler know to show the window
     win.__showWhenRendered = showWhenRendered
     if (win.__showWhenRendered) {
@@ -501,6 +515,20 @@ const api = {
     return windowState.WINDOW_ID_NONE
   },
 
+  /**
+   * Provides an array of all Browser Windows which are actual
+   * main windows (not background workers), and are not destroyed
+   */
+  getAllRendererWindows: () => {
+    return Object.keys(currentWindows)
+      .map(key => currentWindows[key])
+      .filter(win => win && !win.isDestroyed())
+  },
+
+  on: (...args) => publicEvents.on(...args),
+  off: (...args) => publicEvents.off(...args),
+  once: (...args) => publicEvents.once(...args),
+
   privateMethods: () => {
     return process.env.NODE_ENV === 'test'
     ? {

app/sessionStoreShutdown.js

@@ -4,8 +4,9 @@
 
 'use strict'
 
-const {BrowserWindow, app, ipcMain} = require('electron')
+const {app, ipcMain} = require('electron')
 const sessionStore = require('./sessionStore')
+const windows = require('./browser/windows')
 const updateStatus = require('../js/constants/updateStatus')
 const updater = require('./updater')
 const appConfig = require('../js/constants/appConfig')
@@ -32,6 +33,10 @@ let immutableWindowStateCache
 let sessionStoreQueue
 let appStore
 
+windows.on('new-window-state', (windowId, immutableWindowState) => {
+  immutableWindowStateCache = immutableWindowStateCache.set(windowId, immutableWindowState)
+})
+
 // Useful for automated tests
 const reset = () => {
   immutablePerWindowState = Immutable.List()
@@ -76,7 +81,7 @@ const saveAppState = (forceSave = false) => {
   }
 
   let immutableAppState = appStore.getState().set('perWindowState', immutablePerWindowState)
-  const receivedAllWindows = immutablePerWindowState.size === BrowserWindow.getAllWindows().length
+  const receivedAllWindows = immutablePerWindowState.size === windows.getAllRendererWindows().length
   if (receivedAllWindows) {
     clearTimeout(saveAppStateTimeout)
   }
@@ -138,12 +143,14 @@ const initiateSessionStateSave = () => {
     if (isAllWindowsClosed && immutableLastWindowClosedState) {
       immutablePerWindowState = immutablePerWindowState.push(immutableLastWindowClosedState)
       saveAppState(true)
-    } else if (BrowserWindow.getAllWindows().length > 0) {
+    } else if (windows.getAllRendererWindows().length > 0) {
       ++windowCloseRequestId
-      const windowIds = BrowserWindow.getAllWindows().map((win) => {
+      const windowIds = windows.getAllRendererWindows().map((win) => {
         return win.id
       })
-      BrowserWindow.getAllWindows().forEach((win) => win.webContents.send(messages.REQUEST_WINDOW_STATE, windowCloseRequestId))
+      windows.getAllRendererWindows().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(() => {
@@ -166,10 +173,6 @@ const removeWindowFromCache = (windowId) => {
   immutableWindowStateCache = immutableWindowStateCache.delete(windowId)
 }
 
-const initWindowCacheState = (windowId, immutableWindowState) => {
-  immutableWindowStateCache = immutableWindowStateCache.set(windowId, immutableWindowState)
-}
-
 app.on('before-quit', (e) => {
   if (shuttingDown && sessionStateStoreComplete) {
     return
@@ -241,6 +244,5 @@ module.exports = {
   startSessionSaveInterval,
   initiateSessionStateSave,
   reset,
-  removeWindowFromCache,
-  initWindowCacheState
+  removeWindowFromCache
 }

test/unit/app/sessionStoreShutdownTest.js

@@ -139,6 +139,7 @@ describe('sessionStoreShutdown unit tests', function () {
 
   describe('app before-quit event', function () {
     before(function () {
+      this.windowsAPI = require('../../../app/browser/windows')
       this.appStoreGetStateStub = sinon.stub(appStore, 'getState', () => {
         return Immutable.fromJS({})
       })
@@ -160,7 +161,7 @@ describe('sessionStoreShutdown unit tests', function () {
     describe('with no windows', function () {
       before(function () {
         this.requestId = 1
-        this.getAllWindowsStub = sinon.stub(fakeElectron.BrowserWindow, 'getAllWindows', () => {
+        this.getAllWindowsStub = sinon.stub(this.windowsAPI, 'getAllRendererWindows', () => {
           return []
         })
       })
@@ -234,7 +235,7 @@ describe('sessionStoreShutdown unit tests', function () {
       before(function () {
         this.fakeWindow1 = new FakeWindow(1)
         this.requestId = 1
-        this.getAllWindowsStub = sinon.stub(fakeElectron.BrowserWindow, 'getAllWindows', () => {
+        this.getAllWindowsStub = sinon.stub(this.windowsAPI, 'getAllRendererWindows', () => {
           return [this.fakeWindow1]
         })
       })
@@ -286,7 +287,7 @@ describe('sessionStoreShutdown unit tests', function () {
         this.fakeWindow2 = new FakeWindow(2)
         this.fakeWindow3 = new FakeWindow(3)
         this.requestId = 1
-        this.getAllWindowsStub = sinon.stub(fakeElectron.BrowserWindow, 'getAllWindows', () => {
+        this.getAllWindowsStub = sinon.stub(this.windowsAPI, 'getAllRendererWindows', () => {
           return [this.fakeWindow1, this.fakeWindow2, this.fakeWindow3]
         })
       })