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

Remove the custom HANDLE stream #56

Merged

Conversation

gabrielesvelto
Copy link
Contributor

No description provided.

@Jake-Shadle
Copy link
Collaborator

I'm wondering if we should even have this at all any longer? I only added this because it was present in Breakpad, but Crashpad doesn't include it and giving how much they have over-engineered everything in Crashpad that kind of leads me to believe that this handle stream was actually not worth porting over?

@gabrielesvelto
Copy link
Contributor Author

I didn't know that code came from Breakpad... we never used it and a quick glance at Microsoft docs reveals that there's a "standard" way of getting that data using the MiniDumpWithHandleData option which should yield a MINIDUMP_HANDLE_DATA_STREAM.

If you're OK with I say we drop it.

@Jake-Shadle
Copy link
Collaborator

Jake-Shadle commented Sep 23, 2022

Cool, I generally feel if neither Chrome nor Firefox use something it's a fairly safe bet that most other projects with (hopefully) fewer crash reports/users are not going to find it useful as well, I think it's fine to just nuke it. If someone complains it's not there we can just add it back behind a feature flag in the future.

@gabrielesvelto gabrielesvelto changed the title Make generation of the custom HANDLE stream optional Remove the custom HANDLE stream Sep 23, 2022
@gabrielesvelto
Copy link
Contributor Author

Thanks! I seem to have flushed out the last test failure, can you merge?

@Jake-Shadle Jake-Shadle merged commit 8375951 into rust-minidump:main Sep 24, 2022
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