diff --git a/CHANGELOG.md b/CHANGELOG.md index 00063c18e..0b4448c11 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ - The persisted installation ID is no longer required to be a UUID, so a platform integration can supply any ID that `user.id` accepts. ([#2177](https://github.com/getsentry/sentry-native/pull/2177)) - Parsing a timestamp with a malformed fractional-second part no longer writes a leftover debug line to `stdout`. ([#2187](https://github.com/getsentry/sentry-native/pull/2187)) +- Crashpad/Windows: Breadcrumb and scope IPC updates are no longer blocked by earlier disk writes keeping the handler's pipe instances busy. ([#2190](https://github.com/getsentry/sentry-native/pull/2190), [crashpad#174](https://github.com/getsentry/crashpad/pull/174)) **Other changes**: diff --git a/external/crashpad b/external/crashpad index d0fb04940..493151b22 160000 --- a/external/crashpad +++ b/external/crashpad @@ -1 +1 @@ -Subproject commit d0fb04940eeaf22dda8bfe0ae894d0fe96bdfcdf +Subproject commit 493151b224a6536bcedc3467d042863a063addc4 diff --git a/src/backends/sentry_backend_crashpad.cpp b/src/backends/sentry_backend_crashpad.cpp index 5e49e9ccc..b2f5cda66 100644 --- a/src/backends/sentry_backend_crashpad.cpp +++ b/src/backends/sentry_backend_crashpad.cpp @@ -1407,6 +1407,20 @@ crashpad_backend_prune_database(sentry_backend_t *backend) crashpad::PruneCrashReportDatabase(data->db, &condition); } +#ifdef SENTRY_PLATFORM_WINDOWS +static int +crashpad_backend_reinstall(sentry_backend_t *backend) +{ + auto *data = static_cast(backend->data); + if (!data->client) { + return 1; + } + + data->client->RegisterHandlers(); + return 0; +} +#endif + void sentry__backend_preload(void) { @@ -1437,6 +1451,9 @@ sentry__backend_new(void) backend->get_last_crash_func = crashpad_backend_last_crash; backend->process_old_run_func = crashpad_backend_process_old_run; backend->prune_database_func = crashpad_backend_prune_database; +#ifdef SENTRY_PLATFORM_WINDOWS + backend->reinstall_func = crashpad_backend_reinstall; +#endif backend->data = data; backend->can_capture_after_shutdown = true; diff --git a/tests/assertions.py b/tests/assertions.py index ca86d58d1..9176a63e2 100644 --- a/tests/assertions.py +++ b/tests/assertions.py @@ -585,21 +585,22 @@ def is_valid_timestamp(timestamp): return False -def _validate_breadcrumb_seq(seq, breadcrumb_func): - for i in seq: - breadcrumb = breadcrumb_func(i) - assert breadcrumb["message"] == str(i) - assert is_valid_timestamp(breadcrumb["timestamp"]) - - -def assert_overflowing_breadcrumb(attachments): - if len(attachments.breadcrumb1) > 3: - _validate_breadcrumb_seq(range(97), lambda i: attachments.breadcrumb1[3 + i]) - _validate_breadcrumb_seq( - range(97, 101), lambda i: attachments.breadcrumb2[i - 97] +def assert_overflowing_breadcrumb(attachments, count): + max_breadcrumbs = 100 + messages = ["default level is info", "debug crumb", "lf\ncrlf\r\nlf\n..."] + messages += [str(i) for i in range(count)] + total = len(messages) + start = max(0, ((total - 1) // max_breadcrumbs - 1) * max_breadcrumbs) + for file, breadcrumbs in enumerate( + (attachments.breadcrumb1, attachments.breadcrumb2) + ): + expected = [ + messages[i] for i in range(start, total) if i // max_breadcrumbs % 2 == file + ] + assert [breadcrumb["message"] for breadcrumb in breadcrumbs] == expected + assert all( + is_valid_timestamp(breadcrumb["timestamp"]) for breadcrumb in breadcrumbs ) - else: - assert_breadcrumb_inner(attachments.breadcrumb1) def assert_crashpad_upload( @@ -608,13 +609,17 @@ def assert_crashpad_upload( expect_view_hierarchy=False, expect_breadcrumbs=True, expect_default_scope=True, + breadcrumb_count=0, ): multipart = gzip.decompress(req.get_data()) msg = email.message_from_bytes(bytes(str(req.headers), encoding="utf8") + multipart) attachments = _load_crashpad_attachments(msg) if expect_breadcrumbs: - assert_overflowing_breadcrumb(attachments) + if breadcrumb_count: + assert_overflowing_breadcrumb(attachments, breadcrumb_count) + else: + assert_breadcrumb_inner(attachments.breadcrumb1) if expect_default_scope: assert_event_meta(attachments.event, integrations=["crashpad"]) if expect_attachment: diff --git a/tests/fixtures/integration.c b/tests/fixtures/integration.c index b70f2e9a0..87b6bf8db 100644 --- a/tests/fixtures/integration.c +++ b/tests/fixtures/integration.c @@ -1223,9 +1223,13 @@ main(int argc, char **argv) } if (has_arg(argc, argv, "overflow-breadcrumbs")) { - for (size_t i = 0; i < 101; i++) { - char buffer[4]; - snprintf(buffer, 4, "%zu", i); + const char *breadcrumb_count + = get_arg_value(argc, argv, "breadcrumb-count"); + size_t count + = breadcrumb_count ? strtoull(breadcrumb_count, NULL, 10) : 101; + for (size_t i = 0; i < count; i++) { + char buffer[32]; + snprintf(buffer, sizeof(buffer), "%zu", i); sentry_add_breadcrumb(sentry_value_new_breadcrumb(0, buffer)); } } diff --git a/tests/test_integration_crashpad.py b/tests/test_integration_crashpad.py index f2214f644..e3b488606 100644 --- a/tests/test_integration_crashpad.py +++ b/tests/test_integration_crashpad.py @@ -376,6 +376,9 @@ def test_crashpad_crash_proxy(cmake, httpserver, run_args, proxy_running): def test_crashpad_reinstall(cmake, httpserver): tmp_path = cmake(["sentry_test_integration"], {"SENTRY_BACKEND": "crashpad"}) + args = ["log"] + if sys.platform == "win32": + args.append("overflow-breadcrumbs") env = dict(os.environ, SENTRY_DSN=make_dsn(httpserver)) httpserver.expect_oneshot_request("/api/123456/minidump/").respond_with_data("OK") @@ -384,7 +387,7 @@ def test_crashpad_reinstall(cmake, httpserver): run( tmp_path, "sentry_test_integration", - ["log", "reinstall", "crash"], + [*args, "reinstall", "crash"], expect_failure=True, env=env, ) @@ -395,6 +398,9 @@ def test_crashpad_reinstall(cmake, httpserver): assert len(httpserver.log) == 1 + if sys.platform == "win32": + assert_crashpad_upload(httpserver.log[0][0], breadcrumb_count=101) + @pytest.mark.skipif( sys.platform == "darwin", @@ -517,7 +523,10 @@ def test_crashpad_wer_crash(cmake, httpserver, run_args): assert_session(envelope, {"status": "crashed", "errors": 1}) assert_crashpad_upload( - multipart, expect_attachment=True, expect_view_hierarchy=True + multipart, + expect_attachment=True, + expect_view_hierarchy=True, + breadcrumb_count=101, ) assert wait_for_no_werfault() @@ -608,6 +617,7 @@ def test_crashpad_dumping_crash(cmake, httpserver, run_args, build_args): multipart, expect_attachment=expect_attachments, expect_view_hierarchy=expect_attachments, + breadcrumb_count=101, ) callback = next( (arg for arg in run_args if arg in ("before-send", "on-crash")), None @@ -1413,3 +1423,64 @@ def test_crashpad_early_init(cmake): [], ) assert result.returncode == 0 + + +@pytest.mark.parametrize("count", [97, 98, 101, 997, 1000, 1097]) +@pytest.mark.parametrize( + "reinstall", + [ + False, + pytest.param( + True, + marks=pytest.mark.skipif( + sys.platform != "win32", + reason="reinstall keeps the handler only on Windows", + ), + ), + ], +) +@pytest.mark.parametrize( + "crash_arg", + [ + "crash", + pytest.param( + "fastfail", + marks=[ + pytest.mark.with_wer, + pytest.mark.skipif( + sys.platform != "win32" or bool(os.environ.get("TEST_MINGW")), + reason="fast-fail is only available in MSVC Windows builds", + ), + ], + ), + ], +) +def test_crashpad_breadcrumbs(cmake, httpserver, count, reinstall, crash_arg): + tmp_path = cmake(["sentry_test_integration"], {"SENTRY_BACKEND": "crashpad"}) + args = [ + "log", + "overflow-breadcrumbs", + "breadcrumb-count", + str(count), + "crashpad-wait-for-upload", + ] + if reinstall: + args.append("reinstall") + + httpserver.expect_oneshot_request("/api/123456/minidump/").respond_with_data("OK") + + with httpserver.wait(timeout=10) as waiting: + run( + tmp_path, + "sentry_test_integration", + [*args, crash_arg], + expect_failure=True, + env=dict(os.environ, SENTRY_DSN=make_dsn(httpserver)), + ) + + assert waiting.result + assert len(httpserver.log) == 1 + assert_crashpad_upload(httpserver.log[0][0], breadcrumb_count=count) + + if crash_arg == "fastfail": + assert wait_for_no_werfault()