Skip to content

Commit

Permalink
fix: do not destroy thread in UI thread (#23495)
Browse files Browse the repository at this point in the history
  • Loading branch information
zcbenz committed May 12, 2020
1 parent ef176ce commit 16b41dc
Show file tree
Hide file tree
Showing 5 changed files with 136 additions and 111 deletions.
2 changes: 2 additions & 0 deletions filenames.gni
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
107 changes: 23 additions & 84 deletions shell/browser/ui/file_dialog_win.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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());
RunState run_state;
if (!CreateDialogThread(&run_state)) {
dict.Set("canceled", true);
dict.Set("filePaths", std::vector<base::FilePath>());
auto done = [](gin_helper::Promise<gin_helper::Dictionary> promise,
bool success, std::vector<base::FilePath> result) {
v8::Locker locker(promise.isolate());
v8::HandleScope handle_scope(promise.isolate());
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(
FROM_HERE, base::BindOnce(&RunOpenDialogInNewThread, run_state,
settings, std::move(promise)));
}
};
dialog_thread::Run(base::BindOnce(ShowOpenDialogSync, settings),
base::BindOnce(done, std::move(promise)));
}

bool ShowSaveDialogSync(const DialogSettings& settings, base::FilePath* path) {
Expand Down Expand Up @@ -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;
if (!CreateDialogThread(&run_state)) {
gin_helper::Dictionary dict =
gin::Dictionary::CreateEmpty(promise.isolate());
dict.Set("canceled", true);
dict.Set("filePath", base::FilePath());
auto done = [](gin_helper::Promise<gin_helper::Dictionary> promise,
bool success, base::FilePath result) {
v8::Locker locker(promise.isolate());
v8::HandleScope handle_scope(promise.isolate());
gin::Dictionary dict = gin::Dictionary::CreateEmpty(promise.isolate());
dict.Set("canceled", !success);
dict.Set("filePath", result);
promise.Resolve(dict);
} else {
run_state.dialog_thread->task_runner()->PostTask(
FROM_HERE, base::BindOnce(&RunSaveDialogInNewThread, run_state,
settings, std::move(promise)));
}
};
dialog_thread::Run(base::BindOnce(ShowSaveDialogSync, settings),
base::BindOnce(done, std::move(promise)));
}

} // namespace file_dialog
34 changes: 7 additions & 27 deletions shell/browser/ui/message_box_win.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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) {
Expand All @@ -224,19 +211,12 @@ int ShowMessageBoxSync(const MessageBoxSettings& settings) {

void ShowMessageBox(const MessageBoxSettings& settings,
MessageBoxCallback callback) {
auto thread =
std::make_unique<base::Thread>(ELECTRON_PRODUCT_NAME "MessageBoxThread");
thread->init_com_with_mta(false);
if (!thread->Start()) {
std::move(callback).Run(settings.cancel_id, settings.checkbox_checked);
return;
}

base::Thread* unretained = thread.release();
unretained->task_runner()->PostTask(
FROM_HERE,
base::BindOnce(&RunMessageBoxInNewThread, base::Unretained(unretained),
settings, std::move(callback)));
dialog_thread::Run(base::BindOnce(&ShowTaskDialogUTF8, settings),
base::BindOnce(
[](MessageBoxCallback callback, DialogResult result) {
std::move(callback).Run(result.first, result.second);
},
std::move(callback)));
}

void ShowErrorBox(const base::string16& title, const base::string16& content) {
Expand Down
23 changes: 23 additions & 0 deletions shell/browser/ui/win/dialog_thread.cc
Original file line number Diff line number Diff line change
@@ -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
81 changes: 81 additions & 0 deletions shell/browser/ui/win/dialog_thread.h
Original file line number Diff line number Diff line change
@@ -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_

0 comments on commit 16b41dc

Please sign in to comment.