Skip to content

Commit 512b9f2

Browse files
zcbenzzcbenz
authored
fix: do not destroy thread in UI thread (electron#23551)
1 parent 66b407e commit 512b9f2

File tree

5 files changed

+137
-109
lines changed

5 files changed

+137
-109
lines changed

‎filenames.gni

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -384,6 +384,8 @@ filenames = {
384384
"shell/browser/ui/win/atom_desktop_native_widget_aura.h",
385385
"shell/browser/ui/win/atom_desktop_window_tree_host_win.cc",
386386
"shell/browser/ui/win/atom_desktop_window_tree_host_win.h",
387+
"shell/browser/ui/win/dialog_thread.cc",
388+
"shell/browser/ui/win/dialog_thread.h",
387389
"shell/browser/ui/win/jump_list.cc",
388390
"shell/browser/ui/win/jump_list.h",
389391
"shell/browser/ui/win/notify_icon_host.cc",

‎shell/browser/ui/file_dialog_win.cc

Lines changed: 24 additions & 82 deletions
Original file line numberDiff line numberDiff line change
@@ -16,10 +16,9 @@
1616
#include "base/strings/string_split.h"
1717
#include "base/strings/string_util.h"
1818
#include "base/strings/utf_string_conversions.h"
19-
#include "base/threading/thread.h"
20-
#include "base/threading/thread_task_runner_handle.h"
2119
#include "base/win/registry.h"
2220
#include "shell/browser/native_window_views.h"
21+
#include "shell/browser/ui/win/dialog_thread.h"
2322
#include "shell/browser/unresponsive_suppressor.h"
2423

2524
namespace file_dialog {
@@ -64,65 +63,6 @@ void ConvertFilters(const Filters& filters,
6463
}
6564
}
6665

67-
struct RunState {
68-
base::Thread* dialog_thread;
69-
scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner;
70-
};
71-
72-
bool CreateDialogThread(RunState* run_state) {
73-
auto thread =
74-
std::make_unique<base::Thread>(ELECTRON_PRODUCT_NAME "FileDialogThread");
75-
thread->init_com_with_mta(false);
76-
if (!thread->Start())
77-
return false;
78-
79-
run_state->dialog_thread = thread.release();
80-
run_state->ui_task_runner = base::ThreadTaskRunnerHandle::Get();
81-
return true;
82-
}
83-
84-
void OnDialogOpened(electron::util::Promise promise,
85-
bool canceled,
86-
std::vector<base::FilePath> paths) {
87-
mate::Dictionary dict = mate::Dictionary::CreateEmpty(promise.isolate());
88-
dict.Set("canceled", canceled);
89-
dict.Set("filePaths", paths);
90-
promise.Resolve(dict.GetHandle());
91-
}
92-
93-
void RunOpenDialogInNewThread(const RunState& run_state,
94-
const DialogSettings& settings,
95-
electron::util::Promise promise) {
96-
std::vector<base::FilePath> paths;
97-
bool result = ShowOpenDialogSync(settings, &paths);
98-
run_state.ui_task_runner->PostTask(
99-
FROM_HERE,
100-
base::BindOnce(&OnDialogOpened, std::move(promise), !result, paths));
101-
run_state.ui_task_runner->DeleteSoon(FROM_HERE, run_state.dialog_thread);
102-
}
103-
104-
void OnSaveDialogDone(electron::util::Promise promise,
105-
bool canceled,
106-
const base::FilePath path) {
107-
mate::Dictionary dict = mate::Dictionary::CreateEmpty(promise.isolate());
108-
dict.Set("canceled", canceled);
109-
dict.Set("filePath", path);
110-
promise.Resolve(dict.GetHandle());
111-
}
112-
113-
void RunSaveDialogInNewThread(const RunState& run_state,
114-
const DialogSettings& settings,
115-
electron::util::Promise promise) {
116-
base::FilePath path;
117-
bool result = ShowSaveDialogSync(settings, &path);
118-
run_state.ui_task_runner->PostTask(
119-
FROM_HERE,
120-
base::BindOnce(&OnSaveDialogDone, std::move(promise), !result, path));
121-
run_state.ui_task_runner->DeleteSoon(FROM_HERE, run_state.dialog_thread);
122-
}
123-
124-
} // namespace
125-
12666
static HRESULT GetFileNameFromShellItem(IShellItem* pShellItem,
12767
SIGDN type,
12868
LPWSTR lpstr,
@@ -217,6 +157,8 @@ static void ApplySettings(IFileDialog* dialog, const DialogSettings& settings) {
217157
}
218158
}
219159

160+
} // namespace
161+
220162
bool ShowOpenDialogSync(const DialogSettings& settings,
221163
std::vector<base::FilePath>* paths) {
222164
ATL::CComPtr<IFileOpenDialog> file_open_dialog;
@@ -273,17 +215,17 @@ bool ShowOpenDialogSync(const DialogSettings& settings,
273215

274216
void ShowOpenDialog(const DialogSettings& settings,
275217
electron::util::Promise promise) {
276-
mate::Dictionary dict = mate::Dictionary::CreateEmpty(promise.isolate());
277-
RunState run_state;
278-
if (!CreateDialogThread(&run_state)) {
279-
dict.Set("canceled", true);
280-
dict.Set("filePaths", std::vector<base::FilePath>());
281-
promise.Resolve(dict.GetHandle());
282-
} else {
283-
run_state.dialog_thread->task_runner()->PostTask(
284-
FROM_HERE, base::BindOnce(&RunOpenDialogInNewThread, run_state,
285-
settings, std::move(promise)));
286-
}
218+
auto done = [](electron::util::Promise promise, bool success,
219+
std::vector<base::FilePath> result) {
220+
v8::Locker locker(promise.isolate());
221+
v8::HandleScope handle_scope(promise.isolate());
222+
mate::Dictionary dict = mate::Dictionary::CreateEmpty(promise.isolate());
223+
dict.Set("canceled", !success);
224+
dict.Set("filePaths", result);
225+
promise.Resolve(dict);
226+
};
227+
dialog_thread::Run(base::BindOnce(ShowOpenDialogSync, settings),
228+
base::BindOnce(done, std::move(promise)));
287229
}
288230

289231
bool ShowSaveDialogSync(const DialogSettings& settings, base::FilePath* path) {
@@ -318,17 +260,17 @@ bool ShowSaveDialogSync(const DialogSettings& settings, base::FilePath* path) {
318260

319261
void ShowSaveDialog(const DialogSettings& settings,
320262
electron::util::Promise promise) {
321-
RunState run_state;
322-
if (!CreateDialogThread(&run_state)) {
263+
auto done = [](electron::util::Promise promise, bool success,
264+
base::FilePath result) {
265+
v8::Locker locker(promise.isolate());
266+
v8::HandleScope handle_scope(promise.isolate());
323267
mate::Dictionary dict = mate::Dictionary::CreateEmpty(promise.isolate());
324-
dict.Set("canceled", true);
325-
dict.Set("filePath", base::FilePath());
326-
promise.Resolve(dict.GetHandle());
327-
} else {
328-
run_state.dialog_thread->task_runner()->PostTask(
329-
FROM_HERE, base::BindOnce(&RunSaveDialogInNewThread, run_state,
330-
settings, std::move(promise)));
331-
}
268+
dict.Set("canceled", !success);
269+
dict.Set("filePath", result);
270+
promise.Resolve(dict);
271+
};
272+
dialog_thread::Run(base::BindOnce(ShowSaveDialogSync, settings),
273+
base::BindOnce(done, std::move(promise)));
332274
}
333275

334276
} // namespace file_dialog

‎shell/browser/ui/message_box_win.cc

Lines changed: 7 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -14,12 +14,10 @@
1414
#include "base/strings/string_util.h"
1515
#include "base/strings/utf_string_conversions.h"
1616
#include "base/task/post_task.h"
17-
#include "base/threading/thread.h"
1817
#include "base/win/scoped_gdi_object.h"
19-
#include "content/public/browser/browser_task_traits.h"
20-
#include "content/public/browser/browser_thread.h"
2118
#include "shell/browser/browser.h"
2219
#include "shell/browser/native_window_views.h"
20+
#include "shell/browser/ui/win/dialog_thread.h"
2321
#include "shell/browser/unresponsive_suppressor.h"
2422
#include "ui/gfx/icon_util.h"
2523
#include "ui/gfx/image/image_skia.h"
@@ -204,17 +202,6 @@ DialogResult ShowTaskDialogUTF8(const MessageBoxSettings& settings) {
204202
checkbox_label_16, settings.checkbox_checked, settings.icon);
205203
}
206204

207-
void RunMessageBoxInNewThread(base::Thread* thread,
208-
const MessageBoxSettings& settings,
209-
MessageBoxCallback callback) {
210-
DialogResult result = ShowTaskDialogUTF8(settings);
211-
base::PostTaskWithTraits(
212-
FROM_HERE, {content::BrowserThread::UI},
213-
base::BindOnce(std::move(callback), result.first, result.second));
214-
content::BrowserThread::DeleteSoon(content::BrowserThread::UI, FROM_HERE,
215-
thread);
216-
}
217-
218205
} // namespace
219206

220207
int ShowMessageBoxSync(const MessageBoxSettings& settings) {
@@ -225,19 +212,12 @@ int ShowMessageBoxSync(const MessageBoxSettings& settings) {
225212

226213
void ShowMessageBox(const MessageBoxSettings& settings,
227214
MessageBoxCallback callback) {
228-
auto thread =
229-
std::make_unique<base::Thread>(ELECTRON_PRODUCT_NAME "MessageBoxThread");
230-
thread->init_com_with_mta(false);
231-
if (!thread->Start()) {
232-
std::move(callback).Run(settings.cancel_id, settings.checkbox_checked);
233-
return;
234-
}
235-
236-
base::Thread* unretained = thread.release();
237-
unretained->task_runner()->PostTask(
238-
FROM_HERE,
239-
base::BindOnce(&RunMessageBoxInNewThread, base::Unretained(unretained),
240-
settings, std::move(callback)));
215+
dialog_thread::Run(base::BindOnce(&ShowTaskDialogUTF8, settings),
216+
base::BindOnce(
217+
[](MessageBoxCallback callback, DialogResult result) {
218+
std::move(callback).Run(result.first, result.second);
219+
},
220+
std::move(callback)));
241221
}
242222

243223
void ShowErrorBox(const base::string16& title, const base::string16& content) {

‎shell/browser/ui/win/dialog_thread.cc

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
// Copyright (c) 2020 The Chromium Authors. All rights reserved.
2+
// Use of this source code is governed by a BSD-style license that can be
3+
// found in the LICENSE file.
4+
5+
#include "shell/browser/ui/win/dialog_thread.h"
6+
7+
#include "base/win/scoped_com_initializer.h"
8+
9+
namespace dialog_thread {
10+
11+
// Creates a SingleThreadTaskRunner to run a shell dialog on. Each dialog
12+
// requires its own dedicated single-threaded sequence otherwise in some
13+
// situations where a singleton owns a single instance of this object we can
14+
// have a situation where a modal dialog in one window blocks the appearance
15+
// of a modal dialog in another.
16+
TaskRunner CreateDialogTaskRunner() {
17+
return CreateCOMSTATaskRunner(
18+
{base::ThreadPool(), base::TaskPriority::USER_BLOCKING,
19+
base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN, base::MayBlock()},
20+
base::SingleThreadTaskRunnerThreadMode::DEDICATED);
21+
}
22+
23+
} // namespace dialog_thread

‎shell/browser/ui/win/dialog_thread.h

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,81 @@
1+
// Copyright (c) 2020 The Chromium Authors. All rights reserved.
2+
// Use of this source code is governed by a BSD-style license that can be
3+
// found in the LICENSE file.
4+
5+
#ifndef SHELL_BROWSER_UI_WIN_DIALOG_THREAD_H_
6+
#define SHELL_BROWSER_UI_WIN_DIALOG_THREAD_H_
7+
8+
#include <memory>
9+
#include <utility>
10+
11+
#include "base/memory/scoped_refptr.h"
12+
#include "base/task/post_task.h"
13+
#include "content/public/browser/browser_thread.h"
14+
15+
namespace dialog_thread {
16+
17+
// Returns the dedicated single-threaded sequence that the dialog will be on.
18+
using TaskRunner = scoped_refptr<base::SingleThreadTaskRunner>;
19+
TaskRunner CreateDialogTaskRunner();
20+
21+
// Runs the |execute| in dialog thread and pass result to |done| in UI thread.
22+
template <typename R>
23+
void Run(base::OnceCallback<R()> execute, base::OnceCallback<void(R)> done) {
24+
// dialogThread.postTask(() => {
25+
// r = execute()
26+
// uiThread.postTask(() => {
27+
// done(r)
28+
// }
29+
// })
30+
TaskRunner task_runner = CreateDialogTaskRunner();
31+
task_runner->PostTask(
32+
FROM_HERE,
33+
base::BindOnce(
34+
[](TaskRunner task_runner, base::OnceCallback<R()> execute,
35+
base::OnceCallback<void(R)> done) {
36+
R r = std::move(execute).Run();
37+
base::PostTask(
38+
FROM_HERE, {content::BrowserThread::UI},
39+
base::BindOnce(
40+
[](TaskRunner task_runner, base::OnceCallback<void(R)> done,
41+
R r) {
42+
std::move(done).Run(std::move(r));
43+
// Task runner will destroyed automatically after the
44+
// scope ends.
45+
},
46+
std::move(task_runner), std::move(done), std::move(r)));
47+
},
48+
std::move(task_runner), std::move(execute), std::move(done)));
49+
}
50+
51+
// Adaptor to handle the |execute| that returns bool.
52+
template <typename R>
53+
void Run(base::OnceCallback<bool(R*)> execute,
54+
base::OnceCallback<void(bool, R)> done) {
55+
// run(() => {
56+
// result = execute(&value)
57+
// return {result, value}
58+
// }, ({result, value}) => {
59+
// done(result, value)
60+
// })
61+
struct Result {
62+
bool result;
63+
R value;
64+
};
65+
Run(base::BindOnce(
66+
[](base::OnceCallback<bool(R*)> execute) {
67+
Result r;
68+
r.result = std::move(execute).Run(&r.value);
69+
return r;
70+
},
71+
std::move(execute)),
72+
base::BindOnce(
73+
[](base::OnceCallback<void(bool, R)> done, Result r) {
74+
std::move(done).Run(r.result, std::move(r.value));
75+
},
76+
std::move(done)));
77+
}
78+
79+
} // namespace dialog_thread
80+
81+
#endif // SHELL_BROWSER_UI_WIN_DIALOG_THREAD_H_

0 commit comments

Comments
 (0)