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

v8: fix process.abort() interaction with V8 #13985

Closed
wants to merge 2 commits into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Jun 29, 2017

Since V8 5.9 V8 installs a default signal handler for some signals
when creating a default platform instance that prints a stack trace.

However, Node already does the same thing, so it would seem like the
two different stack traces would be printed; also, the V8 handler
would lead to a SIGSEGV under some circumstances, rather than
letting the abort continue normally.

Fixes: #13865

@nodejs/v8 I don’t have the time to deal with any upstream concerns regarding this, and probably not to look into the segfault as well, so if we want to report it/have that V8 change reverted, somebody else needs to do it.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

v8

CI: https://ci.nodejs.org/job/node-test-pull-request/8862/

@nodejs-github-bot nodejs-github-bot added the v8 engine Issues and PRs related to the V8 dependency. label Jun 29, 2017
@refack
Copy link
Contributor

refack commented Jun 29, 2017

Maybe also remove the FLAKY marker from https://github.com/nodejs/node/blob/master/test/async-hooks/async-hooks.status (at least the linux one)

@refack
Copy link
Contributor

refack commented Jun 29, 2017

image

Yay, it works.

@bnoordhuis
Copy link
Member

See v8/v8@3f45368, it was made configurable upstream. We should probably just cherry-pick that patch.

API/ABI stability is not a concern because libplatform and v8::Platform is not something add-ons use.

Original commit message:

    d8: Make in process stack dumping optional

    Adds a flag (--disable-in-process-stack-traces) to not install
    signal handlers so that e.g. ASan signal handlers will work.

    This flag mirrors chromium's one.

    [email protected]
    BUG=chromium:716235

    Review-Url: https://codereview.chromium.org/2854173002
    Cr-Commit-Position: refs/heads/master@{nodejs#45142}

PR-URL: nodejs#13985
@addaleax
Copy link
Member Author

@bnoordhuis Thanks for the pointer; I’ve updated this.

CI: https://ci.nodejs.org/job/node-test-commit/10814/

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM with a comment.

assert.strictEqual(signal, 'SIGABRT');
}
}));
}
Copy link
Member

@bnoordhuis bnoordhuis Jun 29, 2017

Choose a reason for hiding this comment

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

This should probably go into test/abort. I'm kind of surprised to discover that the test from #12914 went into test/parallel, that seems wrong. (edit: oh wait, that test only runs on Windows.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Just for context: because of the core dumps taking up storage?

Copy link
Member

Choose a reason for hiding this comment

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

Correct.

Copy link
Member

Choose a reason for hiding this comment

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

Same goes for async_hooks/test-callback-error.js. But it is problematic that we don't have any abort tests running on CI, it is only because we put async_hooks/test-callback-error.js in the wrong directory that we discovered this bug.

Copy link
Member Author

Choose a reason for hiding this comment

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

I’ve moved the file.

Copy link
Member

@Trott Trott Jun 30, 2017

Choose a reason for hiding this comment

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

But it is problematic that we don't have any abort tests running on CI, it is only because we put async_hooks/test-callback-error.js in the wrong directory that we discovered this bug.

I wonder if @nodejs/build can set up a single machine that is suitable to run those tests. Not sure what is required: That it be configured to not write core dumps? Or that it have a large enough disk to write core dumps and has a clean up task to delete them when the tests are done?

Anyway, if it's possible, maybe CI runs can at least run the tests in abort on a single platform, which is better than not running them at all ever. And it might make a good prototype for also running pummel and maybe even internet tests...

Copy link
Member Author

Choose a reason for hiding this comment

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

That it be configured to not write core dumps?

That would be the easiest, I guess. I don’t see why we wouldn’t be able to just do that in the test runner:

import resource
resource.setrlimit(resource.RLIMIT_CORE, (0,0))

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if @nodejs/build can set up a single machine that is suitable to run those tests.

IMHO it won't be enough... there have been at least 3 abort related fixes in the last couple of weeks, on three different platforms...
An alternative might be a programmatic way to delete the core-dumps from within the test a la common.refreshTmpDir()

Copy link
Contributor

Choose a reason for hiding this comment

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

Since V8 5.9 V8 installs a default signal handler for some signals
when creating a default platform instance that prints a stack trace.

