Repository: electron/electron (language: cpp)

Issue:
[Bug]: gin_helper::MicrotasksScope is instantiated with wrong arguments
The constructor for gin_helper::MicrotasksScope is declared this way:

```c++
    explicit MicrotasksScope(v8::Isolate* isolate,
                             v8::MicrotaskQueue* microtask_queue,
                             bool ignore_browser_checkpoint = false,
                             v8::MicrotasksScope::Type scope_type =
                                 v8::MicrotasksScope::kRunMicrotasks);
```

The `scope_type` arg was added in 2eb3bddb05b6efc92e36c22ed55c37021d84276a (#29058).

v8::MicrotasksScope is an `enum`, rather than an `enum class`:

```c++
enum Type { kRunMicrotasks, kDoNotRunMicrotasks };
```

There are many places where we incorrectly instantiate with a v8::MicrotasksScope::Type as the third argument. I was surprised that this compiles; but since `v8::MicrotasksScope::Type` is declared as an `enum` rather than an `enum class`, my guess is that the compiler is implicitly converting `Type` to a `bool`?

Examples:

```sh
shell/common/v8_value_serializer.cc
37:    gin_helper::MicrotasksScope microtasks_scope(
38-        isolate_, isolate_->GetCurrentContext()->GetMicrotaskQueue(),
39-        v8::MicrotasksScope::kDoNotRunMicrotasks);

shell/common/node_bindings.cc
286:    gin_helper::MicrotasksScope microtasks_scope(
287-        isolate, env->context()->GetMicrotaskQueue(),
288-        v8::MicrotasksScope::kDoNotRunMicrotasks);

shell/renderer/api/electron_api_spell_check_client.cc
223:  gin_helper::MicrotasksScope microtasks_scope(
224-      isolate_, context->GetMicrotaskQueue(),
225-      v8::MicrotasksScope::kDoNotRunMicrotasks);

```

So these MicrotasksScopes are being instantiated with both the wrong flag _and_ the wrong type. This raises several questions:

- Is this code actually working?
- If it _is_ working, is it because these instantiations aren't necessary?
- If it _isn't_ working, why didn't we find out sooner? Do we not have tests covering this code?

CC @deepak1556 who didn't introduce this bug, but who probably has the most context on when to use `gin_helper::MicrotasksScope`

Background info on `gin_helper::MicrotasksScope`: #24131


Discussion / hints:
Oh wow, thanks for noticing this issue! You are right about the implicit conversion https://godbolt.org/z/hjf73GWfv

> Is this code actually working?

I don't think so, both in the browser process and renderer process we are performing the opposite of the callsite intention.

> If it isn't working, why didn't we find out sooner? Do we not have tests covering this code?

We have been lucky I guess, the checks in V8 side affect only scoped microstasks for ex: https://source.chromium.org/chromium/chromium/src/+/main:v8/src/api/api-inl.h;l=205-213. The callsites for most cases will be explicit mode which will just no-op when it cannot run the microtasks. Our tests should be improved to capture these cases.
@deepak1556 I came across this because I was looking at where we instantiate `gin_helper::MicrotasksScope` to try and understand it better. I understand what its implementation is doing, but don't fully understand when to use it & when to not use it. Is there a heuristic you use for saying "ah, we need to add our MicrotasksScope here"?

Fix patch:
diff --git a/shell/browser/api/electron_api_auto_updater.cc b/shell/browser/api/electron_api_auto_updater.cc
index d24db3c3193db..2d870bba83bd7 100644
--- a/shell/browser/api/electron_api_auto_updater.cc
+++ b/shell/browser/api/electron_api_auto_updater.cc
@@ -46,9 +46,9 @@ void AutoUpdater::OnError(const std::string& message) {
         gin::StringToV8(isolate, message),
     };
 
-    gin_helper::MicrotasksScope microtasks_scope(
+    gin_helper::MicrotasksScope microtasks_scope{
         isolate, wrapper->GetCreationContextChecked()->GetMicrotaskQueue(),
-        true);
+        true, v8::MicrotasksScope::kRunMicrotasks};
 
     node::MakeCallback(isolate, wrapper, "emit", args.size(), args.data(),
                        {0, 0});
diff --git a/shell/common/api/electron_bindings.cc b/shell/common/api/electron_bindings.cc
index 2d9d7f72cecb6..acae6ba083dd9 100644
--- a/shell/common/api/electron_bindings.cc
+++ b/shell/common/api/electron_bindings.cc
@@ -237,8 +237,9 @@ void ElectronBindings::DidReceiveMemoryDump(
   v8::HandleScope handle_scope(isolate);
   v8::Local<v8::Context> local_context =
       v8::Local<v8::Context>::New(isolate, context);
-  gin_helper::MicrotasksScope microtasks_scope(
-      isolate, local_context->GetMicrotaskQueue(), true);
+  gin_helper::MicrotasksScope microtasks_scope{
+      isolate, local_context->GetMicrotaskQueue(), true,
+      v8::MicrotasksScope::kRunMicrotasks};
   v8::Context::Scope context_scope(local_context);
 
   if (!success) {
diff --git a/shell/common/gin_helper/callback.h b/shell/common/gin_helper/callback.h
index a4f41b9511f55..7187cc61db048 100644
--- a/shell/common/gin_helper/callback.h
+++ b/shell/common/gin_helper/callback.h
@@ -50,8 +50,9 @@ struct V8FunctionInvoker<v8::Local<v8::Value>(ArgTypes...)> {
       return v8::Null(isolate);
     v8::Local<v8::Function> holder = function.NewHandle(isolate);
     v8::Local<v8::Context> context = holder->GetCreationContextChecked();
-    gin_helper::MicrotasksScope microtasks_scope(
-        isolate, context->GetMicrotaskQueue(), true);
+    gin_helper::MicrotasksScope microtasks_scope{
+        isolate, context->GetMicrotaskQueue(), true,
+        v8::MicrotasksScope::kRunMicrotasks};
     v8::Context::Scope context_scope(context);
     std::vector<v8::Local<v8::Value>> args{
         gin::ConvertToV8(isolate, std::forward<ArgTypes>(raw))...};
@@ -75,8 +76,9 @@ struct V8FunctionInvoker<void(ArgTypes...)> {
       return;
     v8::Local<v8::Function> holder = function.NewHandle(isolate);
     v8::Local<v8::Context> context = holder->GetCreationContextChecked();
-    gin_helper::MicrotasksScope microtasks_scope(
-        isolate, context->GetMicrotaskQueue(), true);
+    gin_helper::MicrotasksScope microtasks_scope{
+        isolate, context->GetMicrotaskQueue(), true,
+        v8::MicrotasksScope::kRunMicrotasks};
     v8::Context::Scope context_scope(context);
     std::vector<v8::Local<v8::Value>> args{
         gin::ConvertToV8(isolate, std::forward<ArgTypes>(raw))...};
@@ -99,8 +101,9 @@ struct V8FunctionInvoker<ReturnType(ArgTypes...)> {
       return ret;
     v8::Local<v8::Function> holder = function.NewHandle(isolate);
     v8::Local<v8::Context> context = holder->GetCreationContextChecked();
-    gin_helper::MicrotasksScope microtasks_scope(
-        isolate, context->GetMicrotaskQueue(), true);
+    gin_helper::MicrotasksScope microtasks_scope{
+        isolate, context->GetMicrotaskQueue(), true,
+        v8::MicrotasksScope::kRunMicrotasks};
     v8::Context::Scope context_scope(context);
     std::vector<v8::Local<v8::Value>> args{
         gin::ConvertToV8(isolate, std::forward<ArgTypes>(raw))...};
diff --git a/shell/common/gin_helper/event_emitter_caller.cc b/shell/common/gin_helper/event_emitter_caller.cc
index c18c2a7895a0b..124a300cb4e91 100644
--- a/shell/common/gin_helper/event_emitter_caller.cc
+++ b/shell/common/gin_helper/event_emitter_caller.cc
@@ -24,8 +24,9 @@ v8::Local<v8::Value> CallMethodWithArgs(v8::Isolate* isolate,
   }
 
   // Perform microtask checkpoint after running JavaScript.
-  gin_helper::MicrotasksScope microtasks_scope(
-      isolate, obj->GetCreationContextChecked()->GetMicrotaskQueue(), true);
+  gin_helper::MicrotasksScope microtasks_scope{
+      isolate, obj->GetCreationContextChecked()->GetMicrotaskQueue(), true,
+      v8::MicrotasksScope::kRunMicrotasks};
 
   // node::MakeCallback will also run pending tasks in Node.js.
   v8::MaybeLocal<v8::Value> ret = node::MakeCallback(
diff --git a/shell/common/gin_helper/function_template.h b/shell/common/gin_helper/function_template.h
index 3cc8bf025fff9..0737c8b47448e 100644
--- a/shell/common/gin_helper/function_template.h
+++ b/shell/common/gin_helper/function_template.h
@@ -267,9 +267,10 @@ class Invoker<std::index_sequence<indices...>, ArgTypes...>
   template <typename ReturnType>
   void DispatchToCallback(
       base::RepeatingCallback<ReturnType(ArgTypes...)> callback) {
-    gin_helper::MicrotasksScope microtasks_scope(
+    gin_helper::MicrotasksScope microtasks_scope{
         args_->isolate(),
-        args_->GetHolderCreationContext()->GetMicrotaskQueue(), true);
+        args_->GetHolderCreationContext()->GetMicrotaskQueue(), true,
+        v8::MicrotasksScope::kRunMicrotasks};
     args_->Return(
         callback.Run(std::move(ArgumentHolder<indices, ArgTypes>::value)...));
   }
@@ -278,9 +279,10 @@ class Invoker<std::index_sequence<indices...>, ArgTypes...>
   // expression to foo. As a result, we must specialize the case of Callbacks
   // that have the void return type.
   void DispatchToCallback(base::RepeatingCallback<void(ArgTypes...)> callback) {
-    gin_helper::MicrotasksScope microtasks_scope(
+    gin_helper::MicrotasksScope microtasks_scope{
         args_->isolate(),
-        args_->GetHolderCreationContext()->GetMicrotaskQueue(), true);
+        args_->GetHolderCreationContext()->GetMicrotaskQueue(), true,
+        v8::MicrotasksScope::kRunMicrotasks};
     callback.Run(std::move(ArgumentHolder<indices, ArgTypes>::value)...);
   }
 
diff --git a/shell/common/gin_helper/microtasks_scope.h b/shell/common/gin_helper/microtasks_scope.h
index aff5a3f6eac12..69c52552fd5ee 100644
--- a/shell/common/gin_helper/microtasks_scope.h
+++ b/shell/common/gin_helper/microtasks_scope.h
@@ -16,11 +16,10 @@ namespace gin_helper {
 // In the render process creates a v8::MicrotasksScope.
 class MicrotasksScope {
  public:
-  explicit MicrotasksScope(v8::Isolate* isolate,
-                           v8::MicrotaskQueue* microtask_queue,
-                           bool ignore_browser_checkpoint = false,
-                           v8::MicrotasksScope::Type scope_type =
-                               v8::MicrotasksScope::kRunMicrotasks);
+  MicrotasksScope(v8::Isolate* isolate,
+                  v8::MicrotaskQueue* microtask_queue,
+                  bool ignore_browser_checkpoint,
+                  v8::MicrotasksScope::Type scope_type);
   ~MicrotasksScope();
 
   // disable copy
diff --git a/shell/common/gin_helper/promise.cc b/shell/common/gin_helper/promise.cc
index 5282f803e1c02..9fef990ab6e6e 100644
--- a/shell/common/gin_helper/promise.cc
+++ b/shell/common/gin_helper/promise.cc
@@ -30,8 +30,9 @@ PromiseBase& PromiseBase::operator=(PromiseBase&&) = default;
 
 v8::Maybe<bool> PromiseBase::Reject() {
   v8::HandleScope handle_scope(isolate());
-  gin_helper::MicrotasksScope microtasks_scope(
-      isolate(), GetContext()->GetMicrotaskQueue());
+  gin_helper::MicrotasksScope microtasks_scope{
+      isolate(), GetContext()->GetMicrotaskQueue(), false,
+      v8::MicrotasksScope::kRunMicrotasks};
   v8::Context::Scope context_scope(GetContext());
 
   return GetInner()->Reject(GetContext(), v8::Undefined(isolate()));
@@ -39,8 +40,9 @@ v8::Maybe<bool> PromiseBase::Reject() {
 
 v8::Maybe<bool> PromiseBase::Reject(v8::Local<v8::Value> except) {
   v8::HandleScope handle_scope(isolate());
-  gin_helper::MicrotasksScope microtasks_scope(
-      isolate(), GetContext()->GetMicrotaskQueue());
+  gin_helper::MicrotasksScope microtasks_scope{
+      isolate(), GetContext()->GetMicrotaskQueue(), false,
+      v8::MicrotasksScope::kRunMicrotasks};
   v8::Context::Scope context_scope(GetContext());
 
   return GetInner()->Reject(GetContext(), except);
@@ -49,8 +51,9 @@ v8::Maybe<bool> PromiseBase::Reject(v8::Local<v8::Value> except) {
 v8::Maybe<bool> PromiseBase::RejectWithErrorMessage(
     const std::string_view message) {
   v8::HandleScope handle_scope(isolate());
-  gin_helper::MicrotasksScope microtasks_scope(
-      isolate(), GetContext()->GetMicrotaskQueue());
+  gin_helper::MicrotasksScope microtasks_scope{
+      isolate(), GetContext()->GetMicrotaskQueue(), false,
+      v8::MicrotasksScope::kRunMicrotasks};
   v8::Context::Scope context_scope(GetContext());
 
   v8::Local<v8::Value> error =
@@ -92,8 +95,9 @@ v8::Local<v8::Promise> Promise<void>::ResolvedPromise(v8::Isolate* isolate) {
 
 v8::Maybe<bool> Promise<void>::Resolve() {
   v8::HandleScope handle_scope(isolate());
-  gin_helper::MicrotasksScope microtasks_scope(
-      isolate(), GetContext()->GetMicrotaskQueue());
+  gin_helper::MicrotasksScope microtasks_scope{
+      isolate(), GetContext()->GetMicrotaskQueue(), false,
+      v8::MicrotasksScope::kRunMicrotasks};
   v8::Context::Scope context_scope(GetContext());
 
   return GetInner()->Resolve(GetContext(), v8::Undefined(isolate()));
diff --git a/shell/common/gin_helper/promise.h b/shell/common/gin_helper/promise.h
index ff23cc1ce8a66..e46c5991ee725 100644
--- a/shell/common/gin_helper/promise.h
+++ b/shell/common/gin_helper/promise.h
@@ -123,8 +123,9 @@ class Promise : public PromiseBase {
   v8::Maybe<bool> Resolve(const RT& value) {
     gin_helper::Locker locker(isolate());
     v8::HandleScope handle_scope(isolate());
-    gin_helper::MicrotasksScope microtasks_scope(
-        isolate(), GetContext()->GetMicrotaskQueue());
+    gin_helper::MicrotasksScope microtasks_scope{
+        isolate(), GetContext()->GetMicrotaskQueue(), false,
+        v8::MicrotasksScope::kRunMicrotasks};
     v8::Context::Scope context_scope(GetContext());
 
     return GetInner()->Resolve(GetContext(),
diff --git a/shell/common/node_bindings.cc b/shell/common/node_bindings.cc
index cf7f754b08406..fb1326d95dd29 100644
--- a/shell/common/node_bindings.cc
+++ b/shell/common/node_bindings.cc
@@ -285,7 +285,7 @@ void ErrorMessageListener(v8::Local<v8::Message> message,
   node::Environment* env = node::Environment::GetCurrent(isolate);
   if (env) {
     gin_helper::MicrotasksScope microtasks_scope(
-        isolate, env->context()->GetMicrotaskQueue(),
+        isolate, env->context()->GetMicrotaskQueue(), false,
         v8::MicrotasksScope::kDoNotRunMicrotasks);
     // Emit the after() hooks now that the exception has been handled.
     // Analogous to node/lib/internal/process/execution.js#L176-L180
diff --git a/shell/common/v8_value_serializer.cc b/shell/common/v8_value_serializer.cc
index 5420adb4dbfd8..96251a149ad30 100644
--- a/shell/common/v8_value_serializer.cc
+++ b/shell/common/v8_value_serializer.cc
@@ -34,9 +34,9 @@ class V8Serializer : public v8::ValueSerializer::Delegate {
   ~V8Serializer() override = default;
 
   bool Serialize(v8::Local<v8::Value> value, blink::CloneableMessage* out) {
-    gin_helper::MicrotasksScope microtasks_scope(
-        isolate_, isolate_->GetCurrentContext()->GetMicrotaskQueue(),
-        v8::MicrotasksScope::kDoNotRunMicrotasks);
+    gin_helper::MicrotasksScope microtasks_scope{
+        isolate_, isolate_->GetCurrentContext()->GetMicrotaskQueue(), false,
+        v8::MicrotasksScope::kDoNotRunMicrotasks};
     WriteBlinkEnvelope(19);
 
     serializer_.WriteHeader();
diff --git a/shell/renderer/api/electron_api_spell_check_client.cc b/shell/renderer/api/electron_api_spell_check_client.cc
index 07ab0115c8ae9..6d8cb4dd8a66c 100644
--- a/shell/renderer/api/electron_api_spell_check_client.cc
+++ b/shell/renderer/api/electron_api_spell_check_client.cc
@@ -221,9 +221,9 @@ void SpellCheckClient::SpellCheckWords(const SpellCheckScope& scope,
   DCHECK(!scope.spell_check_.IsEmpty());
 
   auto context = isolate_->GetCurrentContext();
-  gin_helper::MicrotasksScope microtasks_scope(
-      isolate_, context->GetMicrotaskQueue(),
-      v8::MicrotasksScope::kDoNotRunMicrotasks);
+  gin_helper::MicrotasksScope microtasks_scope{
+      isolate_, context->GetMicrotaskQueue(), false,
+      v8::MicrotasksScope::kDoNotRunMicrotasks};
 
   v8::Local<v8::FunctionTemplate> templ = gin_helper::CreateFunctionTemplate(
       isolate_, base::BindRepeating(&SpellCheckClient::OnSpellCheckDone,
diff --git a/shell/renderer/electron_sandboxed_renderer_client.cc b/shell/renderer/electron_sandboxed_renderer_client.cc
index 50f03c641c113..8568ec59b8d9e 100644
--- a/shell/renderer/electron_sandboxed_renderer_client.cc
+++ b/shell/renderer/electron_sandboxed_renderer_client.cc
@@ -204,9 +204,9 @@ void ElectronSandboxedRendererClient::WillReleaseScriptContext(
     return;
 
   auto* isolate = context->GetIsolate();
-  gin_helper::MicrotasksScope microtasks_scope(
-      isolate, context->GetMicrotaskQueue(),
-      v8::MicrotasksScope::kDoNotRunMicrotasks);
+  gin_helper::MicrotasksScope microtasks_scope{
+      isolate, context->GetMicrotaskQueue(), false,
+      v8::MicrotasksScope::kDoNotRunMicrotasks};
   v8::HandleScope handle_scope(isolate);
   v8::Context::Scope context_scope(context);
   InvokeEmitProcessEvent(context, "exit");
@@ -223,9 +223,9 @@ void ElectronSandboxedRendererClient::EmitProcessEvent(
   v8::HandleScope handle_scope{isolate};
 
   v8::Local<v8::Context> context = GetContext(frame, isolate);
-  gin_helper::MicrotasksScope microtasks_scope(
-      isolate, context->GetMicrotaskQueue(),
-      v8::MicrotasksScope::kDoNotRunMicrotasks);
+  gin_helper::MicrotasksScope microtasks_scope{
+      isolate, context->GetMicrotaskQueue(), false,
+      v8::MicrotasksScope::kDoNotRunMicrotasks};
   v8::Context::Scope context_scope(context);
 
   InvokeEmitProcessEvent(context, event_name);


Test patch: