fix: use Node.js isolate setup logic in bindings

[codebytere]

Description of Change

Closes #24577.

This fixes an issue whereby any code in Node.js that invokes various assert functionality in the browser or renderer processes will throw with TypeError: call.getFileName is not a function. The reason this happens is that Node.js overrides the Promise rejection callback used by the Isolate, so the WeakMap used in their assert logic is never populated.

To remedy this, the best long-term solution, I feel, is to better align our isolate setup functionality with that available to us via the embedder-focused SetIsolateUpForNode. We already approximated a lot of this functionality, so this allows us to remove that. It does, however create a small new issue wherein we do not want to use the promise rejection callback that Node.js uses in the renderer process, because it does not send PromiseRejectionEvents to the global script context. This causes code like:

window.addEventListener('unhandledrejection', function (event) {
  // ...your code here to handle the unhandled rejection...

  // Prevent the default handling (such as outputting the
  // error to the console)

  event.preventDefault();
});

to fail since the 'unhandledrejection' rejection event would never be fired. We need thus to use the one Blink already provides, and to augment IsolateSettings accordingly.

I've upstreamed this at nodejs/node#34387.

cc @nornagon @zcbenz @MarshallOfSound

Checklist

• PR description included and stakeholders cc'd
• npm test passes
• PR title follows semantic commit guidelines
• PR release notes describe the change in a way relevant to app developers, and are capitalized, punctuated, and past tense.
• This is NOT A BREAKING CHANGE. Breaking changes may not be merged to master until 11-x-y is branched.

Release Notes

Notes: Fixed an issue where many uses of the Node.js assert module would throw in both the browser and renderer processes.

[deepak1556]

Couple of question,

• The reason this happens is that Node.js overrides the Promise rejection callback used by the Isolate,

AFAIK in the browser process we setup the callbacks in https://github.com/electron/electron/blob/master/patches/node/feat_add_flags_for_low-level_hooks_and_exceptions.patch as g_standalone_mode and in the renderer blink does it. Where does node override here ? Maybe the problem/solution statement is a bit confusing, can you please elaborate whats happening ?

• Also there is no blink in the browser process, so why are those isolate settings being removed for the browser process too ?

• We setup our own fatal error handlers for all the process in https://github.com/electron/electron/blob/master/shell/common/api/electron_bindings.cc#L89 , now will that be overriden by the handler from SetIsolateUpForNode ?

[codebytere]

@deepak1556 you're totally right - we can remove a lot of that low-level flag logic now, as it's just being redundantly set/called twice 😅

I've also moved the flags into process-specific locations. With regard to:

AFAIK in the browser process we setup the callbacks in https://github.com/electron/electron/blob/master/patches/node/feat_add_flags_for_low-level_hooks_and_exceptions.patch as g_standalone_mode and in the renderer blink does it

I removed the invocation in the patch, so it's now just disabled for the renderer process where Blink does it.

We setup our own fatal error handlers for all the process in https://github.com/electron/electron/blob/master/shell/common/api/electron_bindings.cc#L89 , now will that be overriden by the handler from SetIsolateUpForNode ?

Yes - we can remove this now as the embedder interface handles it.

[MarshallOfSound]

I think we should avoid letting node control blinks isolate, can't we fix the assert module to work without nodes hooks? Isolate wide modifications are bad in the renderer as not every context in the given isolate are actually node contexts. So this can have unexpected effects / context leakage in the renderer process.

[codebytere]

tl;dr the comparable surface area of difference is small enough, and the net benefit great enough of using embedding functionality provided to us, that I think choosing to leverage this functionality is a better choice than to call the vast majority of it all ourselves in the exact same way Node.js would :)

A more in-depth breakdown:

The embedder interface this change leverages, SetIsolateUpForNode, calls 8 discrete pieces of isolate functionality:

isolate->AddMessageListenerWithErrorLevel( errors::PerIsolateMessageListenerg);
isolate->SetAbortOnUncaughtExceptionCallback(abort_callback);
isolate->SetFatalErrorHandler(fatal_error_cb);
isolate->SetPrepareStackTraceCallback(prepare_stack_trace_cb);
isolate->SetMicrotasksPolicy(s.policy);
isolate->SetAllowWasmCodeGenerationCallback(allow_wasm_codegen_cb);
v8::CpuProfiler::UseDetailedSourcePositionsForProfiling(isolate);

