ref(on_crash): follow-up to make on_crash releasable (#734)

* mentioned in the header docs `before_send` vs `on_crash` behavior
* adapted `sentry__prepare_event` to provide a flag for backends on
  whether `before_send` should be invoked
* adapted backends to prevent running `before_send` even if `on_crash`
  returns true
* added `on_crash` vs `before_send` integration tests for `inproc` and
  `breakpad` (`crashpad` will follow)
* changed the signature of `on_crash` to be closer to `before_send`
* added remaining CHANGELOG entries for upcoming release

.github/workflows/ci.yml

@@ -136,7 +136,7 @@ jobs:
 
       - name: Expose llvm PATH for Mac
         if: ${{ runner.os == 'macOS' }}
-        run: echo $(brew --prefix llvm@13)/bin >> $GITHUB_PATH
+        run: echo $(brew --prefix llvm@14)/bin >> $GITHUB_PATH
 
       - name: Installing Android SDK Dependencies
         if: ${{ env['ANDROID_API'] }}

CHANGELOG.md

@@ -2,12 +2,31 @@
 
 ## Unreleased
 
+**Features**
+
+- Provide `on_crash()` callback to allow clients to act on detected crashes.
+  Users often inquired about distinguishing between crashes and "normal" events in the `before_send()` hook.
+  `on_crash()` can be considered a replacement for `before_send()` for crash events, where the goal is to use 
+  `before_send()` only for normal events, while `on_crash()` is only invoked for crashes. This change is backward
+  compatible for current users of `before_send()` and allows gradual migration to `on_crash()`
+  ([see the docs for details](https://docs.sentry.io/platforms/native/configuration/filtering/)). 
+  ([#724](https://github.com/getsentry/sentry-native/pull/724), 
+   [#734](https://github.com/getsentry/sentry-native/pull/734))
+
 **Fixes**
 
+- Make Windows ModuleFinder more resilient to missing Debug Info 
+  ([#732](https://github.com/getsentry/sentry-native/pull/732))
 - Aligned pre-send event processing in `sentry_capture_event()` with the 
   [cross-SDK session filter order](https://develop.sentry.dev/sdk/sessions/#filter-order) 
   ([#729](https://github.com/getsentry/sentry-native/pull/729))
 
+**Thank you**:
+
+Features, fixes and improvements in this release have been contributed by:
+
+- [@espkk](https://github.com/espkk)
+
 ## 0.4.18
 
 **Features**:

CONTRIBUTING.md

@@ -140,3 +140,7 @@ The example currently supports the following commends:
 - `sleep`: Introduces a 10 second sleep.
 - `add-stacktrace`: Adds the current thread stacktrace to the captured event.
 - `disable-backend`: Disables the build-configured crash-handler backend.
+- `before-send`: Installs a `before_send()` callback that retains the event.
+- `discarding-before-send`: Installs a `before_send()` callback that retains the event.
+- `on-crash`: Installs an `on_crash()` callback that retains the crash event. 
+- `discarding-on-crash`: Installs an `on_crash()` callback that discards the crash event.

examples/example.c

@@ -23,6 +23,54 @@
 #    define sleep_s(SECONDS) sleep(SECONDS)
 #endif
 
+static sentry_value_t
+before_send_callback(sentry_value_t event, void *hint, void *closure)
+{
+    (void)hint;
+    (void)closure;
+
+    // make our mark on the event
+    sentry_value_set_by_key(
+        event, "adapted_by", sentry_value_new_string("before_send"));
+
+    // tell the backend to proceed with the event
+    return event;
+}
+
+static sentry_value_t
+discarding_before_send_callback(sentry_value_t event, void *hint, void *closure)
+{
+    (void)hint;
+    (void)closure;
+
+    // discard event and signal backend to stop further processing
+    sentry_value_decref(event);
+    return sentry_value_new_null();
+}
+
+static sentry_value_t
+discarding_on_crash_callback(
+    const sentry_ucontext_t *uctx, sentry_value_t event, void *closure)
+{
+    (void)uctx;
+    (void)closure;
+
+    // discard event and signal backend to stop further processing
+    sentry_value_decref(event);
+    return sentry_value_new_null();
+}
+
+static sentry_value_t
+on_crash_callback(
+    const sentry_ucontext_t *uctx, sentry_value_t event, void *closure)
+{
+    (void)uctx;
+    (void)closure;
+
+    // tell the backend to retain the event
+    return event;
+}
+
 static void
 print_envelope(sentry_envelope_t *envelope, void *unused_state)
 {
@@ -105,6 +153,24 @@ main(int argc, char **argv)
         sentry_options_set_max_spans(options, 5);
     }
 
+    if (has_arg(argc, argv, "before-send")) {
+        sentry_options_set_before_send(options, before_send_callback, NULL);
+    }
+
+    if (has_arg(argc, argv, "discarding-before-send")) {
+        sentry_options_set_before_send(
+            options, discarding_before_send_callback, NULL);
+    }
+
+    if (has_arg(argc, argv, "on-crash")) {
+        sentry_options_set_on_crash(options, on_crash_callback, NULL);
+    }
+
+    if (has_arg(argc, argv, "discarding-on-crash")) {
+        sentry_options_set_on_crash(
+            options, discarding_on_crash_callback, NULL);
+    }
+
     sentry_init(options);
 
     if (!has_arg(argc, argv, "no-setup")) {

include/sentry.h

@@ -86,7 +86,6 @@ extern "C" {
 
 #include <inttypes.h>
 #include <stdarg.h>
-#include <stdbool.h>
 #include <stddef.h>
 
 /* context type dependencies */
@@ -771,6 +770,11 @@ SENTRY_API void sentry_options_set_transport(
  * call `sentry_value_decref` on the provided event, and return a
  * `sentry_value_new_null()` instead.
  *
+ * If you have set an `on_crash` callback (independent of whether it discards or
+ * retains the event), `before_send` will no longer be invoked for crash-events,
+ * which allows you to better distinguish between crashes and all other events
+ * in client-side pre-processing.
+ *
  * This function may be invoked inside of a signal handler and must be safe for
  * that purpose, see https://man7.org/linux/man-pages/man7/signal-safety.7.html.
  * On Windows, it may be called from inside of a `UnhandledExceptionFilter`, see
@@ -798,20 +802,48 @@ SENTRY_API void sentry_options_set_before_send(
 /**
  * Type of the `on_crash` callback.
  *
- * Does not work with crashpad on macOS.
- * The callback passes a pointer to sentry_ucontext_s structure when exception
- * handler is invoked. For breakpad on Linux this pointer is NULL.
+ * The `on_crash` callback replaces the `before_send` callback for crash events.
+ * The interface is analogous to `before_send` in that the callback takes
+ * ownership of the `event`, and should usually return that same event. In case
+ * the event should be discarded, the callback needs to call
+ * `sentry_value_decref` on the provided event, and return a
+ * `sentry_value_new_null()` instead.
  *
- * If the callback returns false outgoing crash report will be discarded.
+ * Only the `inproc` backend currently fills the passed-in event with useful
+ * data and processes any modifications to the return value. Since both
+ * `breakpad` and `crashpad` use minidumps to capture the crash state, the
+ * passed-in event is empty when using these backends, and they ignore any
+ * changes to the return value.
+ *
+ * If you set this callback in the options, it prevents a concurrently enabled
+ * `before_send` callback from being invoked in the crash case. This allows for
+ * better differentiation between crashes and other events and gradual migration
+ * from existing `before_send` implementations:
+ *
+ *  - if you have a `before_send` implementation and do not define an `on_crash`
+ *    callback your application will receive both normal and crash events as
+ *    before
+ *  - if you have a `before_send` implementation but only want to handle normal
+ *    events with it, then you can define an `on_crash` callback that returns
+ *    the passed-in event and does nothing else
+ *  - if you are not interested in normal events, but only want to act on
+ *    crashes (within the limits mentioned below), then only define an
+ *    `on_crash` callback with the option to filter (on all backends) or enrich
+ *    (only inproc) the crash event
  *
  * This function may be invoked inside of a signal handler and must be safe for
  * that purpose, see https://man7.org/linux/man-pages/man7/signal-safety.7.html.
  * On Windows, it may be called from inside of a `UnhandledExceptionFilter`, see
  * the documentation on SEH (structured exception handling) for more information
  * https://docs.microsoft.com/en-us/windows/win32/debug/structured-exception-handling
+ *
+ * Platform-specific behavior:
+ *
+ *  - does not work with crashpad on macOS.
+ *  - for breakpad on Linux the `uctx` parameter is always NULL.
  */
-typedef bool (*sentry_crash_function_t)(
-    const sentry_ucontext_t *uctx, void *closure);
+typedef sentry_value_t (*sentry_crash_function_t)(
+    const sentry_ucontext_t *uctx, sentry_value_t event, void *closure);
 
 /**
  * Sets the `on_crash` callback.

src/backends/sentry_backend_breakpad.cpp

@@ -91,6 +91,7 @@ sentry__breakpad_backend_callback(
 #else
     dump_path = sentry__path_new(descriptor.path());
 #endif
+    sentry_value_t event = sentry_value_new_event();
 
     SENTRY_WITH_OPTIONS (options) {
         sentry__write_crash_marker(options);
@@ -107,14 +108,14 @@ sentry__breakpad_backend_callback(
 #endif
 
             SENTRY_TRACE("invoking `on_crash` hook");
-            should_handle
-                = options->on_crash_func(uctx, options->on_crash_data);
+            sentry_value_t result
+                = options->on_crash_func(uctx, event, options->on_crash_data);
+            should_handle = !sentry_value_is_null(result);
         }
 
         if (should_handle) {
-            sentry_value_t event = sentry_value_new_event();
-            sentry_envelope_t *envelope
-                = sentry__prepare_event(options, event, NULL);
+            sentry_envelope_t *envelope = sentry__prepare_event(
+                options, event, nullptr, !options->on_crash_func);
             // the event we just prepared is empty,
             // so no error is recorded for it
             sentry__record_errors_on_current_session(1);
@@ -152,6 +153,7 @@ sentry__breakpad_backend_callback(
             sentry__path_free(dump_path);
         } else {
             SENTRY_TRACE("event was discarded by the `on_crash` hook");
+            sentry_value_decref(event);
         }
 
         // after capturing the crash event, try to dump all the in-flight

src/backends/sentry_backend_crashpad.cpp

@@ -43,6 +43,7 @@ extern "C" {
 extern "C" {
 
 #ifdef SENTRY_PLATFORM_LINUX
+#    include <unistd.h>
 #    define SIGNAL_STACK_SIZE 65536
 static stack_t g_signal_stack;
 
@@ -133,10 +134,10 @@ sentry__crashpad_handler(int signum, siginfo_t *info, ucontext_t *user_context)
 #    endif
     SENTRY_DEBUG("flushing session and queue before crashpad handler");
 
-    bool should_handle = true;
+    bool should_dump = true;
+    sentry_value_t event = sentry_value_new_event();
 
     SENTRY_WITH_OPTIONS (options) {
-        sentry__write_crash_marker(options);
 
         if (options->on_crash_func) {
             sentry_ucontext_t uctx;
@@ -149,17 +150,19 @@ sentry__crashpad_handler(int signum, siginfo_t *info, ucontext_t *user_context)
 #    endif
 
             SENTRY_TRACE("invoking `on_crash` hook");
-            should_handle
-                = options->on_crash_func(&uctx, options->on_crash_data);
+            event
+                = options->on_crash_func(&uctx, event, options->on_crash_data);
         } else if (options->before_send_func) {
-            sentry_value_t event = sentry_value_new_event();
             SENTRY_TRACE("invoking `before_send` hook");
             event = options->before_send_func(
                 event, nullptr, options->before_send_data);
-            sentry_value_decref(event);
         }
+        should_dump = !sentry_value_is_null(event);
+        sentry_value_decref(event);
+
+        if (should_dump) {
+            sentry__write_crash_marker(options);
 
-        if (should_handle) {
             sentry__record_errors_on_current_session(1);
             sentry_session_t *session = sentry__end_current_session_with_status(
                 SENTRY_SESSION_STATUS_CRASHED);
@@ -175,18 +178,52 @@ sentry__crashpad_handler(int signum, siginfo_t *info, ucontext_t *user_context)
                 sentry_transport_free(disk_transport);
             }
         } else {
-            SENTRY_TRACE("event was discarded by the `on_crash` hook");
+            SENTRY_TRACE("event was discarded");
         }
-
         sentry__transport_dump_queue(options->transport, options->run);
     }
 
     SENTRY_DEBUG("handing control over to crashpad");
 #    ifndef SENTRY_PLATFORM_WINDOWS
     sentry__leave_signal_handler();
 #    endif
-    // further handling can be skipped via on_crash hook
-    return !should_handle;
+
+    // If we __don't__ want a minidump produced by crashpad we need to either
+    // exit or longjmp at this point. The crashpad client handler which calls
+    // back here (SetFirstChanceExceptionHandler) does the same if the
+    // application is not shutdown via the crashpad_handler process.
+    //
+    // If we would return `true` here without changing any of the global signal-
+    // handling state or rectifying the cause of the signal, this would turn
+    // into a signal-handler/exception-filter loop, because some
+    // signals/exceptions (like SIGSEGV) are unrecoverable.
+    //
+    // Ideally the SetFirstChanceExceptionHandler would accept more than a
+    // boolean to differentiate between:
+    //
+    // * we accept our fate and want a minidump (currently returning `false`)
+    // * we accept our fate and don't want a minidump (currently not available)
+    // * we rectified the situation, so crashpads signal-handler can simply
+    //   return, thereby letting the not-rectified signal-cause trigger a
+    //   signal-handler/exception-filter again, which probably leads to us
+    //   (currently returning `true`)
+    //
+    // TODO(supervacuus):
+    // * we need integration tests for more signal/exception types not only
+    //   for unmapped memory access (which is the current crash in example.c).
+    // * we should adapt the SetFirstChanceExceptionHandler interface in
+    // crashpad
+    if (!should_dump) {
+#    ifdef SENTRY_PLATFORM_WINDOWS
+        // TerminateProcess(GetCurrentProcess(), kTerminationCodeCrashNoDump);
+        TerminateProcess(GetCurrentProcess(), 1);
+#    else
+        _exit(EXIT_FAILURE);
+#    endif
+    }
+
+    // we did not "handle" the signal, so crashpad should do that.
+    return false;
 }
 #endif
 

src/backends/sentry_backend_inproc.c

@@ -530,13 +530,13 @@ handle_ucontext(const sentry_ucontext_t *uctx)
 
         if (options->on_crash_func) {
             SENTRY_TRACE("invoking `on_crash` hook");
-            should_handle
-                = options->on_crash_func(uctx, options->on_crash_data);
+            event = options->on_crash_func(uctx, event, options->on_crash_data);
+            should_handle = !sentry_value_is_null(event);
         }
 
         if (should_handle) {
-            sentry_envelope_t *envelope
-                = sentry__prepare_event(options, event, NULL);
+            sentry_envelope_t *envelope = sentry__prepare_event(
+                options, event, NULL, !options->on_crash_func);
             // TODO(tracing): Revisit when investigating transaction flushing
             // during hard crashes.
 
@@ -552,6 +552,7 @@ handle_ucontext(const sentry_ucontext_t *uctx)
             sentry_transport_free(disk_transport);
         } else {
             SENTRY_TRACE("event was discarded by the `on_crash` hook");
+            sentry_value_decref(event);
         }
 
         // after capturing the crash event, dump all the envelopes to disk

src/sentry_core.c

@@ -411,7 +411,7 @@ sentry__capture_event(sentry_value_t event)
         if (sentry__event_is_transaction(event)) {
             envelope = sentry__prepare_transaction(options, event, &event_id);
         } else {
-            envelope = sentry__prepare_event(options, event, &event_id);
+            envelope = sentry__prepare_event(options, event, &event_id, true);
         }
         if (envelope) {
             if (options->session) {
@@ -460,7 +460,7 @@ sentry__should_send_transaction(sentry_value_t tx_cxt)
 
 sentry_envelope_t *
 sentry__prepare_event(const sentry_options_t *options, sentry_value_t event,
-    sentry_uuid_t *event_id)
+    sentry_uuid_t *event_id, bool invoke_before_send)
 {
     sentry_envelope_t *envelope = NULL;
 
@@ -477,7 +477,7 @@ sentry__prepare_event(const sentry_options_t *options, sentry_value_t event,
         sentry__scope_apply_to_event(scope, options, event, mode);
     }
 
-    if (options->before_send_func) {
+    if (options->before_send_func && invoke_before_send) {
         SENTRY_TRACE("invoking `before_send` hook");
         event
             = options->before_send_func(event, NULL, options->before_send_data);