fix: do not run dialog callback inside transaction commit (#31657)

Co-authored-by: Cheng Zhao <zcbenz@gmail.com>

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',

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<base::FilePath>* paths) {
   ReadDialogPathsWithBookmarks(dialog, paths, &ignored_bookmarks);
 }
 
+void ResolvePromiseInNextTick(gin_helper::Promise<v8::Local<v8::Value>> promise,
+                              v8::Local<v8::Value> 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<v8::Local<v8::Value>> promise,
+             v8::Global<v8::Value> global) {
+            v8::Isolate* isolate = promise.isolate();
+            v8::Locker locker(isolate);
+            v8::HandleScope handle_scope(isolate);
+            v8::Local<v8::Value> value = global.Get(isolate);
+            promise.Resolve(value);
+          },
+          std::move(promise), v8::Global<v8::Value>(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<std::string>());
 #endif
-    promise.Resolve(dict);
   } else {
     std::vector<base::FilePath> 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<v8::Local<v8::Value>>(),
+                           dict.GetHandle());
 }
 
 void ShowOpenDialog(const DialogSettings& settings,
@@ -410,7 +434,8 @@ void SaveDialogCompletion(int chosen,
     }
 #endif
   }
-  promise.Resolve(dict);
+  ResolvePromiseInNextTick(promise.As<v8::Local<v8::Value>>(),
+                           dict.GetHandle());
 }
 
 void ShowSaveDialog(const DialogSettings& settings,

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<int> 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];
   }
 }
 

shell/common/gin_helper/promise.h

@@ -106,6 +106,12 @@ class Promise : public PromiseBase {
     return resolved.GetHandle();
   }
 
+  // Convert to another type.
+  template <typename NT>
+  Promise<NT> As() {
+    return Promise<NT>(isolate(), GetInner());
+  }
+
   // Promise resolution is a microtask
   // We use the MicrotasksRunner to trigger the running of pending microtasks
   v8::Maybe<bool> Resolve(const RT& value) {