feat: Add Breakpad support for Windows

[Mixaill]

Kinda working for me. Tested on example project.

Several questions:

• Is it intended that breakpad backend sends data on second run?
• Is it OK to create .envelope even when DSN is not set? In such case .dmp is not available until DSN will be set and envelope will be sended to the sentry.
• Is there any utility to inspect/unpack .envelope ?

references #170
references #190

[jan-auer]

Thanks, @Mixaill. Answering your questions first:

• Is it intended that breakpad backend sends data on second run?

This is intended, yes. Since breakpad runs everything in-process after the crash, there are only limited possibilities. Particularly, it is no longer possible to run reliable network communication, much less to upload an entire minidump. This is why we only write it to disk and upload it on the next run.

• Is it OK to create .envelope even when DSN is not set? In such case .dmp is not available until DSN will be set and envelope will be sended to the sentry.

Excellent question. The current backend implementations (both Crashpad and Breakpad) do create the envelope and write it into the run folder. Ideally, however, you would not create the envelope. Not setting a DSN is intended to disable most of the SDK's functionality, and as such should also skip writing an envelope. This is also something for @Swatinem to chime in on.

Is there any utility to inspect/unpack .envelope ?

There is sentry__envelope_from_path, which loads an envelope. That only stores the raw payload, though, and you cannot inspect its contents. So far, we have not had the need to read an envelope once it's been serialized -- what would be your use case here?

[Mixaill]

what would be your use case here

In my case I want to display something like that I had with CrashRpt before:

Sorry for Russian, but the common sense should be clear:

• the first window with the general information and link to the Privacy Policy is displayed after crash
• the second one with submission content is displayed on Inspect button click

So, I plan to implement it something like that:

• Application crash
• Breakpad UnhandledExceptionFilter execution
• Start other process which waits for original application termination
• After original application termination, initialize sentry in second application with the same DB folder
• Display window like the first screenshot and display envelope content on inspect button
• Send envelope on Send button and delete envelope on Close button.

[jan-auer]

That sounds excellent! In case you would like to show the files contained in the envelope, you can find a reference of its structure here.

If your background-uploader works well and you would like to share it, I would love to take a look at it. Uploading from a background process sounds like a useful addition to the SDK in general, even without a UI.

[Swatinem]

This looks awesome already!

I think changing this condition here should be enough to run the relevant breakpad tests also on windows:

sentry-native/tests/conditions.py

Line 11 in dd6ec0c

has_breakpad = sys.platform == "linux"

[Swatinem]

btw, the CI failure in Create Release Archive Job is our fault and we will try to fix this today.

[Mixaill]

I think changing this condition here should be enough to run the relevant breakpad tests also on windows:

Enabled

[Swatinem]

I think you need to run make format to make python formatter happy ;-)

[Mixaill]

Well, there are two problems

• Assertion failed: !bgw->running && bgw->task_count == 0, file D:\a\1\s\src\sentry_sync.c, line 159 WinHTTP transport was destroyed before all tasks were executed?
• CI hangs on assert instead of fail.

[Swatinem]

* WinHTTP transport was destroyed before all tasks were executed?

oh, its possible we are running into this:

sentry-native/src/sentry_core.c

Lines 127 to 133 in 9651561

	// Ideally, it should default to 2s as per
	// https://docs.sentry.io/error-reporting/configuration/?platform=rust#shutdown-timeout
	// but we hit that timeout in our own integration tests, so rather
	// increase it to 5s, as it was before.
	if (!sentry__transport_shutdown(options->transport, 5000)) {
	SENTRY_DEBUG("transport did not shut down cleanly");
	}

I remember I was actually on windows when adding that comment.

That may be related to this:

sentry-native/src/sentry_sync.c

Lines 131 to 148 in 9651561

	/* TODO: this is dangerous and should be monotonic time */
	uint64_t started = sentry__msec_time();
	while (true) {
	sentry__mutex_lock(&bgw->task_lock);
	bool done = bgw->task_count == 0 && !bgw->running;
	sentry__mutex_unlock(&bgw->task_lock);
	if (done) {
	sentry__thread_join(bgw->thread_id);
	return 0;
	}
	sentry__mutex_lock(&bgw->done_signal_lock);
	sentry__cond_wait_timeout(
	&bgw->done_signal, &bgw->done_signal_lock, 250);
	sentry__mutex_unlock(&bgw->done_signal_lock);
	uint64_t now = sentry__msec_time();
	if (now > started && now - started > timeout) {
	return 1;
	}

I remember we did observe time going backwards, I think that was actually on windows @jan-auer? Maybe @mitsuhiko can also chime in, since he wrote that original thread joining code.

I cant imagine why a HTTP request to localhost may take up more than 5s, so maybe our timer is jumping wildly.

[Swatinem]

so, what should we be doing in that case anyway? In the very least, we will leak memory from the background thread. Not quite sure if we should trigger a hard assert there?

[Mixaill]

I cant imagine why a HTTP request to localhost may take up more than 5s

Firewall configuration on test machine / VM?

[eakoli]

Ill have to look through this pr more closely as I didnt run into any of these issues. But I'm also based off of 0.3.1 not master.

also whenever I see a 00000000-0000-0000-0000-000000000000.envelope its due to a second crash while processing the current crash. Which was normally because of memory corruption etc.

[Mixaill]

00000000-0000-0000-0000-000000000000.envelope contains session information. It is also created on first run if you start sentry_example with start-session argument. It is full of zeroes in filename because session does not contains event ID.

[eakoli]

@Swatinem It seems like it would be a good idea to add a way for internals to add an attachment via the event, so that the the Breakpad transport would no longer be needed.

[Swatinem]

Indeed it would! We discussed this briefly to have a more fine-grained attachment support, but we didn’t reach a conclusion there yet. The native SDK is actually the first one which has attachment support AFAIK, and one of the few that works with envelopes internally.

[Mixaill]

@Swatinem rebased on top of master with merged monotonic time

[Swatinem]

🎉 Amazing stuff, thanks for sticking with it this long ;-)

[eakoli]

@Mixaill @Swatinem I think this introduces a leak, as the join and append return a new path object.

[Swatinem]

Good catch! I think you are right.