fix: Invalidate weak ptrs before window Javascript object is destroyed (backport: 3-0-x) (electron#14591)

trop[bot] · ckerr · commit 02b1069fd805 · 2018-09-12T15:22:05.000-05:00
* fix: Invalidate weak ptrs before window Javascript object is destroyed * chore: add regression test for electron#14513 This test is similar to the original gist at https://gist.github.com/bpasero/a02a645e11f4946dcca1331d0299149d -- the key is to open multiple windows and add an `app.on('browser-window-focus') listener that accesses window.id. * fix: last commit didn't test the right thing. The test needs to run in the main process to reproduce the conditions reported in electron#14513
diff --git a/atom/browser/api/atom_api_top_level_window.cc b/atom/browser/api/atom_api_top_level_window.cc
@@ -142,6 +142,11 @@ void TopLevelWindow::WillCloseWindow(bool* prevent_default) {
 }
 
 void TopLevelWindow::OnWindowClosed() {
+  // Invalidate weak ptrs before the Javascript object is destroyed,
+  // there might be some delayed emit events which shouldn't be
+  // triggered after this.
+  weak_factory_.InvalidateWeakPtrs();
+
   RemoveFromWeakMap();
   window_->RemoveObserver(this);
 
diff --git a/spec/api-browser-window-spec.js b/spec/api-browser-window-spec.js
@@ -210,6 +210,11 @@ describe('BrowserWindow module', () => {
         contents.getId()
       }, /Object has been destroyed/)
     })
+    it('should not crash when destroying windows with pending events', (done) => {
+      const responseEvent = 'destroy-test-completed'
+      ipcRenderer.on(responseEvent, () => done())
+      ipcRenderer.send('test-browserwindow-destroy', { responseEvent })
+    })
   })
 
   describe('BrowserWindow.loadURL(url)', () => {
diff --git a/spec/static/main.js b/spec/static/main.js
@@ -379,6 +379,26 @@ ipcMain.on('test-webcontents-navigation-observer', (event, options) => {
   contents.loadURL(options.url)
 })
 
+ipcMain.on('test-browserwindow-destroy', (event, testOptions) => {
+  let focusListener = (event, win) => win.id
+  app.on('browser-window-focus', focusListener)
+  const windowCount = 3
+  const windowOptions = {
+    show: false,
+    width: 400,
+    height: 400,
+    webPreferences: {
+      backgroundThrottling: false
+    }
+  }
+  const windows = Array.from(Array(windowCount)).map(x => new BrowserWindow(windowOptions))
+  windows.forEach(win => win.show())
+  windows.forEach(win => win.focus())
+  windows.forEach(win => win.destroy())
+  app.removeListener('browser-window-focus', focusListener)
+  event.sender.send(testOptions.responseEvent)
+})
+
 // Suspend listeners until the next event and then restore them
 const suspendListeners = (emitter, eventName, callback) => {
   const listeners = emitter.listeners(eventName)
