-
Notifications
You must be signed in to change notification settings - Fork 80
fix(ses): fix 2943 error trapping issues #2945
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
base: markm-test-2941-error-traps
Are you sure you want to change the base?
Conversation
37d8080
to
cdca569
Compare
e05786a
to
3b18865
Compare
cdca569
to
fbb0ed1
Compare
3b18865
to
4fd18f5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes error trapping issues in SES by improving error reporting when exceptions occur without catch handlers. The fix handles cases where the platform's error event may not contain an actual error object.
- Modified error event handling to fall back to the event object itself when
event.error
is not available - Updated documentation to explain platform-specific behavior differences in error reporting
Reviewed Changes
Copilot reviewed 2 out of 4 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
packages/ses/src/error/tame-console.js | Adds fallback logic to report the event object when event.error is undefined |
packages/ses/error-codes/SES_UNCAUGHT_EXCEPTION.md | Documents platform-specific error reporting behavior and fallback mechanism |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
86276e1
to
d3972dd
Compare
4fd18f5
to
079a1e1
Compare
d3972dd
to
766a1d8
Compare
079a1e1
to
bcce95e
Compare
766a1d8
to
bdf841f
Compare
bcce95e
to
cf6d174
Compare
bdf841f
to
7e11152
Compare
cf6d174
to
e28ff91
Compare
This is now Ready for Review |
9af8047
to
e5da7f3
Compare
d189d54
to
494e5ce
Compare
Staged on #2944
Closes: #2941
Refs: #XXXX
Description
In browsers, if
errorTrapping:
is set to anything other than'none'
, thenglobalWindow.addEventListener('error', event => {...});
is used to log a diagnostic that we wish contain the reason from an unhandled rejection, where the reason should be
event.error
.However, in at least some browsers,
event.error
is absent,null
, orundefined
. We don't care which. In that case, all we've got is the event, so we log that instead.Security Considerations
errorTrapping
only provides a fixed menu, with the repair doing the privileged registration of a closure. This mechanism does not give post-lockdown user code any non-logging abilities it does not otherwise have. And the logging abilities are consistent with the log being write-only.With this PR, an event object does get logged, which may contain info we would have liked to redact for normal user code. But this extra info only appears in the log, from which we never redact info anyway.
Scaling Considerations
none
Documentation Considerations
This PR does add an explanation to SES_UNCAUGHT_EXCEPTION.md, which is where it is most likely to be discovered by those who encounter this.
Testing Considerations
The relevant test were already introduced by #2944 , so this PR could illustrate the fix by the diff between it and #2944.
Compatibility Considerations
none. It is hard to imagine anything was depending on the diagnostic being inadvertently empty.
Upgrade Considerations
none