Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

node::IsolateData allocates V8 objects before initializing platform #20171

Closed
ulan opened this issue Apr 20, 2018 · 5 comments
Closed

node::IsolateData allocates V8 objects before initializing platform #20171

ulan opened this issue Apr 20, 2018 · 5 comments
Labels
v8 engine Issues and PRs related to the V8 dependency.

Comments

@ulan
Copy link
Contributor

ulan commented Apr 20, 2018

The constructor of node::IsolateData allocates strings in V8 heap before registering itself with the platform. This is not safe because an allocation can trigger GC, which relies on the platform to post tasks.

The problem reproduces with --stress-incremental-marking flag.

> out/Release/node --stress-incremental-marking
out/Release/node[168537]: ../src/node_platform.cc:248:std::shared_ptr<node::PerIsolatePlatformData> node::NodePlatform::ForIsolate(v8::Isolate*): Assertion `data' failed.
 1: node::Abort() [out/Release/node]
 2: 0x5645d4c25a8d [out/Release/node]
 3: node::NodePlatform::ForIsolate(v8::Isolate*) [out/Release/node]
 4: node::NodePlatform::CallOnForegroundThread(v8::Isolate*, v8::Task*) [out/Release/node]
 5: v8::internal::IncrementalMarking::Start(v8::internal::GarbageCollectionReason) [out/Release/node]
 6: v8::internal::PagedSpace::RefillLinearAllocationAreaFromFreeList(unsigned long) [out/Release/node]
 7: v8::internal::PagedSpace::RawSlowRefillLinearAllocationArea(int) [out/Release/node]
 8: v8::internal::PagedSpace::SlowRefillLinearAllocationArea(int) [out/Release/node]
 9: v8::internal::Heap::AllocateOneByteInternalizedString(v8::internal::Vector<unsigned char const>, unsigned int) [out/Release/node]
10: v8::internal::Factory::NewOneByteInternalizedString(v8::internal::Vector<unsigned char const>, unsigned int) [out/Release/node]
11: v8::internal::StringTable::LookupKey(v8::internal::Isolate*, v8::internal::StringTableKey*) [out/Release/node]
12: v8::internal::Factory::InternalizeOneByteString(v8::internal::Vector<unsigned char const>) [out/Release/node]
13: v8::String::NewFromOneByte(v8::Isolate*, unsigned char const*, v8::NewStringType, int) [out/Release/node]
14: node::IsolateData::IsolateData(v8::Isolate*, uv_loop_s*, node::MultiIsolatePlatform*, unsigned int*) [out/Release/node]
15: node::Start(uv_loop_s*, int, char const* const*, int, char const* const*) [out/Release/node]
16: node::Start(int, char**) [out/Release/node]
17: __libc_start_main [/lib/x86_64-linux-gnu/libc.so.6]
18: _start [out/Release/node]

A fix would be to move the allocating code after platform_->RegisterIsolate(this, event_loop);

If that sounds good, I will create a PR.

@addaleax
Copy link
Member

If that sounds good, I will create a PR.

I think that’s fine, yes.

@addaleax addaleax added the v8 engine Issues and PRs related to the V8 dependency. label Apr 20, 2018
MylesBorins pushed a commit that referenced this issue May 4, 2018
Allocation of strings may cause a garbage collection that uses
the platform to post tasks.

PR-URL: #20175
Fixes: #20171
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yang Guo <yangguo@chromium.org>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
addaleax pushed a commit that referenced this issue Jun 29, 2018
Allocation of strings may cause a garbage collection that uses
the platform to post tasks.

PR-URL: #20175
Fixes: #20171
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yang Guo <yangguo@chromium.org>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
rvagg pushed a commit that referenced this issue Aug 16, 2018
Allocation of strings may cause a garbage collection that uses
the platform to post tasks.

PR-URL: #20175
Fixes: #20171
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yang Guo <yangguo@chromium.org>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
@nathancarter
Copy link

I'm encountering the exact error above in a node script I'm running. I'm using node 10.13.0, which was released after this fix seems to have been committed. Was this change included in 10.13.0?

But if I run node --stress-incremental-marking as the OP reports, I just get the node prompt, no errors.

Other searches for the same error yielded only people finding bugs on ARM architectures, but I'm on a recent Macbook.

Am I barking up the wrong tree here?

node[4094]: ../src/node_platform.cc:383:std::shared_ptr<PerIsolatePlatformData> node::NodePlatform::ForIsolate(v8::Isolate *): Assertion `data' failed.
 1: 0x10003ae75 node::Abort() [/Users/nathan/.nvm/versions/node/v10.13.0/bin/node]
 2: 0x100039ed3 node::AddEnvironmentCleanupHook(v8::Isolate*, void (*)(void*), void*) [/Users/nathan/.nvm/versions/node/v10.13.0/bin/node]
 3: 0x1000b1c90 node::NodePlatform::CallOnBackgroundThread(v8::Task*, v8::Platform::ExpectedRuntime) [/Users/nathan/.nvm/versions/node/v10.13.0/bin/node]
 4: 0x1000b1ceb node::NodePlatform::CallOnForegroundThread(v8::Isolate*, v8::Task*) [/Users/nathan/.nvm/versions/node/v10.13.0/bin/node]
 5: 0x100589ca3 v8::internal::IncrementalMarking::Start(v8::internal::GarbageCollectionReason) [/Users/nathan/.nvm/versions/node/v10.13.0/bin/node]
 6: 0x10056fd50 v8::internal::Heap::CollectGarbage(v8::internal::AllocationSpace, v8::internal::GarbageCollectionReason, v8::GCCallbackFlags) [/Users/nathan/.nvm/versions/node/v10.13.0/bin/node]
 7: 0x10057c2dc v8::internal::Heap::AllocateRawWithLigthRetry(int, v8::internal::AllocationSpace, v8::internal::AllocationAlignment) [/Users/nathan/.nvm/versions/node/v10.13.0/bin/node]
 8: 0x10057c35f v8::internal::Heap::AllocateRawWithRetryOrFail(int, v8::internal::AllocationSpace, v8::internal::AllocationAlignment) [/Users/nathan/.nvm/versions/node/v10.13.0/bin/node]
 9: 0x10054b6f6 v8::internal::Factory::NewFixedArrayWithFiller(v8::internal::Heap::RootListIndex, int, v8::internal::Object*, v8::internal::PretenureFlag) [/Users/nathan/.nvm/versions/node/v10.13.0/bin/node]
