feat: crashpad backend allows inspecting the event-id in the on_crash/before_send hooks #840
Conversation
|
supervacuus
force-pushed
the
feat/crashpad_hooks_support_event_id
branch
from
6a2f469 to
ceff52f
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #840 +/- ##
==========================================
+ Coverage 82.08% 82.56% +0.47%
==========================================
Files 52 53 +1
Lines 7141 7354 +213
Branches 1149 1185 +36
==========================================
+ Hits 5862 6072 +210
+ Misses 1181 1173 -8
- Partials 98 109 +11 |
|
So, this basic implementation "works" as it does what it's supposed to do. But as mentioned in the second paragraph here: #779 (comment), why don't we take this further? According to this comment in relay, And yes, we don't have to flush the scope in the crash handler as suggested in this PR, we can also only serialize the |
| @@ -141,7 +146,9 @@ sentry__crashpad_handler(int signum, siginfo_t *info, ucontext_t *user_context) | |||
| sentry_value_t event = sentry_value_new_event(); | |||
|
|
|||
| SENTRY_WITH_OPTIONS (options) { | |||
|
|
|||
| auto *data = static_cast<crashpad_state_t *>(options->backend->data); | |||
| data->event_id = sentry_value_get_by_key(event, "event_id"); | |||
There was a problem hiding this comment.
I guess there should never be the situation where data->event_id has already been set / needs to be freed first right?
There was a problem hiding this comment.
It is a good question. If we end up in the handler function the way we currently return to the FirstChanceHandler (or plain exit at the end), there is practically no way for the handler to execute again. And since it is the first mutation of event_id where an allocation happens, I guess we are fine (also considering that we are in the process of crashing).
| } crashpad_state_t; | ||
|
|
||
| static void | ||
| sentry__crashpad_backend_user_consent_changed(sentry_backend_t *backend) | ||
| crashpad_backend_user_consent_changed(sentry_backend_t *backend) |
There was a problem hiding this comment.
is there a particular reason for renaming all these functions?
There was a problem hiding this comment.
The main reason is that across the modules, it seemed that the naming scheme is (or was at some point):
sentry_*for all publicly available interfaces viasentry.hsentry__*for all private interfaces which cross module boundaries (i.e., are only internally exposed via module headers)- no prefix for all static (i.e., module-private) functions, which shouldn't easily conflict in any "namespace"
So one part is trivial consistency, but it also gets easier to navigate if I know from the name if there might be an external contract to consider.
|
closing in favor of #843 |
fixes #779