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

Properly pass null exception to error handlers #3185

Merged
merged 5 commits into from
May 22, 2021
Merged

Properly pass null exception to error handlers #3185

merged 5 commits into from
May 22, 2021

Conversation

mbest
Copy link
Contributor

@mbest mbest commented May 7, 2021

Check for null or undefined error values before trying to access properties.

Fixes #3183

Copy link
Member

@domenic domenic 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 working on this!

Per CONTRIBUTING.md, no new tests should be added to to-port-to-wpts/ (as that increases our porting burden). Instead, the test for the onerror handler should be added to the appropriate location in web-platform-tests/to-upstream, and the test for the virtual console behavior should be added to api/jsdom-errors.js

@mbest
Copy link
Contributor Author

mbest commented May 7, 2021

Can you tell me the appropriate location?

Per CONTRIBUTING.md, no new tests should be added to to-port-to-wpts/ (as that increases our porting burden). Instead, the test for the onerror handler should be added to the appropriate location in web-platform-tests/to-upstream, and the test for the virtual console behavior should be added to api/jsdom-errors.js

@mbest
Copy link
Contributor Author

mbest commented May 7, 2021

I think I got the right place. Let me know.

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

I've told GitHub to run the CI. I think it will fail on the lint in at least some cases.

Thanks for moving the tests. I think a better location for web platform tests would be html/webappapis/scripting/processing-model-2 (you'll need to create a couple folders), instead of dom/events. That's where existing tests for uncaught exceptions live in upstream.

@mbest
Copy link
Contributor Author

mbest commented May 8, 2021

Thanks for pointing out the lint issues. These are fixed now. I picked the WPT location based on a search for tests that involve throwing errors in an event handler. Although I see a lot of error-related tests in processing-model-2, I don't see any related to event handlers.

@TimothyGu
Copy link
Member

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

Almost there :)

@mbest mbest requested a review from domenic May 10, 2021 20:27
Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

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

Sorry, one last thing. Otherwise LGTM.

@mbest
Copy link
Contributor Author

mbest commented May 20, 2021

@domenic Anything else?

@domenic domenic merged commit 39b7972 into jsdom:master May 22, 2021
@mbest mbest deleted the handle-thrown-null branch November 13, 2023 20:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

throw undefined turns into a TypeError: Cannot read property 'message' of undefined
3 participants