10: 0x1006848fa v8::internal::JSObject::MigrateToMap(v8::internal::Handle<v8::internal::JSObject>, v8::internal::Handle<v8::internal::Map>, int) [/Users/nathan/.nvm/versions/node/v10.13.0/bin/node]
11: 0x1006652b7 v8::internal::LookupIterator::TransitionToAccessorProperty(v8::internal::Handle<v8::internal::Object>, v8::internal::Handle<v8::internal::Object>, v8::internal::PropertyAttributes) [/Users/nathan/.nvm/versions/node/v10.13.0/bin/node]
12: 0x100693d6a v8::internal::JSObject::DefineAccessor(v8::internal::LookupIterator*, v8::internal::Handle<v8::internal::Object>, v8::internal::Handle<v8::internal::Object>, v8::internal::PropertyAttributes) [/Users/nathan/.nvm/versions/node/v10.13.0/bin/node]
13: 0x10069399f v8::internal::JSReceiver::ValidateAndApplyPropertyDescriptor(v8::internal::Isolate*, v8::internal::LookupIterator*, bool, v8::internal::PropertyDescriptor*, v8::internal::PropertyDescriptor*, v8::internal::ShouldThrow, v8::internal::Handle<v8::internal::Name>) [/Users/nathan/.nvm/versions/node/v10.13.0/bin/node]
14: 0x1006933f6 v8::internal::JSReceiver::OrdinaryDefineOwnProperty(v8::internal::LookupIterator*, v8::internal::PropertyDescriptor*, v8::internal::ShouldThrow) [/Users/nathan/.nvm/versions/node/v10.13.0/bin/node]
15: 0x100692ee7 v8::internal::JSReceiver::OrdinaryDefineOwnProperty(v8::internal::Isolate*, v8::internal::Handle<v8::internal::JSObject>, v8::internal::Handle<v8::internal::Object>, v8::internal::PropertyDescriptor*, v8::internal::ShouldThrow) [/Users/nathan/.nvm/versions/node/v10.13.0/bin/node]
16: 0x100691f36 v8::internal::JSReceiver::DefineProperty(v8::internal::Isolate*, v8::internal::Handle<v8::internal::Object>, v8::internal::Handle<v8::internal::Object>, v8::internal::Handle<v8::internal::Object>) [/Users/nathan/.nvm/versions/node/v10.13.0/bin/node]
17: 0x100268968 v8::internal::Builtin_ObjectDefineProperty(int, v8::internal::Object**, v8::internal::Isolate*) [/Users/nathan/.nvm/versions/node/v10.13.0/bin/node]
18: 0x3b64ce6dbf9d
19: 0x3b64ce758669
20: 0x3b64ce68d02f
21: 0x3b64ce761f25
/Users/nathan/Development/path/to/my/file: line 19:  4094 Abort trap: 6           coffee bin/myfile.litcoffee "/Users/nathan/example/file.txt"

@nathancarter
Copy link

Looks like it was added to 10.1.0, if I'm reading this page correctly. So I guess there's more than one way to cause this problem. I realize this is not a support forum, but any tips would be welcome!

@ulan
Copy link
Contributor Author

ulan commented Jan 22, 2019

@nathancarter the assertion looks similar, but the root cause might be different.

Your stack trace suggests that node is already running some script (lines 18-21 looks like generated code). In my case the crash happened during node initialization before running the scripts.

I'd suggest opening a new issue with steps to reproduce.

@nathancarter
Copy link

OK. I'll do that as soon as I can get to a minimal working example. Current example is huge. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants