WIP: feat(native): introduce WER mode for better WER interop#1833
Closed
jpnurmi wants to merge 2 commits into
Closed
WIP: feat(native): introduce WER mode for better WER interop#1833jpnurmi wants to merge 2 commits into
jpnurmi wants to merge 2 commits into
Conversation
Comment on lines
+1140
to
+1146
| * SEH crashes: the SDK's unhandled exception filter returns | ||
| * `EXCEPTION_CONTINUE_SEARCH`, which proceeds with normal exception | ||
| * handling and lets WER also process the crash. | ||
| * Fast-fail crashes: the WER module does not claim ownership, allowing WER | ||
| * to also process them. | ||
| */ | ||
| SENTRY_WER_MODE_SHARED = 2, |
There was a problem hiding this comment.
Bug: The SEH crash handler crash_exception_filter unconditionally returns EXCEPTION_CONTINUE_SEARCH, ignoring the wer_mode for SENTRY_WER_MODE_EXCLUSIVE and SENTRY_WER_MODE_NONE.
Severity: HIGH
Suggested Fix
Add logic to the crash_exception_filter function to check ctx->wer_mode. It should return EXCEPTION_EXECUTE_HANDLER for SENTRY_WER_MODE_EXCLUSIVE and SENTRY_WER_MODE_NONE, and EXCEPTION_CONTINUE_SEARCH for SENTRY_WER_MODE_SHARED.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: include/sentry.h#L1140-L1146
Potential issue: The SEH crash handler in `src/backends/native/sentry_crash_handler.c`,
specifically the function `crash_exception_filter`, unconditionally returns
`EXCEPTION_CONTINUE_SEARCH`. This contradicts the documented behavior for
`SENTRY_WER_MODE_EXCLUSIVE` and `SENTRY_WER_MODE_NONE`, which require the function to
return `EXCEPTION_EXECUTE_HANDLER` to prevent Windows Error Reporting (WER) from
processing the crash. The implementation fails to check the `ctx->wer_mode`, thus
violating the API contract and causing incorrect WER interop behavior for SEH crashes on
Windows.
Did we get this right? 👍 / 👎 to inform future reviews.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Allow configuring whether SEH and fast-fail crashes propagate to WER.