From acb2c099f6dde04bffa8e3c2cf422409abee8987 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Thu, 22 Oct 2015 14:56:57 +0800 Subject: [PATCH 01/15] Make sure current task runner is available When calling makeSingleInstance we have to ensure current task runnder is available, otherwise crash may happen. --- atom/browser/atom_browser_main_parts.cc | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/atom/browser/atom_browser_main_parts.cc b/atom/browser/atom_browser_main_parts.cc index 4c11176997ca..dfbdf4477fba 100644 --- a/atom/browser/atom_browser_main_parts.cc +++ b/atom/browser/atom_browser_main_parts.cc @@ -62,17 +62,15 @@ void AtomBrowserMainParts::PreEarlyInitialization() { void AtomBrowserMainParts::PostEarlyInitialization() { brightray::BrowserMainParts::PostEarlyInitialization(); - { - // Temporary set the bridge_task_runner_ as current thread's task runner, - // so we can fool gin::PerIsolateData to use it as its task runner, instead - // of getting current message loop's task runner, which is null for now. - bridge_task_runner_ = new BridgeTaskRunner; - base::ThreadTaskRunnerHandle handle(bridge_task_runner_); + // Temporary set the bridge_task_runner_ as current thread's task runner, + // so we can fool gin::PerIsolateData to use it as its task runner, instead + // of getting current message loop's task runner, which is null for now. + bridge_task_runner_ = new BridgeTaskRunner; + base::ThreadTaskRunnerHandle handle(bridge_task_runner_); - // The ProxyResolverV8 has setup a complete V8 environment, in order to - // avoid conflicts we only initialize our V8 environment after that. - js_env_.reset(new JavascriptEnvironment); - } + // The ProxyResolverV8 has setup a complete V8 environment, in order to + // avoid conflicts we only initialize our V8 environment after that. + js_env_.reset(new JavascriptEnvironment); node_bindings_->Initialize(); From 310954713f96245549f4f9cfc28c01ade72fde15 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Thu, 22 Oct 2015 14:59:12 +0800 Subject: [PATCH 02/15] Simplify the usage of singleton --- atom/browser/api/atom_api_app.cc | 38 ++++++++----------- atom/browser/api/atom_api_app.h | 1 - .../chrome/browser/process_singleton_posix.cc | 2 +- .../browser/process_singleton_startup_lock.cc | 2 +- .../browser/process_singleton_startup_lock.h | 2 +- 5 files changed, 18 insertions(+), 27 deletions(-) diff --git a/atom/browser/api/atom_api_app.cc b/atom/browser/api/atom_api_app.cc index c46f0748e3b6..2dfc3e056da5 100644 --- a/atom/browser/api/atom_api_app.cc +++ b/atom/browser/api/atom_api_app.cc @@ -163,10 +163,7 @@ void App::OnQuit() { Emit("quit"); if (process_singleton_.get()) { - if (process_notify_result_ == ProcessSingleton::PROCESS_NONE) { - process_singleton_->Cleanup(); - } - + process_singleton_->Cleanup(); process_singleton_.reset(); } } @@ -196,9 +193,8 @@ void App::OnFinishLaunching() { auto handle = Session::CreateFrom(isolate(), browser_context); default_session_.Reset(isolate(), handle.ToV8()); - if (process_singleton_.get()) { + if (process_singleton_startup_lock_.get()) process_singleton_startup_lock_->Unlock(); - } Emit("ready"); } @@ -282,35 +278,31 @@ v8::Local App::DefaultSession(v8::Isolate* isolate) { } bool App::MakeSingleInstance(ProcessSingleton::NotificationCallback callback) { - base::FilePath userDir; - PathService::Get(brightray::DIR_USER_DATA, &userDir); + if (process_singleton_.get()) + return false; - if (!process_singleton_.get()) { - auto browser = Browser::Get(); - process_singleton_startup_lock_.reset( + base::FilePath user_dir; + PathService::Get(brightray::DIR_USER_DATA, &user_dir); + + process_singleton_startup_lock_.reset( new ProcessSingletonStartupLock(callback)); - - process_singleton_.reset( + process_singleton_.reset( new ProcessSingleton( - userDir, + user_dir, process_singleton_startup_lock_->AsNotificationCallback())); - if (browser->is_ready()) { - process_singleton_startup_lock_->Unlock(); - } + if (Browser::Get()->is_ready()) + process_singleton_startup_lock_->Unlock(); - process_notify_result_ = process_singleton_->NotifyOtherProcessOrCreate(); - } - - switch (process_notify_result_) { + switch (process_singleton_->NotifyOtherProcessOrCreate()) { case ProcessSingleton::NotifyResult::PROCESS_NONE: return false; case ProcessSingleton::NotifyResult::LOCK_ERROR: case ProcessSingleton::NotifyResult::PROFILE_IN_USE: case ProcessSingleton::NotifyResult::PROCESS_NOTIFIED: + process_singleton_.reset(); + process_singleton_startup_lock_.reset(); return true; - default: - return false; } } diff --git a/atom/browser/api/atom_api_app.h b/atom/browser/api/atom_api_app.h index 314eeea8811d..8d55fc27c3b2 100644 --- a/atom/browser/api/atom_api_app.h +++ b/atom/browser/api/atom_api_app.h @@ -80,7 +80,6 @@ class App : public mate::EventEmitter, scoped_ptr process_singleton_; scoped_ptr process_singleton_startup_lock_; - ProcessSingleton::NotifyResult process_notify_result_; DISALLOW_COPY_AND_ASSIGN(App); }; diff --git a/chromium_src/chrome/browser/process_singleton_posix.cc b/chromium_src/chrome/browser/process_singleton_posix.cc index 5ab6eb6d8a78..4117dff8863e 100644 --- a/chromium_src/chrome/browser/process_singleton_posix.cc +++ b/chromium_src/chrome/browser/process_singleton_posix.cc @@ -1046,4 +1046,4 @@ void ProcessSingleton::KillProcess(int pid) { // progress of shutting down and finishes before we try to kill it). DCHECK(rv == 0 || errno == ESRCH) << "Error killing process: " << base::safe_strerror(errno); -} \ No newline at end of file +} diff --git a/chromium_src/chrome/browser/process_singleton_startup_lock.cc b/chromium_src/chrome/browser/process_singleton_startup_lock.cc index b97ada658c35..f564de7dca4d 100644 --- a/chromium_src/chrome/browser/process_singleton_startup_lock.cc +++ b/chromium_src/chrome/browser/process_singleton_startup_lock.cc @@ -50,4 +50,4 @@ bool ProcessSingletonStartupLock::NotificationCallbackImpl( } else { return original_callback_.Run(command_line, current_directory); } -} \ No newline at end of file +} diff --git a/chromium_src/chrome/browser/process_singleton_startup_lock.h b/chromium_src/chrome/browser/process_singleton_startup_lock.h index 3df51d8b177b..187dd35bce3d 100644 --- a/chromium_src/chrome/browser/process_singleton_startup_lock.h +++ b/chromium_src/chrome/browser/process_singleton_startup_lock.h @@ -54,4 +54,4 @@ class ProcessSingletonStartupLock : public base::NonThreadSafe { DISALLOW_COPY_AND_ASSIGN(ProcessSingletonStartupLock); }; -#endif // CHROME_BROWSER_PROCESS_SINGLETON_STARTUP_LOCK_H_ \ No newline at end of file +#endif // CHROME_BROWSER_PROCESS_SINGLETON_STARTUP_LOCK_H_ From bcb78ebc00e9f04299fd26955b3aa0e97c20a2e4 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Thu, 22 Oct 2015 15:02:32 +0800 Subject: [PATCH 03/15] app.makeSingleInstance is not available on OS X --- atom/browser/api/atom_api_app.cc | 4 ++++ docs/api/app.md | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/atom/browser/api/atom_api_app.cc b/atom/browser/api/atom_api_app.cc index 2dfc3e056da5..5ce918be78ad 100644 --- a/atom/browser/api/atom_api_app.cc +++ b/atom/browser/api/atom_api_app.cc @@ -278,6 +278,10 @@ v8::Local App::DefaultSession(v8::Isolate* isolate) { } bool App::MakeSingleInstance(ProcessSingleton::NotificationCallback callback) { +#if defined(OS_MACOSX) + LOG(ERROR) << "MakeSingleInstance is not implemnted on OS X"; + return false; +#endif if (process_singleton_.get()) return false; diff --git a/docs/api/app.md b/docs/api/app.md index 95aa80b6dc83..bd017456a78c 100644 --- a/docs/api/app.md +++ b/docs/api/app.md @@ -290,7 +290,7 @@ URLs that fall under "Local Intranet" sites (i.e. are in the same domain as you) However, this detection often fails when corporate networks are badly configured, so this lets you co-opt this behavior and enable it for all URLs. -### `app.makeSingleInstance(callback)` +### `app.makeSingleInstance(callback)` _Windows_ _Linux_ * `callback` Function From 05c63003294ccd8ef8b8b25524e713b1a0de9752 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Thu, 22 Oct 2015 15:54:27 +0800 Subject: [PATCH 04/15] Don't discard tasks in BridgeTaskRunner --- atom/browser/atom_browser_main_parts.cc | 1 + atom/browser/bridge_task_runner.cc | 32 +++++++++++++++++++++---- atom/browser/bridge_task_runner.h | 13 +++++++++- 3 files changed, 40 insertions(+), 6 deletions(-) diff --git a/atom/browser/atom_browser_main_parts.cc b/atom/browser/atom_browser_main_parts.cc index dfbdf4477fba..5fae5bfdbedd 100644 --- a/atom/browser/atom_browser_main_parts.cc +++ b/atom/browser/atom_browser_main_parts.cc @@ -105,6 +105,7 @@ void AtomBrowserMainParts::PreMainMessageLoopRun() { 1000)); brightray::BrowserMainParts::PreMainMessageLoopRun(); + BridgeTaskRunner::MessageLoopIsReady(); #if defined(USE_X11) libgtk2ui::GtkInitFromCommandLine(*base::CommandLine::ForCurrentProcess()); diff --git a/atom/browser/bridge_task_runner.cc b/atom/browser/bridge_task_runner.cc index 24572f3990d8..882a3050de48 100644 --- a/atom/browser/bridge_task_runner.cc +++ b/atom/browser/bridge_task_runner.cc @@ -8,13 +8,33 @@ namespace atom { +// static +std::vector BridgeTaskRunner::tasks_; +std::vector BridgeTaskRunner::non_nestable_tasks_; + +// static +void BridgeTaskRunner::MessageLoopIsReady() { + auto message_loop = base::MessageLoop::current(); + CHECK(message_loop); + for (const TaskPair& task : tasks_) { + message_loop->task_runner()->PostDelayedTask( + base::get<0>(task), base::get<1>(task), base::get<2>(task)); + } + for (const TaskPair& task : non_nestable_tasks_) { + message_loop->task_runner()->PostNonNestableDelayedTask( + base::get<0>(task), base::get<1>(task), base::get<2>(task)); + } +} + bool BridgeTaskRunner::PostDelayedTask( const tracked_objects::Location& from_here, const base::Closure& task, base::TimeDelta delay) { auto message_loop = base::MessageLoop::current(); - if (!message_loop) - return false; + if (!message_loop) { + tasks_.push_back(base::MakeTuple(from_here, task, delay)); + return true; + } return message_loop->task_runner()->PostDelayedTask(from_here, task, delay); } @@ -22,7 +42,7 @@ bool BridgeTaskRunner::PostDelayedTask( bool BridgeTaskRunner::RunsTasksOnCurrentThread() const { auto message_loop = base::MessageLoop::current(); if (!message_loop) - return false; + return true; return message_loop->task_runner()->RunsTasksOnCurrentThread(); } @@ -32,8 +52,10 @@ bool BridgeTaskRunner::PostNonNestableDelayedTask( const base::Closure& task, base::TimeDelta delay) { auto message_loop = base::MessageLoop::current(); - if (!message_loop) - return false; + if (!message_loop) { + non_nestable_tasks_.push_back(base::MakeTuple(from_here, task, delay)); + return true; + } return message_loop->task_runner()->PostNonNestableDelayedTask( from_here, task, delay); diff --git a/atom/browser/bridge_task_runner.h b/atom/browser/bridge_task_runner.h index fb42aa3852f9..12508f009d17 100644 --- a/atom/browser/bridge_task_runner.h +++ b/atom/browser/bridge_task_runner.h @@ -5,17 +5,23 @@ #ifndef ATOM_BROWSER_BRIDGE_TASK_RUNNER_H_ #define ATOM_BROWSER_BRIDGE_TASK_RUNNER_H_ +#include + #include "base/single_thread_task_runner.h" +#include "base/tuple.h" namespace atom { // Post all tasks to the current message loop's task runner if available, -// otherwise fail silently. +// otherwise delay the work until message loop is ready. class BridgeTaskRunner : public base::SingleThreadTaskRunner { public: BridgeTaskRunner() {} ~BridgeTaskRunner() override {} + // Called when message loop is ready. + static void MessageLoopIsReady(); + // base::SingleThreadTaskRunner: bool PostDelayedTask(const tracked_objects::Location& from_here, const base::Closure& task, @@ -27,6 +33,11 @@ class BridgeTaskRunner : public base::SingleThreadTaskRunner { base::TimeDelta delay) override; private: + using TaskPair = base::Tuple< + tracked_objects::Location, base::Closure, base::TimeDelta>; + static std::vector tasks_; + static std::vector non_nestable_tasks_; + DISALLOW_COPY_AND_ASSIGN(BridgeTaskRunner); }; From f01e84a4182eb2ce7aa86fb987b14b30cdfdabae Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Thu, 22 Oct 2015 15:54:58 +0800 Subject: [PATCH 05/15] linux: Delay listening to socket until message loop is ready --- .../chrome/browser/process_singleton.h | 3 +++ .../chrome/browser/process_singleton_posix.cc | 27 +++++++++++++------ 2 files changed, 22 insertions(+), 8 deletions(-) diff --git a/chromium_src/chrome/browser/process_singleton.h b/chromium_src/chrome/browser/process_singleton.h index 3265c9597a77..05288dc37cfe 100644 --- a/chromium_src/chrome/browser/process_singleton.h +++ b/chromium_src/chrome/browser/process_singleton.h @@ -133,6 +133,9 @@ class ProcessSingleton : public base::NonThreadSafe { base::FilePath user_data_dir_; ShouldKillRemoteProcessCallback should_kill_remote_process_callback_; #elif defined(OS_POSIX) && !defined(OS_ANDROID) + // Start listening to the socket. + void StartListening(int sock); + // Return true if the given pid is one of our child processes. // Assumes that the current pid is the root of all pids of the current // instance. diff --git a/chromium_src/chrome/browser/process_singleton_posix.cc b/chromium_src/chrome/browser/process_singleton_posix.cc index 4117dff8863e..5a8aac90793e 100644 --- a/chromium_src/chrome/browser/process_singleton_posix.cc +++ b/chromium_src/chrome/browser/process_singleton_posix.cc @@ -316,8 +316,7 @@ bool IsChromeProcess(pid_t pid) { PathService::Get(base::FILE_EXE, &exec_path); return (!other_chrome_path.empty() && - other_chrome_path.BaseName() == - exec_path.BaseName()); + other_chrome_path.BaseName() == exec_path.BaseName()); } // A helper class to hold onto a socket. @@ -988,13 +987,15 @@ bool ProcessSingleton::Create() { if (listen(sock, 5) < 0) NOTREACHED() << "listen failed: " << base::safe_strerror(errno); - DCHECK(BrowserThread::IsMessageLoopValid(BrowserThread::IO)); - BrowserThread::PostTask( - BrowserThread::IO, + // In Electron the ProcessSingleton is created earlier than the IO + // thread gets created, so we have to postpone the call until message + // loop is up an running. + scoped_refptr task_runner( + base::ThreadTaskRunnerHandle::Get()); + task_runner->PostTask( FROM_HERE, - base::Bind(&ProcessSingleton::LinuxWatcher::StartListening, - watcher_.get(), - sock)); + base::Bind(&ProcessSingleton::StartListening, + base::Unretained(this), sock)); return true; } @@ -1005,6 +1006,16 @@ void ProcessSingleton::Cleanup() { UnlinkPath(lock_path_); } +void ProcessSingleton::StartListening(int sock) { + DCHECK(BrowserThread::IsMessageLoopValid(BrowserThread::IO)); + BrowserThread::PostTask( + BrowserThread::IO, + FROM_HERE, + base::Bind(&ProcessSingleton::LinuxWatcher::StartListening, + watcher_.get(), + sock)); +} + bool ProcessSingleton::IsSameChromeInstance(pid_t pid) { pid_t cur_pid = current_pid_; while (pid != cur_pid) { From 230f2760e7c9136f94b2893d94b90339a7e0208c Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Thu, 22 Oct 2015 15:57:37 +0800 Subject: [PATCH 06/15] linux: Delay creating watcher until message loop is ready --- chromium_src/chrome/browser/process_singleton_posix.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/chromium_src/chrome/browser/process_singleton_posix.cc b/chromium_src/chrome/browser/process_singleton_posix.cc index 5a8aac90793e..811c5c16a24e 100644 --- a/chromium_src/chrome/browser/process_singleton_posix.cc +++ b/chromium_src/chrome/browser/process_singleton_posix.cc @@ -715,8 +715,7 @@ ProcessSingleton::ProcessSingleton( const base::FilePath& user_data_dir, const NotificationCallback& notification_callback) : notification_callback_(notification_callback), - current_pid_(base::GetCurrentProcId()), - watcher_(new LinuxWatcher(this)) { + current_pid_(base::GetCurrentProcId()) { socket_path_ = user_data_dir.Append(kSingletonSocketFilename); lock_path_ = user_data_dir.Append(kSingletonLockFilename); cookie_path_ = user_data_dir.Append(kSingletonCookieFilename); @@ -1007,6 +1006,7 @@ void ProcessSingleton::Cleanup() { } void ProcessSingleton::StartListening(int sock) { + watcher_ = new LinuxWatcher(this); DCHECK(BrowserThread::IsMessageLoopValid(BrowserThread::IO)); BrowserThread::PostTask( BrowserThread::IO, From ca876e424bd71de5fe09d6efb56e79482cfffed9 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Thu, 22 Oct 2015 16:06:37 +0800 Subject: [PATCH 07/15] Fix crash when calling app.quit() before app is ready --- atom/browser/browser.cc | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/atom/browser/browser.cc b/atom/browser/browser.cc index d8bb94103cd9..79ebe91a87e6 100644 --- a/atom/browser/browser.cc +++ b/atom/browser/browser.cc @@ -53,8 +53,14 @@ void Browser::Shutdown() { is_quiting_ = true; FOR_EACH_OBSERVER(BrowserObserver, observers_, OnQuit()); - base::MessageLoop::current()->PostTask( - FROM_HERE, base::MessageLoop::QuitWhenIdleClosure()); + + if (base::MessageLoop::current()) { + base::MessageLoop::current()->PostTask( + FROM_HERE, base::MessageLoop::QuitWhenIdleClosure()); + } else { + // There is no message loop available so we are in early stage. + exit(0); + } } std::string Browser::GetVersion() const { From e14fd62f4626bb016f3bf2f2e48ec7d892924304 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Thu, 22 Oct 2015 16:24:35 +0800 Subject: [PATCH 08/15] Correctly handle notification callback when shutting down When returning false in the notification callback the ProcessSingleton will assume current process is quitting, we should met its expectation. --- atom/browser/api/atom_api_app.cc | 15 +++++++++++++-- atom/browser/api/atom_api_app.h | 6 ++---- atom/browser/browser.h | 1 + 3 files changed, 16 insertions(+), 6 deletions(-) diff --git a/atom/browser/api/atom_api_app.cc b/atom/browser/api/atom_api_app.cc index 5ce918be78ad..fa2a97de4d90 100644 --- a/atom/browser/api/atom_api_app.cc +++ b/atom/browser/api/atom_api_app.cc @@ -112,6 +112,15 @@ int GetPathConstant(const std::string& name) { return -1; } +// Run the NotificationCallback and returns whether browser is shuting down. +bool NotificationCallbackWrapper( + const ProcessSingleton::NotificationCallback& callback, + const base::CommandLine& command_line, + const base::FilePath& current_directory) { + callback.Run(command_line, current_directory); + return !Browser::Get()->is_shutting_down(); +} + void OnClientCertificateSelected( v8::Isolate* isolate, std::shared_ptr delegate, @@ -277,7 +286,8 @@ v8::Local App::DefaultSession(v8::Isolate* isolate) { return v8::Local::New(isolate, default_session_); } -bool App::MakeSingleInstance(ProcessSingleton::NotificationCallback callback) { +bool App::MakeSingleInstance( + const ProcessSingleton::NotificationCallback& callback) { #if defined(OS_MACOSX) LOG(ERROR) << "MakeSingleInstance is not implemnted on OS X"; return false; @@ -289,7 +299,8 @@ bool App::MakeSingleInstance(ProcessSingleton::NotificationCallback callback) { PathService::Get(brightray::DIR_USER_DATA, &user_dir); process_singleton_startup_lock_.reset( - new ProcessSingletonStartupLock(callback)); + new ProcessSingletonStartupLock( + base::Bind(NotificationCallbackWrapper, callback))); process_singleton_.reset( new ProcessSingleton( user_dir, diff --git a/atom/browser/api/atom_api_app.h b/atom/browser/api/atom_api_app.h index 8d55fc27c3b2..bd87b3885035 100644 --- a/atom/browser/api/atom_api_app.h +++ b/atom/browser/api/atom_api_app.h @@ -68,11 +68,9 @@ class App : public mate::EventEmitter, void SetDesktopName(const std::string& desktop_name); void SetAppUserModelId(const std::string& app_id); - void AllowNTLMCredentialsForAllDomains(bool should_allow); - - bool MakeSingleInstance(ProcessSingleton::NotificationCallback callback); - + bool MakeSingleInstance( + const ProcessSingleton::NotificationCallback& callback); std::string GetLocale(); v8::Local DefaultSession(v8::Isolate* isolate); diff --git a/atom/browser/browser.h b/atom/browser/browser.h index 3c5abd2f0405..bae281d4d370 100644 --- a/atom/browser/browser.h +++ b/atom/browser/browser.h @@ -130,6 +130,7 @@ class Browser : public WindowListObserver { observers_.RemoveObserver(obs); } + bool is_shutting_down() const { return is_shutdown_; } bool is_quiting() const { return is_quiting_; } bool is_ready() const { return is_ready_; } From f9d797d1ea00e9239a7b1c74bae3861cefb8e023 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Thu, 22 Oct 2015 16:55:54 +0800 Subject: [PATCH 09/15] win: Fix compiler warning --- atom/browser/api/atom_api_app.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/atom/browser/api/atom_api_app.cc b/atom/browser/api/atom_api_app.cc index fa2a97de4d90..a9011c6e2e75 100644 --- a/atom/browser/api/atom_api_app.cc +++ b/atom/browser/api/atom_api_app.cc @@ -310,14 +310,15 @@ bool App::MakeSingleInstance( process_singleton_startup_lock_->Unlock(); switch (process_singleton_->NotifyOtherProcessOrCreate()) { - case ProcessSingleton::NotifyResult::PROCESS_NONE: - return false; case ProcessSingleton::NotifyResult::LOCK_ERROR: case ProcessSingleton::NotifyResult::PROFILE_IN_USE: case ProcessSingleton::NotifyResult::PROCESS_NOTIFIED: process_singleton_.reset(); process_singleton_startup_lock_.reset(); return true; + case ProcessSingleton::NotifyResult::PROCESS_NONE: + default: // Shouldn't be needed, but VS warns if it is not there. + return false; } } From 70e74d05e0b4cca5824b1d69b705207025c251b7 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Thu, 22 Oct 2015 17:03:18 +0800 Subject: [PATCH 10/15] Revert "app.makeSingleInstance is not available on OS X" This reverts commit bcb78ebc00e9f04299fd26955b3aa0e97c20a2e4. --- atom/browser/api/atom_api_app.cc | 4 ---- docs/api/app.md | 2 +- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/atom/browser/api/atom_api_app.cc b/atom/browser/api/atom_api_app.cc index a9011c6e2e75..42feaf8dc4ec 100644 --- a/atom/browser/api/atom_api_app.cc +++ b/atom/browser/api/atom_api_app.cc @@ -288,10 +288,6 @@ v8::Local App::DefaultSession(v8::Isolate* isolate) { bool App::MakeSingleInstance( const ProcessSingleton::NotificationCallback& callback) { -#if defined(OS_MACOSX) - LOG(ERROR) << "MakeSingleInstance is not implemnted on OS X"; - return false; -#endif if (process_singleton_.get()) return false; diff --git a/docs/api/app.md b/docs/api/app.md index bd017456a78c..95aa80b6dc83 100644 --- a/docs/api/app.md +++ b/docs/api/app.md @@ -290,7 +290,7 @@ URLs that fall under "Local Intranet" sites (i.e. are in the same domain as you) However, this detection often fails when corporate networks are badly configured, so this lets you co-opt this behavior and enable it for all URLs. -### `app.makeSingleInstance(callback)` _Windows_ _Linux_ +### `app.makeSingleInstance(callback)` * `callback` Function From afc1fff7920da5c7dc5c7ea526b5705309128b7a Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Thu, 22 Oct 2015 17:12:09 +0800 Subject: [PATCH 11/15] Remove the need for ProcessSingletonStartupLock --- atom/browser/api/atom_api_app.cc | 33 +++++------ atom/browser/api/atom_api_app.h | 2 - .../browser/process_singleton_startup_lock.cc | 53 ----------------- .../browser/process_singleton_startup_lock.h | 57 ------------------- filenames.gypi | 3 +- 5 files changed, 16 insertions(+), 132 deletions(-) delete mode 100644 chromium_src/chrome/browser/process_singleton_startup_lock.cc delete mode 100644 chromium_src/chrome/browser/process_singleton_startup_lock.h diff --git a/atom/browser/api/atom_api_app.cc b/atom/browser/api/atom_api_app.cc index 42feaf8dc4ec..84b9301316f6 100644 --- a/atom/browser/api/atom_api_app.cc +++ b/atom/browser/api/atom_api_app.cc @@ -112,12 +112,20 @@ int GetPathConstant(const std::string& name) { return -1; } -// Run the NotificationCallback and returns whether browser is shuting down. bool NotificationCallbackWrapper( const ProcessSingleton::NotificationCallback& callback, - const base::CommandLine& command_line, - const base::FilePath& current_directory) { - callback.Run(command_line, current_directory); + const base::CommandLine& cmd, + const base::FilePath& cwd) { + // Make sure the callback is called after app gets ready. + if (Browser::Get()->is_ready()) { + callback.Run(cmd, cwd); + } else { + scoped_refptr task_runner( + base::ThreadTaskRunnerHandle::Get()); + task_runner->PostTask( + FROM_HERE, base::Bind(base::IgnoreResult(callback), cmd, cwd)); + } + // ProcessSingleton needs to know whether current process is quiting. return !Browser::Get()->is_shutting_down(); } @@ -202,9 +210,6 @@ void App::OnFinishLaunching() { auto handle = Session::CreateFrom(isolate(), browser_context); default_session_.Reset(isolate(), handle.ToV8()); - if (process_singleton_startup_lock_.get()) - process_singleton_startup_lock_->Unlock(); - Emit("ready"); } @@ -293,24 +298,14 @@ bool App::MakeSingleInstance( base::FilePath user_dir; PathService::Get(brightray::DIR_USER_DATA, &user_dir); - - process_singleton_startup_lock_.reset( - new ProcessSingletonStartupLock( - base::Bind(NotificationCallbackWrapper, callback))); - process_singleton_.reset( - new ProcessSingleton( - user_dir, - process_singleton_startup_lock_->AsNotificationCallback())); - - if (Browser::Get()->is_ready()) - process_singleton_startup_lock_->Unlock(); + process_singleton_.reset(new ProcessSingleton( + user_dir, base::Bind(NotificationCallbackWrapper, callback))); switch (process_singleton_->NotifyOtherProcessOrCreate()) { case ProcessSingleton::NotifyResult::LOCK_ERROR: case ProcessSingleton::NotifyResult::PROFILE_IN_USE: case ProcessSingleton::NotifyResult::PROCESS_NOTIFIED: process_singleton_.reset(); - process_singleton_startup_lock_.reset(); return true; case ProcessSingleton::NotifyResult::PROCESS_NONE: default: // Shouldn't be needed, but VS warns if it is not there. diff --git a/atom/browser/api/atom_api_app.h b/atom/browser/api/atom_api_app.h index bd87b3885035..94cd9bbc68f9 100644 --- a/atom/browser/api/atom_api_app.h +++ b/atom/browser/api/atom_api_app.h @@ -11,7 +11,6 @@ #include "atom/browser/browser_observer.h" #include "atom/common/native_mate_converters/callback.h" #include "chrome/browser/process_singleton.h" -#include "chrome/browser/process_singleton_startup_lock.h" #include "content/public/browser/gpu_data_manager_observer.h" #include "native_mate/handle.h" @@ -77,7 +76,6 @@ class App : public mate::EventEmitter, v8::Global default_session_; scoped_ptr process_singleton_; - scoped_ptr process_singleton_startup_lock_; DISALLOW_COPY_AND_ASSIGN(App); }; diff --git a/chromium_src/chrome/browser/process_singleton_startup_lock.cc b/chromium_src/chrome/browser/process_singleton_startup_lock.cc deleted file mode 100644 index f564de7dca4d..000000000000 --- a/chromium_src/chrome/browser/process_singleton_startup_lock.cc +++ /dev/null @@ -1,53 +0,0 @@ -// Copyright (c) 2013 The Chromium Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#include "chrome/browser/process_singleton_startup_lock.h" - -#include "base/bind.h" -#include "base/logging.h" - -ProcessSingletonStartupLock::ProcessSingletonStartupLock( - const ProcessSingleton::NotificationCallback& original_callback) - : locked_(true), - original_callback_(original_callback) {} - -ProcessSingletonStartupLock::~ProcessSingletonStartupLock() {} - -ProcessSingleton::NotificationCallback -ProcessSingletonStartupLock::AsNotificationCallback() { - return base::Bind(&ProcessSingletonStartupLock::NotificationCallbackImpl, - base::Unretained(this)); -} - -void ProcessSingletonStartupLock::Unlock() { - DCHECK(CalledOnValidThread()); - locked_ = false; - - // Replay the command lines of the messages which were received while the - // ProcessSingleton was locked. Only replay each message once. - std::set replayed_messages; - for (std::vector::const_iterator it = - saved_startup_messages_.begin(); - it != saved_startup_messages_.end(); ++it) { - if (replayed_messages.find(*it) != replayed_messages.end()) - continue; - original_callback_.Run(base::CommandLine(it->first), it->second); - replayed_messages.insert(*it); - } - saved_startup_messages_.clear(); -} - -bool ProcessSingletonStartupLock::NotificationCallbackImpl( - const base::CommandLine& command_line, - const base::FilePath& current_directory) { - if (locked_) { - // If locked, it means we are not ready to process this message because - // we are probably in a first run critical phase. - saved_startup_messages_.push_back( - std::make_pair(command_line.argv(), current_directory)); - return true; - } else { - return original_callback_.Run(command_line, current_directory); - } -} diff --git a/chromium_src/chrome/browser/process_singleton_startup_lock.h b/chromium_src/chrome/browser/process_singleton_startup_lock.h deleted file mode 100644 index 187dd35bce3d..000000000000 --- a/chromium_src/chrome/browser/process_singleton_startup_lock.h +++ /dev/null @@ -1,57 +0,0 @@ -// Copyright (c) 2013 The Chromium Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#ifndef CHROME_BROWSER_PROCESS_SINGLETON_STARTUP_LOCK_H_ -#define CHROME_BROWSER_PROCESS_SINGLETON_STARTUP_LOCK_H_ - -#include -#include -#include - -#include "base/basictypes.h" -#include "base/command_line.h" -#include "base/files/file_path.h" -#include "base/threading/non_thread_safe.h" -#include "chrome/browser/process_singleton.h" - -// Provides a ProcessSingleton::NotificationCallback that can queue up -// command-line invocations during startup and execute them when startup -// completes. -// -// The object starts in a locked state. |Unlock()| must be called -// when the process is prepared to handle command-line invocations. -// -// Once unlocked, notifications are forwarded to a wrapped NotificationCallback. -class ProcessSingletonStartupLock : public base::NonThreadSafe { - public: - explicit ProcessSingletonStartupLock( - const ProcessSingleton::NotificationCallback& original_callback); - ~ProcessSingletonStartupLock(); - - // Returns the ProcessSingleton::NotificationCallback. - // The callback is only valid during the lifetime of the - // ProcessSingletonStartupLock instance. - ProcessSingleton::NotificationCallback AsNotificationCallback(); - - // Executes previously queued command-line invocations and allows future - // invocations to be executed immediately. - void Unlock(); - - bool locked() { return locked_; } - - private: - typedef std::pair - DelayedStartupMessage; - - bool NotificationCallbackImpl(const base::CommandLine& command_line, - const base::FilePath& current_directory); - - bool locked_; - std::vector saved_startup_messages_; - ProcessSingleton::NotificationCallback original_callback_; - - DISALLOW_COPY_AND_ASSIGN(ProcessSingletonStartupLock); -}; - -#endif // CHROME_BROWSER_PROCESS_SINGLETON_STARTUP_LOCK_H_ diff --git a/filenames.gypi b/filenames.gypi index ec7ab3291b3f..1c52cf50b2ae 100644 --- a/filenames.gypi +++ b/filenames.gypi @@ -347,6 +347,7 @@ 'chromium_src/chrome/browser/browser_process.cc', 'chromium_src/chrome/browser/browser_process.h', 'chromium_src/chrome/browser/chrome_process_finder_win.cc', + 'chromium_src/chrome/browser/chrome_process_finder_win.h', 'chromium_src/chrome/browser/chrome_notification_types.h', 'chromium_src/chrome/browser/extensions/global_shortcut_listener.cc', 'chromium_src/chrome/browser/extensions/global_shortcut_listener.h', @@ -376,8 +377,8 @@ 'chromium_src/chrome/browser/printing/print_preview_message_handler.cc', 'chromium_src/chrome/browser/printing/print_preview_message_handler.h', 'chromium_src/chrome/browser/process_singleton_posix.cc', - 'chromium_src/chrome/browser/process_singleton_startup_lock.cc', 'chromium_src/chrome/browser/process_singleton_win.cc', + 'chromium_src/chrome/browser/process_singleton.h', 'chromium_src/chrome/browser/renderer_host/pepper/chrome_browser_pepper_host_factory.cc', 'chromium_src/chrome/browser/renderer_host/pepper/chrome_browser_pepper_host_factory.h', 'chromium_src/chrome/browser/renderer_host/pepper/pepper_broker_message_filter.cc', From 93a3a946f3c7e6e4156d9491c3ba52b4cbf6df5f Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Thu, 22 Oct 2015 17:43:55 +0800 Subject: [PATCH 12/15] posix: Pass original command line --- chromium_src/chrome/browser/process_singleton_posix.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/chromium_src/chrome/browser/process_singleton_posix.cc b/chromium_src/chrome/browser/process_singleton_posix.cc index 811c5c16a24e..e338b55c5f88 100644 --- a/chromium_src/chrome/browser/process_singleton_posix.cc +++ b/chromium_src/chrome/browser/process_singleton_posix.cc @@ -52,6 +52,7 @@ #include #include +#include "atom/common/atom_command_line.h" #include "base/base_paths.h" #include "base/basictypes.h" #include "base/bind.h" @@ -817,7 +818,7 @@ ProcessSingleton::NotifyResult ProcessSingleton::NotifyOtherProcessWithTimeout( return PROCESS_NONE; to_send.append(current_dir.value()); - const std::vector& argv = cmd_line.argv(); + const std::vector& argv = atom::AtomCommandLine::argv(); for (std::vector::const_iterator it = argv.begin(); it != argv.end(); ++it) { to_send.push_back(kTokenDelimiter); From d52ef50b017725f5b20daf8f168d45268eab564f Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Thu, 22 Oct 2015 17:51:51 +0800 Subject: [PATCH 13/15] win: Pass original command line --- chromium_src/chrome/browser/chrome_process_finder_win.cc | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/chromium_src/chrome/browser/chrome_process_finder_win.cc b/chromium_src/chrome/browser/chrome_process_finder_win.cc index 299877a65db6..5a662258a0be 100644 --- a/chromium_src/chrome/browser/chrome_process_finder_win.cc +++ b/chromium_src/chrome/browser/chrome_process_finder_win.cc @@ -42,8 +42,6 @@ NotifyChromeResult AttemptToNotifyRunningChrome(HWND remote_window, if (!thread_id || !process_id) return NOTIFY_FAILED; - base::CommandLine command_line(*base::CommandLine::ForCurrentProcess()); - // Send the command line to the remote chrome window. // Format is "START\0<<>>\0<<>>". std::wstring to_send(L"START\0", 6); // want the NULL in the string. @@ -52,7 +50,7 @@ NotifyChromeResult AttemptToNotifyRunningChrome(HWND remote_window, return NOTIFY_FAILED; to_send.append(cur_dir.value()); to_send.append(L"\0", 1); // Null separator. - to_send.append(command_line.GetCommandLineString()); + to_send.append(::GetCommandLineW()); to_send.append(L"\0", 1); // Null separator. // Allow the current running browser window to make itself the foreground From 6bfe06ec4e5bc1dac93542bb920f680c7898698c Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Thu, 22 Oct 2015 19:02:21 +0800 Subject: [PATCH 14/15] Pass original argv in callback --- atom/browser/api/atom_api_app.cc | 3 +- atom/common/api/atom_bindings.cc | 2 +- .../command_line_converter.h | 38 ------------------- .../chrome/browser/process_singleton.h | 2 +- .../chrome/browser/process_singleton_posix.cc | 2 +- .../chrome/browser/process_singleton_win.cc | 19 ++++++++-- 6 files changed, 20 insertions(+), 46 deletions(-) delete mode 100644 atom/common/native_mate_converters/command_line_converter.h diff --git a/atom/browser/api/atom_api_app.cc b/atom/browser/api/atom_api_app.cc index 84b9301316f6..c79dea9f7c5c 100644 --- a/atom/browser/api/atom_api_app.cc +++ b/atom/browser/api/atom_api_app.cc @@ -19,7 +19,6 @@ #include "atom/browser/api/atom_api_web_contents.h" #include "atom/common/native_mate_converters/callback.h" #include "atom/common/native_mate_converters/file_path_converter.h" -#include "atom/common/native_mate_converters/command_line_converter.h" #include "atom/common/node_includes.h" #include "atom/common/options_switches.h" #include "base/command_line.h" @@ -114,7 +113,7 @@ int GetPathConstant(const std::string& name) { bool NotificationCallbackWrapper( const ProcessSingleton::NotificationCallback& callback, - const base::CommandLine& cmd, + const base::CommandLine::StringVector& cmd, const base::FilePath& cwd) { // Make sure the callback is called after app gets ready. if (Browser::Get()->is_ready()) { diff --git a/atom/common/api/atom_bindings.cc b/atom/common/api/atom_bindings.cc index 06fc30e7e3cd..60052a7216d0 100644 --- a/atom/common/api/atom_bindings.cc +++ b/atom/common/api/atom_bindings.cc @@ -41,7 +41,7 @@ void FatalErrorCallback(const char* location, const char* message) { } void Log(const base::string16& message) { - std::cout << message; + std::cout << message << std::flush; } } // namespace diff --git a/atom/common/native_mate_converters/command_line_converter.h b/atom/common/native_mate_converters/command_line_converter.h deleted file mode 100644 index a704ed7e92b7..000000000000 --- a/atom/common/native_mate_converters/command_line_converter.h +++ /dev/null @@ -1,38 +0,0 @@ -// Copyright (c) 2014 GitHub, Inc. -// Use of this source code is governed by the MIT license that can be -// found in the LICENSE file. - -#ifndef ATOM_COMMON_NATIVE_MATE_CONVERTERS_COMMAND_LINE_CONVERTER_H_ -#define ATOM_COMMON_NATIVE_MATE_CONVERTERS_COMMAND_LINE_CONVERTER_H_ - -#include - -#include "atom/common/native_mate_converters/string16_converter.h" -#include "base/command_line.h" - -namespace mate { - -template<> -struct Converter { - static v8::Local ToV8(v8::Isolate* isolate, - const base::CommandLine& val) { - return Converter::ToV8( - isolate, val.GetCommandLineString()); - } - static bool FromV8(v8::Isolate* isolate, - v8::Local val, - base::CommandLine* out) { - base::FilePath::StringType path; - - if (Converter::FromV8(isolate, val, &path)) { - *out = base::CommandLine(base::FilePath(path)); - return true; - } else { - return false; - } - } -}; - -} // namespace mate - -#endif // ATOM_COMMON_NATIVE_MATE_CONVERTERS_COMMAND_LINE_CONVERTER_H_ diff --git a/chromium_src/chrome/browser/process_singleton.h b/chromium_src/chrome/browser/process_singleton.h index 05288dc37cfe..3eeb53393e12 100644 --- a/chromium_src/chrome/browser/process_singleton.h +++ b/chromium_src/chrome/browser/process_singleton.h @@ -60,7 +60,7 @@ class ProcessSingleton : public base::NonThreadSafe { // handled within the current browser instance or false if the remote process // should handle it (i.e., because the current process is shutting down). using NotificationCallback = - base::Callback; ProcessSingleton(const base::FilePath& user_data_dir, diff --git a/chromium_src/chrome/browser/process_singleton_posix.cc b/chromium_src/chrome/browser/process_singleton_posix.cc index e338b55c5f88..b03ce431e47e 100644 --- a/chromium_src/chrome/browser/process_singleton_posix.cc +++ b/chromium_src/chrome/browser/process_singleton_posix.cc @@ -600,7 +600,7 @@ void ProcessSingleton::LinuxWatcher::HandleMessage( DCHECK(ui_message_loop_ == base::MessageLoop::current()); DCHECK(reader); - if (parent_->notification_callback_.Run(base::CommandLine(argv), + if (parent_->notification_callback_.Run(argv, base::FilePath(current_dir))) { // Send back "ACK" message to prevent the client process from starting up. reader->FinishWithACK(kACKToken, arraysize(kACKToken) - 1); diff --git a/chromium_src/chrome/browser/process_singleton_win.cc b/chromium_src/chrome/browser/process_singleton_win.cc index 0007b713b13b..14e53bec5fa7 100644 --- a/chromium_src/chrome/browser/process_singleton_win.cc +++ b/chromium_src/chrome/browser/process_singleton_win.cc @@ -77,8 +77,21 @@ BOOL CALLBACK BrowserWindowEnumeration(HWND window, LPARAM param) { return !*result; } +// Convert Command line string to argv. +base::CommandLine::StringVector CommandLineStringToArgv( + const std::wstring& command_line_string) { + int num_args = 0; + wchar_t** args = NULL; + args = ::CommandLineToArgvW(command_line_string.c_str(), &num_args); + base::CommandLine::StringVector argv; + for (int i = 0; i < num_args; ++i) + argv.push_back(std::wstring(args[i])); + LocalFree(args); + return argv; +} + bool ParseCommandLine(const COPYDATASTRUCT* cds, - base::CommandLine* parsed_command_line, + base::CommandLine::StringVector* parsed_command_line, base::FilePath* current_directory) { // We should have enough room for the shortest command (min_message_size) // and also be a multiple of wchar_t bytes. The shortest command @@ -131,7 +144,7 @@ bool ParseCommandLine(const COPYDATASTRUCT* cds, // Get command line. const std::wstring cmd_line = msg.substr(second_null + 1, third_null - second_null); - *parsed_command_line = base::CommandLine::FromString(cmd_line); + *parsed_command_line = CommandLineStringToArgv(cmd_line); return true; } return false; @@ -149,7 +162,7 @@ bool ProcessLaunchNotification( // Handle the WM_COPYDATA message from another process. const COPYDATASTRUCT* cds = reinterpret_cast(lparam); - base::CommandLine parsed_command_line(base::CommandLine::NO_PROGRAM); + base::CommandLine::StringVector parsed_command_line; base::FilePath current_directory; if (!ParseCommandLine(cds, &parsed_command_line, ¤t_directory)) { *result = TRUE; From 61f07307cb7c15b36e26cb4f099708b07609121b Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Thu, 22 Oct 2015 19:26:05 +0800 Subject: [PATCH 15/15] docs: New behaviors of makeSingleInstance --- docs/api/app.md | 62 ++++++++++++++++++++++++++++--------------------- 1 file changed, 36 insertions(+), 26 deletions(-) diff --git a/docs/api/app.md b/docs/api/app.md index 95aa80b6dc83..bb1509b68688 100644 --- a/docs/api/app.md +++ b/docs/api/app.md @@ -299,41 +299,51 @@ allowing multiple instances of your app to run, this will ensure that only a single instance of your app is running, and other instances signal this instance and exit. -`callback` is called when a second instance has been executed, and provides -the command-line (including Chromium flags) and the working directory of the -secondary instance. Usually applications respond to this by making their -primary window focused and non-minimized. +`callback` will be called with `callback(argv, workingDirectory)` when a second +instance has been executed. `argv` is an Array of the second instance's command +line arguments, and `workingDirectory` is its current working directory. Usually +applications respond to this by making their primary window focused and +non-minimized. -`callback` should return `true` if the message was successfully handled, or -`false` if the secondary process should retry sending it or it failed. +The `callback` is guaranteed to be executed after the `ready` event of `app` +gets emitted. + +This method returns `false` if your process is the primary instance of the +application and your app should continue loading. And returns `true` if your +process has sent its parameters to another instance, and you should immediately +quit. + +On OS X the system enforces single instance automatically when users try to open +a second instance of your app in Finder, and the `open-file` and `open-url` +events will be emitted for that. However when users start your app in command +line the system's single instance machanism will be bypassed and you have to +use this method to ensure single instance. + +An example of activating the window of primary instance when a second instance +starts: ```js -var myWindow; -app.on('ready', function() { - var shouldQuit = app.makeSingleInstance(function(commandLine, workingDirectory) { - // Someone tried to run a second instance, we should focus our window - if (myWindow) { - if (myWindow.isMinimized()) myWindow.restore(); - myWindow.focus(); - } +var myWindow = null; - // We successfully handled the command line - return true; - }); - - if (shouldQuit) { - app.quit(); - return; +var shouldQuit = app.makeSingleInstance(function(commandLine, workingDirectory) { + // Someone tried to run a second instance, we should focus our window + if (myWindow) { + if (myWindow.isMinimized()) myWindow.restore(); + myWindow.focus(); } + return true; +}); - // Create myWindow, load the rest of the app, etc... +if (shouldQuit) { + app.quit(); + return; +} + +// Create myWindow, load the rest of the app, etc... +app.on('ready', function() { }); ``` -Returns a Boolean - if `false`, your process is the primary instance of the -application and your app should continue loading. If `true`, your process has -sent its parameters to another instance, and you should immediately quit. - ### `app.commandLine.appendSwitch(switch[, value])` Append a switch (with optional `value`) to Chromium's command line.