X Tutup
Skip to content

Commit c4d35cd

Browse files
zcbenzzcbenz
authored
fix: do not run dialog callback inside transaction commit (electron#31606)
1 parent 1e618ef commit c4d35cd

File tree

4 files changed

+58
-17
lines changed

4 files changed

+58
-17
lines changed

‎script/lint.js‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,7 @@ const LINTERS = [{
9090
spawnAndCheckExitCode('python', ['script/run-clang-format.py', ...filenames]);
9191
}
9292
const filter = [
93+
'-readability/braces',
9394
'-readability/casting',
9495
'-whitespace/braces',
9596
'-whitespace/indent',

‎shell/browser/ui/file_dialog_mac.mm‎

Lines changed: 28 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,9 @@
1616
#include "base/mac/mac_util.h"
1717
#include "base/mac/scoped_cftyperef.h"
1818
#include "base/strings/sys_string_conversions.h"
19+
#include "base/task/post_task.h"
20+
#include "content/public/browser/browser_task_traits.h"
21+
#include "content/public/browser/browser_thread.h"
1922
#include "shell/browser/native_window.h"
2023
#include "shell/common/gin_converters/file_path_converter.h"
2124

@@ -290,6 +293,27 @@ void ReadDialogPaths(NSOpenPanel* dialog, std::vector<base::FilePath>* paths) {
290293
ReadDialogPathsWithBookmarks(dialog, paths, &ignored_bookmarks);
291294
}
292295

296+
void ResolvePromiseInNextTick(gin_helper::Promise<v8::Local<v8::Value>> promise,
297+
v8::Local<v8::Value> value) {
298+
// The completionHandler runs inside a transaction commit, and we should
299+
// not do any runModal inside it. However since we can not control what
300+
// users will run in the microtask, we have to delay the resolution until
301+
// next tick, otherwise crash like this may happen:
302+
// https://github.com/electron/electron/issues/26884
303+
base::PostTask(
304+
FROM_HERE, {content::BrowserThread::UI},
305+
base::BindOnce(
306+
[](gin_helper::Promise<v8::Local<v8::Value>> promise,
307+
v8::Global<v8::Value> global) {
308+
v8::Isolate* isolate = promise.isolate();
309+
v8::Locker locker(isolate);
310+
v8::HandleScope handle_scope(isolate);
311+
v8::Local<v8::Value> value = global.Get(isolate);
312+
promise.Resolve(value);
313+
},
314+
std::move(promise), v8::Global<v8::Value>(promise.isolate(), value)));
315+
}
316+
293317
} // namespace
294318

295319
bool ShowOpenDialogSync(const DialogSettings& settings,
@@ -320,7 +344,6 @@ void OpenDialogCompletion(int chosen,
320344
#if defined(MAS_BUILD)
321345
dict.Set("bookmarks", std::vector<std::string>());
322346
#endif
323-
promise.Resolve(dict);
324347
} else {
325348
std::vector<base::FilePath> paths;
326349
dict.Set("canceled", false);
@@ -336,8 +359,9 @@ void OpenDialogCompletion(int chosen,
336359
ReadDialogPaths(dialog, &paths);
337360
dict.Set("filePaths", paths);
338361
#endif
339-
promise.Resolve(dict);
340362
}
363+
ResolvePromiseInNextTick(promise.As<v8::Local<v8::Value>>(),
364+
dict.GetHandle());
341365
}
342366

343367
void ShowOpenDialog(const DialogSettings& settings,
@@ -410,7 +434,8 @@ void SaveDialogCompletion(int chosen,
410434
}
411435
#endif
412436
}
413-
promise.Resolve(dict);
437+
ResolvePromiseInNextTick(promise.As<v8::Local<v8::Value>>(),
438+
dict.GetHandle());
414439
}
415440

416441
void ShowSaveDialog(const DialogSettings& settings,

‎shell/browser/ui/message_box_mac.mm‎

Lines changed: 23 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,9 @@
1717
#include "base/mac/scoped_nsobject.h"
1818
#include "base/no_destructor.h"
1919
#include "base/strings/sys_string_conversions.h"
20+
#include "base/task/post_task.h"
21+
#include "content/public/browser/browser_task_traits.h"
22+
#include "content/public/browser/browser_thread.h"
2023
#include "shell/browser/native_window.h"
2124
#include "skia/ext/skia_utils_mac.h"
2225
#include "ui/gfx/image/image_skia.h"
@@ -160,20 +163,26 @@ void ShowMessageBox(const MessageBoxSettings& settings,
160163
__block absl::optional<int> id = std::move(settings.id);
161164
__block int cancel_id = settings.cancel_id;
162165

163-
[alert beginSheetModalForWindow:window
164-
completionHandler:^(NSModalResponse response) {
165-
if (id)
166-
GetDialogsMap().erase(*id);
167-
// When the alert is cancelled programmatically, the
168-
// response would be something like -1000. This currently
169-
// only happens when users call CloseMessageBox API, and we
170-
// should return cancelId as result.
171-
if (response < 0)
172-
response = cancel_id;
173-
std::move(callback_).Run(
174-
response, alert.suppressionButton.state == NSOnState);
175-
[alert release];
176-
}];
166+
auto handler = ^(NSModalResponse response) {
167+
if (id)
168+
GetDialogsMap().erase(*id);
169+
// When the alert is cancelled programmatically, the response would be
170+
// something like -1000. This currently only happens when users call
171+
// CloseMessageBox API, and we should return cancelId as result.
172+
if (response < 0)
173+
response = cancel_id;
174+
bool suppressed = alert.suppressionButton.state == NSOnState;
175+
[alert release];
176+
// The completionHandler runs inside a transaction commit, and we should
177+
// not do any runModal inside it. However since we can not control what
178+
// users will run in the callback, we have to delay running the callback
179+
// until next tick, otherwise crash like this may happen:
180+
// https://github.com/electron/electron/issues/26884
181+
base::PostTask(
182+
FROM_HERE, {content::BrowserThread::UI},
183+
base::BindOnce(std::move(callback_), response, suppressed));
184+
};
185+
[alert beginSheetModalForWindow:window completionHandler:handler];
177186
}
178187
}
179188

‎shell/common/gin_helper/promise.h‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,12 @@ class Promise : public PromiseBase {
106106
return resolved.GetHandle();
107107
}
108108

109+
// Convert to another type.
110+
template <typename NT>
111+
Promise<NT> As() {
112+
return Promise<NT>(isolate(), GetInner());
113+
}
114+
109115
// Promise resolution is a microtask
110116
// We use the MicrotasksRunner to trigger the running of pending microtasks
111117
v8::Maybe<bool> Resolve(const RT& value) {

0 commit comments

Comments
 (0)
X Tutup