diff --git a/CHANGELOG.md b/CHANGELOG.md index 3ccb95a325..034cae85eb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,10 @@ ## Unreleased +**Features**: + +- Add `before_screenshot` hook. ([#1641](https://github.com/getsentry/sentry-native/pull/1641)) + **Fixes**: - Reset client report counters during initialization ([#1632](https://github.com/getsentry/sentry-native/pull/1632)) diff --git a/include/sentry.h b/include/sentry.h index 26f7adbd33..8d85de20f4 100644 --- a/include/sentry.h +++ b/include/sentry.h @@ -1580,13 +1580,47 @@ SENTRY_API void sentry_options_add_view_hierarchy_n( * Enables or disables attaching screenshots to fatal error events. Disabled by * default. * - * This feature is currently supported by all backends on Windows. Only the - * `crashpad` backend can capture screenshots of fast-fail crashes that bypass - * SEH (structured exception handling). + * This feature is currently supported by all backends on Windows. The + * `crashpad` and `native` backends capture screenshots from an out-of-process + * handler. Only the `crashpad` backend can capture screenshots of fast-fail + * crashes that bypass SEH (structured exception handling). + * + * To decide per-event whether a screenshot should be captured, set a + * `before_screenshot` callback via `sentry_options_set_before_screenshot`. */ SENTRY_EXPERIMENTAL_API void sentry_options_set_attach_screenshot( sentry_options_t *opts, int val); +/** + * Type of the `before_screenshot` callback. + * + * The callback is invoked before a screenshot is captured for an event. It + * receives the event (without ownership) and should return non-zero to capture + * the screenshot, or zero to skip it. The `attach_screenshot` option must be + * enabled for screenshots to be considered at all. + * + * Capturing screenshots is only supported on Windows, so the callback is + * only invoked there. The callback is not supported by the `crashpad` + * backend, which captures screenshots from its out-of-process handler. + * + * The callback may be called from inside an `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 int (*sentry_before_screenshot_function_t)( + sentry_value_t event, void *user_data); + +/** + * Sets the `before_screenshot` callback. + * + * See the `sentry_before_screenshot_function_t` typedef above for more + * information. + */ +SENTRY_EXPERIMENTAL_API void sentry_options_set_before_screenshot( + sentry_options_t *opts, sentry_before_screenshot_function_t func, + void *user_data); + /** * Sets the path to the crashpad handler if the crashpad backend is used. * diff --git a/src/backends/native/sentry_crash_daemon.c b/src/backends/native/sentry_crash_daemon.c index 0798c84492..901cf9c92f 100644 --- a/src/backends/native/sentry_crash_daemon.c +++ b/src/backends/native/sentry_crash_daemon.c @@ -2392,7 +2392,7 @@ write_envelope_with_native_stacktrace(const sentry_options_t *options, } // Add screenshot attachment if captured by the daemon - if (options && options->attach_screenshot && run_folder) { + if (ctx->attach_screenshot && run_folder) { sentry_path_t *screenshot_path = sentry__path_join_str(run_folder, "screenshot.png"); if (screenshot_path) { @@ -2417,8 +2417,9 @@ write_envelope_with_native_stacktrace(const sentry_options_t *options, */ static bool write_envelope_with_minidump(const sentry_options_t *options, - const char *envelope_path, const char *event_msgpack_path, - const char *minidump_path, sentry_path_t *run_folder) + const sentry_crash_context_t *ctx, const char *envelope_path, + const char *event_msgpack_path, const char *minidump_path, + sentry_path_t *run_folder) { // Read event JSON data size_t event_size = 0; @@ -2626,7 +2627,7 @@ write_envelope_with_minidump(const sentry_options_t *options, } // Add screenshot attachment if captured by the daemon - if (options && options->attach_screenshot && run_folder) { + if (ctx->attach_screenshot && run_folder) { sentry_path_t *screenshot_path = sentry__path_join_str(run_folder, "screenshot.png"); if (screenshot_path) { @@ -2802,7 +2803,7 @@ sentry__process_crash(const sentry_options_t *options, sentry_crash_ipc_t *ipc) // This is done in the daemon process (out-of-process) because // screenshot capture is NOT signal-safe (uses LoadLibrary, GDI+, etc.) #if defined(SENTRY_PLATFORM_WINDOWS) - if (options && options->attach_screenshot && run_folder) { + if (ctx->attach_screenshot && run_folder) { SENTRY_DEBUG("Capturing screenshot"); sentry_path_t *screenshot_path = sentry__path_join_str(run_folder, "screenshot.png"); @@ -2867,7 +2868,7 @@ sentry__process_crash(const sentry_options_t *options, sentry_crash_ipc_t *ipc) // Mode 0 (MINIDUMP only) SENTRY_DEBUG("Writing envelope with minidump"); envelope_written = write_envelope_with_minidump( - options, envelope_path, event_path, minidump_path, run_folder); + options, ctx, envelope_path, event_path, minidump_path, run_folder); } if (!envelope_written) { diff --git a/src/backends/sentry_backend_breakpad.cpp b/src/backends/sentry_backend_breakpad.cpp index e57f138204..a62ca917f7 100644 --- a/src/backends/sentry_backend_breakpad.cpp +++ b/src/backends/sentry_backend_breakpad.cpp @@ -162,6 +162,16 @@ breakpad_backend_callback(const google_breakpad::MinidumpDescriptor &descriptor, } if (should_handle) { + bool capture_screenshot = options->attach_screenshot; +#ifdef SENTRY_PLATFORM_WINDOWS + if (capture_screenshot && options->before_screenshot_func) { + SENTRY_DEBUG("invoking `before_screenshot` hook"); + capture_screenshot = options->before_screenshot_func( + event, options->before_screenshot_data) + != 0; + } +#endif + sentry_envelope_t *envelope = sentry__prepare_event( options, event, nullptr, !options->on_crash_func, nullptr); sentry_session_t *session = sentry__end_current_session_with_status( @@ -180,7 +190,7 @@ breakpad_backend_callback(const google_breakpad::MinidumpDescriptor &descriptor, sentry_value_new_string(sentry__path_filename(dump_path))); } - if (options->attach_screenshot) { + if (capture_screenshot) { sentry_attachment_t *screenshot = sentry__attachment_from_path( sentry__screenshot_get_path(options)); if (screenshot diff --git a/src/backends/sentry_backend_inproc.c b/src/backends/sentry_backend_inproc.c index 9da1fce26f..edb376d8cf 100644 --- a/src/backends/sentry_backend_inproc.c +++ b/src/backends/sentry_backend_inproc.c @@ -1198,6 +1198,16 @@ process_ucontext_deferred(const sentry_ucontext_t *uctx, } TEST_CRASH_POINT("before_capture"); if (should_handle) { + bool capture_screenshot = options->attach_screenshot; +#ifdef SENTRY_PLATFORM_WINDOWS + if (capture_screenshot && options->before_screenshot_func) { + SENTRY_DEBUG("invoking `before_screenshot` hook"); + capture_screenshot = options->before_screenshot_func( + event, options->before_screenshot_data) + != 0; + } +#endif + sentry_envelope_t *envelope = sentry__prepare_event(options, event, NULL, !options->on_crash_func && !skip_hooks, NULL); // TODO(tracing): Revisit when investigating transaction flushing @@ -1207,7 +1217,7 @@ process_ucontext_deferred(const sentry_ucontext_t *uctx, SENTRY_SESSION_STATUS_CRASHED); sentry__envelope_add_session(envelope, session); - if (options->attach_screenshot) { + if (capture_screenshot) { sentry_attachment_t *screenshot = sentry__attachment_from_path( sentry__screenshot_get_path(options)); if (screenshot diff --git a/src/backends/sentry_backend_native.c b/src/backends/sentry_backend_native.c index 32328fe499..b606e73e25 100644 --- a/src/backends/sentry_backend_native.c +++ b/src/backends/sentry_backend_native.c @@ -857,6 +857,24 @@ native_backend_except(sentry_backend_t *backend, const sentry_ucontext_t *uctx) device_context, "arch", sentry_value_new_string("arm64")); # endif sentry_value_set_by_key(contexts, "device", device_context); + + // The screenshot is captured by the daemon out-of-process, so + // we invoke the hook here (in the crashing process, where + // user callbacks can run) and communicate the decision to the + // daemon by flipping attach_screenshot in the shared crash + // context. Screenshots are only captured on Windows. + if (options->attach_screenshot + && options->before_screenshot_func && state && state->ipc + && state->ipc->shmem) { + SENTRY_DEBUG("invoking `before_screenshot` hook"); + if (options->before_screenshot_func( + event, options->before_screenshot_data) + == 0) { + SENTRY_DEBUG("screenshot skipped by " + "`before_screenshot` hook"); + state->ipc->shmem->attach_screenshot = false; + } + } #endif // Write event as JSON file diff --git a/src/sentry_options.c b/src/sentry_options.c index cdb4974848..412a47492d 100644 --- a/src/sentry_options.c +++ b/src/sentry_options.c @@ -631,6 +631,14 @@ sentry_options_set_attach_screenshot(sentry_options_t *opts, int val) opts->attach_screenshot = !!val; } +void +sentry_options_set_before_screenshot(sentry_options_t *opts, + sentry_before_screenshot_function_t func, void *user_data) +{ + opts->before_screenshot_func = func; + opts->before_screenshot_data = user_data; +} + void sentry_options_set_handler_path(sentry_options_t *opts, const char *path) { diff --git a/src/sentry_options.h b/src/sentry_options.h index 23bae517e1..0ca46a3cac 100644 --- a/src/sentry_options.h +++ b/src/sentry_options.h @@ -41,6 +41,8 @@ struct sentry_options_s { bool symbolize_stacktraces; bool system_crash_reporter_enabled; bool attach_screenshot; + sentry_before_screenshot_function_t before_screenshot_func; + void *before_screenshot_data; bool crashpad_wait_for_upload; bool enable_logging_when_crashed; bool propagate_traceparent; diff --git a/tests/fixtures/screenshot/screenshot_win32.c b/tests/fixtures/screenshot/screenshot_win32.c index c3042e9a25..d8ded11a39 100644 --- a/tests/fixtures/screenshot/screenshot_win32.c +++ b/tests/fixtures/screenshot/screenshot_win32.c @@ -46,6 +46,14 @@ trigger_fastfail_crash() __fastfail(77); } +static int +before_screenshot_skip(sentry_value_t event, void *user_data) +{ + (void)event; + (void)user_data; + return 0; +} + static LRESULT CALLBACK wnd_proc(HWND hwnd, UINT uMsg, WPARAM wParam, LPARAM lParam) { @@ -74,16 +82,20 @@ int WINAPI wWinMain(HINSTANCE hInstance, HINSTANCE hPrevInstance, LPWSTR lpCmdLine, int nCmdShow) { + int argc = 0; + LPWSTR *argv = CommandLineToArgvW(lpCmdLine, &argc); + sentry_options_t *options = sentry_options_new(); sentry_options_set_release(options, "sentry-screenshot"); sentry_options_set_auto_session_tracking(options, false); sentry_options_set_attach_screenshot(options, true); sentry_options_set_debug(options, true); + if (has_arg(argc, argv, L"before-screenshot")) { + sentry_options_set_before_screenshot( + options, before_screenshot_skip, NULL); + } sentry_init(options); - int argc = 0; - LPWSTR *argv = CommandLineToArgvW(lpCmdLine, &argc); - #if (WINVER >= 0x0605) if (has_arg(argc, argv, L"dpi-unaware")) { SetProcessDpiAwarenessContext(DPI_AWARENESS_CONTEXT_UNAWARE); diff --git a/tests/test_integration_screenshot.py b/tests/test_integration_screenshot.py index 09f528e2ca..ad6c5472cd 100644 --- a/tests/test_integration_screenshot.py +++ b/tests/test_integration_screenshot.py @@ -19,6 +19,15 @@ def assert_screenshot_file(database_path): assert screenshot_path.stat().st_size > 0, "Screenshot file is empty" +def assert_no_screenshot_file(database_path): + run_dirs = [d for d in database_path.glob("*.run") if d.is_dir()] + assert ( + len(run_dirs) == 1 + ), f"Expected exactly one .run directory, found {len(run_dirs)}" + screenshot_path = run_dirs[0] / "screenshot.png" + assert not screenshot_path.exists(), "Unexpected screenshot file was captured" + + def assert_screenshot_envelope(envelope): found_screenshot = False for item in envelope.items: @@ -30,7 +39,7 @@ def assert_screenshot_envelope(envelope): assert item.headers.get("attachment_type") == "event.attachment" assert len(item.payload.bytes) > 0 found_screenshot = True - assert found_screenshot, "No screenshot item found in envelope" + return found_screenshot def assert_screenshot_upload(req): @@ -79,7 +88,9 @@ def test_capture_screenshot_native(cmake, httpserver): assert len(httpserver.log) == 1 envelope = Envelope.deserialize(httpserver.log[0][0].get_data()) - assert_screenshot_envelope(envelope) + assert ( + assert_screenshot_envelope(envelope) == True + ), "No screenshot item found in envelope" @pytest.mark.skipif( @@ -124,3 +135,55 @@ def test_capture_screenshot_crashpad(cmake, httpserver, run_args): ) def test_capture_screenshot_crashpad_wer(cmake, httpserver, run_args): test_capture_screenshot_crashpad(cmake, httpserver, run_args) + + +@pytest.mark.skipif( + sys.platform != "win32", + reason="Screenshots are only supported on Windows", +) +@pytest.mark.parametrize("backend", ["inproc", "breakpad"]) +def test_before_screenshot(cmake, httpserver, backend): + tmp_path = cmake( + ["sentry_screenshot"], {"SENTRY_BACKEND": backend, "SENTRY_TRANSPORT": "none"} + ) + + env = dict(os.environ, SENTRY_DSN=make_dsn(httpserver)) + + run( + tmp_path, + "sentry_screenshot", + ["crash", "before-screenshot"], + expect_failure=True, + env=env, + ) + + assert_no_screenshot_file(tmp_path / ".sentry-native") + + +@pytest.mark.skipif( + sys.platform != "win32", + reason="Screenshots are only supported on Windows", +) +def test_before_screenshot_native(cmake, httpserver): + tmp_path = cmake(["sentry_screenshot"], {"SENTRY_BACKEND": "native"}) + + env = dict(os.environ, SENTRY_DSN=make_dsn(httpserver)) + + httpserver.expect_oneshot_request("/api/123456/envelope/").respond_with_data("OK") + + with httpserver.wait(timeout=10) as waiting: + run( + tmp_path, + "sentry_screenshot", + ["crash", "before-screenshot"], + expect_failure=True, + env=env, + ) + + assert waiting.result + + assert len(httpserver.log) == 1 + envelope = Envelope.deserialize(httpserver.log[0][0].get_data()) + assert ( + assert_screenshot_envelope(envelope) == False + ), "Screenshot item was unexpectedly found in envelope"