fix: window.open causing occasional Node.js crashes

[codebytere]

Description of Change

Closes #37404.
Closes #36858.

Refs #35999

Fixes an issue where window.open can interfere with various aspects of Node.js functionality.

Node.js stores IsolateData internally on the Environment as a member, whereas Blink stores it using V8PerIsolateData. This creates a situation where, in the renderer process, a previous IsolateData could be used for a new v8::Context, which then created a schism in the intended connection between the two. This would, for example, be a problem when using the vm module, because ContextifyScript::InstanceOf would try to check this condition from the IsolateData which was mismatched. We fix this by adding a new embedder index and associating it with the context, so that we ensure the IsolateData Node.js is using is correctly associated with the current V8::Context.

Checklist

• PR description included and stakeholders cc'd
• npm test passes
• tests are changed or added
• PR release notes describe the change in a way relevant to app developers, and are capitalized, punctuated, and past tense.

Release Notes

Notes: Fixed an issue where window.open can interfere with various aspects of Node.js functionality.

[deepak1556]

This creates a situation where, in the renderer process, a previous IsolateData could be used for a new v8::Context,

I am a bit confused by this statement, IsolateData is meant to be associated with a v8::Isolate and for both Node.js, Blink it is one per thread. The only difference is Blink stores the data on a particular slot in the isolate via v8::Isolate::SetData while Node.js expects the Environment class to hold it. A thread can have multiple contexts, so what would be a previous IsolateData ?

I am just going by the test case here, looks like window.open triggers a new context creation on the main thread in the main render frame and we end up creating a new node environment due to the following block

electron/shell/renderer/electron_renderer_client.cc

Lines 88 to 93 in 678d1aa

	// Setup node environment for each window.
	v8::Maybe<bool> initialized = node::InitializeContext(renderer_context);
	CHECK(!initialized.IsNothing() && initialized.FromJust());
	node::Environment* env =
	node_bindings_->CreateEnvironment(renderer_context, nullptr);

which then causes the vm api in the next line to crash because the old node Environment and its corresponding isolate data has been invalidated ?

In that case, wouldn't a simpler fix be to not create the node environment if there is one already existing for a given render frame ? I find it a bit weird that we are retrieving isolate data from a context rather than from the environment.

[zcbenz]

I think #37404 and #36858 are essentially different bugs and should be fixed separately.

#37404 consists of 2 bugs:

1. Calling vm.runInContext after window.open would throw an exception from Node;
2. Throwing an exception in a vm context would crash the exception handler in blink.

#36858 is a race condition that, when closing window one node env marks the isolate as disallow_js during cleanup, while another env node still runs code in the same isolate, which triggers an assertion.

[deepak1556]

@zcbenz here is what we found while debugging this PR,

Today Electron allows creation of multiple Node::Environment per thread, this can happen in the following situations

1. When nodeIntegration: true for main BrowserWindow, we create an environment for the main frame and then when a window.open call happens another environment is created via ElectronRendererClient::DidCreateScriptContext which gets associated to the child window proxy context.

2. When nodeIntegration: true for main BrowserWindow and also nodeIntegrationInSubFrame: true, we create an environment for the main frame and when a same origin iframe gets created another environment is created in the same thread via ElectronRendererClient::DidCreateScriptContext which gets associated to the iframe window context.

Apart from the above two we allow creating one Node::Environment for every web worker thread which is expected and the lifetime of the environment is associated with the worker thread in this case. But in the above two situations the lifetime of the environment is associated with the v8::Context since multiple context got created on the same main thread.

On the contrary, in Node.js at a given time only a single Node::Environment is associated with a given thread. For the main thread, the environment is associated with NodeMainInstance::CreateEnvironment and for every Node.js worker thread the environment gets associated with WorkerThreadData. When there are multiple contexts created per thread, in the case of using vm module the created context gets associated with an existing environment on the thread via Environment::AssignToContext instead of creating a new environment.

Given the current implementation in Electron leads to the race situation where destructing one environment can disallow JS that triggers #36858 and also in cases where vm module gets used the destruction of an environment can cause the incorrect association of IsolateData. I am proposing that we mimic the behavior of Node.js and only allow creation of a single environment per thread. I am also unsure what would be the purpose of having multiple environments per thread since that is not expected today in Node.js. Thoughts ?

Tl:dr; I think this PR is intended to address Calling vm.runInContext after window.open would throw an exception from Node; and also the core of the change will address #36858

[zcbenz]

If we share the Node Environment across multiple contexts, will modules only load in the first context?

[release-clerk]

Release Notes Persisted

Fixed an issue where window.open can interfere with various aspects of Node.js functionality.

[trop]

I was unable to backport this PR to "24-x-y" cleanly;
you will need to perform this backport manually.

[trop]

I was unable to backport this PR to "26-x-y" cleanly;
you will need to perform this backport manually.

[trop]

I was unable to backport this PR to "25-x-y" cleanly;
you will need to perform this backport manually.

[gpetrov]

@codebytere what is the status here? Seems not really merged with the main - are there still any issues? Like with the Appveyor?

[panuhorsmalahti]

So is this in 26.x but not in 25.x or earlier?

[KochiyaOcean]

Is there any plan to backport this fix to the previous version? 26.x is still not stable and the version below 25 still crashes

[gpetrov]

Well the fix is still not merged fully, we all hope that @codebytere manage to merge it soon to all supported versions as it is desperately needed

[gpetrov]

@codebytere any news on this merge as it seems it isn't even included in the final electron 26.

We really desperately need it and can't move on electron 23+ because of this issue...

[gpetrov]

@zcbenz could you please assist in merging this?

[gpetrov]

@codebytere could you please continue working on this issue as it isn't merged yet. It is essential to get this fixed so we can move from Electron 22 to newer versions.

[erikjalevik]

Thanks to @deepak1556 and the other participants in this thread for shedding light on some obscure issues that have been plaguing my app since the beginning. Since the app is strictly desktop only, and quite a heavy user of Node fs APIs, I have nodeIntegration enabled in renderer windows.

Pre-Electron 13, the issues mostly surfaced as strange Node failures and hangs after a window reload, especially around child windows and iframes. But they could mostly be worked around by fiddling with the (now deprecated) app.allowRendererProcessReuse flag.

After finally managing to refactor away remote usage, I managed to upgrade through to version 29, and initially everything seemed to work fine, including window reloads and opening child windows. However, after some more extensive testing, I've sadly concluded that the frequency of renderer crashes seen just after closing a child window opened with window.open, is currently a blocker for releasing a new version. Based on the stack traces and console output, it seems very likely that this is caused by the same underlying issue as #36858.

Therefore, I would like to ask if you can think of any other workarounds except avoiding use of window.open altogether? Since it's a race condition, would it maybe be possible to reliably work around it for now by somehow timing any Node interactions differently around the point of closing child windows?