Repository: electron/electron
Pull request: fix: crash when attempting to resolve modules during process exit

#### Description of Change

Refs https://github.com/microsoft/vscode/issues/256031

<details>
<summary>_Analysis based on crash dumps_</summary>

Only listing the interesting threads below

```
Crashed Thread:        12

Exception Type:        EXC_CRASH (SIGABRT)
Exception Codes:       0x0000000000000000, 0x0000000000000000

Termination Reason:    Namespace SIGNAL, Code 6 Abort trap: 6
Terminating Process:   Electron [56936]

Application Specific Information:
abort() called


Thread 0::  Dispatch queue: com.apple.main-thread
0   libsystem_kernel.dylib        	       0x190fe39b8 __ulock_wait + 8
1   libsystem_pthread.dylib       	       0x19102606c _pthread_join + 608
2   Electron Framework            	       0x119600a5c uv_thread_join + 20
3   Electron Framework            	       0x119a1b2ac node::worker::Worker::JoinThread() + 60
4   Electron Framework            	       0x11988d5b4 node::Environment::stop_sub_worker_contexts() + 216
5   Electron Framework            	       0x119827460 node::DefaultProcessExitHandler(node::Environment*, int) + 44
6   Electron Framework            	       0x11988d4c0 node::Environment::Exit(node::ExitCode) + 412
7   ???                           	       0x138d90278 ???
8   ???                           	       0x138d8e270 ???
9   ???                           	       0x138d8e270 ???
10  ???                           	       0x1310e7734 ???
11  ???                           	       0x1310f8b7c ???
12  ???                           	       0x138d8b228 ???
13  ???                           	       0x138d8ae74 ???
14  Electron Framework            	       0x1172db5cc v8::Function::Call(v8::Isolate*, v8::Local<v8::Context>, v8::Local<v8::Value>, int, v8::Local<v8::Value>*) + 3576
15  Electron Framework            	       0x1172da968 v8::Function::Call(v8::Isolate*, v8::Local<v8::Context>, v8::Local<v8::Value>, int, v8::Local<v8::Value>*) + 404
16  Electron Framework            	       0x11988bf20 node::Environment::RunTimers(uv_timer_s*) + 356
17  Electron Framework            	       0x1195f0ea0 uv__run_timers + 144
18  Electron Framework            	       0x1195f4320 uv_run + 600
19  Electron Framework            	       0x119821b64 node::SpinEventLoopInternal(node::Environment*) + 360
20  Electron Framework            	       0x119822a60 node::SpinEventLoop(node::Environment*) + 12
21  Electron Framework            	       0x11960ad38 ElectronInitializeICUandStartNode + 17784
22  Electron Framework            	       0x1196068cc ElectronInitializeICUandStartNode + 268
23  dyld                          	       0x190c82b98 start + 6076

...

Thread 12 Crashed:
0   libsystem_kernel.dylib        	       0x190fea388 __pthread_kill + 8
1   libsystem_pthread.dylib       	       0x191023848 pthread_kill + 296
2   libsystem_c.dylib             	       0x190f2c9e4 abort + 124
3   Electron Framework            	       0x11990195c node::OnFatalError(char const*, char const*) + 252
4   Electron Framework            	       0x11a5f9318 v8::PropertyDescriptor::set() const + 4540844
5   Electron Framework            	       0x11995ec20 node::modules::BindingData::GetPackageJSON(node::Realm*, std::__Cr::basic_string_view<char, std::__Cr::char_traits<char>>, node::modules::BindingData::ErrorContext*) + 5712
6   Electron Framework            	       0x119961b54 void node::modules::BindingData::GetPackageScopeConfig<false>(v8::FunctionCallbackInfo<v8::Value> const&) + 1604
7   ???                           	       0x138d90278 ???
8   ???                           	       0x138d8e270 ???
9   ???                           	       0x138d8e270 ???
10  ???                           	       0x131054f88 ???
11  ???                           	       0x138d8e270 ???
12  ???                           	       0x138d8e270 ???
13  ???                           	       0x138d8e270 ???
14  ???                           	       0x131054cb4 ???
15  ???                           	       0x131053dfc ???
16  ???                           	       0x13104ee74 ???
17  ???                           	       0x131059d64 ???
18  ???                           	       0x13104ee74 ???
19  ???                           	       0x131052d74 ???
20  ???                           	       0x131051b54 ???
21  ???                           	       0x131048f1c ???
22  ???                           	       0x138d8e270 ???
23  ???                           	       0x138d8b228 ???
24  ???                           	       0x138d8ae74 ???
25  Electron Framework            	       0x1172db5cc v8::Function::Call(v8::Isolate*, v8::Local<v8::Context>, v8::Local<v8::Value>, int, v8::Local<v8::Value>*) + 3576
26  Electron Framework            	       0x1172da968 v8::Function::Call(v8::Isolate*, v8::Local<v8::Context>, v8::Local<v8::Value>, int, v8::Local<v8::Value>*) + 404
27  Electron Framework            	       0x11982115c node::InternalMakeCallback(node::Environment*, v8::Local<v8::Object>, v8::Local<v8::Object>, v8::Local<v8::Function>, int, v8::Local<v8::Value>*, node::async_context, v8::Local<v8::Value>) + 504
28  Electron Framework            	       0x11983002c node::AsyncWrap::MakeCallback(v8::Local<v8::Function>, int, v8::Local<v8::Value>*) + 240
29  Electron Framework            	       0x1199550ec node::worker::MessagePort::OnMessage(node::worker::MessagePort::MessageProcessingMode) + 800
30  Electron Framework            	       0x1195f3d74 uv__async_fork + 772
31  Electron Framework            	       0x119606030 uv__io_poll + 1336
32  Electron Framework            	       0x1195f4240 uv_run + 376
33  Electron Framework            	       0x119821b64 node::SpinEventLoopInternal(node::Environment*) + 360
34  Electron Framework            	       0x119a1afb0 node::worker::Worker::Run() + 2008
35  Electron Framework            	       0x119a1fa14 _register_external_reference_worker(node::ExternalReferenceRegistry*) + 4712
36  libsystem_pthread.dylib       	       0x191023bc8 _pthread_start + 136
37  libsystem_pthread.dylib       	       0x19101eb80 thread_start + 8

(lldb) frame select 5
frame #5: 0x000000010d377c80 Electron Framework`node::modules::BindingData::GetPackageJSON(node::Realm*, std::__Cr::basic_string_view<char, std::__Cr::char_traits<char>>, node::modules::BindingData::ErrorContext*) at v8-local-handle.h:770
General Purpose Registers:
        x8 = 0x0000000000020019
       x19 = 0x0000011800091598
       x20 = 0x0000011800091560
       x21 = 0x0000011800a7dff0
       x22 = 0x0000011800a7dff0
       x23 = 0x0000011800a7c060
       x24 = 0x0000011800ae0000
       x25 = 0x0000011800121f80
       x26 = 0x0000000173bd87e0
       x27 = 0x0000000173bd8858
       x28 = 0x0000000000000064
        fp = 0x0000000173bd8d10
        lr = 0x000000010d377c80  Electron Framework`node::modules::BindingData::GetPackageJSON(node::Realm*, std::__Cr::basic_string_view<char, std::__Cr::char_traits<char>>, node::modules::BindingData::ErrorContext*) + 5852 at v8-local-handle.h
        sp = 0x0000000173bd8720
        pc = 0x000000010d377c80  Electron Framework`node::modules::BindingData::GetPackageJSON(node::Realm*, std::__Cr::basic_string_view<char, std::__Cr::char_traits<char>>, node::modules::BindingData::ErrorContext*) + 5852 at v8-local-handle.h
(lldb) disassemble
...
    0x10d37683c <+664>:  bl     0x10b07ad08    ; v8::Function::Call(v8::Local<v8::Context>, v8::Local<v8::Value>, int, v8::Local<v8::Value>*) at api.cc:5384
    0x10d376840 <+668>:  mov    x22, x0
    0x10d376844 <+672>:  cbz    x0, 0x10d377c7c ; <+5848> at v8-local-handle.h:770
    ...
    0x10d377c7c <+5848>: bl     0x10dede2c4    ; v8::api_internal::ToLocalEmpty() at api.h:217
->  0x10d377c80 <+5852>: b      0x10d376848    ; <+676> at v8-internal.h:1787
```

We are attempting to resolve an empty handle returned by `modules_read_file_sync->Call` that leads to the Api failure check. As to why this happens, with a try catch scope added

```
(v8::TryCatch) try_catch = {
  i_isolate_ = 0x0000011800ac0000
  next_ = nullptr
  exception_ = 0x000009e000020019
  message_obj_ = 0x000009e000020001
  js_stack_comparable_address_ = 6236767984
  is_verbose_ = false
  can_continue_ = false
  capture_message_ = true
  rethrow_ = false
}
(lldb) command script import src/v8/tools/lldb_commands.py
(lldb) job 0x000009e000020001
0x09e000020001 <the_hole_value>
(lldb) job 0x000009e000020019
0x09e000020019 <termination_exception>
```
</details>

We got a termination exception from V8 when the function call was executed that results in the empty handle for `result`, thread 0 shows we are in the shutdown phase which calls [isolate->TerminateExecution](https://github.com/nodejs/node/blob/4451309e99e37da7d4b44b5fb136db1c6a1dea90/src/env.cc#L1215-L1216) when the worker threads are [stopped](https://github.com/nodejs/node/blob/6706b22e350490d3e8e697377b5f388df9305aa0/src/node_worker.cc#L1344) setting thread local interrupt flags. The interrupt handler from the worker thread raises the exception when attempting to enter JS.

The change in this PR makes the JS re-entrancy resilient to this exception. cc @indutny-signal 

However, the stop call from the `Worker::Exit` is ignoring the flags we would ideally be calling for main thread of all our processes, ex: https://github.com/electron/electron/blob/1ff8e8014ab4a6e152e357b62665a7f917a6c541/shell/app/node_main.cc#L316. This should be addressed in a followup PR (possibly missed case in https://github.com/nodejs/node/pull/46583) cc @codebytere 


#### Release Notes

Notes: fix crash when attempting to resolve modules during process exit

Changed files:
filename: patches/node/api_remove_deprecated_getisolate.patch
patch: @@ -586,10 +586,10 @@ index 57e068ae249d618c2658638f9f3b03e1fedb6524..8c51ae4e0a435971c6d0288af8781087
    data_.Reset();
    return ret;
 diff --git a/src/node_modules.cc b/src/node_modules.cc
-index eea4ba313d8dbcf7b88b79f5a3e9ba2eb39d7c3e..529b7cfc15536c3fe5e7798c0f82698121751f2a 100644
+index 6b1a9e257b0ce52820838f88c44ec546068fe419..5355f2f96e9c9f6548ae43fd38b0d89a825e6b60 100644
 --- a/src/node_modules.cc
 +++ b/src/node_modules.cc
-@@ -69,7 +69,7 @@ void BindingData::Deserialize(v8::Local<v8::Context> context,
+@@ -70,7 +70,7 @@ void BindingData::Deserialize(v8::Local<v8::Context> context,
                                int index,
                                InternalFieldInfoBase* info) {
    DCHECK_IS_SNAPSHOT_SLOT(index);
@@ -598,7 +598,7 @@ index eea4ba313d8dbcf7b88b79f5a3e9ba2eb39d7c3e..529b7cfc15536c3fe5e7798c0f826981
    Realm* realm = Realm::GetCurrent(context);
    BindingData* binding = realm->AddBindingData<BindingData>(holder);
    CHECK_NOT_NULL(binding);
-@@ -696,7 +696,7 @@ void BindingData::CreatePerContextProperties(Local<Object> target,
+@@ -709,7 +709,7 @@ void BindingData::CreatePerContextProperties(Local<Object> target,
    Realm* realm = Realm::GetCurrent(context);
    realm->AddBindingData<BindingData>(target);
  

filename: patches/node/fix_ensure_traverseparent_bails_on_resource_path_exit.patch
patch: @@ -8,10 +8,10 @@ resource path. This commit ensures that the TraverseParent function
 bails out if the parent path is outside of the resource path.
 
 diff --git a/src/node_modules.cc b/src/node_modules.cc
-index cbc3283fc2d511cce2eae0048cc9bf0fa917d38a..3b4d82da2ad30cafa96611eaa2d68ddf1badeac0 100644
+index 15686c00524b6e9a31d6d27069605b1d9ebd5d38..98d4dec64c904f45ae09bf3cd8ec9b18b275bc44 100644
 --- a/src/node_modules.cc
 +++ b/src/node_modules.cc
-@@ -320,8 +320,41 @@ const BindingData::PackageConfig* BindingData::TraverseParent(
+@@ -333,8 +333,41 @@ const BindingData::PackageConfig* BindingData::TraverseParent(
      Realm* realm, const std::filesystem::path& check_path) {
    std::filesystem::path current_path = check_path;
    auto env = realm->env();
@@ -53,7 +53,7 @@ index cbc3283fc2d511cce2eae0048cc9bf0fa917d38a..3b4d82da2ad30cafa96611eaa2d68ddf
    do {
      current_path = current_path.parent_path();
  
-@@ -341,6 +374,12 @@ const BindingData::PackageConfig* BindingData::TraverseParent(
+@@ -354,6 +387,12 @@ const BindingData::PackageConfig* BindingData::TraverseParent(
        }
      }
  

filename: patches/node/fix_expose_readfilesync_override_for_modules.patch
patch: @@ -20,47 +20,66 @@ index 39803ae466fc81a6b2ff6e12093cdf2082790a9f..29ce687a03ccc8e45881f70d34e009e0
    V(performance_entry_callback, v8::Function)                                  \
    V(prepare_stack_trace_callback, v8::Function)                                \
 diff --git a/src/node_modules.cc b/src/node_modules.cc
-index 9eec93f52f0d0b2e45ae04ff357b4ced0770782f..eea4ba313d8dbcf7b88b79f5a3e9ba2eb39d7c3e 100644
+index 9eec93f52f0d0b2e45ae04ff357b4ced0770782f..6b1a9e257b0ce52820838f88c44ec546068fe419 100644
 --- a/src/node_modules.cc
 +++ b/src/node_modules.cc
-@@ -23,6 +23,7 @@ namespace modules {
+@@ -23,12 +23,14 @@ namespace modules {
  using v8::Array;
  using v8::Context;
  using v8::External;
 +using v8::Function;
  using v8::FunctionCallbackInfo;
  using v8::HandleScope;
  using v8::Integer;
-@@ -94,6 +95,7 @@ Local<Array> BindingData::PackageConfig::Serialize(Realm* realm) const {
+ using v8::Isolate;
+ using v8::Local;
+ using v8::LocalVector;
++using v8::MaybeLocal;
+ using v8::Name;
+ using v8::NewStringType;
+ using v8::Null;
+@@ -94,6 +96,7 @@ Local<Array> BindingData::PackageConfig::Serialize(Realm* realm) const {
  
  const BindingData::PackageConfig* BindingData::GetPackageJSON(
      Realm* realm, std::string_view path, ErrorContext* error_context) {
 +  auto isolate = realm->isolate();
    auto binding_data = realm->GetBindingData<BindingData>();
  
    auto cache_entry = binding_data->package_configs_.find(path.data());
-@@ -103,8 +105,36 @@ const BindingData::PackageConfig* BindingData::GetPackageJSON(
+@@ -103,8 +106,48 @@ const BindingData::PackageConfig* BindingData::GetPackageJSON(
  
    PackageConfig package_config{};
    package_config.file_path = path;
 +
 +  Local<Function> modules_read_file_sync = realm->modules_read_file_sync();
 +
-+  int read_err;
++  int read_err = -1;
    // No need to exclude BOM since simdjson will skip it.
 -  if (ReadFileSync(&package_config.raw_json, path.data()) < 0) {
 +  if (modules_read_file_sync.IsEmpty()) {
 +    read_err = ReadFileSync(&package_config.raw_json, path.data());
-+  } else {
++  } else if (realm->env()->can_call_into_js()) {
++    v8::TryCatch try_catch(isolate);
 +    Local<Value> args[] = {
 +        v8::String::NewFromUtf8(isolate, path.data()).ToLocalChecked(),
 +    };
-+    Local<Value> result = modules_read_file_sync->Call(
++    MaybeLocal<Value> maybe_result = modules_read_file_sync->Call(
 +        realm->context(),
 +        Undefined(isolate),
 +        arraysize(args),
-+        args).ToLocalChecked();
++        args);
 +
++    if (maybe_result.IsEmpty()) {
++      CHECK(try_catch.HasCaught());
++    }
++
++    if (try_catch.HasCaught()) {
++      if (!try_catch.HasTerminated())
++        try_catch.ReThrow();
++      return nullptr;
++    }
++
++    Local<Value> result = maybe_result.ToLocalChecked();
 +    if (result->IsUndefined()) {
 +      // Fallback
 +      read_err = ReadFileSync(&package_config.raw_json, path.data());
@@ -77,7 +96,7 @@ index 9eec93f52f0d0b2e45ae04ff357b4ced0770782f..eea4ba313d8dbcf7b88b79f5a3e9ba2e
      return nullptr;
    }
    simdjson::ondemand::document document;
-@@ -242,6 +272,12 @@ const BindingData::PackageConfig* BindingData::GetPackageJSON(
+@@ -242,6 +285,12 @@ const BindingData::PackageConfig* BindingData::GetPackageJSON(
    return &cached.first->second;
  }
  
@@ -90,7 +109,7 @@ index 9eec93f52f0d0b2e45ae04ff357b4ced0770782f..eea4ba313d8dbcf7b88b79f5a3e9ba2e
  void BindingData::ReadPackageJSON(const FunctionCallbackInfo<Value>& args) {
    CHECK_GE(args.Length(), 1);  // path, [is_esm, base, specifier]
    CHECK(args[0]->IsString());  // path
-@@ -635,6 +671,8 @@ void SaveCompileCacheEntry(const FunctionCallbackInfo<Value>& args) {
+@@ -635,6 +684,8 @@ void SaveCompileCacheEntry(const FunctionCallbackInfo<Value>& args) {
  void BindingData::CreatePerIsolateProperties(IsolateData* isolate_data,
                                               Local<ObjectTemplate> target) {
    Isolate* isolate = isolate_data->isolate();
@@ -99,7 +118,7 @@ index 9eec93f52f0d0b2e45ae04ff357b4ced0770782f..eea4ba313d8dbcf7b88b79f5a3e9ba2e
    SetMethod(isolate, target, "readPackageJSON", ReadPackageJSON);
    SetMethod(isolate,
              target,
-@@ -694,6 +732,8 @@ void BindingData::CreatePerContextProperties(Local<Object> target,
+@@ -694,6 +745,8 @@ void BindingData::CreatePerContextProperties(Local<Object> target,
  
  void BindingData::RegisterExternalReferences(
      ExternalReferenceRegistry* registry) {

filename: patches/node/src_use_cp_utf8_for_wide_file_names_on_win32.patch
patch: @@ -136,10 +136,10 @@ index 969e7d08086f8442bed476feaf15599b8c79db7c..e7459654401c275dfb86207831016ed7
                      std::make_error_code(std::errc::file_exists),
                      "cp",
 diff --git a/src/node_modules.cc b/src/node_modules.cc
-index 529b7cfc15536c3fe5e7798c0f82698121751f2a..cbc3283fc2d511cce2eae0048cc9bf0fa917d38a 100644
+index 5355f2f96e9c9f6548ae43fd38b0d89a825e6b60..15686c00524b6e9a31d6d27069605b1d9ebd5d38 100644
 --- a/src/node_modules.cc
 +++ b/src/node_modules.cc
-@@ -332,22 +332,24 @@ const BindingData::PackageConfig* BindingData::TraverseParent(
+@@ -345,22 +345,24 @@ const BindingData::PackageConfig* BindingData::TraverseParent(
  
      // Stop the search when the process doesn't have permissions
      // to walk upwards
@@ -172,7 +172,7 @@ index 529b7cfc15536c3fe5e7798c0f82698121751f2a..cbc3283fc2d511cce2eae0048cc9bf0f
      if (package_json != nullptr) {
        return package_json;
      }
-@@ -369,20 +371,12 @@ void BindingData::GetNearestParentPackageJSONType(
+@@ -382,20 +384,12 @@ void BindingData::GetNearestParentPackageJSONType(
  
    ToNamespacedPath(realm->env(), &path_value);