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

src: print exceptions from PromiseRejectCallback #29513

Closed

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Sep 9, 2019

Previously, leaving the exception lying around would leave the JS
engine in an invalid state, as it was not expecting exceptions to
be thrown from the C++ PromiseRejectCallback, and lead to hard
crashes under some conditions (e.g. with coverage enabled).

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

Previously, leaving the exception lying around would leave the JS
engine in an invalid state, as it was not expecting exceptions to
be thrown from the C++ `PromiseRejectCallback`, and lead to hard
crashes under some conditions (e.g. with coverage enabled).
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Sep 9, 2019
@addaleax addaleax added lib / src Issues and PRs related to general changes in the lib or src directory. promises Issues and PRs related to ECMAScript promises. labels Sep 9, 2019
@benjamingr
Copy link
Member

benjamingr commented Sep 9, 2019

I am confused, when would our PromiseRejectCallback ever throw? I am probably tired but:

I am not sure how/why handlePromiseRejections would throw. I might be completely misunderstanding things :]


Actual technical changes LGTM, I am not ✅ing simply because I did not checkout the code and reproduce what this fixed because I am not sure how to make it throw.

@addaleax
Copy link
Member Author

addaleax commented Sep 9, 2019

@benjamingr If you look at test/parallel/test-ttywrap-stack.js, you’ll see that it generates unhandled rejections near the stack limit. In that case, the PromiseRejectCallback may also throw an exception, namely a stack depth exceeded exception, when it tries to run JS code. Currently, when running the test with NODE_V8_COVERAGE set, that makes the test crash hard, because the exception isn’t used anywhere and the inspector’s console.log() code in V8 doesn’t expect to be run while there is a scheduled/pending exception.

(I’m not sure but it’s probably also possible to make the callback crash by overriding internals enough, e.g. WeakMap.prototype.set.)

Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

Thanks for explaining how to test this Anna :]

Changes LGTM

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 12, 2019
@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member

Trott commented Sep 12, 2019

Some minor JS lint errors to fix:

/home/travis/build/nodejs/node/test/parallel/test-promise-reject-callback-exception.js
   2:7  error  'common' is assigned a value but never used   no-unused-vars
  25:1  error  Expected indentation of 9 spaces but found 6  indent
  28:1  error  Expected indentation of 9 spaces but found 6  indent

@Trott Trott removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 12, 2019
@addaleax
Copy link
Member Author

Thanks, CI should be fixed now – also updated another test to account for the changes here.

@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member

Trott commented Sep 12, 2019

Documenting here so people don't do futile "Resume Build" runs: At least some CI failures appear to be relevant edge cases. One example:

https://ci.nodejs.org/job/node-test-commit-linux-containered/14774/nodes=ubuntu1604_sharedlibs_withoutintl_x64/console

test-digitalocean-ubuntu1604_sharedlibs_container-x64-10

13:09:28 not ok 1630 parallel/test-promise-reject-callback-exception
13:09:28   ---
13:09:28   duration_ms: 0.266
13:09:28   severity: fail
13:09:28   exitcode: 1
13:09:28   stack: |-
13:09:28     assert.js:373
13:09:28         throw err;
13:09:28         ^
13:09:28     
13:09:28     AssertionError [ERR_ASSERTION]: stdout: <>
13:09:28         at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-linux-containered/test/parallel/test-promise-reject-callback-exception.js:23:3)
13:09:28         at Module._compile (internal/modules/cjs/loader.js:936:30)
13:09:28         at Object.Module._extensions..js (internal/modules/cjs/loader.js:947:10)
13:09:28         at Module.load (internal/modules/cjs/loader.js:789:32)
13:09:28         at Function.Module._load (internal/modules/cjs/loader.js:702:12)
13:09:28         at Function.Module.runMain (internal/modules/cjs/loader.js:999:10)
13:09:28         at internal/main/run_main_module.js:17:11 {
13:09:28       generatedMessage: false,
13:09:28       code: 'ERR_ASSERTION',
13:09:28       actual: false,
13:09:28       expected: true,
13:09:28       operator: '=='
13:09:28     }
13:09:28   ...

@addaleax
Copy link
Member Author

@Trott Thanks, I’ve opened #29542 to track that, and added a workaround to the test for now.

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 13, 2019
@nodejs-github-bot
Copy link
Collaborator

danbev pushed a commit that referenced this pull request Sep 16, 2019
Previously, leaving the exception lying around would leave the JS
engine in an invalid state, as it was not expecting exceptions to
be thrown from the C++ `PromiseRejectCallback`, and lead to hard
crashes under some conditions (e.g. with coverage enabled).

PR-URL: #29513
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@danbev
Copy link
Contributor

danbev commented Sep 16, 2019

Landed in e095e64.

@danbev danbev closed this Sep 16, 2019
targos pushed a commit that referenced this pull request Sep 20, 2019
Previously, leaving the exception lying around would leave the JS
engine in an invalid state, as it was not expecting exceptions to
be thrown from the C++ `PromiseRejectCallback`, and lead to hard
crashes under some conditions (e.g. with coverage enabled).

PR-URL: #29513
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Sep 24, 2019
BridgeAR pushed a commit that referenced this pull request Sep 25, 2019
Previously, leaving the exception lying around would leave the JS
engine in an invalid state, as it was not expecting exceptions to
be thrown from the C++ `PromiseRejectCallback`, and lead to hard
crashes under some conditions (e.g. with coverage enabled).

PR-URL: #29513
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@gengjiawen
Copy link
Member

Just want to say thank you from 2020 😭

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. promises Issues and PRs related to ECMAScript promises.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants