From 31c7ed9b8c9c4444ee27fec4564ec6d4d896444a Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Sat, 2 Feb 2019 00:21:49 +0900 Subject: [PATCH] fix: shutdown after message loop is ready (#16671) --- atom/browser/atom_browser_main_parts.cc | 2 +- atom/browser/browser.cc | 17 +++++++++-------- atom/browser/browser.h | 5 ++++- spec/api-app-spec.js | 7 ------- spec/api-browser-view-spec.js | 7 ------- spec/api-web-contents-spec.js | 7 ------- spec/api-web-contents-view-spec.js | 7 ------- 7 files changed, 14 insertions(+), 38 deletions(-) diff --git a/atom/browser/atom_browser_main_parts.cc b/atom/browser/atom_browser_main_parts.cc index 3140af8cbc27..41f41c85abf9 100644 --- a/atom/browser/atom_browser_main_parts.cc +++ b/atom/browser/atom_browser_main_parts.cc @@ -455,7 +455,7 @@ bool AtomBrowserMainParts::MainMessageLoopRun(int* result_code) { void AtomBrowserMainParts::PreDefaultMainMessageLoopRun( base::OnceClosure quit_closure) { - Browser::SetMainMessageLoopQuitClosure(std::move(quit_closure)); + Browser::Get()->SetMainMessageLoopQuitClosure(std::move(quit_closure)); } void AtomBrowserMainParts::PostMainMessageLoopStart() { diff --git a/atom/browser/browser.cc b/atom/browser/browser.cc index e37de8104288..e4962b12e862 100644 --- a/atom/browser/browser.cc +++ b/atom/browser/browser.cc @@ -25,9 +25,6 @@ namespace atom { -// Null until/unless the default main message loop is running. -base::NoDestructor g_quit_main_message_loop; - Browser::LoginItemSettings::LoginItemSettings() = default; Browser::LoginItemSettings::~LoginItemSettings() = default; Browser::LoginItemSettings::LoginItemSettings(const LoginItemSettings& other) = @@ -95,11 +92,12 @@ void Browser::Shutdown() { for (BrowserObserver& observer : observers_) observer.OnQuit(); - if (*g_quit_main_message_loop) { - std::move(*g_quit_main_message_loop).Run(); + if (quit_main_message_loop_) { + std::move(quit_main_message_loop_).Run(); } else { - // There is no message loop available so we are in early stage. - exit(0); + // There is no message loop available so we are in early stage, wait until + // the quit_main_message_loop_ is available. + // Exiting now would leave defunct processes behind. } } @@ -196,7 +194,10 @@ void Browser::PreMainMessageLoopRun() { } void Browser::SetMainMessageLoopQuitClosure(base::OnceClosure quit_closure) { - *g_quit_main_message_loop = std::move(quit_closure); + if (is_shutdown_) + std::move(quit_closure).Run(); + else + quit_main_message_loop_ = std::move(quit_closure); } void Browser::NotifyAndShutdown() { diff --git a/atom/browser/browser.h b/atom/browser/browser.h index 98fba93fc513..f01ece367b52 100644 --- a/atom/browser/browser.h +++ b/atom/browser/browser.h @@ -244,7 +244,7 @@ class Browser : public WindowListObserver { // Stores the supplied |quit_closure|, to be run when the last Browser // instance is destroyed. - static void SetMainMessageLoopQuitClosure(base::OnceClosure quit_closure); + void SetMainMessageLoopQuitClosure(base::OnceClosure quit_closure); void AddObserver(BrowserObserver* obs) { observers_.AddObserver(obs); } @@ -287,6 +287,9 @@ class Browser : public WindowListObserver { // The browser is being shutdown. bool is_shutdown_ = false; + // Null until/unless the default main message loop is running. + base::OnceClosure quit_main_message_loop_; + int badge_count_ = 0; util::Promise* ready_promise_ = nullptr; diff --git a/spec/api-app-spec.js b/spec/api-app-spec.js index 8a08733d88c9..c27f35c3a632 100644 --- a/spec/api-app-spec.js +++ b/spec/api-app-spec.js @@ -1211,13 +1211,6 @@ describe('default behavior', () => { }) describe('window-all-closed', () => { - before(function () { - // FIXME(jkleinsc): Test is consistently failing on Windows 32 bit. - if (process.arch === 'ia32') { - this.skip() - } - }) - it('quits when the app does not handle the event', async () => { const result = await runTestApp('window-all-closed') expect(result).to.equal(false) diff --git a/spec/api-browser-view-spec.js b/spec/api-browser-view-spec.js index dd4175413da6..fbbb4ebe0180 100644 --- a/spec/api-browser-view-spec.js +++ b/spec/api-browser-view-spec.js @@ -228,13 +228,6 @@ describe('BrowserView module', () => { }) describe('new BrowserView()', () => { - before(function () { - // FIXME(jkleinsc): Test is consistently failing on Windows 32 bit. - if (process.arch === 'ia32') { - this.skip() - } - }) - it('does not crash on exit', async () => { const appPath = path.join(fixtures, 'api', 'leak-exit-browserview.js') const electronPath = remote.getGlobal('process').execPath diff --git a/spec/api-web-contents-spec.js b/spec/api-web-contents-spec.js index 6ae30d4efcb2..139dc7678aaa 100644 --- a/spec/api-web-contents-spec.js +++ b/spec/api-web-contents-spec.js @@ -916,13 +916,6 @@ describe('webContents module', () => { }) describe('create()', () => { - before(function () { - // FIXME(jkleinsc): Test is consistently failing on Windows 32 bit. - if (process.arch === 'ia32') { - this.skip() - } - }) - it('does not crash on exit', async () => { const appPath = path.join(__dirname, 'fixtures', 'api', 'leak-exit-webcontents.js') const electronPath = remote.getGlobal('process').execPath diff --git a/spec/api-web-contents-view-spec.js b/spec/api-web-contents-view-spec.js index e75b3eccfe32..8610dda5a787 100644 --- a/spec/api-web-contents-view-spec.js +++ b/spec/api-web-contents-view-spec.js @@ -34,13 +34,6 @@ describe('WebContentsView', () => { }) describe('new WebContentsView()', () => { - before(function () { - // FIXME(jkleinsc): Test is consistently failing on Windows 32 bit. - if (process.arch === 'ia32') { - this.skip() - } - }) - it('does not crash on exit', async () => { const appPath = path.join(__dirname, 'fixtures', 'api', 'leak-exit-webcontentsview.js') const electronPath = remote.getGlobal('process').execPath