Heap Allocation changes causing DCHECKs in heap snapshotting

[codebytere]

In our latest V8 roll via Chromium update, we're seeing a lot of crashes in heap snapshot tests - we've tracked these to https://bugs.chromium.org/p/v8/issues/detail?id=14617 and the associated CLs therein.

Failing tests we observed:

• parallel/test-heapdump-async-hooks-init-promise.js
• parallel/test-http2-ping-settings-heapdump.js
• parallel/test-inspector-heapdump.js
• parallel/test-inspector-heap-allocation-tracker.js
• test-worker-heap-snapshot.js
• parallel/test-worker-exit-heapsnapshot.js
• sequential/test-get-heapsnapshot-options.js
• sequential/test-heapdump.js
• sequential/test-worker-heapsnapshot-options.js

The stacktraces for each differ subtly, but one example is below

Sample Stacktrace

electron on git:main ❯ node script/node-spec-runner.js sequential/test-heapdump.js
=== release test-heapdump ===
Path: sequential/test-heapdump
#
# Fatal error in ../../v8/src/heap/heap-allocator-inl.h, line 72
# Debug check failed: AllowHeapAllocation::IsAllowed().
#
#
#
#FailureMessage Object: 0x16b1f5238
----- Native stack trace -----

 1: 0x124071c40 node::NodePlatform::GetStackTracePrinter()::$_0::__invoke() [/Users/codebytere/Developer/electron-gn/src/out/Testing/Electron.app/Contents/Frameworks/Electron Framework.framework/Versions/A/Electron Framework]
 2: 0x120866008 V8_Fatal(char const*, int, char const*, ...) [/Users/codebytere/Developer/electron-gn/src/out/Testing/Electron.app/Contents/Frameworks/Electron Framework.framework/Versions/A/Electron Framework]
 3: 0x120865bb8 v8::base::FatalOOM(v8::base::OOMType, char const*) [/Users/codebytere/Developer/electron-gn/src/out/Testing/Electron.app/Contents/Frameworks/Electron Framework.framework/Versions/A/Electron Framework]
 4: 0x11b939fc8 v8::internal::AllocationResult v8::internal::HeapAllocator::AllocateRaw<(v8::internal::AllocationType)0>(int, v8::internal::AllocationOrigin, v8::internal::AllocationAlignment) [/Users/codebytere/Developer/electron-gn/src/out/Testing/Electron.app/Contents/Frameworks/Electron Framework.framework/Versions/A/Electron Framework]
 5: 0x11b909f40 v8::internal::Factory::AllocateRawWithAllocationSite(v8::internal::Handle<v8::internal::Map>, v8::internal::AllocationType, v8::internal::Handle<v8::internal::AllocationSite>) [/Users/codebytere/Developer/electron-gn/src/out/Testing/Electron.app/Contents/Frameworks/Electron Framework.framework/Versions/A/Electron Framework]
 6: 0x11b911e94 v8::internal::Factory::NewJSObjectFromMap(v8::internal::Handle<v8::internal::Map>, v8::internal::AllocationType, v8::internal::Handle<v8::internal::AllocationSite>, v8::internal::NewJSObjectType) [/Users/codebytere/Developer/electron-gn/src/out/Testing/Electron.app/Contents/Frameworks/Electron Framework.framework/Versions/A/Electron Framework]
 7: 0x11b92c89c v8::internal::Factory::NewJSArrayBuffer(std::__Cr::shared_ptr<v8::internal::BackingStore>, v8::internal::AllocationType) [/Users/codebytere/Developer/electron-gn/src/out/Testing/Electron.app/Contents/Frameworks/Electron Framework.framework/Versions/A/Electron Framework]
 8: 0x11b5809f4 v8::ArrayBuffer::New(v8::Isolate*, std::__Cr::shared_ptr<v8::BackingStore>) [/Users/codebytere/Developer/electron-gn/src/out/Testing/Electron.app/Contents/Frameworks/Electron Framework.framework/Versions/A/Electron Framework]
 9: 0x1240f1458 node::EmitToJSStreamListener::OnStreamRead(long, uv_buf_t const&) [/Users/codebytere/Developer/electron-gn/src/out/Testing/Electron.app/Contents/Frameworks/Electron Framework.framework/Versions/A/Electron Framework]
10: 0x123fad054 non-virtual thunk to node::heap::(anonymous namespace)::HeapSnapshotStream::WriteAsciiChunk(char*, int) [/Users/codebytere/Developer/electron-gn/src/out/Testing/Electron.app/Contents/Frameworks/Electron Framework.framework/Versions/A/Electron Framework]
11: 0x11bf02724 v8::internal::OutputStreamWriter::AddSubstring(char const*, int) [/Users/codebytere/Developer/electron-gn/src/out/Testing/Electron.app/Contents/Frameworks/Electron Framework.framework/Versions/A/Electron Framework]
12: 0x11befe234 v8::internal::HeapSnapshotJSONSerializer::SerializeNode(v8::internal::HeapEntry const*) [/Users/codebytere/Developer/electron-gn/src/out/Testing/Electron.app/Contents/Frameworks/Electron Framework.framework/Versions/A/Electron Framework]
13: 0x11befbbd4 v8::internal::HeapSnapshotJSONSerializer::SerializeImpl() [/Users/codebytere/Developer/electron-gn/src/out/Testing/Electron.app/Contents/Frameworks/Electron Framework.framework/Versions/A/Electron Framework]
14: 0x11befb838 v8::internal::HeapSnapshotJSONSerializer::Serialize(v8::OutputStream*) [/Users/codebytere/Developer/electron-gn/src/out/Testing/Electron.app/Contents/Frameworks/Electron Framework.framework/Versions/A/Electron Framework]
15: 0x11b593378 v8::HeapSnapshot::Serialize(v8::OutputStream*, v8::HeapSnapshot::SerializationFormat) const [/Users/codebytere/Developer/electron-gn/src/out/Testing/Electron.app/Contents/Frameworks/Electron Framework.framework/Versions/A/Electron Framework]
16: 0x123face8c non-virtual thunk to node::heap::(anonymous namespace)::HeapSnapshotStream::ReadStart() [/Users/codebytere/Developer/electron-gn/src/out/Testing/Electron.app/Contents/Frameworks/Electron Framework.framework/Versions/A/Electron Framework]
17: 0x1240f01bc void node::StreamBase::JSMethod<&node::StreamBase::ReadStartJS(v8::FunctionCallbackInfo<v8::Value> const&)>(v8::FunctionCallbackInfo<v8::Value> const&) [/Users/codebytere/Developer/electron-gn/src/out/Testing/Electron.app/Contents/Frameworks/Electron Framework.framework/Versions/A/Electron Framework]
18: 0x177d93f9c
19: 0x177d91718
20: 0x177d91718
21: 0x177d91718
22: 0x177d91718
23: 0x177d8e908
24: 0x177d8e554
25: 0x11b81bbe4 v8::internal::(anonymous namespace)::Invoke(v8::internal::Isolate*, v8::internal::(anonymous namespace)::InvokeParams const&) [/Users/codebytere/Developer/electron-gn/src/out/Testing/Electron.app/Contents/Frameworks/Electron Framework.framework/Versions/A/Electron Framework]
26: 0x11b81aa40 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>*) [/Users/codebytere/Developer/electron-gn/src/out/Testing/Electron.app/Contents/Frameworks/Electron Framework.framework/Versions/A/Electron Framework]
27: 0x11b5605b4 v8::Function::Call(v8::Local<v8::Context>, v8::Local<v8::Value>, int, v8::Local<v8::Value>*) [/Users/codebytere/Developer/electron-gn/src/out/Testing/Electron.app/Contents/Frameworks/Electron Framework.framework/Versions/A/Electron Framework]
28: 0x123f6c1ec node::InternalCallbackScope::Close() [/Users/codebytere/Developer/electron-gn/src/out/Testing/Electron.app/Contents/Frameworks/Electron Framework.framework/Versions/A/Electron Framework]
29: 0x123f6bde8 node::InternalCallbackScope::~InternalCallbackScope() [/Users/codebytere/Developer/electron-gn/src/out/Testing/Electron.app/Contents/Frameworks/Electron Framework.framework/Versions/A/Electron Framework]
30: 0x123fb259c node::StartExecution(node::Environment*, std::__Cr::function<v8::MaybeLocal<v8::Value> (node::StartExecutionCallbackInfo const&)>) [/Users/codebytere/Developer/electron-gn/src/out/Testing/Electron.app/Contents/Frameworks/Electron Framework.framework/Versions/A/Electron Framework]
31: 0x123f6fee0 node::LoadEnvironment(node::Environment*, std::__Cr::function<v8::MaybeLocal<v8::Value> (node::StartExecutionCallbackInfo const&)>, std::__Cr::function<void (node::Environment*, v8::Local<v8::Value>, v8::Local<v8::Value>)>) [/Users/codebytere/Developer/electron-gn/src/out/Testing/Electron.app/Contents/Frameworks/Electron Framework.framework/Versions/A/Electron Framework]
32: 0x119c56da0 electron::NodeMain(int, char**) [/Users/codebytere/Developer/electron-gn/src/out/Testing/Electron.app/Contents/Frameworks/Electron Framework.framework/Versions/A/Electron Framework]
33: 0x119c52e08 ElectronInitializeICUandStartNode [/Users/codebytere/Developer/electron-gn/src/out/Testing/Electron.app/Contents/Frameworks/Electron Framework.framework/Versions/A/Electron Framework]
34: 0x183d8a0e0 start [/usr/lib/dyld]
Command: /Users/codebytere/Developer/electron-gn/src/out/Testing/Electron.app/Contents/MacOS/Electron /Users/codebytere/Developer/electron-gn/src/third_party/electron_node/test/sequential/test-heapdump.js
--- CRASHED (Signal: 5) ---


