Repository: electron/electron
Pull request: fix: defer Wrappable destruction in SecondWeakCallback to a posted task

Backport of #50688

See that PR for details.


Notes: Fixed an intermittent `Invoke in DisallowJavascriptExecutionScope` crash on application quit when a `WebContents` (or other JS-emitting native object) is garbage-collected during shutdown.


Changed files:
filename: shell/common/gin_helper/wrappable.cc
patch: @@ -4,6 +4,7 @@
 
 #include "shell/common/gin_helper/wrappable.h"
 
+#include "base/task/sequenced_task_runner.h"
 #include "gin/object_template_builder.h"
 #include "gin/public/isolate_holder.h"
 #include "shell/common/gin_helper/dictionary.h"
@@ -90,7 +91,22 @@ void WrappableBase::SecondWeakCallback(
   if (gin::IsolateHolder::DestroyedMicrotasksRunner()) {
     return;
   }
-  delete static_cast<WrappableBase*>(data.GetInternalField(0));
+  // Defer destruction to a posted task. V8's second-pass weak callbacks run
+  // inside a DisallowJavascriptExecutionScope (they may touch the V8 API but
+  // must not invoke JS). Several Electron Wrappables (e.g. WebContents) emit
+  // JS events from their destructors, so deleting synchronously here can
+  // crash with "Invoke in DisallowJavascriptExecutionScope" — see
+  // https://github.com/electron/electron/issues/47420. Posting via the
+  // current sequence's task runner ensures the destructor runs once V8 has
+  // left the GC scope. If no task runner is available (e.g. early/late in
+  // process lifetime), fall back to synchronous deletion.
+  auto* wrappable = static_cast<WrappableBase*>(data.GetInternalField(0));
+  if (base::SequencedTaskRunner::HasCurrentDefault()) {
+    base::SequencedTaskRunner::GetCurrentDefault()->DeleteSoon(FROM_HERE,
+                                                               wrappable);
+  } else {
+    delete wrappable;
+  }
 }
 
 DeprecatedWrappableBase::DeprecatedWrappableBase() = default;
@@ -126,9 +142,19 @@ void DeprecatedWrappableBase::SecondWeakCallback(
     const v8::WeakCallbackInfo<DeprecatedWrappableBase>& data) {
   if (gin::IsolateHolder::DestroyedMicrotasksRunner())
     return;
+  // See WrappableBase::SecondWeakCallback for why deletion is posted: V8's
+  // second-pass weak callbacks run inside a DisallowJavascriptExecutionScope,
+  // and several Wrappables emit JS events from their destructors.
+  // https://github.com/electron/electron/issues/47420
   DeprecatedWrappableBase* wrappable = data.GetParameter();
-  if (wrappable)
+  if (!wrappable)
+    return;
+  if (base::SequencedTaskRunner::HasCurrentDefault()) {
+    base::SequencedTaskRunner::GetCurrentDefault()->DeleteSoon(FROM_HERE,
+                                                               wrappable);
+  } else {
     delete wrappable;
+  }
 }
 
 v8::MaybeLocal<v8::Object> DeprecatedWrappableBase::GetWrapperImpl(

filename: shell/common/gin_helper/wrappable_base.h
patch: @@ -6,6 +6,7 @@
 #define ELECTRON_SHELL_COMMON_GIN_HELPER_WRAPPABLE_BASE_H_
 
 #include "base/memory/raw_ptr.h"
+#include "base/task/sequenced_task_runner_helpers.h"
 #include "v8/include/v8-forward.h"
 
 namespace gin {
@@ -75,6 +76,11 @@ class DeprecatedWrappableBase {
   DeprecatedWrappableBase();
   virtual ~DeprecatedWrappableBase();
 
+  // SecondWeakCallback posts destruction via DeleteSoon so that destructors
+  // (which may emit JS events) run outside V8's GC scope. DeleteSoon needs
+  // access to the protected destructor.
+  friend class base::DeleteHelper<DeprecatedWrappableBase>;
+
   // Overrides of this method should be declared final and not overridden again.
   virtual gin::ObjectTemplateBuilder GetObjectTemplateBuilder(
       v8::Isolate* isolate);

filename: spec/fixtures/crash-cases/wrappable-gc-weak-callback-emit/index.js
patch: @@ -0,0 +1,29 @@
+const { app, WebContentsView } = require('electron');
+
+const v8 = require('node:v8');
+
+// Force V8 to schedule incremental-marking finalization steps as foreground
+// tasks on every allocation. Those tasks run inside V8's
+// DisallowJavascriptExecutionScope. Prior to the fix in
+// shell/common/gin_helper/wrappable.cc, gin_helper::SecondWeakCallback would
+// `delete` the Wrappable synchronously inside that scope, and Wrappables
+// whose destructors emit JS events (WebContents emits 'will-destroy' /
+// 'destroyed') would crash with "Invoke in DisallowJavascriptExecutionScope".
+//
+// Regression test for https://github.com/electron/electron/issues/47420.
+v8.setFlagsFromString('--stress-incremental-marking');
+
+app.whenReady().then(() => {
+  // Leak several WebContentsView instances so at least one wrapper's
+  // collection lands in a foreground GC task during app.quit()'s uv_run
+  // drain. Three is enough for the crash to reproduce 10/10 on a Linux
+  // testing build before the fix.
+  // eslint-disable-next-line no-new
+  new WebContentsView();
+  // eslint-disable-next-line no-new
+  new WebContentsView();
+  // eslint-disable-next-line no-new
+  new WebContentsView();
+
+  app.quit();
+});