src: don't run bootstrapper in CreateEnvironment

[codebytere]

Refs electron/electron#22342.

#26788 added a call to prepareMainThreadExecution to CreateEnvironment, which would prepare the main thread for execution any time an embedder created a new environment. However, this caused an unfortunate doubling-up effect, and meant that Electron would see stuff like:

> process.emit('warning', new Error('warn'))
(node:55797) Error: warn
(node:55797) Error: warn

Since both execution pathways (env and whatever the actual execution was) were being exercised. To fix this, I believe we should remove bootstrapping code from CreateEnvironment.

cc @joyeecheung@addaleax

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• commit message follows commit guidelines

[gireeshpunathil]

where is the other bootstrapping that is happening?

[codebytere]

@gireeshpunathil:

node/src/node.cc

Lines 404 to 448 in 84b8857

	MaybeLocal<Value> StartMainThreadExecution(Environment* env) {
	// To allow people to extend Node in different ways, this hook allows
	// one to drop a file lib/_third_party_main.js into the build
	// directory which will be executed instead of Node's normal loading.
	if (NativeModuleEnv::Exists("_third_party_main")) {
	returnStartExecution(env, "internal/main/run_third_party_main");
	}
	std::string first_argv;
	if (env->argv().size() > 1) {
	first_argv = env->argv()[1];
	}
	if (first_argv == "inspect" || first_argv == "debug") {
	returnStartExecution(env, "internal/main/inspect");
	}
	if (per_process::cli_options->print_help) {
	returnStartExecution(env, "internal/main/print_help");
	}
	if (env->options()->prof_process) {
	returnStartExecution(env, "internal/main/prof_process");
	}
	// -e/--eval without -i/--interactive
	if (env->options()->has_eval_string && !env->options()->force_repl) {
	returnStartExecution(env, "internal/main/eval_string");
	}
	if (env->options()->syntax_check_only) {
	returnStartExecution(env, "internal/main/check_syntax");
	}
	if (!first_argv.empty() && first_argv != "-") {
	returnStartExecution(env, "internal/main/run_main_module");
	}
	if (env->options()->force_repl || uv_guess_handle(STDIN_FILENO) == UV_TTY) {
	returnStartExecution(env, "internal/main/repl");
	}
	returnStartExecution(env, "internal/main/eval_stdin");
	}

See that all those js files call prepareMainThreadExecution(true).

[himself65]

related: #31909

[joyeecheung]

By the way, I think a quick band-aid fix to this issue without introducing breaking changes would be to maintain something like isInitialized in pre_execution.js for prepareMainThreadExecution() and return early there if it's set to true. It's not ideal but would lead to less impact.

[addaleax]

LGTM

#30467 contains a non-breaking version of this along the lines of what Joyee suggested, but given that I don’t see how the current state is usable, I’m good with landing this as a semver-patch change.

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[codebytere]

Landed in 65e18a8

[addaleax]

🎉 Thanks for landing this!