[00:00|% 100|+   0|-   1]: Done

Failed tests:
/Users/codebytere/Developer/electron-gn/src/out/Testing/Electron.app/Contents/MacOS/Electron /Users/codebytere/Developer/electron-gn/src/third_party/electron_node/test/sequential/test-heapdump.js

We've reverted these in Electron temporarily (see electron/electron#42125) but they should likely hit you all in canary soon! Happy to help with a PR once there's a path forward.

[targos]

/cc @joyeecheung

[joyeecheung]

From the stack trace it seems somehow the no-allocation invariant is enforced during snapshot serialization (I am pretty sure the original intent was to only apply it to generation? cc @jdapena). If that’s the case it seems strange that this passes the Node.js integration tests in V8 CI and Node.js’s own CI and only get caught in Electron’s test suite.

[targos]

We don't have debug CI for canary builds.

[targos]

Here's a test build: https://ci.nodejs.org/job/node-test-commit-arm-debug/12857/

[targos]

^ Reproduced

[jdapena]

I will take a look. The idea was also covering the JSON serialization stage, as we do not want the heap to be modified for generating a heap snapshot, not even on serialization step.

I will need to remove the check for serialization stage, and add it explicitely on tests that we know are not going to creates themselves heap buffers in serialization stage.

[joyeecheung]

Right for Node.js’s use case the serialization needs to allow allocation because there is an API that allows users to consume the heap snapshot in a readable stream in JavaScript. It doesn’t seem very useful to keep that restriction beyond heap snapshot generation.

[jdapena]

I am preparing a fix in V8 side: https://chromium-review.googlesource.com/c/v8/v8/+/5531355

[targos]

Should be fixed by https://chromium-review.googlesource.com/c/v8/v8/+/5531355, thanks @jdapena