src: keep the first snapshot blob alive for later isolates · nodejs/node@64649ab · GitHub
Skip to content

Commit 64649ab

Browse files
codebytereaduh95
authored andcommitted
src: keep the first snapshot blob alive for later isolates
`NewIsolate()` creates every isolate from the snapshot blob the first isolate in the process used, because V8 shares the read-only heap between isolates, and did so by keeping a pointer to the first `CreateParams`. When that blob came from an `EmbedderSnapshotData` the embedder had since released, e.g. a second `CommonEnvironmentSetup::CreateFromSnapshot()` after the first setup and its snapshot were destroyed, V8 deserialized freed memory. Record the first blob and external references under a mutex instead of copying the caller's `CreateParams`, and make `~SnapshotData()` leave that one blob allocated, since its owner can go away before the last isolate is created. Nothing is copied and `node` itself is unaffected. embedtest grows an `--embedder-run-twice` switch so the sequence can be tested. Refs: #45885 Signed-off-by: Shelley Vohr <shelley.vohr@gmail.com> PR-URL: #65779 Refs: #32984 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
1 parent b44c072 commit 64649ab

6 files changed

Lines changed: 89 additions & 12 deletions

File tree

‎src/api/environment.cc‎

Lines changed: 29 additions & 9 deletions

‎src/node.h‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1022,6 +1022,9 @@ class NODE_EXTERN CommonEnvironmentSetup {
10221022
// will be empty.
10231023
// env_args will be passed through as arguments to CreateEnvironment(), after
10241024
// `isolate_data` and `context`.
1025+
// `snapshot_data` has to stay alive as long as the setup created from it,
1026+
// and every setup in a process has to use the same snapshot: all isolates
1027+
// are created from the blob the first one used.
10251028
template <typename... EnvironmentArgs>
10261029
static std::unique_ptr<CommonEnvironmentSetup> Create(
10271030
MultiIsolatePlatform* platform,

‎src/node_internals.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -355,6 +355,8 @@ void DefineZlibConstants(v8::Local<v8::Object> target);
355355
// addresses, so this should be used with care.
356356
v8::IsolateGroup GetOrCreateIsolateGroup();
357357

358+
// The blob every isolate is created from, see NewIsolate(). It is never freed.
359+
bool IsFirstSnapshotBlob(const char* data);
358360
v8::Isolate* NewIsolate(v8::Isolate::CreateParams* params,
359361
uv_loop_t* event_loop,
360362
MultiIsolatePlatform* platform,

‎src/node_snapshotable.cc‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -699,7 +699,8 @@ bool SnapshotData::Check() const {
699699

700700
SnapshotData::~SnapshotData() {
701701
if (data_ownership == DataOwnership::kOwned &&
702-
v8_snapshot_blob_data.data != nullptr) {
702+
v8_snapshot_blob_data.data != nullptr &&
703+
!IsFirstSnapshotBlob(v8_snapshot_blob_data.data)) {
703704
delete[] v8_snapshot_blob_data.data;
704705
}
705706
}

‎test/embedding/embedtest.cc‎

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -65,8 +65,20 @@ NODE_MAIN(int argc, node::argv_type raw_argv[]) {
6565
cppgc::InitializeProcess(platform->GetPageAllocator());
6666
V8::Initialize();
6767

68-
int ret =
69-
RunNodeInstance(platform.get(), result->args(), result->exec_args());
68+
// --embedder-run-twice: two sequential instances in one process, each
69+
// loading (and freeing) its own copy of the snapshot.
70+
std::vector<std::string> instance_args = result->args();
71+
auto twice = std::find(
72+
instance_args.begin(), instance_args.end(), "--embedder-run-twice");
73+
int runs = 1;
74+
if (twice != instance_args.end()) {
75+
instance_args.erase(twice);
76+
runs = 2;
77+
}
78+
int ret = 0;
79+
for (int i = 0; i < runs && ret == 0; i++) {
80+
ret = RunNodeInstance(platform.get(), instance_args, result->exec_args());
81+
}
7082

7183
V8::Dispose();
7284
V8::DisposePlatform();
Lines changed: 39 additions & 0 deletions

0 commit comments

Comments
 (0)