Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add breakpad support for Windows #363

Merged
merged 9 commits into from
Sep 4, 2023
Merged

Add breakpad support for Windows #363

merged 9 commits into from
Sep 4, 2023

Conversation

tustanivsky
Copy link
Collaborator

@tustanivsky tustanivsky commented Aug 25, 2023

This PR switches sentry-native backend from crashpad to breakpad. It allows Sentry to comply with the UE Marketplace publishing rules (2.6.2.e) which restrict the distribution of certain file types.

breakpad is an in-proc crash handler represented by a library unlike the crashpad which is essentially a separate app. The key difference it introduces for the crash-capturing flow is that game has to be re-launched in order to send captured crash to Sentry.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 25, 2023

Fails
🚫 Please consider adding a changelog entry for the next release.
Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Instructions and example for changelog

Please add an entry to CHANGELOG.md to the "Unreleased" section. Make sure the entry includes this PR's number.

Example:

## Unreleased

- Add breakpad support for Windows ([#363](https://github.com/getsentry/sentry-unreal/pull/363))

If none of the above apply, you can opt out of this check by adding #skip-changelog to the PR description.

Generated by 🚫 dangerJS against f47b482

@tustanivsky tustanivsky marked this pull request as draft August 25, 2023 09:01
@Edstub207
Copy link
Contributor

Edstub207 commented Aug 25, 2023

@tustanivsky I'm intrigued RE: "The key difference it introduces for the crash-capturing flow is that game has to be re-launched in order to send captured crash to Sentry." - I can imagine there are cases where the process won't be restarted on the exact same code, or even potentially the local persistence might have been reset. For example, if a Linux Server is within a Docker Container and doesn't have a mounted volume for persistence or an update has been pushed since the title crashed (meaning a brand new container / image)

Is this a hard limitation of Breakpad? Or is there something that can be done to ensure crashes are properly tracked in the above scenarios.

@tustanivsky
Copy link
Collaborator Author

Is this a hard limitation of Breakpad? Or is there something that can be done to ensure crashes are properly tracked in the above scenarios.

Yes, that's expected breakpad behavior which can't be changed by any means.

We haven't come up with the final decision regarding migrating to breakpad yet and this PR currently is more like a proof of concept thing.

@Edstub207
Copy link
Contributor

That makes sense, thanks for confirming @tustanivsky - Once it's in a testable state, let me know, I'd be up for doing some testing comparing the two.

@tustanivsky
Copy link
Collaborator Author

That makes sense, thanks for confirming @tustanivsky - Once it's in a testable state, let me know, I'd be up for doing some testing comparing the two.

The branch is ready for testing so feel free to give breakpad a try.

@Edstub207
Copy link
Contributor

Edstub207 commented Aug 29, 2023

Thanks for confirming @tustanivsky - I've tried testing this locally and didn't see crashes uploaded on a Windows Cooked Build.

The data in the .sentry-native folder doesn't appear to demonstrate any crash reports and the last_crash file has 2023-08-17T10:26:24.830Z as the timestamp, which doesn't seem correct.

However, on second boot of the client, after Sentry Init'd it did appear to try and send something, although, got a 404?

[2023.08.29-23.08.58:294][ 0]LogSentrySdk: received response: HTTP/1.1 404 Not Found Date: Tue, 29 Aug 2023 23:08:58 GMT Via: 1.1 google Content-Length: 14 Content-Type: text/plain Server: nginx Strict-Transport-Security: max-age=31536000; includeSubDomains; preload Alt-Svc: h3=":443"; ma=2592000,h3-29=":443"; ma=2592000

@tustanivsky
Copy link
Collaborator Author

@tustanivsky Did you enable the EnableAutoCrashCapturing option in plugin settings? Also, that is supposed to work for UE 5.2+ only.

@Edstub207
Copy link
Contributor

Edstub207 commented Aug 30, 2023

@tustanivsky Did you enable the EnableAutoCrashCapturing option in plugin settings? Also, that is supposed to work for UE 5.2+ only.

I'll do some testing around that today, I thought I did but maybe I missed it - Is that a requirement for breakpad?

@tustanivsky
Copy link
Collaborator Author

I'll do some testing around that today, I thought I did but maybe I missed it - Is that a requirement for breakpad?

Yes, the setup should be the same as it was for Crashpad.

@Edstub207
Copy link
Contributor

I'll do some testing around that today, I thought I did but maybe I missed it - Is that a requirement for breakpad?

Yes, the setup should be the same as it was for Crashpad.

Still been having some issues during testing, but I believe I spotted where the mistake was made earlier. Will keep on investigating.

@Edstub207
Copy link
Contributor

I'll do some testing around that today, I thought I did but maybe I missed it - Is that a requirement for breakpad?

Yes, the setup should be the same as it was for Crashpad.

@tustanivsky Based on further discussions with the team (And other projects as well) If breakpad is a route which is taken for this plugin, it might be worth having options to switch between Crashpad or Breakpad. There are some situations where breakpad is not suitable.

@tustanivsky tustanivsky changed the title Add breakpad support for Windows/Linux Add breakpad support for Windows Sep 4, 2023
@tustanivsky
Copy link
Collaborator Author

Based on further discussions with the team (And other projects as well) If breakpad is a route which is taken for this plugin, it might be worth having options to switch between Crashpad or Breakpad. There are some situations where breakpad is not suitable.

@Edstub207 For now we've decided to move on with breakpad for Windows only and leave the Linux implementation without changes. Technically it should allow the plugin to finally make its way to the UE Marketplace while avoiding potential issues with Linux Server you've mentioned above. If that goes well we'll consider adding some mechanism giving users the ability to switch between crashpad/breakpad later.

@tustanivsky tustanivsky marked this pull request as ready for review September 4, 2023 08:03
@tustanivsky tustanivsky merged commit 9992811 into main Sep 4, 2023
@tustanivsky tustanivsky deleted the feat/breakpad branch September 4, 2023 08:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants