fix: use Node.js isolate setup logic in bindings (electron#24579)

* fix: use Node.js isolate setup logic in bindings

* Flags should be more process-specific

* Remove redundant isolate function setting

* Remove old SetFatalErrorHandler call

patches/node/.patches

@@ -42,3 +42,4 @@ lib_src_switch_buffer_kmaxlength_to_size_t.patch
 update_tests_after_increasing_typed_array_size.patch
 darwin_work_around_clock_jumping_back_in_time.patch
 fix_do_not_register_the_esm_loader_in_renderer_processes.patch
+fix_allow_preventing_setpromiserejectcallback.patch

patches/node/chore_sethostcleanupfinalizationgroupcallback_has_been_removed_from.patch

@@ -62,7 +62,7 @@ index c6ef9dc13ab6f1d1a778871a62a0a98a01d84ec6..222555831aa1bf0b7b29b4b46e235c98
      const CleanupHookCallback& cb) const {
    return std::hash<void*>()(cb.arg_);
 diff --git a/src/env.cc b/src/env.cc
-index c56a25f31e42aaf8bcbc0e3bb3db4f7aad0cf124..58f2372d664c4c6f8a513cb4206c24940891aa6d 100644
+index 657d711e539d81bfd03166bbaaae7f0b5db91a5f..02c5ba259c94bb160972005998007d66731d9dde 100644
 --- a/src/env.cc
 +++ b/src/env.cc
 @@ -30,7 +30,6 @@ using v8::ArrayBuffer;
@@ -73,15 +73,15 @@ index c56a25f31e42aaf8bcbc0e3bb3db4f7aad0cf124..58f2372d664c4c6f8a513cb4206c2494
  using v8::Function;
  using v8::FunctionTemplate;
  using v8::HandleScope;
-@@ -500,7 +499,6 @@ void Environment::InitializeLibuv(bool start_profiler_idle_notifier) {
+@@ -494,7 +493,6 @@ void Environment::InitializeLibuv(bool start_profiler_idle_notifier) {
        [](uv_async_t* async) {
          Environment* env = ContainerOf(
              &Environment::task_queues_async_, async);
 -        env->CleanupFinalizationGroups();
          env->RunAndClearNativeImmediates();
        });
    uv_unref(reinterpret_cast<uv_handle_t*>(&idle_prepare_handle_));
-@@ -1127,25 +1125,6 @@ void Environment::RunWeakRefCleanup() {
+@@ -1121,25 +1119,6 @@ void Environment::RunWeakRefCleanup() {
    isolate()->ClearKeptObjects();
  }
 
@@ -131,10 +131,10 @@ index b6e02a2910cd8fe5ff3a17d6e1a98b937323ae3a..c1966a9f55126bdd65d8c9d529d93497
    static int const kNodeContextTag;
 
 diff --git a/src/node.h b/src/node.h
-index 709d03145e3d5acdb67502110917e8147c275c60..a279cc7cc6205907eb5f9c3f6237513b2354f6be 100644
+index 638a1a85b046ce4db303d532f7cf36cca2271db5..b9b11b4331bd3ae4a87f65758ee09af25222e19a 100644
 --- a/src/node.h
 +++ b/src/node.h
-@@ -323,8 +323,6 @@ struct IsolateSettings {
+@@ -322,8 +322,6 @@ struct IsolateSettings {
    v8::PromiseRejectCallback promise_reject_callback = nullptr;
    v8::AllowWasmCodeGenerationCallback
        allow_wasm_code_generation_callback = nullptr;

patches/node/feat_add_flags_for_low-level_hooks_and_exceptions.patch

@@ -23,52 +23,20 @@ Environment on the V8 context of blink, so no new V8 context is created.
 
 As a result, a renderer process may have multiple Node Environments in it.
 
-diff --git a/src/env.cc b/src/env.cc
-index 657d711e539d81bfd03166bbaaae7f0b5db91a5f..c56a25f31e42aaf8bcbc0e3bb3db4f7aad0cf124 100644
---- a/src/env.cc
-+++ b/src/env.cc
-@@ -410,6 +410,12 @@ Environment::Environment(IsolateData* isolate_data,
-   // TODO(joyeecheung): deserialize when the snapshot covers the environment
-   // properties.
-   CreateProperties();
-+
-+  // TODO(addaleax): the per-isolate state should not be controlled by
-+  // a single Environment.
-+  if (g_standalone_mode) {
-+    isolate()->SetPromiseRejectCallback(task_queue::PromiseRejectCallback);
-+  }
- }
-
- Environment::~Environment() {
 diff --git a/src/node.cc b/src/node.cc
-index 4ff7824b0011685289716d61b02427c3e264965d..2a21c6dfe74b97c0d4ca8608486f8f35c77cc831 100644
+index 4ff7824b0011685289716d61b02427c3e264965d..f2b0b1585a14eaf6ffdb69a28888b42a4928f36b 100644
 --- a/src/node.cc
 +++ b/src/node.cc
-@@ -122,6 +122,9 @@ using v8::Undefined;
+@@ -122,6 +122,8 @@ using v8::Undefined;
  using v8::V8;
  using v8::Value;
 
-+bool g_standalone_mode = true;
 +bool g_upstream_node_mode = true;
 +
  namespace per_process {
 
  // node_revert.h
-@@ -337,6 +340,13 @@ MaybeLocal<Value> Environment::RunBootstrapping() {
-
-   CHECK(!has_run_bootstrapping_code());
-
-+  if (g_standalone_mode) {
-+    isolate()->AddMessageListener(errors::PerIsolateMessageListener);
-+  }
-+  if (g_upstream_node_mode) {
-+    isolate()->SetFatalErrorHandler(OnFatalError);
-+  }
-+
-   if (BootstrapInternalLoaders().IsEmpty()) {
-     return MaybeLocal<Value>();
-   }
-@@ -733,7 +743,9 @@ int InitializeNodeWithArgs(std::vector<std::string>* argv,
+@@ -733,7 +735,9 @@ int InitializeNodeWithArgs(std::vector<std::string>* argv,
    binding::RegisterBuiltinModules();
 
    // Make inherited handles noninheritable.
@@ -79,7 +47,7 @@ index 4ff7824b0011685289716d61b02427c3e264965d..2a21c6dfe74b97c0d4ca8608486f8f35
 
    // Cache the original command line to be
    // used in diagnostic reports.
-@@ -767,6 +779,8 @@ int InitializeNodeWithArgs(std::vector<std::string>* argv,
+@@ -767,6 +771,8 @@ int InitializeNodeWithArgs(std::vector<std::string>* argv,
      if (exit_code != 0) return exit_code;
    }
  #endif
@@ -88,7 +56,7 @@ index 4ff7824b0011685289716d61b02427c3e264965d..2a21c6dfe74b97c0d4ca8608486f8f35
 
    const int exit_code = ProcessGlobalArgs(argv,
                                            exec_argv,
-@@ -811,6 +825,7 @@ int InitializeNodeWithArgs(std::vector<std::string>* argv,
+@@ -811,6 +817,7 @@ int InitializeNodeWithArgs(std::vector<std::string>* argv,
    }
    per_process::metadata.versions.InitializeIntlVersions();
  #endif
@@ -97,15 +65,14 @@ index 4ff7824b0011685289716d61b02427c3e264965d..2a21c6dfe74b97c0d4ca8608486f8f35
    NativeModuleEnv::InitializeCodeCache();
 
 diff --git a/src/node.h b/src/node.h
-index 886216e2cb533e7337bc4f6816e2de796f64f81e..19d5eff164a543a38b4c77f99c2f15c30a226f77 100644
+index 886216e2cb533e7337bc4f6816e2de796f64f81e..8378553f28671e4685b4ed20279b2be5d202e527 100644
 --- a/src/node.h
 +++ b/src/node.h
-@@ -211,6 +211,9 @@ namespace node {
+@@ -211,6 +211,8 @@ namespace node {
 
  class IsolateData;
  class Environment;
 +// Whether node should open some low level hooks.
-+NODE_EXTERN extern bool g_standalone_mode;
 +NODE_EXTERN extern bool g_upstream_node_mode;
 
  // TODO(addaleax): Officially deprecate this and replace it with something

patches/node/fix_allow_preventing_setpromiserejectcallback.patch

@@ -0,0 +1,50 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: Shelley Vohr <shelley.vohr@gmail.com>
+Date: Wed, 15 Jul 2020 18:43:32 -0700
+Subject: fix: allow preventing SetPromiseRejectCallback
+
+We do not want to use the promise rejection callback that Node.js uses,
+because it does not send PromiseRejectionEvents to the global script context.
+We need to use the one Blink already provides, and so we need to
+slightly augment IsolateSettings to allow for that.
+
+diff --git a/src/api/environment.cc b/src/api/environment.cc
+index 21980987644c6e83029157785dea463070456c77..20d9f91dcf6b5def05a706cf3389d64e9edbcf3e 100644
+--- a/src/api/environment.cc
++++ b/src/api/environment.cc
+@@ -241,9 +241,11 @@ void SetIsolateMiscHandlers(v8::Isolate* isolate, const IsolateSettings& s) {
+     s.allow_wasm_code_generation_callback : AllowWasmCodeGenerationCallback;
+   isolate->SetAllowWasmCodeGenerationCallback(allow_wasm_codegen_cb);
+
+-  auto* promise_reject_cb = s.promise_reject_callback ?
+-    s.promise_reject_callback : task_queue::PromiseRejectCallback;
+-  isolate->SetPromiseRejectCallback(promise_reject_cb);
++  if (s.flags & SHOULD_SET_PROMISE_REJECTION_CALLBACK) {
++    auto* promise_reject_cb = s.promise_reject_callback ?
++      s.promise_reject_callback : task_queue::PromiseRejectCallback;
++    isolate->SetPromiseRejectCallback(promise_reject_cb);
++  }
+
+   if (s.flags & DETAILED_SOURCE_POSITIONS_FOR_PROFILING)
+     v8::CpuProfiler::UseDetailedSourcePositionsForProfiling(isolate);
+diff --git a/src/node.h b/src/node.h
+index b9b11b4331bd3ae4a87f65758ee09af25222e19a..60ecc3bd3499c23b297bc11e7f052aede20520c2 100644
+--- a/src/node.h
++++ b/src/node.h
+@@ -304,12 +304,14 @@ class NODE_EXTERN MultiIsolatePlatform : public v8::Platform {
+
+ enum IsolateSettingsFlags {
+   MESSAGE_LISTENER_WITH_ERROR_LEVEL = 1 << 0,
+-  DETAILED_SOURCE_POSITIONS_FOR_PROFILING = 1 << 1
++  DETAILED_SOURCE_POSITIONS_FOR_PROFILING = 1 << 1,
++  SHOULD_SET_PROMISE_REJECTION_CALLBACK = 1 << 2
+ };
+
+ struct IsolateSettings {
+   uint64_t flags = MESSAGE_LISTENER_WITH_ERROR_LEVEL |
+-      DETAILED_SOURCE_POSITIONS_FOR_PROFILING;
++      DETAILED_SOURCE_POSITIONS_FOR_PROFILING | 
++      SHOULD_SET_PROMISE_REJECTION_CALLBACK;
+   v8::MicrotasksPolicy policy = v8::MicrotasksPolicy::kExplicit;
+
+   // Error handling callbacks

patches/node/fix_expose_tracing_agent_and_use_tracing_tracingcontroller_instead.patch

@@ -20,7 +20,7 @@ index 09d71b34581268bfe17c3182029cb3949d857d48..60d30b7eff7329c4235024c315251072
      int thread_pool_size,
      node::tracing::TracingController* tracing_controller) {
 diff --git a/src/node.h b/src/node.h
-index 19d5eff164a543a38b4c77f99c2f15c30a226f77..709d03145e3d5acdb67502110917e8147c275c60 100644
+index 8378553f28671e4685b4ed20279b2be5d202e527..638a1a85b046ce4db303d532f7cf36cca2271db5 100644
 --- a/src/node.h
 +++ b/src/node.h
 @@ -116,6 +116,7 @@ namespace node {
@@ -31,7 +31,7 @@ index 19d5eff164a543a38b4c77f99c2f15c30a226f77..709d03145e3d5acdb67502110917e814
  class TracingController;
 
  }
-@@ -387,6 +388,8 @@ NODE_EXTERN Environment* GetCurrentEnvironment(v8::Local<v8::Context> context);
+@@ -386,6 +387,8 @@ NODE_EXTERN Environment* GetCurrentEnvironment(v8::Local<v8::Context> context);
  // it returns nullptr.
  NODE_EXTERN MultiIsolatePlatform* GetMainThreadMultiIsolatePlatform();
 

shell/app/node_main.cc

@@ -167,10 +167,6 @@ int NodeMain(int argc, char* argv[]) {
     feature_list->InitializeFromCommandLine("", "");
     base::FeatureList::SetInstance(std::move(feature_list));
 
-    // We do not want to double-set the error level and promise rejection
-    // callback.
-    node::g_standalone_mode = false;
-
     // Explicitly register electron's builtin modules.
     NodeBindings::RegisterBuiltinModules();
 

shell/common/api/electron_bindings.cc

@@ -33,17 +33,6 @@
 
 namespace electron {
 
-namespace {
-
-// Called when there is a fatal error in V8, we just crash the process here so
-// we can get the stack trace.
-void FatalErrorCallback(const char* location, const char* message) {
-  LOG(ERROR) << "Fatal error in V8: " << location << " " << message;
-  ElectronBindings::Crash();
-}
-
-}  // namespace
-
 ElectronBindings::ElectronBindings(uv_loop_t* loop) {
   uv_async_init(loop, &call_next_tick_async_, OnCallNextTick);
   call_next_tick_async_.data = this;
@@ -86,8 +75,6 @@ void ElectronBindings::BindProcess(v8::Isolate* isolate,
 
 void ElectronBindings::BindTo(v8::Isolate* isolate,
                               v8::Local<v8::Object> process) {
-  isolate->SetFatalErrorHandler(FatalErrorCallback);
-
   gin_helper::Dictionary dict(isolate, process);
   BindProcess(isolate, &dict, metrics_.get());
 

shell/common/node_bindings.cc

@@ -221,13 +221,6 @@ void SetNodeOptions(base::Environment* env) {
   }
 }
 
-bool AllowWasmCodeGenerationCallback(v8::Local<v8::Context> context,
-                                     v8::Local<v8::String>) {
-  v8::Local<v8::Value> wasm_code_gen = context->GetEmbedderData(
-      node::ContextEmbedderIndex::kAllowWasmCodeGeneration);
-  return wasm_code_gen->IsUndefined() || wasm_code_gen->IsTrue();
-}
-
 }  // namespace
 
 namespace electron {
@@ -308,7 +301,6 @@ bool NodeBindings::IsInitialized() {
 void NodeBindings::Initialize() {
   TRACE_EVENT0("electron", "NodeBindings::Initialize");
   // Open node's error reporting system for browser process.
-  node::g_standalone_mode = browser_env_ == BrowserEnvironment::BROWSER;
   node::g_upstream_node_mode = false;
 
 #if defined(OS_LINUX)
@@ -403,28 +395,31 @@ node::Environment* NodeBindings::CreateEnvironment(
     global.Delete("_noBrowserGlobals");
   }
 
+  node::IsolateSettings is;
+
   if (browser_env_ == BrowserEnvironment::BROWSER) {
-    // This policy requires that microtask checkpoints be explicitly invoked.
-    // Node.js requires this.
-    context->GetIsolate()->SetMicrotasksPolicy(v8::MicrotasksPolicy::kExplicit);
+    // Node.js requires that microtask checkpoints be explicitly invoked.
+    is.policy = v8::MicrotasksPolicy::kExplicit;
   } else {
     // Match Blink's behavior by allowing microtasks invocation to be controlled
     // by MicrotasksScope objects.
-    context->GetIsolate()->SetMicrotasksPolicy(v8::MicrotasksPolicy::kScoped);
+    is.policy = v8::MicrotasksPolicy::kScoped;
+
+    // We do not want to use Node.js' message listener as it interferes with
+    // Blink's.
+    is.flags &= ~node::IsolateSettingsFlags::MESSAGE_LISTENER_WITH_ERROR_LEVEL;
+
+    // We do not want to use the promise rejection callback that Node.js uses,
+    // because it does not send PromiseRejectionEvents to the global script
+    // context. We need to use the one Blink already provides.
+    is.flags &=
+        ~node::IsolateSettingsFlags::SHOULD_SET_PROMISE_REJECTION_CALLBACK;
   }
 
   // This needs to be called before the inspector is initialized.
   env->InitializeDiagnostics();
 
-  // Set the callback to invoke to check if wasm code generation should be
-  // allowed.
-  context->GetIsolate()->SetAllowWasmCodeGenerationCallback(
-      AllowWasmCodeGenerationCallback);
-
-  // Generate more detailed source positions to code objects. This results in
-  // better results when mapping profiling samples to script source.
-  v8::CpuProfiler::UseDetailedSourcePositionsForProfiling(
-      context->GetIsolate());
+  node::SetIsolateUpForNode(context->GetIsolate(), is);
 
   gin_helper::Dictionary process(context->GetIsolate(), env->process_object());
   process.SetReadOnly("type", process_type);