Merge pull request #9133 from electron/app-exit-failure

Fix app.exit() not closing all windows

atom/browser/api/atom_api_auto_updater.cc

@@ -87,16 +87,14 @@ void AutoUpdater::SetFeedURL(const std::string& url, mate::Arguments* args) {
 
 void AutoUpdater::QuitAndInstall() {
   // If we don't have any window then quitAndInstall immediately.
-  WindowList* window_list = WindowList::GetInstance();
-  if (window_list->empty()) {
+  if (WindowList::IsEmpty()) {
     auto_updater::AutoUpdater::QuitAndInstall();
     return;
   }
 
   // Otherwise do the restart after all windows have been closed.
-  window_list->AddObserver(this);
-  for (NativeWindow* window : *window_list)
-    window->Close();
+  WindowList::AddObserver(this);
+  WindowList::CloseAllWindows();
 }
 
 // static

atom/browser/browser.cc

@@ -43,11 +43,10 @@ void Browser::Quit() {
   if (!is_quiting_)
     return;
 
-  atom::WindowList* window_list = atom::WindowList::GetInstance();
-  if (window_list->empty())
+  if (atom::WindowList::IsEmpty())
     NotifyAndShutdown();
-
-  window_list->CloseAllWindows();
+  else
+    atom::WindowList::CloseAllWindows();
 }
 
 void Browser::Exit(mate::Arguments* args) {
@@ -65,14 +64,12 @@ void Browser::Exit(mate::Arguments* args) {
     is_exiting_ = true;
 
     // Must destroy windows before quitting, otherwise bad things can happen.
-    atom::WindowList* window_list = atom::WindowList::GetInstance();
-    if (window_list->empty()) {
+    if (atom::WindowList::IsEmpty()) {
       Shutdown();
     } else {
       // Unlike Quit(), we do not ask to close window, but destroy the window
       // without asking.
-      for (NativeWindow* window : *window_list)
-        window->CloseContents(nullptr);  // e.g. Destroy()
+      atom::WindowList::DestroyAllWindows();
     }
   }
 }

atom/browser/browser_linux.cc

@@ -16,9 +16,7 @@ namespace atom {
 
 void Browser::Focus() {
   // Focus on the first visible window.
-  WindowList* list = WindowList::GetInstance();
-  for (WindowList::iterator iter = list->begin(); iter != list->end(); ++iter) {
-    NativeWindow* window = *iter;
+  for (const auto& window : WindowList::GetWindows()) {
     if (window->IsVisible()) {
       window->Focus(true);
       break;

atom/browser/browser_mac.mm

@@ -204,9 +204,8 @@ std::string Browser::DockGetBadgeText() {
 }
 
 void Browser::DockHide() {
-  WindowList* list = WindowList::GetInstance();
-  for (WindowList::iterator it = list->begin(); it != list->end(); ++it)
-    [(*it)->GetNativeWindow() setCanHide:NO];
+  for (const auto& window : WindowList::GetWindows())
+    [window->GetNativeWindow() setCanHide:NO];
 
   ProcessSerialNumber psn = { 0, kCurrentProcess };
   TransformProcessType(&psn, kProcessTransformToUIElementApplication);

atom/browser/native_window.cc

@@ -104,8 +104,7 @@ NativeWindow::~NativeWindow() {
 // static
 NativeWindow* NativeWindow::FromWebContents(
     content::WebContents* web_contents) {
-  WindowList& window_list = *WindowList::GetInstance();
-  for (NativeWindow* window : window_list) {
+  for (const auto& window : WindowList::GetWindows()) {
     if (window->web_contents() == web_contents)
       return window;
   }

atom/browser/window_list.cc

@@ -26,6 +26,16 @@ WindowList* WindowList::GetInstance() {
   return instance_;
 }
 
+// static
+WindowList::WindowVector WindowList::GetWindows() {
+  return GetInstance()->windows_;
+}
+
+// static
+bool WindowList::IsEmpty() {
+  return GetInstance()->windows_.empty();
+}
+
 // static
 void WindowList::AddWindow(NativeWindow* window) {
   DCHECK(window);
@@ -76,6 +86,13 @@ void WindowList::CloseAllWindows() {
       window->Close();
 }
 
+// static
+void WindowList::DestroyAllWindows() {
+  WindowVector windows = GetInstance()->windows_;
+  for (const auto& window : windows)
+    window->CloseContents(nullptr);  // e.g. Destroy()
+}
+
 WindowList::WindowList() {
 }
 

atom/browser/window_list.h

@@ -19,23 +19,9 @@ class WindowListObserver;
 class WindowList {
  public:
   typedef std::vector<NativeWindow*> WindowVector;
-  typedef WindowVector::iterator iterator;
-  typedef WindowVector::const_iterator const_iterator;
 
-  // Windows are added to the list before they have constructed windows,
-  // so the |window()| member function may return NULL.
-  const_iterator begin() const { return windows_.begin(); }
-  const_iterator end() const { return windows_.end(); }
-
-  iterator begin() { return windows_.begin(); }
-  iterator end() { return windows_.end(); }
-
-  bool empty() const { return windows_.empty(); }
-  size_t size() const { return windows_.size(); }
-
-  NativeWindow* get(size_t index) const { return windows_[index]; }
-
-  static WindowList* GetInstance();
+  static WindowVector GetWindows();
+  static bool IsEmpty();
 
   // Adds or removes |window| from the list it is associated with.
   static void AddWindow(NativeWindow* window);
@@ -51,7 +37,12 @@ class WindowList {
   // Closes all windows.
   static void CloseAllWindows();
 
+  // Destroy all windows.
+  static void DestroyAllWindows();
+
  private:
+  static WindowList* GetInstance();
+
   WindowList();
   ~WindowList();
 

spec/api-app-spec.js

@@ -137,6 +137,16 @@ describe('app module', function () {
         done()
       })
     })
+
+    it('closes all windows', function (done) {
+      var appPath = path.join(__dirname, 'fixtures', 'api', 'exit-closes-all-windows-app')
+      var electronPath = remote.getGlobal('process').execPath
+      appProcess = ChildProcess.spawn(electronPath, [appPath])
+      appProcess.on('close', function (code) {
+        assert.equal(code, 123)
+        done()
+      })
+    })
   })
 
   describe('app.relaunch', function () {

spec/fixtures/api/exit-closes-all-windows-app/main.js

@@ -0,0 +1,19 @@
+const {app, BrowserWindow} = require('electron')
+
+const windows = []
+
+function createWindow (id) {
+  const window = new BrowserWindow({show: false})
+  window.loadURL(`data:,window${id}`)
+  windows.push(window)
+}
+
+app.once('ready', () => {
+  for (let i = 1; i <= 5; i++) {
+    createWindow(i)
+  }
+
+  setImmediate(function () {
+    app.exit(123)
+  })
+})

spec/fixtures/api/exit-closes-all-windows-app/package.json

@@ -0,0 +1,4 @@
+{
+  "name": "electron-exit-closes-all-windows",
+  "main": "main.js"
+}