[fuchsia] Log status on error for ZX_PROP_PROCESS_DEBUG_ADDR [crashpad/crashpad : main]
Thomas Gales (Gerrit)
Thomas Gales added 2 comments![]( "Open in Gerrit")
If Crashpad encounters an error, such as for ZX_PROP_PROCESS_DEBUG_ADDR, Crashpad should propagate an error up to the caller so that Fuchsia knows that no meaningful minidump could be produced.Thomas GalesCommit messages should wrap at 72 columns.
Done
if (!InitializeModules()) {
return false;Thomas GalesWhy should this block setting up `memory_map_`?
Mark MentovaiOur thinking was that an error in module initialization should result in no minidump being produced, since a minidump without modules isn't very useful. Here's where Fuchsia calls this function, and would bail on the minidump if this function returns false: https://cs.opensource.google/fuchsia/fuchsia/+/main:src/developer/forensics/exceptions/handler/minidump.cc;l=147-151;drc=3af0c9b250edb3f1dc3660f0bd1f640176838719
If you think we should handle it differently, I'm all ears
Thomas GalesOur thinking was that an error in module initialization should result in no minidump being produced, since a minidump without modules isn't very useful.
Crashpad disagrees with this in principle.
It’s true that a minidump without modules is less useful than one with. It’s not true that it’s useless. Having some signal that there was a crash is still valuable, even if the presence of a crash is the only signal that the minidump is carrying. Usually, though, we can find even more signal by stuffing the minidump with other information that we are able to scrape together.
The bug you’re trying to solve is actually a good example of how this principle is working: you’re holding a batch of minidumps that aren’t as useful as you’d like them to be. Ideally, we would understand and correct that problem, to improve our coverage. If we just bailed out and refused to generate a minidump when we couldn’t figure out what modules ought to be included, you wouldn’t be holding this batch of minidumps, and you might be blissfully ignorant to the magnitude of the problem, or even the fact that there’s a problem at all.
Adding the status logging to help understand why this is happening is uncontentious and a very good idea. You should do that.
I don’t see a strong reason to make the system more brittle by treating failures of individual data collection activities as fatal, or by (as this patch proposes) singling out one class of data collection as more critical and more fatal than others. Why, for example, are you treating modules as critical but threads as best-effort?
Thomas GalesA couple things I think might be relevant:
- Even without a minidump, Fuchsia will still upload a crash report. Server-side, we may still be able to extract a stack trace from the logs (Fuchsia has a lightweight program print the stack trace before passing the baton to our full crash reporting flow)
- We're focusing on missing modules rather than threads because in practice we've found missing modules to be a frequent problem, but haven't encountered missing threads
Mark MentovaiAnother consideration is if we don't plan on using the minidump, omitting it from the crash report will save on power and bandwidth
Thomas GalesI’ve described how this approach would be contrary to Crashpad’s philosophy, which was set with intention. This portion of your change would cause Fuchsia’s implementation to differ from other platforms’ for what amounts to a reason of opinion. I’m not convinced.
On the other hand, you highlighted the calling code where you were hoping to pick up this signal. Since you’re holding a `crashpad::ProcessSnapshotFuchsia` in that code, it’s trivial for you to achieve your objective right there, without having to make any changes to Crashpad:
```cpp
if (process_snapshot.Modules().empty()) {
FX_LOGS(WARNING) << "Process " << process_name << ": no modules";
return {};
}
```I don’t favor that approach, but in that code, you don’t need my approval.
I still support the change you’re proposing in snapshot/fuchsia/process_reader_fuchsia.cc.
Works for me. Pared this down to just logging the status on error.
Related details
- Alex Pankhurst
- Mark Mentovai
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai voted Code-Review+1![]( "Open in Gerrit")
- Alex Pankhurst
- Thomas Gales
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Gales (Gerrit)
Thomas Gales voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Alex Pankhurst
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Gales (Gerrit)
Thomas Gales added 1 comment![]( "Open in Gerrit")
- Alex Pankhurst
- Joshua Peraza
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joshua Peraza (Gerrit)
Joshua Peraza voted Code-Review+1![]( "Open in Gerrit")
- Alex Pankhurst
- Thomas Gales
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Gales (Gerrit)
Thomas Gales voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Alex Pankhurst
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Crashpad LUCI CQ (Gerrit)
Crashpad LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[fuchsia] Log status on error for ZX_PROP_PROCESS_DEBUG_ADDR
This will help give insight as to why the error occurred.
- M snapshot/fuchsia/process_reader_fuchsia.cc
Code-Review: +1 by Mark Mentovai, +1 by Joshua Peraza
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |