FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

src: print arbitrary javascript exception value in node report by legendecas · Pull Request #38009 · nodejs/node · GitHub

/ node Public

src: print arbitrary javascript exception value in node report - #38009

Closed
legendecas wants to merge 2 commits into
nodejs:masterfrom
legendecas:node-report/exception
ConversationCommits (2)ChecksFiles changed
Closed

src: print arbitrary javascript exception value in node report#38009
legendecas wants to merge 2 commits into
nodejs:masterfrom
legendecas:node-report/exception

Conversation

legendecas commented Mar 31, 2021

Copy link
Copy Markdown
Member

Any JavaScript values can be thrown and they have to be printed in node-report to prevent confusion.

This PR is trying to print primitive values' ToString value in the report as the javascriptStack.message.

nodejs-github-bot added 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. needs-ci PRs that need a full CI run. labels Mar 31, 2021
legendecas added the report Issues and PRs related to process.report. label Mar 31, 2021
legendecas force-pushed the node-report/exception branch from 68d2012 to 8292057 Compare March 31, 2021 18:03

mhdawson commented Mar 31, 2021

Copy link
Copy Markdown
Member

@legendecas is there any related issue or additional context?

addaleax reviewed Mar 31, 2021
View reviewed changes
Comment thread src/node_report.cc Outdated

legendecas commented Apr 1, 2021

Copy link
Copy Markdown
Member Author

@mhdawson is there any related issue or additional context?

No, I didn't find any issues raised.

Please checkout the tests included in the PR. Basically the issue is that node-report is unable to print those thrown primitives as uncaught exceptions.

legendecas force-pushed the node-report/exception branch 2 times, most recently from 86ae6c4 to 545cad3 Compare April 1, 2021 16:55

This comment has been minimized.

Sign in to view
mhdawson approved these changes Apr 8, 2021
View reviewed changes

mhdawson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

mhdawson commented Apr 8, 2021

Copy link
Copy Markdown
Member

@gireeshpunathil what do you think?

This comment has been minimized.

Sign in to view
legendecas force-pushed the node-report/exception branch from 545cad3 to 3504831 Compare April 19, 2021 15:42

legendecas commented Apr 19, 2021

Copy link
Copy Markdown
Member Author

Just pushed with rebase and a fix on crash with V8_ENABLE_CHECKS flag.

This comment has been minimized.

Sign in to view
legendecas force-pushed the node-report/exception branch from 3504831 to be59b67 Compare April 19, 2021 16:37

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

nodejs-github-bot commented Apr 23, 2021

Copy link
Copy Markdown
Collaborator

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

legendecas commented Apr 23, 2021

Copy link
Copy Markdown
Member Author

Looks like the CI failures are produced on pi2-docker, mostly "warning: failed to remove out/Release/.nfs00000000018426c4000015ac: Device or resource busy". And the history looks like the failure is happening to start from days ago.

nodejs-github-bot commented Apr 24, 2021

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Apr 25, 2021

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Apr 25, 2021

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Apr 25, 2021

Copy link
Copy Markdown
Collaborator

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

legendecas added a commit that referenced this pull request Apr 26, 2021
src: print arbitrary javascript exception value in node report
55745a1 
PR-URL: #38009
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

legendecas commented Apr 26, 2021

Copy link
Copy Markdown
Member Author

Landed in 55745a1

legendecas closed this Apr 26, 2021
legendecas deleted the node-report/exception branch April 26, 2021 02:44
targos pushed a commit that referenced this pull request Apr 29, 2021
src: print arbitrary javascript exception value in node report
17856f1 
PR-URL: #38009
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
targos mentioned this pull request May 3, 2021
Merged
targos pushed a commit that referenced this pull request May 30, 2021
src: print arbitrary javascript exception value in node report
953e6f0 
PR-URL: #38009
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

addaleax addaleax left review comments
jasnell jasnell approved these changes
gireeshpunathil gireeshpunathil approved these changes
+1 more reviewer
mhdawson mhdawson approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

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. needs-ci PRs that need a full CI run. report Issues and PRs related to process.report.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL