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

CHANGELOG.md

@@ -2,12 +2,25 @@
 
 ## Unreleased
 
+**Features**
+
+- Provide `on_crash()` callback to allow clients to act on detected crashes
+  ([#724](https://github.com/getsentry/sentry-native/pull/724))
+
 **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**:

examples/example.c

@@ -23,6 +23,31 @@
 #    define sleep_s(SECONDS) sleep(SECONDS)
 #endif
 
+static sentry_value_t
+before_send_callback(sentry_value_t event, 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 int
+discarding_on_crash_callback(const sentry_ucontext_t *uctx, void *closure)
+{
+    // tell the backend to discard the event
+    return 0;
+}
+
+static int
+retaining_on_crash_callback(const sentry_ucontext_t *uctx, void *closure)
+{
+    // tell the backend to retain the event
+    return 1;
+}
+
 static void
 print_envelope(sentry_envelope_t *envelope, void *unused_state)
 {
@@ -105,6 +130,19 @@ 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-on-crash")) {
+        sentry_options_set_on_crash(
+            options, discarding_on_crash_callback, NULL);
+    }
+
+    if (has_arg(argc, argv, "retaining-on-crash")) {
+        sentry_options_set_on_crash(options, retaining_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,10 @@ 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 (even one that only returns 1),
+ * `before_send` will no longer be invoked for crash-events. This allows you to
+ * better differentiate between crashes and all other events.
+ *
  * 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
@@ -804,13 +807,17 @@ SENTRY_API void sentry_options_set_before_send(
  *
  * If the callback returns false outgoing crash report will be discarded.
  *
+ * If this callback is set in the options, a concurrently enabled `before_send`
+ * callback will no longer be called in the crash case. This allows for better
+ * differentiation between crashes and other events.
+ *
  * 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
  */
-typedef bool (*sentry_crash_function_t)(
+typedef int (*sentry_crash_function_t)(
     const sentry_ucontext_t *uctx, void *closure);
 
 /**

src/backends/sentry_backend_breakpad.cpp

@@ -113,8 +113,8 @@ sentry__breakpad_backend_callback(
 
         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);

src/backends/sentry_backend_inproc.c

@@ -535,8 +535,8 @@ handle_ucontext(const sentry_ucontext_t *uctx)
         }
 
         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.
 

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);

src/sentry_core.h

@@ -40,9 +40,8 @@ bool sentry__event_is_transaction(sentry_value_t event);
  * being passed in is not a transaction.
  *
  * More specifically, it will do the following things:
- * - sample the event, possibly discarding it,
  * - apply the scope to it,
- * - call the before_send hook on it,
+ * - call the before_send hook on it (if invoke_before_send == true),
  * - add the event to a new envelope,
  * - record errors on the current session,
  * - add any attachments to the envelope as well
@@ -51,7 +50,7 @@ bool sentry__event_is_transaction(sentry_value_t event);
  * `event_id` out-parameter.
  */
 sentry_envelope_t *sentry__prepare_event(const sentry_options_t *options,
-    sentry_value_t event, sentry_uuid_t *event_id);
+    sentry_value_t event, sentry_uuid_t *event_id, bool invoke_before_send);
 
 /**
  * Sends a sentry event, regardless of its type.

tests/assertions.py

@@ -1,11 +1,11 @@
 import datetime
 import email
 import gzip
-import sys
 import platform
 import re
-from .conditions import is_android
+import sys
 
+from .conditions import is_android
 
 VERSION_RE = re.compile(r"(\d+\.\d+\.\d+)(?:[-\.]?)(.*)")
 
@@ -194,6 +194,25 @@ def assert_crash(envelope):
     assert_stacktrace(envelope, inside_exception=True, check_size=False)
 
 
+def assert_crash_timestamp(has_files, tmp_path):
+    # The crash file should survive a `sentry_init` and should still be there
+    # even after restarts.
+    if has_files:
+        with open("{}/.sentry-native/last_crash".format(tmp_path)) as f:
+            crash_timestamp = f.read()
+        assert_timestamp(crash_timestamp)
+
+
+def assert_before_send(envelope):
+    event = envelope.get_event()
+    assert_matches(event, {"adapted_by": "before_send"})
+
+
+def assert_no_before_send(envelope):
+    event = envelope.get_event()
+    assert ("adapted_by", "before_send") not in event.items()
+
+
 def assert_crashpad_upload(req):
     multipart = gzip.decompress(req.get_data())
     msg = email.message_from_bytes(bytes(str(req.headers), encoding="utf8") + multipart)

tests/test_integration_stdout.py

@@ -1,10 +1,11 @@
-import pytest
+import os
 import subprocess
 import sys
-import os
 import time
+
+import pytest
+
 from . import check_output, run, Envelope
-from .conditions import has_breakpad, has_files
 from .assertions import (
     assert_attachment,
     assert_meta,
@@ -14,8 +15,11 @@
     assert_crash,
     assert_minidump,
     assert_timestamp,
-    assert_session,
+    assert_before_send,
+    assert_no_before_send,
+    assert_crash_timestamp,
 )
+from .conditions import has_breakpad, has_files
 
 
 def test_capture_stdout(cmake):
@@ -111,52 +115,118 @@ def test_multi_process(cmake):
     assert len(runs) == 0
 
 
-def test_inproc_crash_stdout(cmake):
+def run_crash_stdout_for(backend, cmake, example_args):
     tmp_path = cmake(
         ["sentry_example"],
-        {"SENTRY_BACKEND": "inproc", "SENTRY_TRANSPORT": "none"},
+        {"SENTRY_BACKEND": backend, "SENTRY_TRANSPORT": "none"},
     )
 
-    child = run(tmp_path, "sentry_example", ["attachment", "crash"])
-    assert child.returncode  # well, its a crash after all
+    child = run(tmp_path, "sentry_example", ["attachment", "crash"] + example_args)
+    assert child.returncode  # well, it's a crash after all
+
+    return tmp_path, check_output(tmp_path, "sentry_example", ["stdout", "no-setup"])
+
+
+def test_inproc_crash_stdout(cmake):
+    tmp_path, output = run_crash_stdout_for("inproc", cmake, [])
 
-    output = check_output(tmp_path, "sentry_example", ["stdout", "no-setup"])
     envelope = Envelope.deserialize(output)
 
-    # The crash file should survive a `sentry_init` and should still be there
-    # even after restarts.
-    if has_files:
-        with open("{}/.sentry-native/last_crash".format(tmp_path)) as f:
-            crash_timestamp = f.read()
-        assert_timestamp(crash_timestamp)
+    assert_crash_timestamp(has_files, tmp_path)
+    assert_meta(envelope, integration="inproc")
+    assert_breadcrumb(envelope)
+    assert_attachment(envelope)
+    assert_crash(envelope)
+
+
+def test_inproc_crash_stdout_before_send(cmake):
+    tmp_path, output = run_crash_stdout_for("inproc", cmake, ["before-send"])
+
+    envelope = Envelope.deserialize(output)
 
+    assert_crash_timestamp(has_files, tmp_path)
     assert_meta(envelope, integration="inproc")
     assert_breadcrumb(envelope)
     assert_attachment(envelope)
+    assert_crash(envelope)
+    assert_before_send(envelope)
+
+
+def test_inproc_crash_stdout_discarding_on_crash(cmake):
+    tmp_path, output = run_crash_stdout_for("inproc", cmake, ["discarding-on-crash"])
+
+    # since the on_crash() handler discards further processing we expect an empty response
+    assert len(output) == 0
+
+    assert_crash_timestamp(has_files, tmp_path)
+
+
+def test_inproc_crash_stdout_before_send_and_on_crash(cmake):
+    tmp_path, output = run_crash_stdout_for(
+        "inproc", cmake, ["before-send", "retaining-on-crash"]
+    )
+
+    # the on_crash() hook retains the event
+    envelope = Envelope.deserialize(output)
+    # but we expect no event modification from before_send() since setting on_crash() disables before_send()
+    assert_no_before_send(envelope)
 
+    assert_crash_timestamp(has_files, tmp_path)
+    assert_meta(envelope, integration="inproc")
+    assert_breadcrumb(envelope)
+    assert_attachment(envelope)
     assert_crash(envelope)
 
 
 @pytest.mark.skipif(not has_breakpad, reason="test needs breakpad backend")
 def test_breakpad_crash_stdout(cmake):
-    tmp_path = cmake(
-        ["sentry_example"],
-        {"SENTRY_BACKEND": "breakpad", "SENTRY_TRANSPORT": "none"},
-    )
+    tmp_path, output = run_crash_stdout_for("breakpad", cmake, [])
 
-    child = run(tmp_path, "sentry_example", ["attachment", "crash"])
-    assert child.returncode  # well, its a crash after all
+    envelope = Envelope.deserialize(output)
+
+    assert_crash_timestamp(has_files, tmp_path)
+    assert_meta(envelope, integration="breakpad")
+    assert_breadcrumb(envelope)
+    assert_attachment(envelope)
+    assert_minidump(envelope)
 
-    if has_files:
-        with open("{}/.sentry-native/last_crash".format(tmp_path)) as f:
-            crash_timestamp = f.read()
-        assert_timestamp(crash_timestamp)
 
-    output = check_output(tmp_path, "sentry_example", ["stdout", "no-setup"])
+@pytest.mark.skipif(not has_breakpad, reason="test needs breakpad backend")
+def test_breakpad_crash_stdout_before_send(cmake):
+    tmp_path, output = run_crash_stdout_for("breakpad", cmake, ["before-send"])
+
     envelope = Envelope.deserialize(output)
 
+    assert_crash_timestamp(has_files, tmp_path)
     assert_meta(envelope, integration="breakpad")
     assert_breadcrumb(envelope)
     assert_attachment(envelope)
-
     assert_minidump(envelope)
+    assert_before_send(envelope)
+
+
+@pytest.mark.skipif(not has_breakpad, reason="test needs breakpad backend")
+def test_breakpad_crash_stdout_discarding_on_crash(cmake):
+    tmp_path, output = run_crash_stdout_for("breakpad", cmake, ["discarding-on-crash"])
+
+    # since the on_crash() handler discards further processing we expect an empty response
+    assert len(output) == 0
+
+    assert_crash_timestamp(has_files, tmp_path)
+
+
+@pytest.mark.skipif(not has_breakpad, reason="test needs breakpad backend")
+def test_breakpad_crash_stdout_before_send_and_on_crash(cmake):
+    tmp_path, output = run_crash_stdout_for(
+        "breakpad", cmake, ["before-send", "retaining-on-crash"]
+    )
+
+    # the on_crash() hook retains the event
+    envelope = Envelope.deserialize(output)
+    # but we expect no event modification from before_send() since setting on_crash() disables before_send()
+    assert_no_before_send(envelope)
+
+    assert_crash_timestamp(has_files, tmp_path)
+    assert_meta(envelope, integration="breakpad")
+    assert_breadcrumb(envelope)
+    assert_attachment(envelope)