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

Segfault on master #22528

Closed
BridgeAR opened this issue Aug 26, 2018 · 6 comments
Closed

Segfault on master #22528

BridgeAR opened this issue Aug 26, 2018 · 6 comments
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. trace_events Issues and PRs related to V8, Node.js core, and userspace code trace events.

Comments

@BridgeAR
Copy link
Member

Steps to reproduce:

npm i @nearform/bubbleprof
./node

Then run the example from bubbleprof:

const ClinicBubbleprof = require('.')
const bubbleprof = new ClinicBubbleprof()

bubbleprof.collect(['./node', './path-to-script.js'], function (err, filepath) {
  if (err) throw err

  bubbleprof.visualize(filepath, filepath + '.html', function (err) {
    if (err) throw err
  })
})

Then kill the now spawned process.

ps aux | grep node
kill xxxxx

Result in:

FATAL ERROR: v8::HandleScope::CreateHandle() Cannot create a handle without a HandleScope
 1: 0x563b4a4ad0a0 node::Abort() [../../../node/node-cp/node]
 2: 0x563b4a4ad0ee  [../../../node/node-cp/node]
 3: 0x563b4a6e5f0a v8::Utils::ReportApiFailure(char const*, char const*) [../../../node/node-cp/node]
 4: 0x563b4aaffef6 v8::internal::HandleScope::Extend(v8::internal::Isolate*) [../../../node/node-cp/node]
 5: 0x563b4a6e7514 v8::EscapableHandleScope::EscapableHandleScope(v8::Isolate*) [../../../node/node-cp/node]
 6: 0x563b4a6fac9e v8::Function::Call(v8::Local<v8::Context>, v8::Local<v8::Value>, int, v8::Local<v8::Value>*) [../../../node/node-cp/node]
 7: 0x563b4a494dd3 node::Environment::TrackingTraceStateObserver::OnTraceDisabled() [../../../node/node-cp/node]
 8: 0x563b4b1b4be3  [../../../node/node-cp/node]
 9: 0x563b4a582d66 node::tracing::Agent::Disconnect(int) [../../../node/node-cp/node]
10: 0x563b4a4ac7f7 node::SignalExit(int) [../../../node/node-cp/node]
11: 0x7f8fdbe62890  [/lib/x86_64-linux-gnu/libpthread.so.0]
12: 0x7f8fdbb7a839 syscall [/lib/x86_64-linux-gnu/libc.so.6]
13: 0x563b4a62910a  [../../../node/node-cp/node]
14: 0x563b4a627293  [../../../node/node-cp/node]
15: 0x563b4a6158a2 uv_run [../../../node/node-cp/node]
16: 0x563b4a4b8bb5 node::Start(v8::Isolate*, node::IsolateData*, int, char const* const*, int, char const* const*) [../../../node/node-cp/node]
17: 0x563b4a4b7360 node::Start(int, char**) [../../../node/node-cp/node]
18: 0x7f8fdba80b97 __libc_start_main [/lib/x86_64-linux-gnu/libc.so.6]
19: 0x563b4a469ada _start [../../../node/node-cp/node]
Error: process exited by signal SIGABRT
@BridgeAR BridgeAR added the c++ Issues and PRs that require attention from people who are familiar with C++. label Aug 26, 2018
@addaleax addaleax added the trace_events Issues and PRs related to V8, Node.js core, and userspace code trace events. label Aug 26, 2018
@addaleax
Copy link
Member

I think this particular could be fixable by adding a HandleScope handle_scope(isolate); line to UpdateTraceCategoryState in env.cc.

The signal handler is probably doing a lot of things a signal handler shouldn’t be doing, and I remember seeing previous discussion on that somewhere.

@gireeshpunathil
Copy link
Member

previous discussion -> would that be #14802 one?

@addaleax
Copy link
Member

@gireeshpunathil Yes, exactly. :)

@addaleax
Copy link
Member

@BridgeAR Do you think you could help with a regression test for this?

@BridgeAR
Copy link
Member Author

@addaleax I'll try to get a simply test case together.

addaleax added a commit to addaleax/node that referenced this issue Sep 6, 2018
This feature cannot be reasonably implemented this way
without inherently being susceptible to race conditions
that lead to hangs, crashes, etc.

What’s more, implementing this for some signals only
(and it can only be implemented for some signals at all)
may lead to the impression that it is a guaranteed
feature, when really consumers of the tracing output
*need* to be able to handle abrupt ends meaningfully.

Fixes: nodejs#14802
Fixes: nodejs#22528
@addaleax
Copy link
Member

addaleax commented Sep 6, 2018

@BridgeAR #22734 “resolves” this issue by not providing any cleanup at all when encountering a signal. It’s not a great solution, but I believe it’s necessary. :/

targos pushed a commit that referenced this issue Sep 17, 2018
This feature cannot be reasonably implemented this way
without inherently being susceptible to race conditions
that lead to hangs, crashes, etc.

What’s more, implementing this for some signals only
(and it can only be implemented for some signals at all)
may lead to the impression that it is a guaranteed
feature, when really consumers of the tracing output
*need* to be able to handle abrupt ends meaningfully.

Fixes: #14802
Fixes: #22528

PR-URL: #22734
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
targos pushed a commit that referenced this issue Sep 19, 2018
This feature cannot be reasonably implemented this way
without inherently being susceptible to race conditions
that lead to hangs, crashes, etc.

What’s more, implementing this for some signals only
(and it can only be implemented for some signals at all)
may lead to the impression that it is a guaranteed
feature, when really consumers of the tracing output
*need* to be able to handle abrupt ends meaningfully.

Fixes: #14802
Fixes: #22528

PR-URL: #22734
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
targos pushed a commit that referenced this issue Sep 20, 2018
This feature cannot be reasonably implemented this way
without inherently being susceptible to race conditions
that lead to hangs, crashes, etc.

What’s more, implementing this for some signals only
(and it can only be implemented for some signals at all)
may lead to the impression that it is a guaranteed
feature, when really consumers of the tracing output
*need* to be able to handle abrupt ends meaningfully.

Fixes: #14802
Fixes: #22528

PR-URL: #22734
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. trace_events Issues and PRs related to V8, Node.js core, and userspace code trace events.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants