Skip to content

Fix isolate cleanup segfault#245

Merged
szegedi merged 2 commits intoDataDog:mainfrom
Qard:stop-heap-profiler-on-cleanup
Dec 8, 2025
Merged

Fix isolate cleanup segfault#245
szegedi merged 2 commits intoDataDog:mainfrom
Qard:stop-heap-profiler-on-cleanup

Conversation

@Qard
Copy link
Copy Markdown

@Qard Qard commented Nov 29, 2025

What does this PR do?:

Ensures heap profiler is stopped when thread cleanup occurs.

Motivation:

In Node.js 24.x, if you start a heap profiler and fail to stop it before the thread exits it will segfault. If a thread would stop for whatever reason without calling StopSamplingHeapProfiler first it will crash the entire process due to trying to remove an allocation observer while it's still active. The argument could be made that this might actually be a V8 bug, but you'll probably want a workaround in any case.

Here's the crash trace:

  Thread 0 Crashed:: MainThread
  0   libsystem_platform.dylib      0x18982a454 _platform_memmove + 244
  1   node                          0x1026a7734
  v8::internal::AllocationCounter::RemoveAllocationObserver(v8::internal::AllocationObserver*) + 144
  2   node                          0x102709b64
  v8::internal::HeapAllocator::RemoveAllocationObserver(v8::internal::AllocationObserver*,
  v8::internal::AllocationObserver*) + 64
  3   node                          0x102b14d50 v8::internal::SamplingHeapProfiler::~SamplingHeapProfiler()
   + 36
  4   node                          0x102af2f8c v8::internal::HeapProfiler::~HeapProfiler() + 88
  5   node                          0x102af3040 v8::internal::HeapProfiler::~HeapProfiler() + 12
  6   node                          0x1027224c8 v8::internal::Heap::TearDown() + 1252
  7   node                          0x102674744 v8::internal::Isolate::Deinit() + 964
  8   node                          0x1026742bc v8::internal::Isolate::Deinitialize(v8::internal::Isolate*)
   + 144
  9   node                          0x1023503a4 node::MultiIsolatePlatform::DisposeIsolate(v8::Isolate*) +
  28
  10  node                          0x102314a74 node::NodeMainInstance::~NodeMainInstance() + 60
  11  node                          0x10227ad2c node::Start(int, char**) + 612
  12  dyld                          0x189461d54 start + 7184

How to test the change?:

Stop a worker thread without stopping the heap profiler within it first.

@szegedi szegedi added the semver-patch Bug or security fixes, mainly label Dec 1, 2025
@szegedi
Copy link
Copy Markdown

szegedi commented Dec 1, 2025

We did something similar in #215 for the time profiler already.

@Qard
Copy link
Copy Markdown
Author

Qard commented Dec 1, 2025

Ah, I didn't even look at the CPU profiler. I just saw the crash while working on react-pprof and saw a quick fix. 😅

Copy link
Copy Markdown

@szegedi szegedi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, one nit for a simplification

Comment thread bindings/profilers/heap.cc Outdated
Co-authored-by: Attila Szegedi <szegedi@users.noreply.github.com>
@nsavoire
Copy link
Copy Markdown

nsavoire commented Dec 2, 2025

This might be similar to:
#212

@szegedi szegedi merged commit 55db246 into DataDog:main Dec 8, 2025
60 of 61 checks passed
@Qard Qard deleted the stop-heap-profiler-on-cleanup branch December 10, 2025 10:34
szegedi pushed a commit that referenced this pull request Dec 15, 2025
@szegedi szegedi mentioned this pull request Dec 15, 2025
szegedi pushed a commit that referenced this pull request Dec 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

semver-patch Bug or security fixes, mainly

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants