From e860748d6b5c27f2d49447546e94e107a22dafca Mon Sep 17 00:00:00 2001
From: Robo <hop2deep@gmail.com>
Date: Thu, 13 Sep 2018 00:28:04 +0530
Subject: [PATCH] fix: Invalidate weak ptrs before window Javascript object is
 destroyed (#14532)

* fix: Invalidate weak ptrs before window Javascript object is destroyed

* chore: add regression test for #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 #14513
---
 atom/browser/api/atom_api_top_level_window.cc |  5 +++++
 spec/api-browser-window-spec.js               |  5 +++++
 spec/static/main.js                           | 20 +++++++++++++++++++
 3 files changed, 30 insertions(+)

diff --git a/atom/browser/api/atom_api_top_level_window.cc b/atom/browser/api/atom_api_top_level_window.cc
index 557dad7133ae..bbd414cb7f2d 100644
--- 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
index 65ba314024a9..7e33dcd509a0 100644
--- a/spec/api-browser-window-spec.js
+++ b/spec/api-browser-window-spec.js
@@ -221,6 +221,11 @@ describe('BrowserWindow module', () => {
         contents.getProcessId()
       }, /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
index 326f0da5cbf0..07ab5f4a3364 100644
--- a/spec/static/main.js
+++ b/spec/static/main.js
@@ -381,6 +381,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)