From 7a4984f327cad2bb6da286f3d7d064d6465329c2 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Wed, 9 Oct 2024 18:09:40 +0200 Subject: [PATCH 1/2] src: migrate from deprecated SnapshotCreator constructor Previously we have been using the variant of SnapshotCreator that only passes the external references instead of v8::Isolate::CreateParams and it's about to be deprecated. Switch to using the new constructor that takes a fully CreateParams instead. This also makes sure that the snapshot building script is using the Node.js array buffer allocator instead of a separate default one that was previously used by the old constructor. The zero fill toggle in the Node.js array buffer allocator would still be ignored during snapshot building, however, until we fixes the array buffer allocator and let V8 own the toggle backing store instead, because otherwise the snapshot would contain the external toggle address and become unreproducible. --- lib/internal/buffer.js | 18 ++++++++++++++-- lib/internal/process/pre_execution.js | 2 -- src/api/embed_helpers.cc | 30 ++++++++++++++++++++------- src/node_buffer.cc | 6 ++++++ src/node_snapshotable.cc | 5 +++-- 5 files changed, 47 insertions(+), 14 deletions(-) diff --git a/lib/internal/buffer.js b/lib/internal/buffer.js index dc21fb3e334588..b9bec27c0ad37b 100644 --- a/lib/internal/buffer.js +++ b/lib/internal/buffer.js @@ -39,6 +39,13 @@ const { }, } = internalBinding('util'); +const { + namespace: { + isBuildingSnapshot, + }, + addAfterUserSerializeCallback, +} = require('internal/v8/startup_snapshot'); + // Temporary buffers to convert numbers. const float32Array = new Float32Array(1); const uInt8Float32Array = new Uint8Array(float32Array.buffer); @@ -1090,8 +1097,16 @@ function isMarkedAsUntransferable(obj) { // in C++. // |zeroFill| can be undefined when running inside an isolate where we // do not own the ArrayBuffer allocator. Zero fill is always on in that case. -let zeroFill = getZeroFillToggle(); +let zeroFill; function createUnsafeBuffer(size) { + if (!zeroFill) { + zeroFill = getZeroFillToggle(); + if (isBuildingSnapshot()) { + addAfterUserSerializeCallback(() => { + zeroFill = undefined; + }); + } + } zeroFill[0] = 0; try { return new FastBuffer(size); @@ -1116,5 +1131,4 @@ module.exports = { createUnsafeBuffer, readUInt16BE, readUInt32BE, - reconnectZeroFillToggle, }; diff --git a/lib/internal/process/pre_execution.js b/lib/internal/process/pre_execution.js index e4c23e67f13541..65c161584f3d34 100644 --- a/lib/internal/process/pre_execution.js +++ b/lib/internal/process/pre_execution.js @@ -21,7 +21,6 @@ const { refreshOptions, getEmbedderOptions, } = require('internal/options'); -const { reconnectZeroFillToggle } = require('internal/buffer'); const { exposeLazyInterfaces, defineReplaceableLazyAttribute, @@ -91,7 +90,6 @@ function prepareExecution(options) { const { expandArgv1, initializeModules, isMainThread } = options; refreshRuntimeOptions(); - reconnectZeroFillToggle(); // Patch the process object and get the resolved main entry point. const mainEntry = patchProcessObject(expandArgv1); diff --git a/src/api/embed_helpers.cc b/src/api/embed_helpers.cc index 14d2a20c1c18d9..a0aa3d33d1e749 100644 --- a/src/api/embed_helpers.cc +++ b/src/api/embed_helpers.cc @@ -110,22 +110,36 @@ CommonEnvironmentSetup::CommonEnvironmentSetup( } loop->data = this; + impl_->allocator = ArrayBufferAllocator::Create(); + const std::vector& external_references = + SnapshotBuilder::CollectExternalReferences(); + Isolate::CreateParams params; + params.array_buffer_allocator = impl_->allocator.get(); + params.external_references = external_references.data(); Isolate* isolate; + + // Isolates created for snapshotting should be set up differently since + // it will be owned by the snapshot creator and needs to be cleaned up + // before serialization. if (flags & Flags::kIsForSnapshotting) { - const std::vector& external_references = - SnapshotBuilder::CollectExternalReferences(); + // The isolate must be registered before the SnapshotCreator initializes the + // isolate, so that the memory reducer can be initialized. isolate = impl_->isolate = Isolate::Allocate(); - // Must be done before the SnapshotCreator creation so that the - // memory reducer can be initialized. platform->RegisterIsolate(isolate, loop); - impl_->snapshot_creator.emplace(isolate, external_references.data()); + + impl_->snapshot_creator.emplace(isolate, params); isolate->SetCaptureStackTraceForUncaughtExceptions( - true, 10, v8::StackTrace::StackTraceOptions::kDetailed); + true, + static_cast( + per_process::cli_options->per_isolate->stack_trace_limit), + v8::StackTrace::StackTraceOptions::kDetailed); SetIsolateMiscHandlers(isolate, {}); } else { - impl_->allocator = ArrayBufferAllocator::Create(); isolate = impl_->isolate = - NewIsolate(impl_->allocator, &impl_->loop, platform, snapshot_data); + NewIsolate(¶ms, + &impl_->loop, + platform, + SnapshotData::FromEmbedderWrapper(snapshot_data)); } { diff --git a/src/node_buffer.cc b/src/node_buffer.cc index 92d9a4a729d9f3..07461db50bbe9b 100644 --- a/src/node_buffer.cc +++ b/src/node_buffer.cc @@ -1226,7 +1226,13 @@ void GetZeroFillToggle(const FunctionCallbackInfo& args) { // Create a dummy Uint32Array - the JS land can only toggle the C++ land // setting when the allocator uses our toggle. With this the toggle in JS // land results in no-ops. + + ab = ArrayBuffer::New(env->isolate(), sizeof(uint32_t)); + } else if (env->isolate_data()->is_building_snapshot()) { ab = ArrayBuffer::New(env->isolate(), sizeof(uint32_t)); + // TODO(joyeecheung): save ab->GetBackingStore()->Data() in the Node.js + // array buffer allocator and include it into the C++ toggle while the + // Environment is still alive. } else { uint32_t* zero_fill_field = allocator->zero_fill_field(); std::unique_ptr backing = diff --git a/src/node_snapshotable.cc b/src/node_snapshotable.cc index f9acb7b1d1618e..6c7065056306f8 100644 --- a/src/node_snapshotable.cc +++ b/src/node_snapshotable.cc @@ -863,9 +863,10 @@ const std::vector& SnapshotBuilder::CollectExternalReferences() { void SnapshotBuilder::InitializeIsolateParams(const SnapshotData* data, Isolate::CreateParams* params) { - CHECK_NULL(params->external_references); CHECK_NULL(params->snapshot_blob); - params->external_references = CollectExternalReferences().data(); + if (params->external_references == nullptr) { + params->external_references = CollectExternalReferences().data(); + } params->snapshot_blob = const_cast(&(data->v8_snapshot_blob_data)); } From b007f466f323a1ad20f4c909e5725f3bdb1f4eb0 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Sun, 13 Apr 2025 01:15:36 +0200 Subject: [PATCH 2/2] fixup! src: migrate from deprecated SnapshotCreator constructor --- lib/internal/buffer.js | 10 ++-------- src/api/embed_helpers.cc | 10 +++++++--- src/node_buffer.cc | 10 ++++++---- 3 files changed, 15 insertions(+), 15 deletions(-) diff --git a/lib/internal/buffer.js b/lib/internal/buffer.js index b9bec27c0ad37b..9707af66dff860 100644 --- a/lib/internal/buffer.js +++ b/lib/internal/buffer.js @@ -1102,6 +1102,8 @@ function createUnsafeBuffer(size) { if (!zeroFill) { zeroFill = getZeroFillToggle(); if (isBuildingSnapshot()) { + // Reset the toggle so that after serialization, we'll re-create a real + // toggle connected to the C++ one via getZeroFillToggle(). addAfterUserSerializeCallback(() => { zeroFill = undefined; }); @@ -1115,14 +1117,6 @@ function createUnsafeBuffer(size) { } } -// The connection between the JS land zero fill toggle and the -// C++ one in the NodeArrayBufferAllocator gets lost if the toggle -// is deserialized from the snapshot, because V8 owns the underlying -// memory of this toggle. This resets the connection. -function reconnectZeroFillToggle() { - zeroFill = getZeroFillToggle(); -} - module.exports = { FastBuffer, addBufferPrototypeMethods, diff --git a/src/api/embed_helpers.cc b/src/api/embed_helpers.cc index a0aa3d33d1e749..ff9ae20d7b80b2 100644 --- a/src/api/embed_helpers.cc +++ b/src/api/embed_helpers.cc @@ -116,6 +116,10 @@ CommonEnvironmentSetup::CommonEnvironmentSetup( Isolate::CreateParams params; params.array_buffer_allocator = impl_->allocator.get(); params.external_references = external_references.data(); + params.external_references = external_references.data(); + params.cpp_heap = + v8::CppHeap::Create(platform, v8::CppHeapCreateParams{{}}).release(); + Isolate* isolate; // Isolates created for snapshotting should be set up differently since @@ -234,10 +238,10 @@ CommonEnvironmentSetup::~CommonEnvironmentSetup() { *static_cast(data) = true; }, &platform_finished); impl_->platform->UnregisterIsolate(isolate); - if (impl_->snapshot_creator.has_value()) + if (impl_->snapshot_creator.has_value()) { impl_->snapshot_creator.reset(); - else - isolate->Dispose(); + } + isolate->Dispose(); // Wait until the platform has cleaned up all relevant resources. while (!platform_finished) diff --git a/src/node_buffer.cc b/src/node_buffer.cc index 07461db50bbe9b..3507f5e3e2943a 100644 --- a/src/node_buffer.cc +++ b/src/node_buffer.cc @@ -1222,18 +1222,20 @@ void GetZeroFillToggle(const FunctionCallbackInfo& args) { Local ab; // It can be a nullptr when running inside an isolate where we // do not own the ArrayBuffer allocator. - if (allocator == nullptr) { + if (allocator == nullptr || env->isolate_data()->is_building_snapshot()) { // Create a dummy Uint32Array - the JS land can only toggle the C++ land // setting when the allocator uses our toggle. With this the toggle in JS // land results in no-ops. + // When building a snapshot, just use a dummy toggle as well to avoid + // introducing the dynamic external reference. We'll re-initialize the + // toggle with a real one connected to the C++ allocator after snapshot + // deserialization. ab = ArrayBuffer::New(env->isolate(), sizeof(uint32_t)); - } else if (env->isolate_data()->is_building_snapshot()) { - ab = ArrayBuffer::New(env->isolate(), sizeof(uint32_t)); + } else { // TODO(joyeecheung): save ab->GetBackingStore()->Data() in the Node.js // array buffer allocator and include it into the C++ toggle while the // Environment is still alive. - } else { uint32_t* zero_fill_field = allocator->zero_fill_field(); std::unique_ptr backing = ArrayBuffer::NewBackingStore(zero_fill_field,