worker: don't use two-arg NewIsolate

[codebytere]

Electron creates and initializes the V8 platform ourselves, and so need to build with NODE_USE_V8_PLATFORM set to false and as a result were unable to use worker_threads.

This PR fixes this problem by swapping the two-arg call to NewIsolate present in NodeWorkerData for the three-arg version.

Before, the two-arg version calls the three-arg version with GetMainThreadMultiIsolatePlatform(), which is implemented with per_process::v8_platform.Platform(). That returns nullptr when NODE_USE_V8_PLATFORM set to false, and causes a bad access crash.

cc @joyeecheung @addaleax

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• commit message follows commit guidelines

[jasnell]

Generally LGTM with @bnoordhuis comments addressed

[addaleax]

How is Electron initiating Node.js here? The NewIsolate() and CreateIsolateData() variants that take a MultiIsolatePlatform* argument are kind of supposed to solve this problem, but it seems Electron is doing something else?

If we do land this, I’d like to make it clear from the beginning that this is a temporary hack to support Electron, and maybe deprecate it immediately.

[codebytere]

@addaleax when ELECTRON_RUN_AS_NODE is set we call NodeMain here in node_main.cc which then touches the bits relevant to this PR starting here by creating a new js env in javascript_environment.cc

[addaleax]

@codebytere I’m surprised … that code is missing a deallocation corresponding to node::CreateIsolateData(gin_env.isolate(), loop, gin_env.platform()), but otherwise it seems like it should already be working fine – i.e. this might need a bugfix, rather than adding a new API.

Is there an Electron bug report corresponding to this/Can you maybe share more info about the crash you’re referring to in the PR description itself (e.g. a reproduction/stack trace)? Otherwise I’m also happy to try and build Electron myself… :)

[addaleax]

@codebytere Maybe for more context, what I’m thinking is that, at least once the Node.js Environment is set up, all attempts to access the underlying platform should use env->isolate_data()->platform() and check for nullptr, and since it sounds like that’s not the case here, finding out whether and where there is an invalid access to the global per_process::v8_platform object might be a better approach.

[codebytere]

I think the thing i'm not quite sure on at the moment is how this might have worked previously - doesn't per_process::v8_platform.Platform(); return nullptr by design if Node hasn't been initialized with NODE_USE_V8_PLATFORM? That was the root of our issues that i could tell to begin with 🤔

The crash is just that - it fails here @ L233 with EXC_BAD_ACCESS because platform is nullptr from GetMainThreadMultiIsolatePlatform() and we're calling RegisterIsolate on it. Since all the initialization code is guarded by NODE_USE_V8_PLATFORM, is there another viable way to rework that getter without having added a setter that's not guarded in the same way?

[addaleax]

@codebytere Yes, but I’d say that per_process::v8_platform.Platform() shouldn’t be used in the embedder case on the Node.js side at all? Is there a specific call to it that results in nullptr being returned when it shouldn’t?

As far as I can tell, it is only used in Node.js when running Node.js through int node::Start(int argc, char** argv), but Electron doesn’t do that, right?

[codebytere]

In WorkerThreadData here, we allocate a new Isolate, which calls NewIsolate, which then calls GetMainThreadMultiIsolatePlatform(), which is implemented with per_process::v8_platform.Platform(). That returns nullptr for the above reasons, so then when that's passed to the three-arg impl of NewIsolate we see the crash

[addaleax]

@codebytere Thanks! I think that’s a clear oversight (and most likely on my part, sorry). Does this help?

diff --git a/src/node_worker.cc b/src/node_worker.cc
index 11e44a92757e..3dce5e25980c 100644
--- a/src/node_worker.cc
+++ b/src/node_worker.cc
@@ -116,7 +116,10 @@ class WorkerThreadData {
     : w_(w) {
     CHECK_EQ(uv_loop_init(&loop_), 0);
 
-    Isolate* isolate = NewIsolate(w->array_buffer_allocator_.get(), &loop_);
+    Isolate* isolate = NewIsolate(
+        w->array_buffer_allocator_.get(),
+        &loop_,
+        w->platform_);
     CHECK_NOT_NULL(isolate);
 
     {

[codebytere]

Ah, awesome! That's a far simpler fix so i'm all for it haha

I'll update this PR with that simpler change as soon as i verify :D

[codebytere]

@addaleax updated and verified in Electron ⭐️

Thanks for the assist!

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/25878/

[bnoordhuis]

@devnexen @jasnell FYI, this PR got a complete makeover.

[Trott]

Landed in 6db45bf