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.

Sign up for GitHub

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

feat: crashpad backend allows inspecting the event-id in the on_crash/before_send hooks #840

Closed
wants to merge 3 commits into from
Closed

feat: crashpad backend allows inspecting the event-id in the on_crash/before_send hooks #840

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cd8dea9
Basic implementation w/o active flush in the handler
supervacuus May 9, 2023
e3c5f4a
explicitly flush scope in handler
supervacuus May 9, 2023
ceff52f
update change-log
supervacuus May 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@

**Features**:

- disable PC adjustment in the backend for libunwindstack ([#839](https://github.com/getsentry/sentry-native/pull/839))
- Disable PC adjustment in the backend for libunwindstack ([#839](https://github.com/getsentry/sentry-native/pull/839))
- Crashpad backend allows inspecting the event-id in the on_crash/before_send hooks ([#840](https://github.com/getsentry/sentry-native/pull/840))

**Internal**:

Expand Down
92 changes: 50 additions & 42 deletions src/backends/sentry_backend_crashpad.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,31 +78,36 @@ typedef struct {
sentry_path_t *breadcrumb1_path;
sentry_path_t *breadcrumb2_path;
size_t num_breadcrumbs;
sentry_value_t event_id;
} crashpad_state_t;

static void
sentry__crashpad_backend_user_consent_changed(sentry_backend_t *backend)
crashpad_backend_user_consent_changed(sentry_backend_t *backend)
Copy link
Member

Choose a reason for hiding this comment

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

is there a particular reason for renaming all these functions?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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 via sentry.h
  • sentry__* 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.

{
crashpad_state_t *data = (crashpad_state_t *)backend->data;
auto *data = static_cast<crashpad_state_t *>(backend->data);
if (!data->db || !data->db->GetSettings()) {
return;
}
data->db->GetSettings()->SetUploadsEnabled(!sentry__should_skip_upload());
}

static void
sentry__crashpad_backend_flush_scope(
crashpad_backend_flush_scope(
sentry_backend_t *backend, const sentry_options_t *options)
{
const crashpad_state_t *data = (crashpad_state_t *)backend->data;
auto *data = static_cast<crashpad_state_t *>(backend->data);
if (!data->event_path) {
return;
}

// This here is an empty object that we copy the scope into.
// Even though the API is specific to `event`, an `event` has a few default
// properties that we do not want here.
// properties that we do not want here. But we want a stable event_id in
// case of a crash.
sentry_value_t event = sentry_value_new_object();
if (!sentry_value_is_null(data->event_id)) {
sentry_value_set_by_key(event, "event_id", data->event_id);
}
SENTRY_WITH_SCOPE (scope) {
// we want the scope without any modules or breadcrumbs
sentry__scope_apply_to_event(scope, options, event, SENTRY_SCOPE_NONE);
Expand Down Expand Up @@ -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");
Copy link
Member

Choose a reason for hiding this comment

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

I guess there should never be the situation where data->event_id has already been set / needs to be freed first right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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_backend_flush_scope(options->backend, options);
if (options->on_crash_func) {
sentry_ucontext_t uctx;
# ifdef SENTRY_PLATFORM_WINDOWS
Expand Down Expand Up @@ -231,10 +238,10 @@ sentry__crashpad_handler(int signum, siginfo_t *info, ucontext_t *user_context)
#endif

static int
sentry__crashpad_backend_startup(
crashpad_backend_startup(
sentry_backend_t *backend, const sentry_options_t *options)
{
sentry_path_t *owned_handler_path = NULL;
sentry_path_t *owned_handler_path = nullptr;
sentry_path_t *handler_path = options->handler_path;
if (!handler_path) {
sentry_path_t *current_exe = sentry__path_current_exe();
Expand Down Expand Up @@ -272,7 +279,7 @@ sentry__crashpad_backend_startup(
"\"%" SENTRY_PATH_PRI "\"",
absolute_handler_path->path);
sentry_path_t *current_run_folder = options->run->run_path;
crashpad_state_t *data = (crashpad_state_t *)backend->data;
auto *data = static_cast<crashpad_state_t *>(backend->data);

base::FilePath database(options->database_path->path);
base::FilePath handler(absolute_handler_path->path);
Expand All @@ -283,7 +290,7 @@ sentry__crashpad_backend_startup(
// register attachments
for (sentry_attachment_t *attachment = options->attachments; attachment;
attachment = attachment->next) {
attachments.push_back(base::FilePath(attachment->path->path));
attachments.emplace_back(attachment->path->path);
}

// and add the serialized event, and two rotating breadcrumb files
Expand All @@ -299,12 +306,12 @@ sentry__crashpad_backend_startup(
sentry__path_touch(data->breadcrumb1_path);
sentry__path_touch(data->breadcrumb2_path);

attachments.push_back(base::FilePath(data->event_path->path));
attachments.push_back(base::FilePath(data->breadcrumb1_path->path));
attachments.push_back(base::FilePath(data->breadcrumb2_path->path));
attachments.insert(attachments.end(),
{ base::FilePath(data->event_path->path),
base::FilePath(data->breadcrumb1_path->path),
base::FilePath(data->breadcrumb2_path->path) });

std::vector<std::string> arguments;
arguments.push_back("--no-rate-limit");
std::vector<std::string> arguments { "--no-rate-limit" };

// Initialize database first, flushing the consent later on as part of
// `sentry_init` will persist the upload flag.
Expand Down Expand Up @@ -395,7 +402,7 @@ sentry__crashpad_backend_startup(
}

static void
sentry__crashpad_backend_shutdown(sentry_backend_t *backend)
crashpad_backend_shutdown(sentry_backend_t *backend)
{
#ifdef SENTRY_PLATFORM_LINUX
// restore signal handlers to their default state
Expand All @@ -406,7 +413,7 @@ sentry__crashpad_backend_shutdown(sentry_backend_t *backend)
}
#endif

crashpad_state_t *data = (crashpad_state_t *)backend->data;
auto *data = static_cast<crashpad_state_t *>(backend->data);
delete data->db;
data->db = nullptr;

Expand All @@ -419,10 +426,10 @@ sentry__crashpad_backend_shutdown(sentry_backend_t *backend)
}

static void
sentry__crashpad_backend_add_breadcrumb(sentry_backend_t *backend,
crashpad_backend_add_breadcrumb(sentry_backend_t *backend,
sentry_value_t breadcrumb, const sentry_options_t *options)
{
crashpad_state_t *data = (crashpad_state_t *)backend->data;
auto *data = static_cast<crashpad_state_t *>(backend->data);

size_t max_breadcrumbs = options->max_breadcrumbs;
if (!max_breadcrumbs) {
Expand Down Expand Up @@ -457,17 +464,18 @@ sentry__crashpad_backend_add_breadcrumb(sentry_backend_t *backend,
}

static void
sentry__crashpad_backend_free(sentry_backend_t *backend)
crashpad_backend_free(sentry_backend_t *backend)
{
crashpad_state_t *data = (crashpad_state_t *)backend->data;
auto *data = static_cast<crashpad_state_t *>(backend->data);
sentry__path_free(data->event_path);
sentry__path_free(data->breadcrumb1_path);
sentry__path_free(data->breadcrumb2_path);
sentry_value_decref(data->event_id);
sentry_free(data);
}

static void
sentry__crashpad_backend_except(
crashpad_backend_except(
sentry_backend_t *UNUSED(backend), const sentry_ucontext_t *context)
{
#ifdef SENTRY_PLATFORM_WINDOWS
Expand All @@ -486,7 +494,7 @@ report_crash_time(
{
// we do a `+ 1` here, because crashpad timestamps are second resolution,
// but our sessions are ms resolution. at least in our integration tests, we
// can have a session that starts at, eg. `0.471`, whereas the crashpad
// can have a session that starts at, e.g. `0.471`, whereas the crashpad
// report will be `0`, which would mean our heuristic does not trigger due
// to rounding.
uint64_t time = ((uint64_t)report.creation_time + 1) * 1000;
Expand All @@ -496,9 +504,9 @@ report_crash_time(
}

static uint64_t
sentry__crashpad_backend_last_crash(sentry_backend_t *backend)
crashpad_backend_last_crash(sentry_backend_t *backend)
{
crashpad_state_t *data = (crashpad_state_t *)backend->data;
auto *data = static_cast<crashpad_state_t *>(backend->data);

uint64_t crash_time = 0;

Expand All @@ -514,9 +522,9 @@ sentry__crashpad_backend_last_crash(sentry_backend_t *backend)
}

static void
sentry__crashpad_backend_prune_database(sentry_backend_t *backend)
crashpad_backend_prune_database(sentry_backend_t *backend)
{
crashpad_state_t *data = (crashpad_state_t *)backend->data;
auto *data = static_cast<crashpad_state_t *>(backend->data);

// We want to eagerly clean up reports older than 2 days, and limit the
// complete database to a maximum of 8M. That might still be a lot for
Expand All @@ -532,29 +540,29 @@ sentry__crashpad_backend_prune_database(sentry_backend_t *backend)
sentry_backend_t *
sentry__backend_new(void)
{
sentry_backend_t *backend = SENTRY_MAKE(sentry_backend_t);
auto *backend = SENTRY_MAKE(sentry_backend_t);
if (!backend) {
return NULL;
return nullptr;
}
memset(backend, 0, sizeof(sentry_backend_t));

crashpad_state_t *data = SENTRY_MAKE(crashpad_state_t);
auto *data = SENTRY_MAKE(crashpad_state_t);
if (!data) {
sentry_free(backend);
return NULL;
return nullptr;
}
memset(data, 0, sizeof(crashpad_state_t));

backend->startup_func = sentry__crashpad_backend_startup;
backend->shutdown_func = sentry__crashpad_backend_shutdown;
backend->except_func = sentry__crashpad_backend_except;
backend->free_func = sentry__crashpad_backend_free;
backend->flush_scope_func = sentry__crashpad_backend_flush_scope;
backend->add_breadcrumb_func = sentry__crashpad_backend_add_breadcrumb;
backend->user_consent_changed_func
= sentry__crashpad_backend_user_consent_changed;
backend->get_last_crash_func = sentry__crashpad_backend_last_crash;
backend->prune_database_func = sentry__crashpad_backend_prune_database;
data->event_id = sentry_value_new_null();

backend->startup_func = crashpad_backend_startup;
backend->shutdown_func = crashpad_backend_shutdown;
backend->except_func = crashpad_backend_except;
backend->free_func = crashpad_backend_free;
backend->flush_scope_func = crashpad_backend_flush_scope;
backend->add_breadcrumb_func = crashpad_backend_add_breadcrumb;
backend->user_consent_changed_func = crashpad_backend_user_consent_changed;
backend->get_last_crash_func = crashpad_backend_last_crash;
backend->prune_database_func = crashpad_backend_prune_database;
backend->data = data;
backend->can_capture_after_shutdown = true;

Expand Down