search

nodejs / node-v8

Experimental Node.js mirror on V8 lkgr :sparkles::turtle::rocket::sparkles:
405 stars 66 forks source link

Heap Allocation changes causing DCHECKs in heap snapshotting #282

Open codebytere opened 3 weeks ago

codebytere commented 3 weeks ago

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:

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::AllocationType, v8::internal::Handle) [/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::AllocationType, v8::internal::Handle, 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::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) [/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 const&)>(v8::FunctionCallbackInfo 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::Handle, int, v8::internal::Handle*) [/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::Local, int, v8::Local*) [/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 (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 (node::StartExecutionCallbackInfo const&)>, std::__Cr::function, v8::Local)>) [/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 https://github.com/electron/electron/issues/42125) but they should likely hit you all in canary soon! Happy to help with a PR once there's a path forward.

targos commented 3 weeks ago

/cc @joyeecheung

joyeecheung commented 3 weeks ago

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 commented 3 weeks ago

We don't have debug CI for canary builds.

targos commented 3 weeks ago

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

targos commented 3 weeks ago

^ Reproduced

jdapena commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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