However, Node already does the same thing, so it would seem like the
two different stack traces would be printed; also, the V8 handler
would lead to a `SIGSEGV` under some circumstances, rather than
letting the abort continue normally.

Resolve this by disabling V8’s signal handler by default.

Fixes: nodejs#13865
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

Largely rubber stamp LGTM

@Trott
Copy link
Member

Trott commented Jun 30, 2017

This fixes test/abort/test-abort-backtrace.js, so there's one more reason to believe that running those tests somewhere regularly might be useful.

@Trott Trott mentioned this pull request Jun 30, 2017
2 tasks
@Trott
Copy link
Member

Trott commented Jul 2, 2017

Landed in 31349e2 and 4dff05f

@Trott Trott closed this Jul 2, 2017
@Trott
Copy link
Member

Trott commented Jul 2, 2017

A @refack-inspired post-land CI run: https://ci.nodejs.org/job/node-test-commit/10894/

@refack
Copy link
Contributor

refack commented Jul 2, 2017

A @refack-inspired post-land CI run: https://ci.nodejs.org/job/node-test-commit/10894/

YAY for the land! With npm-install of PI3 back to passing, we might have a green CI 💃
P.S. for general sanity I just run linuxone (it finished in < 3 minutes 😄), but this one might actualy turn green, and that's totally worth it.

@Trott
Copy link
Member

Trott commented Jul 2, 2017

I think #14029 still has to land before we get to green.

addaleax pushed a commit to addaleax/node that referenced this pull request Jul 3, 2017
Original commit message:

    d8: Make in process stack dumping optional

    Adds a flag (--disable-in-process-stack-traces) to not install
    signal handlers so that e.g. ASan signal handlers will work.

    This flag mirrors chromium's one.

    [email protected]
    BUG=chromium:716235

    Review-Url: https://codereview.chromium.org/2854173002
    Cr-Commit-Position: refs/heads/master@{nodejs#45142}

PR-URL: nodejs#13985
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax added a commit to addaleax/node that referenced this pull request Jul 3, 2017
Since V8 5.9 V8 installs a default signal handler for some signals
when creating a default platform instance that prints a stack trace.

However, Node already does the same thing, so it would seem like the
two different stack traces would be printed; also, the V8 handler
would lead to a `SIGSEGV` under some circumstances, rather than
letting the abort continue normally.

Resolve this by disabling V8’s signal handler by default.

PR-URL: nodejs#13985
Fixes: nodejs#13865
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@addaleax addaleax mentioned this pull request Jul 3, 2017
addaleax pushed a commit that referenced this pull request Jul 11, 2017
Original commit message:

    d8: Make in process stack dumping optional

    Adds a flag (--disable-in-process-stack-traces) to not install
    signal handlers so that e.g. ASan signal handlers will work.

    This flag mirrors chromium's one.

    [email protected]
    BUG=chromium:716235

    Review-Url: https://codereview.chromium.org/2854173002
    Cr-Commit-Position: refs/heads/master@{#45142}

PR-URL: #13985
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax added a commit that referenced this pull request Jul 11, 2017
Since V8 5.9 V8 installs a default signal handler for some signals
when creating a default platform instance that prints a stack trace.

However, Node already does the same thing, so it would seem like the
two different stack traces would be printed; also, the V8 handler
would lead to a `SIGSEGV` under some circumstances, rather than
letting the abort continue normally.

Resolve this by disabling V8’s signal handler by default.

PR-URL: #13985
Fixes: #13865
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit that referenced this pull request Jul 18, 2017
Original commit message:

    d8: Make in process stack dumping optional

    Adds a flag (--disable-in-process-stack-traces) to not install
    signal handlers so that e.g. ASan signal handlers will work.

    This flag mirrors chromium's one.

    [email protected]
    BUG=chromium:716235

    Review-Url: https://codereview.chromium.org/2854173002
    Cr-Commit-Position: refs/heads/master@{#45142}

PR-URL: #13985
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax added a commit that referenced this pull request Jul 18, 2017
Since V8 5.9 V8 installs a default signal handler for some signals
when creating a default platform instance that prints a stack trace.

However, Node already does the same thing, so it would seem like the
two different stack traces would be printed; also, the V8 handler
would lead to a `SIGSEGV` under some circumstances, rather than
letting the abort continue normally.

Resolve this by disabling V8’s signal handler by default.

PR-URL: #13985
Fixes: #13865
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit to targos/node that referenced this pull request Jul 21, 2017
Original commit message:

    d8: Make in process stack dumping optional

    Adds a flag (--disable-in-process-stack-traces) to not install
    signal handlers so that e.g. ASan signal handlers will work.

    This flag mirrors chromium's one.

    [email protected]
    BUG=chromium:716235

    Review-Url: https://codereview.chromium.org/2854173002
    Cr-Commit-Position: refs/heads/master@{nodejs#45142}

PR-URL: nodejs#13985
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax added a commit to targos/node that referenced this pull request Jul 21, 2017
Since V8 5.9 V8 installs a default signal handler for some signals
when creating a default platform instance that prints a stack trace.

However, Node already does the same thing, so it would seem like the
two different stack traces would be printed; also, the V8 handler
would lead to a `SIGSEGV` under some circumstances, rather than
letting the abort continue normally.

Resolve this by disabling V8’s signal handler by default.

PR-URL: nodejs#13985
Fixes: nodejs#13865
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit that referenced this pull request Jul 24, 2017
Original commit message:

    d8: Make in process stack dumping optional

    Adds a flag (--disable-in-process-stack-traces) to not install
    signal handlers so that e.g. ASan signal handlers will work.

    This flag mirrors chromium's one.

    [email protected]
    BUG=chromium:716235

    Review-Url: https://codereview.chromium.org/2854173002
    Cr-Commit-Position: refs/heads/master@{#45142}

PR-URL: #13985
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax added a commit that referenced this pull request Jul 24, 2017
Since V8 5.9 V8 installs a default signal handler for some signals
when creating a default platform instance that prints a stack trace.

However, Node already does the same thing, so it would seem like the
two different stack traces would be printed; also, the V8 handler
would lead to a `SIGSEGV` under some circumstances, rather than
letting the abort continue normally.

Resolve this by disabling V8’s signal handler by default.

PR-URL: #13985
Fixes: #13865
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@addaleax addaleax mentioned this pull request Jul 24, 2017
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Aug 1, 2017
Since V8 5.9 V8 installs a default signal handler for some signals
when creating a default platform instance that prints a stack trace.

However, Node already does the same thing, so it would seem like the
two different stack traces would be printed; also, the V8 handler
would lead to a `SIGSEGV` under some circumstances, rather than
letting the abort continue normally.

Resolve this by disabling V8’s signal handler by default.

PR-URL: nodejs#13985
Fixes: nodejs#13865
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax added a commit that referenced this pull request Aug 2, 2017
Since V8 5.9 V8 installs a default signal handler for some signals
when creating a default platform instance that prints a stack trace.

However, Node already does the same thing, so it would seem like the
two different stack traces would be printed; also, the V8 handler
would lead to a `SIGSEGV` under some circumstances, rather than
letting the abort continue normally.

Resolve this by disabling V8’s signal handler by default.

Backport-PR-URL: #14574
Backport-Reviewed-By: Anna Henningsen <[email protected]>
Backport-Reviewed-By: Refael Ackermann <[email protected]>

PR-URL: #13985
Fixes: #13865
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@addaleax addaleax mentioned this pull request Aug 2, 2017
@MylesBorins
Copy link
Contributor

assuming this doesn't apply to v6.x and 5.1. LMK if I'm mistaken

@addaleax addaleax deleted the process-abort-v859 branch August 16, 2017 08:40
hferreiro pushed a commit to brave/node that referenced this pull request Sep 27, 2017
Since V8 5.9 V8 installs a default signal handler for some signals
when creating a default platform instance that prints a stack trace.

However, Node already does the same thing, so it would seem like the
two different stack traces would be printed; also, the V8 handler
would lead to a `SIGSEGV` under some circumstances, rather than
letting the abort continue normally.

Resolve this by disabling V8’s signal handler by default.

PR-URL: nodejs/node#13985
Fixes: nodejs/node#13865
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
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 this pull request may close these issues.

10 participants