fix: remove catch-all HandleScope (#22531)

shell/app/node_main.cc

@@ -139,55 +139,59 @@ int NodeMain(int argc, char* argv[]) {
     JavascriptEnvironment gin_env(loop);
 
     v8::Isolate* isolate = gin_env.isolate();
-
-    node::IsolateData* isolate_data =
-        node::CreateIsolateData(isolate, loop, gin_env.platform());
-    CHECK_NE(nullptr, isolate_data);
-
-    v8::Locker locker(isolate);
     v8::Isolate::Scope isolate_scope(isolate);
-    v8::HandleScope handle_scope(isolate);
+    v8::Locker locker(isolate);
+    node::Environment* env = nullptr;
+    node::IsolateData* isolate_data = nullptr;
+    {
+      v8::HandleScope scope(isolate);
 
-    node::Environment* env = node::CreateEnvironment(
-        isolate_data, gin_env.context(), argc, argv, exec_argc, exec_argv);
-    CHECK_NE(nullptr, env);
+      isolate_data = node::CreateIsolateData(isolate, loop, gin_env.platform());
+      CHECK_NE(nullptr, isolate_data);
 
-    // Enable support for v8 inspector.
-    NodeDebugger node_debugger(env);
-    node_debugger.Start();
+      env = node::CreateEnvironment(isolate_data, gin_env.context(), argc, argv,
+                                    exec_argc, exec_argv);
+      CHECK_NE(nullptr, env);
 
-    // TODO(codebytere): we shouldn't have to call this - upstream?
-    env->InitializeDiagnostics();
+      // TODO(codebytere): we shouldn't have to call this - upstream?
+      env->InitializeDiagnostics();
 
-    // This is needed in order to enable v8 host weakref hooks.
-    // TODO(codebytere): we shouldn't have to call this - upstream?
-    isolate->SetHostCleanupFinalizationGroupCallback(
-        HostCleanupFinalizationGroupCallback);
+      // This is needed in order to enable v8 host weakref hooks.
+      // TODO(codebytere): we shouldn't have to call this - upstream?
+      isolate->SetHostCleanupFinalizationGroupCallback(
+          HostCleanupFinalizationGroupCallback);
 
-    gin_helper::Dictionary process(isolate, env->process_object());
+      gin_helper::Dictionary process(isolate, env->process_object());
 #if defined(OS_WIN)
-    process.SetMethod("log", &ElectronBindings::Log);
+      process.SetMethod("log", &ElectronBindings::Log);
 #endif
-    process.SetMethod("crash", &ElectronBindings::Crash);
+      process.SetMethod("crash", &ElectronBindings::Crash);
 
-    // Setup process.crashReporter.start in child node processes
-    gin_helper::Dictionary reporter = gin::Dictionary::CreateEmpty(isolate);
-    reporter.SetMethod("start", &crash_reporter::CrashReporter::StartInstance);
+      // Setup process.crashReporter.start in child node processes
+      gin_helper::Dictionary reporter = gin::Dictionary::CreateEmpty(isolate);
+      reporter.SetMethod("start",
+                         &crash_reporter::CrashReporter::StartInstance);
 
 #if !defined(OS_LINUX)
-    reporter.SetMethod("addExtraParameter", &AddExtraParameter);
-    reporter.SetMethod("removeExtraParameter", &RemoveExtraParameter);
+      reporter.SetMethod("addExtraParameter", &AddExtraParameter);
+      reporter.SetMethod("removeExtraParameter", &RemoveExtraParameter);
 #endif
 
-    process.Set("crashReporter", reporter);
+      process.Set("crashReporter", reporter);
 
-    gin_helper::Dictionary versions;
-    if (process.Get("versions", &versions)) {
-      versions.SetReadOnly(ELECTRON_PROJECT_NAME, ELECTRON_VERSION_STRING);
+      gin_helper::Dictionary versions;
+      if (process.Get("versions", &versions)) {
+        versions.SetReadOnly(ELECTRON_PROJECT_NAME, ELECTRON_VERSION_STRING);
+      }
     }
 
+    // Enable support for v8 inspector.
+    NodeDebugger node_debugger(env);
+    node_debugger.Start();
+
     // TODO(codebytere): we should try to handle this upstream.
     {
+      v8::HandleScope scope(isolate);
       node::InternalCallbackScope callback_scope(
           env, v8::Local<v8::Object>(), {1, 0},
           node::InternalCallbackScope::kAllowEmptyResource |

shell/browser/api/electron_api_app.cc

@@ -788,8 +788,11 @@ void App::RenderProcessReady(content::RenderProcessHost* host) {
   // `RenderProcessPreferences`, so this is at least more explicit...
   content::WebContents* web_contents =
       ElectronBrowserClient::Get()->GetWebContentsFromProcessID(host->GetID());
-  if (web_contents)
-    WebContents::FromOrCreate(v8::Isolate::GetCurrent(), web_contents);
+  if (web_contents) {
+    v8::Isolate* isolate = v8::Isolate::GetCurrent();
+    v8::HandleScope scope(isolate);
+    WebContents::FromOrCreate(isolate, web_contents);
+  }
 }
 
 void App::RenderProcessDisconnected(base::ProcessId host_pid) {

shell/browser/api/electron_api_cookies.cc

@@ -314,6 +314,7 @@ v8::Local<v8::Promise> Cookies::FlushStore() {
 }
 
 void Cookies::OnCookieChanged(const net::CookieChangeInfo& change) {
+  v8::HandleScope scope(isolate());
   Emit("changed", gin::ConvertToV8(isolate(), change.cookie),
        gin::ConvertToV8(isolate(), change.cause),
        gin::ConvertToV8(isolate(),

shell/browser/api/electron_api_menu.cc

@@ -214,6 +214,7 @@ void Menu::OnMenuWillClose() {
 }
 
 void Menu::OnMenuWillShow() {
+  v8::HandleScope scope(isolate());
   g_menus[weak_map_id()] = v8::Global<v8::Object>(isolate(), GetWrapper());
   Emit("menu-will-show");
 }

shell/browser/api/electron_api_service_worker_context.cc

@@ -85,8 +85,10 @@ ServiceWorkerContext::~ServiceWorkerContext() {
 void ServiceWorkerContext::OnReportConsoleMessage(
     int64_t version_id,
     const content::ConsoleMessage& message) {
+  v8::Isolate* isolate = v8::Isolate::GetCurrent();
+  v8::HandleScope scope(isolate);
   Emit("console-message",
-       gin::DataObjectBuilder(v8::Isolate::GetCurrent())
+       gin::DataObjectBuilder(isolate)
            .Set("versionId", version_id)
            .Set("source", MessageSourceToString(message.source))
            .Set("level", static_cast<int32_t>(message.message_level))

shell/browser/api/electron_api_url_loader.cc

@@ -433,6 +433,7 @@ void SimpleURLLoaderWrapper::OnRetry(base::OnceClosure start_retry) {}
 void SimpleURLLoaderWrapper::OnResponseStarted(
     const GURL& final_url,
     const network::mojom::URLResponseHead& response_head) {
+  v8::HandleScope scope(isolate());
   gin::Dictionary dict = gin::Dictionary::CreateEmpty(isolate());
   dict.Set("statusCode", response_head.headers->response_code());
   dict.Set("statusMessage", response_head.headers->GetStatusText());

shell/browser/api/event.cc

@@ -6,8 +6,10 @@
 
 #include <utility>
 
+#include "gin/data_object_builder.h"
 #include "gin/object_template_builder.h"
 #include "shell/common/gin_converters/blink_converter.h"
+#include "shell/common/gin_converters/std_converter.h"
 
 namespace gin_helper {
 
@@ -15,7 +17,16 @@ gin::WrapperInfo Event::kWrapperInfo = {gin::kEmbedderNativeGin};
 
 Event::Event() {}
 
-Event::~Event() = default;
+Event::~Event() {
+  if (callback_) {
+    v8::Isolate* isolate = v8::Isolate::GetCurrent();
+    v8::HandleScope scope(isolate);
+    auto message = gin::DataObjectBuilder(isolate)
+                       .Set("error", "reply was never sent")
+                       .Build();
+    SendReply(isolate, message);
+  }
+}
 
 void Event::SetCallback(InvokeCallback callback) {
   DCHECK(!callback_);

shell/browser/electron_autofill_driver.cc

@@ -28,10 +28,11 @@ void AutofillDriver::ShowAutofillPopup(
     const gfx::RectF& bounds,
     const std::vector<base::string16>& values,
     const std::vector<base::string16>& labels) {
+  v8::Isolate* isolate = v8::Isolate::GetCurrent();
+  v8::HandleScope scope(isolate);
   auto* web_contents =
-      api::WebContents::From(
-          v8::Isolate::GetCurrent(),
-          content::WebContents::FromRenderFrameHost(render_frame_host_))
+      api::WebContents::From(isolate, content::WebContents::FromRenderFrameHost(
+                                          render_frame_host_))
           .get();
   if (!web_contents || !web_contents->owner_window())
     return;

shell/browser/electron_browser_client.cc

@@ -1300,6 +1300,7 @@ bool ElectronBrowserClient::WillInterceptWebSocket(
     return false;
 
   v8::Isolate* isolate = v8::Isolate::GetCurrent();
+  v8::HandleScope scope(isolate);
   auto* browser_context = frame->GetProcess()->GetBrowserContext();
   auto web_request = api::WebRequest::FromOrCreate(isolate, browser_context);
 
@@ -1320,6 +1321,7 @@ void ElectronBrowserClient::CreateWebSocket(
     mojo::PendingRemote<network::mojom::WebSocketHandshakeClient>
         handshake_client) {
   v8::Isolate* isolate = v8::Isolate::GetCurrent();
+  v8::HandleScope scope(isolate);
   auto* browser_context = frame->GetProcess()->GetBrowserContext();
   auto web_request = api::WebRequest::FromOrCreate(isolate, browser_context);
   DCHECK(web_request.get());
@@ -1345,6 +1347,7 @@ bool ElectronBrowserClient::WillCreateURLLoaderFactory(
     bool* disable_secure_dns,
     network::mojom::URLLoaderFactoryOverridePtr* factory_override) {
   v8::Isolate* isolate = v8::Isolate::GetCurrent();
+  v8::HandleScope scope(isolate);
   api::Protocol* protocol =
       api::Protocol::FromWrappedClass(isolate, browser_context);
   DCHECK(protocol);

shell/browser/electron_browser_main_parts.cc

@@ -293,6 +293,8 @@ void ElectronBrowserMainParts::PostEarlyInitialization() {
   // avoid conflicts we only initialize our V8 environment after that.
   js_env_ = std::make_unique<JavascriptEnvironment>(node_bindings_->uv_loop());
 
+  v8::HandleScope scope(js_env_->isolate());
+
   node_bindings_->Initialize();
   // Create the global environment.
   node::Environment* env = node_bindings_->CreateEnvironment(

shell/browser/electron_navigation_throttle.cc

@@ -28,8 +28,9 @@ ElectronNavigationThrottle::WillRedirectRequest() {
     return PROCEED;
   }
 
-  auto api_contents =
-      electron::api::WebContents::From(v8::Isolate::GetCurrent(), contents);
+  v8::Isolate* isolate = v8::Isolate::GetCurrent();
+  v8::HandleScope scope(isolate);
+  auto api_contents = electron::api::WebContents::From(isolate, contents);
   if (api_contents.IsEmpty()) {
     // No need to emit any event if the WebContents is not available in JS.
     return PROCEED;

shell/browser/javascript_environment.cc

@@ -29,13 +29,21 @@ JavascriptEnvironment::JavascriptEnvironment(uv_loop_t* event_loop)
                       gin::IsolateHolder::IsolateType::kUtility,
                       gin::IsolateHolder::IsolateCreationMode::kNormal,
                       isolate_),
-      isolate_scope_(isolate_),
-      locker_(isolate_),
-      handle_scope_(isolate_),
-      context_(isolate_, node::NewContext(isolate_)),
-      context_scope_(v8::Local<v8::Context>::New(isolate_, context_)) {}
+      locker_(isolate_) {
+  isolate_->Enter();
+  v8::HandleScope scope(isolate_);
+  auto context = node::NewContext(isolate_);
+  context_ = v8::Global<v8::Context>(isolate_, context);
+  context->Enter();
+}
 
-JavascriptEnvironment::~JavascriptEnvironment() = default;
+JavascriptEnvironment::~JavascriptEnvironment() {
+  {
+    v8::HandleScope scope(isolate_);
+    context_.Get(isolate_)->Exit();
+  }
+  isolate_->Exit();
+}
 
 v8::Isolate* JavascriptEnvironment::Initialize(uv_loop_t* event_loop) {
   auto* cmd = base::CommandLine::ForCurrentProcess();

shell/browser/javascript_environment.h

@@ -41,11 +41,8 @@ class JavascriptEnvironment {
 
   v8::Isolate* isolate_;
   gin::IsolateHolder isolate_holder_;
-  v8::Isolate::Scope isolate_scope_;
   v8::Locker locker_;
-  v8::HandleScope handle_scope_;
   v8::Global<v8::Context> context_;
-  v8::Context::Scope context_scope_;
 
   std::unique_ptr<MicrotasksRunner> microtasks_runner_;
 

shell/browser/login_handler.cc

@@ -51,6 +51,7 @@ void LoginHandler::EmitEvent(
     scoped_refptr<net::HttpResponseHeaders> response_headers,
     bool first_auth_attempt) {
   v8::Isolate* isolate = v8::Isolate::GetCurrent();
+  v8::HandleScope scope(isolate);
 
   auto api_web_contents = api::WebContents::From(isolate, web_contents());
   if (api_web_contents.IsEmpty()) {
@@ -58,8 +59,6 @@ void LoginHandler::EmitEvent(
     return;
   }
 
-  v8::HandleScope scope(isolate);
-
   auto details = gin::Dictionary::CreateEmpty(isolate);
   details.Set("url", url);
 

shell/browser/net/node_stream_loader.cc

@@ -92,6 +92,7 @@ void NodeStreamLoader::ReadMore() {
   }
   is_reading_ = true;
   auto weak = weak_factory_.GetWeakPtr();
+  v8::HandleScope scope(isolate_);
   // buffer = emitter.read()
   v8::MaybeLocal<v8::Value> ret = node::MakeCallback(
       isolate_, emitter_.Get(isolate_), "read", 0, nullptr, {0, 0});

shell/browser/ui/file_dialog_mac.mm

@@ -302,6 +302,7 @@ void OpenDialogCompletion(int chosen,
                           NSOpenPanel* dialog,
                           bool security_scoped_bookmarks,
                           gin_helper::Promise<gin_helper::Dictionary> promise) {
+  v8::HandleScope scope(promise.isolate());
   gin_helper::Dictionary dict = gin::Dictionary::CreateEmpty(promise.isolate());
   if (chosen == NSFileHandlingPanelCancelButton) {
     dict.Set("canceled", true);
@@ -379,6 +380,7 @@ void SaveDialogCompletion(int chosen,
                           NSSavePanel* dialog,
                           bool security_scoped_bookmarks,
                           gin_helper::Promise<gin_helper::Dictionary> promise) {
+  v8::HandleScope scope(promise.isolate());
   gin_helper::Dictionary dict = gin::Dictionary::CreateEmpty(promise.isolate());
   if (chosen == NSFileHandlingPanelCancelButton) {
     dict.Set("canceled", true);

shell/common/gin_helper/event_emitter.cc

@@ -38,6 +38,7 @@ v8::Local<v8::Object> CreateEvent(v8::Isolate* isolate,
   }
 
   v8::Local<v8::Context> context = isolate->GetCurrentContext();
+  CHECK(!context.IsEmpty());
   v8::Local<v8::Object> event =
       v8::Local<v8::ObjectTemplate>::New(isolate, event_template)
           ->NewInstance(context)

shell/common/gin_helper/trackable_object.h

@@ -55,13 +55,16 @@ class TrackableObject : public TrackableObjectBase, public EventEmitter<T> {
  public:
   // Mark the JS object as destroyed.
   void MarkDestroyed() {
+    v8::HandleScope scope(gin_helper::Wrappable<T>::isolate());
     v8::Local<v8::Object> wrapper = gin_helper::Wrappable<T>::GetWrapper();
     if (!wrapper.IsEmpty()) {
       wrapper->SetAlignedPointerInInternalField(0, nullptr);
+      gin_helper::WrappableBase::wrapper_.ClearWeak();
     }
   }
 
   bool IsDestroyed() {
+    v8::HandleScope scope(gin_helper::Wrappable<T>::isolate());
     v8::Local<v8::Object> wrapper = gin_helper::Wrappable<T>::GetWrapper();
     return wrapper->InternalFieldCount() == 0 ||
            wrapper->GetAlignedPointerFromInternalField(0) == nullptr;
@@ -72,6 +75,7 @@ class TrackableObject : public TrackableObjectBase, public EventEmitter<T> {
     if (!weak_map_)
       return nullptr;
 
+    v8::HandleScope scope(isolate);
     v8::MaybeLocal<v8::Object> object = weak_map_->Get(isolate, id);
     if (object.IsEmpty())
       return nullptr;

shell/common/gin_helper/wrappable.cc

@@ -16,6 +16,7 @@ WrappableBase::~WrappableBase() {
   if (wrapper_.IsEmpty())
     return;
 
+  v8::HandleScope scope(isolate());
   GetWrapper()->SetAlignedPointerInInternalField(0, nullptr);
   wrapper_.ClearWeak();
   wrapper_.Reset();
@@ -49,7 +50,8 @@ void WrappableBase::InitWith(v8::Isolate* isolate,
   isolate_ = isolate;
   wrapper->SetAlignedPointerInInternalField(0, this);
   wrapper_.Reset(isolate, wrapper);
-  wrapper_.SetWeak(this, FirstWeakCallback, v8::WeakCallbackType::kParameter);
+  wrapper_.SetWeak(this, FirstWeakCallback,
+                   v8::WeakCallbackType::kInternalFields);
 
   // Call object._init if we have one.
   v8::Local<v8::Function> init;
@@ -62,24 +64,21 @@ void WrappableBase::InitWith(v8::Isolate* isolate,
 // static
 void WrappableBase::FirstWeakCallback(
     const v8::WeakCallbackInfo<WrappableBase>& data) {
-  WrappableBase* wrappable = data.GetParameter();
-  wrappable->wrapper_.Reset();
-  data.SetSecondPassCallback(SecondWeakCallback);
+  WrappableBase* wrappable =
+      static_cast<WrappableBase*>(data.GetInternalField(0));
+  if (wrappable) {
+    wrappable->wrapper_.Reset();
+    data.SetSecondPassCallback(SecondWeakCallback);
+  }
 }
 
 // static
 void WrappableBase::SecondWeakCallback(
     const v8::WeakCallbackInfo<WrappableBase>& data) {
-  // Certain classes (for example api::WebContents and api::WebContentsView)
-  // are running JS code in the destructor, while V8 may crash when JS code
-  // runs inside weak callback.
-  //
-  // We work around this problem by delaying the deletion to next tick where
-  // garbage collection is done.
-  base::ThreadTaskRunnerHandle::Get()->PostNonNestableTask(
-      FROM_HERE,
-      base::BindOnce([](WrappableBase* wrappable) { delete wrappable; },
-                     base::Unretained(data.GetParameter())));
+  WrappableBase* wrappable =
+      static_cast<WrappableBase*>(data.GetInternalField(0));
+  if (wrappable)
+    delete wrappable;
 }
 
 namespace internal {

shell/common/gin_helper/wrappable_base.h

@@ -52,6 +52,8 @@ class WrappableBase {
   // Helper to init with arguments.
   void InitWithArgs(gin::Arguments* args);
 
+  v8::Global<v8::Object> wrapper_;  // Weak
+
  private:
   static void FirstWeakCallback(
       const v8::WeakCallbackInfo<WrappableBase>& data);
@@ -59,7 +61,6 @@ class WrappableBase {
       const v8::WeakCallbackInfo<WrappableBase>& data);
 
   v8::Isolate* isolate_ = nullptr;
-  v8::Global<v8::Object> wrapper_;  // Weak
 
   DISALLOW_COPY_AND_ASSIGN(WrappableBase);
 };

spec-main/api-ipc-spec.ts

@@ -1,5 +1,8 @@
 import { expect } from 'chai'
 import { BrowserWindow, ipcMain, IpcMainInvokeEvent } from 'electron'
+import { emittedOnce } from './events-helpers'
+
+const v8Util = process.electronBinding('v8_util')
 
 describe('ipc module', () => {
   describe('invoke', () => {
@@ -103,6 +106,17 @@ describe('ipc module', () => {
         ipcMain.removeHandler('test')
       }
     })
+
+    it('throws an error in the renderer if the reply callback is dropped', async () => {
+      // eslint-disable-next-line @typescript-eslint/no-unused-vars
+      ipcMain.handleOnce('test', () => new Promise(resolve => {
+        setTimeout(() => v8Util.requestGarbageCollectionForTesting())
+        /* never resolve */
+      }))
+      w.webContents.executeJavaScript(`(${rendererInvoke})()`)
+      const [, { error }] = await emittedOnce(ipcMain, 'result')
+      expect(error).to.match(/reply was never sent/)
+    })
   })
 
   describe('ordering', () => {