Skip to content

Add support for the MINIDUMP_HANDLE_DATA_STREAM to Linux#94

Merged
gabrielesvelto merged 2 commits intorust-minidump:mainfrom
gabrielesvelto:linux-handle-data-stream
Nov 3, 2023
Merged

Add support for the MINIDUMP_HANDLE_DATA_STREAM to Linux#94
gabrielesvelto merged 2 commits intorust-minidump:mainfrom
gabrielesvelto:linux-handle-data-stream

Conversation

@gabrielesvelto
Copy link
Copy Markdown
Contributor

No description provided.

@gabrielesvelto
Copy link
Copy Markdown
Contributor Author

This relies on my changes in PR rust-minidump/rust-minidump#885 so I need to land that one first and cut a new release of the rust-minidump crate.

@gabrielesvelto gabrielesvelto force-pushed the linux-handle-data-stream branch 2 times, most recently from e63eba4 to f32b32c Compare October 26, 2023 10:20
@Jake-Shadle
Copy link
Copy Markdown
Collaborator

At lunch now, will take a look when I get back.

Comment thread src/linux/minidump_writer.rs Outdated
Comment on lines +330 to +334
let dirent = handle_data_stream::write(self, buffer)?;
dir_section.write_to_file(buffer, Some(dirent))?;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do have a bit of a concern about the potential fallibility of this operation, if it fails we generate no dump at all, even when all previous sections have been dumped successfully, but I don't know if this will matter in real world usage or not. I suppose if Firefox deploys this change and doesn't see it negatively impacting successful dump creation then it's not a big deal, just something I want to bring up.

Actually, I think the linux dumper just needs to be changed so that every section can fallback to being not written if it fails internally due to whatever reason with the hopes that at least some sections can be written so there is at least something being dumped rather than 1 failure leaving the crash with no diagnosable artifact, but that can be discussed separately.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I think the linux dumper just needs to be changed so that every section can fallback to being not written if it fails internally due to whatever reason with the hopes that at least some sections can be written so there is at least something being dumped rather than 1 failure leaving the crash with no diagnosable artifact, but that can be discussed separately.

I agree. I've been monitoring all the possible failure modes when writing minidumps in Firefox and not being able to write a section doesn't happen very often... but it does happen. I can make this particular operation fallible in the meantime.

BTW if we discuss making all the non-necessary sections fallible it would be nice to introduce a mechanism to gather multiple errors. Right now we know what will cause a failure but it's an all-or-nothing situation. It would be nice to have a mechanism so that we get a list of errors to distinguish between scenarios like "we could write the minidump but we don't have the file descriptors" or "we could only partially write a minidump, threads X, Y & Z are missing because we couldn't stop them", etc...

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW if we discuss making all the non-necessary sections fallible it would be nice to introduce a mechanism to gather multiple errors. Right now we know what will cause a failure but it's an all-or-nothing situation. It would be nice to have a mechanism so that we get a list of errors to distinguish between scenarios like "we could write the minidump but we don't have the file descriptors" or "we could only partially write a minidump, threads X, Y & Z are missing because we couldn't stop them", etc...

Totally agree, I'll open up a separate issue for this....oh wait, I already did and forgot about it #31.

@gabrielesvelto gabrielesvelto force-pushed the linux-handle-data-stream branch 2 times, most recently from 1e6ac65 to 883b711 Compare November 2, 2023 15:48
@gabrielesvelto gabrielesvelto force-pushed the linux-handle-data-stream branch from 883b711 to 2d59eb2 Compare November 2, 2023 15:52
@gabrielesvelto
Copy link
Copy Markdown
Contributor Author

@Jake-Shadle I've bumped a couple of dependencies to align this code to rust-minidump (which in turn was aligned with symbolic). If you're OK with those changes I'll merge this.

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