REVIEW: destroy process singleton on sequence where IO is allowed

atom/browser/api/atom_api_app.cc

@@ -529,6 +529,8 @@ void OnIconDataAvailable(v8::Isolate* isolate,
 }  // namespace
 
 App::App(v8::Isolate* isolate) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
   static_cast<AtomBrowserClient*>(AtomBrowserClient::Get())->set_delegate(this);
   Browser::Get()->AddObserver(this);
   content::GpuDataManager::GetInstance()->AddObserver(this);
@@ -566,11 +568,6 @@ void App::OnWindowAllClosed() {
 void App::OnQuit() {
   int exitCode = AtomBrowserMainParts::Get()->GetExitCode();
   Emit("quit", exitCode);
-
-  if (process_singleton_) {
-    process_singleton_->Cleanup();
-    process_singleton_.reset();
-  }
 }
 
 void App::OnOpenFile(bool* prevent_default, const std::string& file_path) {
@@ -598,12 +595,6 @@ void App::OnFinishLaunching(const base::DictionaryValue& launch_info) {
   Emit("ready", launch_info);
 }
 
-void App::OnPreMainMessageLoopRun() {
-  if (process_singleton_) {
-    process_singleton_->OnBrowserReady();
-  }
-}
-
 void App::OnAccessibilitySupportChanged() {
   Emit("accessibility-support-changed", IsAccessibilitySupportEnabled());
 }
@@ -856,20 +847,20 @@ std::string App::GetLocale() {
 
 bool App::MakeSingleInstance(
     const ProcessSingleton::NotificationCallback& callback) {
-  if (process_singleton_)
+  auto process_singleton = AtomBrowserMainParts::Get()->process_singleton();
+  if (process_singleton_created_)
     return false;
 
-  base::FilePath user_dir;
-  PathService::Get(brightray::DIR_USER_DATA, &user_dir);
-  process_singleton_.reset(new ProcessSingleton(
-      user_dir, base::Bind(NotificationCallbackWrapper, callback)));
+  process_singleton->RegisterSingletonNotificationCallback(
+      base::Bind(NotificationCallbackWrapper, callback));
 
-  switch (process_singleton_->NotifyOtherProcessOrCreate()) {
+  switch (process_singleton->NotifyOtherProcessOrCreate()) {
     case ProcessSingleton::NotifyResult::LOCK_ERROR:
     case ProcessSingleton::NotifyResult::PROFILE_IN_USE:
-    case ProcessSingleton::NotifyResult::PROCESS_NOTIFIED:
-      process_singleton_.reset();
+    case ProcessSingleton::NotifyResult::PROCESS_NOTIFIED: {
+      process_singleton_created_ = true;
       return true;
+    }
     case ProcessSingleton::NotifyResult::PROCESS_NONE:
     default:  // Shouldn't be needed, but VS warns if it is not there.
       return false;
@@ -877,9 +868,9 @@ bool App::MakeSingleInstance(
 }
 
 void App::ReleaseSingleInstance() {
-  if (process_singleton_) {
-    process_singleton_->Cleanup();
-    process_singleton_.reset();
+  auto process_singleton = AtomBrowserMainParts::Get()->process_singleton();
+  if (process_singleton) {
+    process_singleton->Cleanup();
   }
 }
 

atom/browser/api/atom_api_app.h

@@ -104,7 +104,6 @@ class App : public AtomBrowserClient::Delegate,
   void OnLogin(LoginHandler* login_handler,
                const base::DictionaryValue& request_details) override;
   void OnAccessibilitySupportChanged() override;
-  void OnPreMainMessageLoopRun() override;
 #if defined(OS_MACOSX)
   void OnWillContinueUserActivity(
       bool* prevent_default,
@@ -218,8 +217,6 @@ class App : public AtomBrowserClient::Delegate,
   JumpListResult SetJumpList(v8::Local<v8::Value> val, mate::Arguments* args);
 #endif  // defined(OS_WIN)
 
-  std::unique_ptr<ProcessSingleton> process_singleton_;
-
 #if defined(USE_NSS_CERTS)
   std::unique_ptr<CertificateManagerModel> certificate_manager_model_;
 #endif
@@ -234,6 +231,8 @@ class App : public AtomBrowserClient::Delegate,
                          std::unique_ptr<atom::ProcessMetric>>;
   ProcessMetricMap app_metrics_;
 
+  bool process_singleton_created_ = false;
+
   DISALLOW_COPY_AND_ASSIGN(App);
 };
 

atom/browser/atom_browser_main_parts.cc

@@ -126,27 +126,7 @@ void AtomBrowserMainParts::PostEarlyInitialization() {
 
   // 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();
-
-  // Create the global environment.
-  node::Environment* env =
-      node_bindings_->CreateEnvironment(js_env_->context());
-  node_env_.reset(new NodeEnvironment(env));
-
-  // Enable support for v8 inspector
-  node_debugger_.reset(new NodeDebugger(env));
-  node_debugger_->Start(js_env_->platform());
-
-  // Add Electron extended APIs.
-  atom_bindings_->BindTo(js_env_->isolate(), env->process_object());
-
-  // Load everything.
-  node_bindings_->LoadEnvironment(env);
-
-  // Wrap the uv loop with global env.
-  node_bindings_->set_uv_env(env);
+  JavascriptEnvironment::Initialize();
 }
 
 void AtomBrowserMainParts::PreMainMessageLoopRun() {
@@ -185,7 +165,8 @@ void AtomBrowserMainParts::PreMainMessageLoopRun() {
   Browser::Get()->DidFinishLaunching(*empty_info);
 #endif
 
-  Browser::Get()->PreMainMessageLoopRun();
+  if (process_singleton_)
+    process_singleton_->OnBrowserReady();
 }
 
 bool AtomBrowserMainParts::MainMessageLoopRun(int* result_code) {
@@ -200,6 +181,32 @@ void AtomBrowserMainParts::PostMainMessageLoopStart() {
 #endif
   device::GeolocationProvider::SetGeolocationDelegate(
       new AtomGeolocationDelegate());
+
+  base::FilePath user_dir;
+  PathService::Get(brightray::DIR_USER_DATA, &user_dir);
+  process_singleton_.reset(new ProcessSingleton(user_dir));
+
+  js_env_.reset(new JavascriptEnvironment);
+
+  node_bindings_->Initialize();
+
+  // Create the global environment.
+  node::Environment* env =
+      node_bindings_->CreateEnvironment(js_env_->context());
+  node_env_.reset(new NodeEnvironment(env));
+
+  // Enable support for v8 inspector
+  node_debugger_.reset(new NodeDebugger(env));
+  node_debugger_->Start();
+
+  // Add Electron extended APIs.
+  atom_bindings_->BindTo(js_env_->isolate(), env->process_object());
+
+  // Load everything.
+  node_bindings_->LoadEnvironment(env);
+
+  // Wrap the uv loop with global env.
+  node_bindings_->set_uv_env(env);
 }
 
 void AtomBrowserMainParts::PostMainMessageLoopRun() {
@@ -223,4 +230,12 @@ void AtomBrowserMainParts::PostMainMessageLoopRun() {
   }
 }
 
+void AtomBrowserMainParts::PostDestroyThreads() {
+  brightray::BrowserMainParts::PostDestroyThreads();
+  if (process_singleton_) {
+    process_singleton_->Cleanup();
+    process_singleton_.reset();
+  }
+}
+
 }  // namespace atom

atom/browser/atom_browser_main_parts.h

@@ -11,6 +11,7 @@
 #include "base/callback.h"
 #include "base/timer/timer.h"
 #include "brightray/browser/browser_main_parts.h"
+#include "chrome/browser/process_singleton.h"
 #include "content/public/browser/browser_context.h"
 
 class BrowserProcess;
@@ -44,6 +45,7 @@ class AtomBrowserMainParts : public brightray::BrowserMainParts {
   base::Closure RegisterDestructionCallback(const base::Closure& callback);
 
   Browser* browser() { return browser_.get(); }
+  ProcessSingleton* process_singleton() { return process_singleton_.get(); }
 
  protected:
   // content::BrowserMainParts:
@@ -56,6 +58,7 @@ class AtomBrowserMainParts : public brightray::BrowserMainParts {
 #if defined(OS_MACOSX)
   void PreMainMessageLoopStart() override;
 #endif
+  void PostDestroyThreads() override;
 
  private:
 #if defined(OS_POSIX)
@@ -84,6 +87,7 @@ class AtomBrowserMainParts : public brightray::BrowserMainParts {
   std::unique_ptr<AtomBindings> atom_bindings_;
   std::unique_ptr<NodeEnvironment> node_env_;
   std::unique_ptr<NodeDebugger> node_debugger_;
+  std::unique_ptr<ProcessSingleton> process_singleton_;
 
   base::Timer gc_timer_;
 

atom/browser/browser.cc

@@ -203,12 +203,6 @@ void Browser::RequestLogin(
     observer.OnLogin(login_handler, *(request_details.get()));
 }
 
-void Browser::PreMainMessageLoopRun() {
-  for (BrowserObserver& observer : observers_) {
-    observer.OnPreMainMessageLoopRun();
-  }
-}
-
 void Browser::NotifyAndShutdown() {
   if (is_shutdown_)
     return;

atom/browser/browser.h

@@ -224,8 +224,6 @@ class Browser : public WindowListObserver {
   void RequestLogin(LoginHandler* login_handler,
                     std::unique_ptr<base::DictionaryValue> request_details);
 
-  void PreMainMessageLoopRun();
-
   void AddObserver(BrowserObserver* obs) {
     observers_.AddObserver(obs);
   }

atom/browser/browser_observer.h

@@ -55,9 +55,6 @@ class BrowserObserver {
   // The browser's accessibility suppport has changed.
   virtual void OnAccessibilitySupportChanged() {}
 
-  // The app message loop is ready
-  virtual void OnPreMainMessageLoopRun() {}
-
 #if defined(OS_MACOSX)
   // The browser wants to report that an user activity will resume. (macOS only)
   virtual void OnWillContinueUserActivity(

atom/browser/javascript_environment.cc

@@ -18,26 +18,8 @@
 
 namespace atom {
 
-JavascriptEnvironment::JavascriptEnvironment()
-    : initialized_(Initialize()),
-      isolate_holder_(base::ThreadTaskRunnerHandle::Get()),
-      isolate_(isolate_holder_.isolate()),
-      isolate_scope_(isolate_),
-      locker_(isolate_),
-      handle_scope_(isolate_),
-      context_(isolate_, v8::Context::New(isolate_)),
-      context_scope_(v8::Local<v8::Context>::New(isolate_, context_)) {
-}
-
-void JavascriptEnvironment::OnMessageLoopCreated() {
-  isolate_holder_.AddRunMicrotasksObserver();
-}
-
-void JavascriptEnvironment::OnMessageLoopDestroying() {
-  isolate_holder_.RemoveRunMicrotasksObserver();
-}
-
-bool JavascriptEnvironment::Initialize() {
+// static
+void JavascriptEnvironment::Initialize() {
   auto cmd = base::CommandLine::ForCurrentProcess();
 
   // --js-flags.
@@ -45,18 +27,26 @@ bool JavascriptEnvironment::Initialize() {
   if (!js_flags.empty())
     v8::V8::SetFlagsFromString(js_flags.c_str(), js_flags.size());
 
-  // The V8Platform of gin relies on Chromium's task schedule, which has not
-  // been started at this point, so we have to rely on Node's V8Platform.
-  platform_ = node::CreatePlatform(
-      base::RecommendedMaxNumberOfThreadsInPool(3, 8, 0.1, 0),
-      uv_default_loop(), nullptr);
-  v8::V8::InitializePlatform(platform_);
-
   gin::IsolateHolder::Initialize(gin::IsolateHolder::kNonStrictMode,
                                  gin::IsolateHolder::kStableV8Extras,
-                                 gin::ArrayBufferAllocator::SharedInstance(),
-                                 false);
-  return true;
+                                 gin::ArrayBufferAllocator::SharedInstance());
+}
+
+JavascriptEnvironment::JavascriptEnvironment()
+    : isolate_holder_(base::ThreadTaskRunnerHandle::Get()),
+      isolate_(isolate_holder_.isolate()),
+      isolate_scope_(isolate_),
+      locker_(isolate_),
+      handle_scope_(isolate_),
+      context_(isolate_, v8::Context::New(isolate_)),
+      context_scope_(v8::Local<v8::Context>::New(isolate_, context_)) {}
+
+void JavascriptEnvironment::OnMessageLoopCreated() {
+  isolate_holder_.AddRunMicrotasksObserver();
+}
+
+void JavascriptEnvironment::OnMessageLoopDestroying() {
+  isolate_holder_.RemoveRunMicrotasksObserver();
 }
 
 NodeEnvironment::NodeEnvironment(node::Environment* env) : env_(env) {

atom/browser/javascript_environment.h

@@ -18,6 +18,8 @@ namespace atom {
 // Manage the V8 isolate and context automatically.
 class JavascriptEnvironment {
  public:
+  static void Initialize();
+
   JavascriptEnvironment();
 
   void OnMessageLoopCreated();
@@ -30,12 +32,6 @@ class JavascriptEnvironment {
   }
 
  private:
-  bool Initialize();
-
-  // Leaked on exit.
-  node::NodePlatform* platform_;
-
-  bool initialized_;
   gin::IsolateHolder isolate_holder_;
   v8::Isolate* isolate_;
   v8::Isolate::Scope isolate_scope_;

chromium_src/chrome/browser/process_singleton.h

@@ -62,8 +62,7 @@ class ProcessSingleton {
       base::Callback<bool(const base::CommandLine::StringVector& command_line,
                           const base::FilePath& current_directory)>;
 
-  ProcessSingleton(const base::FilePath& user_data_dir,
-                   const NotificationCallback& notification_callback);
+  explicit ProcessSingleton(const base::FilePath& user_data_dir);
   ~ProcessSingleton();
 
   // Notify another process, if available. Otherwise sets ourselves as the
@@ -99,6 +98,11 @@ class ProcessSingleton {
       const ShouldKillRemoteProcessCallback& display_dialog_callback);
 #endif
 
+  void RegisterSingletonNotificationCallback(
+      const NotificationCallback& callback) {
+    notification_callback_ = callback;
+  }
+
  protected:
   // Notify another process, if available.
   // Returns true if another process was found and notified, false if we should

chromium_src/chrome/browser/process_singleton_posix.cc

@@ -716,11 +716,8 @@ void ProcessSingleton::LinuxWatcher::SocketReader::FinishWithACK(
 ///////////////////////////////////////////////////////////////////////////////
 // ProcessSingleton
 //
-ProcessSingleton::ProcessSingleton(
-    const base::FilePath& user_data_dir,
-    const NotificationCallback& notification_callback)
-    : notification_callback_(notification_callback),
-      current_pid_(base::GetCurrentProcId()) {
+ProcessSingleton::ProcessSingleton(const base::FilePath& user_data_dir)
+    : current_pid_(base::GetCurrentProcId()) {
   // The user_data_dir may have not been created yet.
   base::CreateDirectoryAndGetError(user_data_dir, nullptr);
 

chromium_src/chrome/browser/process_singleton_win.cc

@@ -182,15 +182,11 @@ bool TerminateAppWithError() {
 
 }  // namespace
 
-ProcessSingleton::ProcessSingleton(
-    const base::FilePath& user_data_dir,
-    const NotificationCallback& notification_callback)
-    : notification_callback_(notification_callback),
-      is_virtualized_(false),
+ProcessSingleton::ProcessSingleton(const base::FilePath& user_data_dir)
+    : is_virtualized_(false),
       lock_file_(INVALID_HANDLE_VALUE),
       user_data_dir_(user_data_dir),
-      should_kill_remote_process_callback_(
-          base::Bind(&TerminateAppWithError)) {
+      should_kill_remote_process_callback_(base::Bind(&TerminateAppWithError)) {
   // The user_data_dir may have not been created yet.
   base::CreateDirectoryAndGetError(user_data_dir, nullptr);
 }

spec/api-app-spec.js

@@ -159,8 +159,7 @@ describe('app module', () => {
     })
   })
 
-  // TODO(deepak1556): Fix and enable for base dchecks.
-  xdescribe('app.makeSingleInstance', () => {
+  describe('app.makeSingleInstance', () => {
     it('prevents the second launch of app', function (done) {
       this.timeout(120000)
       const appPath = path.join(__dirname, 'fixtures', 'api', 'singleton')