Skip to content

fix: node::loader::ImportModuleDynamically crash#41491

Merged
jkleinsc merged 2 commits into28-x-yfrom
fix-hostimportcrash
Mar 1, 2024
Merged

fix: node::loader::ImportModuleDynamically crash#41491
jkleinsc merged 2 commits into28-x-yfrom
fix-hostimportcrash

Conversation

@codebytere
Copy link
Copy Markdown
Member

@codebytere codebytere commented Mar 1, 2024

Description of Change

Refs nodejs/node#48510.
Resolves microsoft/vscode#206630

Fixes a crash in Node.js when dynamically importing inside of a Function. When running the scripts sequentially, the Local<Script> created at src/node_contextify.cc can outlive the ContextifyScript so ImportModuleDynamically may crash because the ContextifyScript is released and the access can be invalid.

Stacktrace

Electron Framework
node::loader::ImportModuleDynamically(v8::Local<v8::Context>, v8::Local<v8::Data>, v8::Local<v8::Value>, v8::Local<v8::String>, v8::Local<v8::FixedArray>) module_wrap.cc
Electron Framework
v8::internal::Isolate::RunHostImportModuleDynamicallyCallback(v8::internal::MaybeHandle<v8::internal::Script>, v8::internal::Handle<v8::internal::Object>, v8::internal::MaybeHandle<v8::internal::Object>) isolate.cc:5362
Electron Framework
v8::internal::Runtime_DynamicImportCall(int, unsigned long*, v8::internal::Isolate*) runtime-module.cc:38
Electron Framework
v8::internal::(anonymous namespace)::Invoke(v8::internal::Isolate*, v8::internal::(anonymous namespace)::InvokeParams const&) simulator.h:178
Electron Framework
v8::internal::Execution::TryRunMicrotasks(v8::internal::Isolate*, v8::internal::MicrotaskQueue*) execution.cc:601
Electron Framework
v8::internal::MicrotaskQueue::PerformCheckpoint(v8::Isolate*) microtask-queue.cc:175
Electron Framework
v8::internal::Execution::Call(v8::internal::Isolate*, v8::internal::Handle<v8::internal::Object>, v8::internal::Handle<v8::internal::Object>, int, v8::internal::Handle<v8::internal::Object>*) simulator.h:178
Electron Framework
node::InternalCallbackScope::Close() callback.cc:164
Electron Framework
node::InternalMakeCallback(node::Environment*, v8::Local<v8::Object>, v8::Local<v8::Object>, v8::Local<v8::Function>, int, v8::Local<v8::Value>*, node::async_context) callback.cc:223
Electron Framework
node::AsyncWrap::MakeCallback(v8::Local<v8::Function>, int, v8::Local<v8::Value>*) async_wrap.cc:664
Electron Framework
node::StreamBase::CallJSOnreadMethod(long, v8::Local<v8::ArrayBuffer>, unsigned long, node::StreamBase::StreamBaseJSChecks) stream_base.cc:476
Electron Framework
node::EmitToJSStreamListener::OnStreamRead(long, uv_buf_t const&) stream_base.cc:666
Electron Framework
node::LibuvStreamWrap::OnUvRead(long, uv_buf_t const*) stream_base-inl.h:79
Electron Framework
node::LibuvStreamWrap::ReadStart()::$_1::__invoke(uv_stream_s*, long, uv_buf_t const*) stream_wrap.cc:213
Electron Framework
uv__stream_io stream.c:1201
Electron Framework
uv__stream_io stream.c:1201
Electron Framework
uv__io_poll kqueue.c:390
Electron Framework
uv_run core.c:406
Electron Framework
electron::NodeBindings::UvRunOnce() node_bindings.cc:763
Electron Framework
base::sequence_manager::internal::ThreadControllerWithMessagePumpImpl::DoWorkImpl(base::LazyNow*) task_annotator.h:89
Electron Framework
non-virtual thunk to base::sequence_manager::internal::ThreadControllerWithMessagePumpImpl::DoWork() thread_controller_with_message_pump_impl.cc:345
Electron Framework
base::MessagePumpDefault::Run(base::MessagePump::Delegate*) message_pump_default.cc:40
Electron Framework
base::sequence_manager::internal::ThreadControllerWithMessagePumpImpl::Run(bool, base::TimeDelta) thread_controller_with_message_pump_impl.cc:645
Electron Framework
base::RunLoop::Run(base::Location const&) run_loop.cc:134
Electron Framework
content::UtilityMain(content::MainFunctionParams) utility_main.cc:409
Electron Framework
content::RunOtherNamedProcessTypeMain(std::__Cr::basic_string<char, std::__Cr::char_traits<char>, std::__Cr::allocator<char>> const&, content::MainFunctionParams, content::ContentMainDelegate*) content_main_runner_impl.cc:775
Electron Framework
content::ContentMainRunnerImpl::Run() content_main_runner_impl.cc:1152
Electron Framework
content::RunContentProcess(content::ContentMainParams, content::ContentMainRunner*) content_main.cc:330
Electron Framework
content::ContentMain(content::ContentMainParams) content_main.cc:347
Electron Framework
ElectronMain electron_library_main.mm:26
Electron Framework
ElectronMain electron_library_main.mm:26

This is fixed in Node.js v20 and above, but wasn't backported upstream owing to the fact that it relies on two V8 commits that break Node.js ABI. However, we're ahead of Node.js' V8 version so we actually already have these commits.

cc @bpasero @deepak1556

Checklist

Release Notes

Notes: Fixed a crash that can result from some kinds of dynamic imports.

@codebytere codebytere added backport This is a backport PR semver/patch backwards-compatible bug fixes backport-check-skip Skip trop's backport validity checking 28-x-y labels Mar 1, 2024
@codebytere codebytere requested a review from deepak1556 March 1, 2024 14:44
@codebytere codebytere requested a review from a team as a code owner March 1, 2024 14:44
@codebytere codebytere force-pushed the fix-hostimportcrash branch from 8e97350 to cfb9c9d Compare March 1, 2024 15:36
@jkleinsc jkleinsc merged commit c96d7db into 28-x-y Mar 1, 2024
@jkleinsc jkleinsc deleted the fix-hostimportcrash branch March 1, 2024 20:51
@release-clerk
Copy link
Copy Markdown

release-clerk Bot commented Mar 1, 2024

Release Notes Persisted

Fixed a crash that can result from some kinds of dynamic imports.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

28-x-y backport This is a backport PR backport-check-skip Skip trop's backport validity checking semver/patch backwards-compatible bug fixes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants