From d9e0025429521a3fa809ddd50b97298b48d90582 Mon Sep 17 00:00:00 2001 From: "trop[bot]" <37223003+trop[bot]@users.noreply.github.com> Date: Tue, 2 Nov 2021 14:55:52 +0100 Subject: [PATCH] fix: do not run dialog callback inside transaction commit (#31657) Co-authored-by: Cheng Zhao --- script/lint.js | 1 + shell/browser/ui/file_dialog_mac.mm | 31 +++++++++++++++++++++--- shell/browser/ui/message_box_mac.mm | 37 ++++++++++++++++++----------- shell/common/gin_helper/promise.h | 6 +++++ 4 files changed, 58 insertions(+), 17 deletions(-) diff --git a/script/lint.js b/script/lint.js index 3ee6ace0bd3d8..92ab99b162c81 100755 --- a/script/lint.js +++ b/script/lint.js @@ -89,6 +89,7 @@ const LINTERS = [{ spawnAndCheckExitCode('python', ['script/run-clang-format.py', ...filenames]); } const filter = [ + '-readability/braces', '-readability/casting', '-whitespace/braces', '-whitespace/indent', diff --git a/shell/browser/ui/file_dialog_mac.mm b/shell/browser/ui/file_dialog_mac.mm index d490e1656a02f..b420870d32b7b 100644 --- a/shell/browser/ui/file_dialog_mac.mm +++ b/shell/browser/ui/file_dialog_mac.mm @@ -16,6 +16,9 @@ #include "base/mac/mac_util.h" #include "base/mac/scoped_cftyperef.h" #include "base/strings/sys_string_conversions.h" +#include "base/task/post_task.h" +#include "content/public/browser/browser_task_traits.h" +#include "content/public/browser/browser_thread.h" #include "shell/browser/native_window.h" #include "shell/common/gin_converters/file_path_converter.h" @@ -290,6 +293,27 @@ void ReadDialogPaths(NSOpenPanel* dialog, std::vector* paths) { ReadDialogPathsWithBookmarks(dialog, paths, &ignored_bookmarks); } +void ResolvePromiseInNextTick(gin_helper::Promise> promise, + v8::Local value) { + // The completionHandler runs inside a transaction commit, and we should + // not do any runModal inside it. However since we can not control what + // users will run in the microtask, we have to delay the resolution until + // next tick, otherwise crash like this may happen: + // https://github.com/electron/electron/issues/26884 + base::PostTask( + FROM_HERE, {content::BrowserThread::UI}, + base::BindOnce( + [](gin_helper::Promise> promise, + v8::Global global) { + v8::Isolate* isolate = promise.isolate(); + v8::Locker locker(isolate); + v8::HandleScope handle_scope(isolate); + v8::Local value = global.Get(isolate); + promise.Resolve(value); + }, + std::move(promise), v8::Global(promise.isolate(), value))); +} + } // namespace bool ShowOpenDialogSync(const DialogSettings& settings, @@ -320,7 +344,6 @@ void OpenDialogCompletion(int chosen, #if defined(MAS_BUILD) dict.Set("bookmarks", std::vector()); #endif - promise.Resolve(dict); } else { std::vector paths; dict.Set("canceled", false); @@ -336,8 +359,9 @@ void OpenDialogCompletion(int chosen, ReadDialogPaths(dialog, &paths); dict.Set("filePaths", paths); #endif - promise.Resolve(dict); } + ResolvePromiseInNextTick(promise.As>(), + dict.GetHandle()); } void ShowOpenDialog(const DialogSettings& settings, @@ -410,7 +434,8 @@ void SaveDialogCompletion(int chosen, } #endif } - promise.Resolve(dict); + ResolvePromiseInNextTick(promise.As>(), + dict.GetHandle()); } void ShowSaveDialog(const DialogSettings& settings, diff --git a/shell/browser/ui/message_box_mac.mm b/shell/browser/ui/message_box_mac.mm index 3d1dfac2baf89..faad840b68c3c 100644 --- a/shell/browser/ui/message_box_mac.mm +++ b/shell/browser/ui/message_box_mac.mm @@ -17,6 +17,9 @@ #include "base/mac/scoped_nsobject.h" #include "base/no_destructor.h" #include "base/strings/sys_string_conversions.h" +#include "base/task/post_task.h" +#include "content/public/browser/browser_task_traits.h" +#include "content/public/browser/browser_thread.h" #include "shell/browser/native_window.h" #include "skia/ext/skia_utils_mac.h" #include "ui/gfx/image/image_skia.h" @@ -160,20 +163,26 @@ void ShowMessageBox(const MessageBoxSettings& settings, __block absl::optional id = std::move(settings.id); __block int cancel_id = settings.cancel_id; - [alert beginSheetModalForWindow:window - completionHandler:^(NSModalResponse response) { - if (id) - GetDialogsMap().erase(*id); - // When the alert is cancelled programmatically, the - // response would be something like -1000. This currently - // only happens when users call CloseMessageBox API, and we - // should return cancelId as result. - if (response < 0) - response = cancel_id; - std::move(callback_).Run( - response, alert.suppressionButton.state == NSOnState); - [alert release]; - }]; + auto handler = ^(NSModalResponse response) { + if (id) + GetDialogsMap().erase(*id); + // When the alert is cancelled programmatically, the response would be + // something like -1000. This currently only happens when users call + // CloseMessageBox API, and we should return cancelId as result. + if (response < 0) + response = cancel_id; + bool suppressed = alert.suppressionButton.state == NSOnState; + [alert release]; + // The completionHandler runs inside a transaction commit, and we should + // not do any runModal inside it. However since we can not control what + // users will run in the callback, we have to delay running the callback + // until next tick, otherwise crash like this may happen: + // https://github.com/electron/electron/issues/26884 + base::PostTask( + FROM_HERE, {content::BrowserThread::UI}, + base::BindOnce(std::move(callback_), response, suppressed)); + }; + [alert beginSheetModalForWindow:window completionHandler:handler]; } } diff --git a/shell/common/gin_helper/promise.h b/shell/common/gin_helper/promise.h index 0da188fe41170..1526c26f9a659 100644 --- a/shell/common/gin_helper/promise.h +++ b/shell/common/gin_helper/promise.h @@ -106,6 +106,12 @@ class Promise : public PromiseBase { return resolved.GetHandle(); } + // Convert to another type. + template + Promise As() { + return Promise(isolate(), GetInner()); + } + // Promise resolution is a microtask // We use the MicrotasksRunner to trigger the running of pending microtasks v8::Maybe Resolve(const RT& value) {