Skip to content

Remove the custom HANDLE stream#56

Merged
Jake-Shadle merged 3 commits intorust-minidump:mainfrom
gabrielesvelto:optional-handles-stream
Sep 24, 2022
Merged

Remove the custom HANDLE stream#56
Jake-Shadle merged 3 commits intorust-minidump:mainfrom
gabrielesvelto:optional-handles-stream

Conversation

@gabrielesvelto
Copy link
Copy Markdown
Contributor

No description provided.

@Jake-Shadle
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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