fix: do not destroy thread in UI thread (#23495)

filenames.gni

@@ -401,6 +401,8 @@ filenames = {
     "shell/browser/ui/views/submenu_button.h",
     "shell/browser/ui/views/win_frame_view.cc",
     "shell/browser/ui/views/win_frame_view.h",
+    "shell/browser/ui/win/dialog_thread.cc",
+    "shell/browser/ui/win/dialog_thread.h",
     "shell/browser/ui/win/electron_desktop_native_widget_aura.cc",
     "shell/browser/ui/win/electron_desktop_native_widget_aura.h",
     "shell/browser/ui/win/electron_desktop_window_tree_host_win.cc",

shell/browser/ui/file_dialog_win.cc

@@ -16,10 +16,9 @@
 #include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
-#include "base/threading/thread.h"
-#include "base/threading/thread_task_runner_handle.h"
 #include "base/win/registry.h"
 #include "shell/browser/native_window_views.h"
+#include "shell/browser/ui/win/dialog_thread.h"
 #include "shell/browser/unresponsive_suppressor.h"
 #include "shell/common/gin_converters/file_path_converter.h"
 
@@ -63,67 +62,6 @@ void ConvertFilters(const Filters& filters,
   }
 }
 
-struct RunState {
-  base::Thread* dialog_thread;
-  scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner;
-};
-
-bool CreateDialogThread(RunState* run_state) {
-  auto thread =
-      std::make_unique<base::Thread>(ELECTRON_PRODUCT_NAME "FileDialogThread");
-  thread->init_com_with_mta(false);
-  if (!thread->Start())
-    return false;
-
-  run_state->dialog_thread = thread.release();
-  run_state->ui_task_runner = base::ThreadTaskRunnerHandle::Get();
-  return true;
-}
-
-void OnDialogOpened(gin_helper::Promise<gin_helper::Dictionary> promise,
-                    bool canceled,
-                    std::vector<base::FilePath> paths) {
-  gin_helper::Dictionary dict = gin::Dictionary::CreateEmpty(promise.isolate());
-  dict.Set("canceled", canceled);
-  dict.Set("filePaths", paths);
-  promise.Resolve(dict);
-}
-
-void RunOpenDialogInNewThread(
-    const RunState& run_state,
-    const DialogSettings& settings,
-    gin_helper::Promise<gin_helper::Dictionary> promise) {
-  std::vector<base::FilePath> paths;
-  bool result = ShowOpenDialogSync(settings, &paths);
-  run_state.ui_task_runner->PostTask(
-      FROM_HERE,
-      base::BindOnce(&OnDialogOpened, std::move(promise), !result, paths));
-  run_state.ui_task_runner->DeleteSoon(FROM_HERE, run_state.dialog_thread);
-}
-
-void OnSaveDialogDone(gin_helper::Promise<gin_helper::Dictionary> promise,
-                      bool canceled,
-                      const base::FilePath path) {
-  gin_helper::Dictionary dict = gin::Dictionary::CreateEmpty(promise.isolate());
-  dict.Set("canceled", canceled);
-  dict.Set("filePath", path);
-  promise.Resolve(dict);
-}
-
-void RunSaveDialogInNewThread(
-    const RunState& run_state,
-    const DialogSettings& settings,
-    gin_helper::Promise<gin_helper::Dictionary> promise) {
-  base::FilePath path;
-  bool result = ShowSaveDialogSync(settings, &path);
-  run_state.ui_task_runner->PostTask(
-      FROM_HERE,
-      base::BindOnce(&OnSaveDialogDone, std::move(promise), !result, path));
-  run_state.ui_task_runner->DeleteSoon(FROM_HERE, run_state.dialog_thread);
-}
-
-}  // namespace
-
 static HRESULT GetFileNameFromShellItem(IShellItem* pShellItem,
                                         SIGDN type,
                                         LPWSTR lpstr,
@@ -218,6 +156,8 @@ static void ApplySettings(IFileDialog* dialog, const DialogSettings& settings) {
   }
 }
 
+}  // namespace
+
 bool ShowOpenDialogSync(const DialogSettings& settings,
                         std::vector<base::FilePath>* paths) {
   ATL::CComPtr<IFileOpenDialog> file_open_dialog;
@@ -276,17 +216,17 @@ bool ShowOpenDialogSync(const DialogSettings& settings,
 
 void ShowOpenDialog(const DialogSettings& settings,
                     gin_helper::Promise<gin_helper::Dictionary> promise) {
-  gin_helper::Dictionary dict = gin::Dictionary::CreateEmpty(promise.isolate());
+  auto done = [](gin_helper::Promise<gin_helper::Dictionary> promise,
-  RunState run_state;
+                 bool success, std::vector<base::FilePath> result) {
-  if (!CreateDialogThread(&run_state)) {
+    v8::Locker locker(promise.isolate());
-    dict.Set("canceled", true);
+    v8::HandleScope handle_scope(promise.isolate());
-    dict.Set("filePaths", std::vector<base::FilePath>());
+    gin::Dictionary dict = gin::Dictionary::CreateEmpty(promise.isolate());
+    dict.Set("canceled", !success);
+    dict.Set("filePaths", result);
     promise.Resolve(dict);
-  } else {
+  };
-    run_state.dialog_thread->task_runner()->PostTask(
+  dialog_thread::Run(base::BindOnce(ShowOpenDialogSync, settings),
-        FROM_HERE, base::BindOnce(&RunOpenDialogInNewThread, run_state,
+                     base::BindOnce(done, std::move(promise)));
-                                  settings, std::move(promise)));
-  }
 }
 
 bool ShowSaveDialogSync(const DialogSettings& settings, base::FilePath* path) {
@@ -326,18 +266,17 @@ bool ShowSaveDialogSync(const DialogSettings& settings, base::FilePath* path) {
 
 void ShowSaveDialog(const DialogSettings& settings,
                     gin_helper::Promise<gin_helper::Dictionary> promise) {
-  RunState run_state;
+  auto done = [](gin_helper::Promise<gin_helper::Dictionary> promise,
-  if (!CreateDialogThread(&run_state)) {
+                 bool success, base::FilePath result) {
-    gin_helper::Dictionary dict =
+    v8::Locker locker(promise.isolate());
-        gin::Dictionary::CreateEmpty(promise.isolate());
+    v8::HandleScope handle_scope(promise.isolate());
-    dict.Set("canceled", true);
+    gin::Dictionary dict = gin::Dictionary::CreateEmpty(promise.isolate());
-    dict.Set("filePath", base::FilePath());
+    dict.Set("canceled", !success);
+    dict.Set("filePath", result);
     promise.Resolve(dict);
-  } else {
+  };
-    run_state.dialog_thread->task_runner()->PostTask(
+  dialog_thread::Run(base::BindOnce(ShowSaveDialogSync, settings),
-        FROM_HERE, base::BindOnce(&RunSaveDialogInNewThread, run_state,
+                     base::BindOnce(done, std::move(promise)));
-                                  settings, std::move(promise)));
-  }
 }
 
 }  // namespace file_dialog

shell/browser/ui/message_box_win.cc

@@ -14,12 +14,10 @@
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/task/post_task.h"
-#include "base/threading/thread.h"
 #include "base/win/scoped_gdi_object.h"
-#include "content/public/browser/browser_task_traits.h"
-#include "content/public/browser/browser_thread.h"
 #include "shell/browser/browser.h"
 #include "shell/browser/native_window_views.h"
+#include "shell/browser/ui/win/dialog_thread.h"
 #include "shell/browser/unresponsive_suppressor.h"
 #include "ui/gfx/icon_util.h"
 #include "ui/gfx/image/image_skia.h"
@@ -203,17 +201,6 @@ DialogResult ShowTaskDialogUTF8(const MessageBoxSettings& settings) {
       checkbox_label_16, settings.checkbox_checked, settings.icon);
 }
 
-void RunMessageBoxInNewThread(base::Thread* thread,
-                              const MessageBoxSettings& settings,
-                              MessageBoxCallback callback) {
-  DialogResult result = ShowTaskDialogUTF8(settings);
-  base::PostTask(
-      FROM_HERE, {content::BrowserThread::UI},
-      base::BindOnce(std::move(callback), result.first, result.second));
-  content::BrowserThread::DeleteSoon(content::BrowserThread::UI, FROM_HERE,
-                                     thread);
-}
-
 }  // namespace
 
 int ShowMessageBoxSync(const MessageBoxSettings& settings) {
@@ -224,19 +211,12 @@ int ShowMessageBoxSync(const MessageBoxSettings& settings) {
 
 void ShowMessageBox(const MessageBoxSettings& settings,
                     MessageBoxCallback callback) {
-  auto thread =
+  dialog_thread::Run(base::BindOnce(&ShowTaskDialogUTF8, settings),
-      std::make_unique<base::Thread>(ELECTRON_PRODUCT_NAME "MessageBoxThread");
+                     base::BindOnce(
-  thread->init_com_with_mta(false);
+                         [](MessageBoxCallback callback, DialogResult result) {
-  if (!thread->Start()) {
+                           std::move(callback).Run(result.first, result.second);
-    std::move(callback).Run(settings.cancel_id, settings.checkbox_checked);
+                         },
-    return;
+                         std::move(callback)));
-  }
-
-  base::Thread* unretained = thread.release();
-  unretained->task_runner()->PostTask(
-      FROM_HERE,
-      base::BindOnce(&RunMessageBoxInNewThread, base::Unretained(unretained),
-                     settings, std::move(callback)));
 }
 
 void ShowErrorBox(const base::string16& title, const base::string16& content) {

shell/browser/ui/win/dialog_thread.cc

@@ -0,0 +1,23 @@
+// Copyright (c) 2020 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 "shell/browser/ui/win/dialog_thread.h"
+
+#include "base/task/thread_pool.h"
+
+namespace dialog_thread {
+
+// Creates a SingleThreadTaskRunner to run a shell dialog on. Each dialog
+// requires its own dedicated single-threaded sequence otherwise in some
+// situations where a singleton owns a single instance of this object we can
+// have a situation where a modal dialog in one window blocks the appearance
+// of a modal dialog in another.
+TaskRunner CreateDialogTaskRunner() {
+  return base::ThreadPool::CreateCOMSTATaskRunner(
+      {base::TaskPriority::USER_BLOCKING,
+       base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN, base::MayBlock()},
+      base::SingleThreadTaskRunnerThreadMode::DEDICATED);
+}
+
+}  // namespace dialog_thread

shell/browser/ui/win/dialog_thread.h

@@ -0,0 +1,81 @@
+// Copyright (c) 2020 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 SHELL_BROWSER_UI_WIN_DIALOG_THREAD_H_
+#define SHELL_BROWSER_UI_WIN_DIALOG_THREAD_H_
+
+#include <memory>
+#include <utility>
+
+#include "base/memory/scoped_refptr.h"
+#include "base/task/post_task.h"
+#include "content/public/browser/browser_thread.h"
+
+namespace dialog_thread {
+
+// Returns the dedicated single-threaded sequence that the dialog will be on.
+using TaskRunner = scoped_refptr<base::SingleThreadTaskRunner>;
+TaskRunner CreateDialogTaskRunner();
+
+// Runs the |execute| in dialog thread and pass result to |done| in UI thread.
+template <typename R>
+void Run(base::OnceCallback<R()> execute, base::OnceCallback<void(R)> done) {
+  // dialogThread.postTask(() => {
+  //   r = execute()
+  //   uiThread.postTask(() => {
+  //     done(r)
+  //   }
+  // })
+  TaskRunner task_runner = CreateDialogTaskRunner();
+  task_runner->PostTask(
+      FROM_HERE,
+      base::BindOnce(
+          [](TaskRunner task_runner, base::OnceCallback<R()> execute,
+             base::OnceCallback<void(R)> done) {
+            R r = std::move(execute).Run();
+            base::PostTask(
+                FROM_HERE, {content::BrowserThread::UI},
+                base::BindOnce(
+                    [](TaskRunner task_runner, base::OnceCallback<void(R)> done,
+                       R r) {
+                      std::move(done).Run(std::move(r));
+                      // Task runner will destroyed automatically after the
+                      // scope ends.
+                    },
+                    std::move(task_runner), std::move(done), std::move(r)));
+          },
+          std::move(task_runner), std::move(execute), std::move(done)));
+}
+
+// Adaptor to handle the |execute| that returns bool.
+template <typename R>
+void Run(base::OnceCallback<bool(R*)> execute,
+         base::OnceCallback<void(bool, R)> done) {
+  // run(() => {
+  //   result = execute(&value)
+  //   return {result, value}
+  // }, ({result, value}) => {
+  //   done(result, value)
+  // })
+  struct Result {
+    bool result;
+    R value;
+  };
+  Run(base::BindOnce(
+          [](base::OnceCallback<bool(R*)> execute) {
+            Result r;
+            r.result = std::move(execute).Run(&r.value);
+            return r;
+          },
+          std::move(execute)),
+      base::BindOnce(
+          [](base::OnceCallback<void(bool, R)> done, Result r) {
+            std::move(done).Run(r.result, std::move(r.value));
+          },
+          std::move(done)));
+}
+
+}  // namespace dialog_thread
+
+#endif  // SHELL_BROWSER_UI_WIN_DIALOG_THREAD_H_