Renderer Isolate Setup Prior to Change

 isolate->SetMicrotasksPolicy(s.policy);
 isolate->SetAllowWasmCodeGenerationCallback(allow_wasm_codegen_cb);
 v8::CpuProfiler::UseDetailedSourcePositionsForProfiling(isolate);
 isolate->AddMessageListenerWithErrorLevel(errors::PerIsolateMessageListeners);

Renderer Isolate Setup After Change

isolate->SetMicrotasksPolicy(s.policy);
isolate->SetAllowWasmCodeGenerationCallback(allow_wasm_codegen_cb);
v8::CpuProfiler::UseDetailedSourcePositionsForProfiling(isolate);
isolate->AddMessageListenerWithErrorLevel( errors::PerIsolateMessageListeners);
isolate->SetAbortOnUncaughtExceptionCallback(abort_callback);
isolate->SetFatalErrorHandler(fatal_error_cb);

That leaves the only difference consequent to this PR in the Node.js-Blink isolate relationship as:


isolate->SetAbortOnUncaughtExceptionCallback(abort_callback);
isolate->SetFatalErrorHandler(fatal_error_cb);



We choose to call isolate->SetFatalErrorHandler(fatal_error_cb) ourselves in Node bindings and bind it to the following function:


void FatalErrorCallback(const char* location, const char* message) {
  LOG(ERROR) << "Fatal error in V8: " << location << " " << message;
  ElectronBindings::Crash();
}




Which compares with the one we’d now be invoking with Node.js:


void OnFatalError(const char* location, const char* message) {
  if (location) {
    FPrintF(stderr, "FATAL ERROR: %s %s\n", location, message);
  } else {
    FPrintF(stderr, "FATAL ERROR: %s\n", message);
  }

  Isolate* isolate = Isolate::GetCurrent();
  Environment* env = Environment::GetCurrent(isolate);
  bool report_on_fatalerror;
  {
    Mutex::ScopedLock lock(node::per_process::cli_options_mutex);
    report_on_fatalerror = per_process::cli_options->report_on_fatalerror;
  }

  if (report_on_fatalerror) {
    report::TriggerNodeReport(
        isolate, env, message, "FatalError", "", Local<Object>());
  }

  fflush(stderr);
  ABORT();
}



Stripping out cli functionality we already prevent in the renderer process:


void OnFatalError(const char* location, const char* message) {
  if (location) {
    FPrintF(stderr, "FATAL ERROR: %s %s\n", location, message);
  } else {
    FPrintF(stderr, "FATAL ERROR: %s\n", message);
  }

  fflush(stderr);
  ABORT();
}



This introduces no side effects and is a comparable output, I would say.

Lastly, we’re left with: isolate->SetAbortOnUncaughtExceptionCallback(abort_callback);


Blink currently never calls this. This callback is used explicitly by embedders to help V8 determine if it should abort when it throws and no internal handler is predicted to catch the exception.

Node.js currently chooses to do this if:

1) The user has passed the —abort-on-uncaught-exception flag, which is irrelevant to is in the renderer as we disallow it

2) The user is in a scope where uncaught exceptions should not result in an abort; this only applies in the context of the Node.js ESM loader which is also already irrelevant in a renderer context as we disallow it in the renderer process as of #24301.


The only other case resulting in an abort in Node.js’ callback is when the Environment itself is stopping. As such, I personally feel comfortable allowing this setup functionality to run, but I would definitely be open to perhaps upstreaming a toggle to prevent calling SetAbortOnUncaughtExceptionCallback to Node.js.



oops accidental approval, the fatal error handler set in bindings has not been removed.

[MarshallOfSound]

Approving given the wonderful explanation, but I would like to err on the side of not backporting this as I'm still quite fearful 🤔

[release-clerk]

Release Notes Persisted

Fixed an issue where many uses of the Node.js assert module would throw in both the browser and renderer processes.