[fuchsia] Bail if module list is empty [crashpad/crashpad : main]
Thomas Gales (Gerrit)
Thomas Gales added 1 comment![]( "Open in Gerrit")
std::optional<
std::reference_wrapper<const std::vector<ProcessReaderFuchsia::Module>>>Thomas GalesI think having to use `std::reference_wrapper` here is too cumbersome. Can we either:
- have a separate method that returns whether the modules were successfully initialized
- store a `std::optional<std::vector<..>>` and return a `const std::optional<std::vector<...>> &`
- use the fact an empty module list is unexpected and check for the vector being empty?
Used the fact that an empty module list is unexpected
Related details
- Alex Pankhurst
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Pankhurst (Gerrit)
Alex Pankhurst added 2 comments![]( "Open in Gerrit")
return false;Add a comment stating why an empty module list is unexpected and we won't be able to make a useful minidump.
Related details
- Thomas Gales
Code-Review
No-Unresolved-Comments
| 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")
Add a comment stating why an empty module list is unexpected and we won't be able to make a useful minidump.
- 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 added 5 comments![]( "Open in Gerrit")
#include <zircon/status.h>This belongs in the section above.
LOG(ERROR)
<< "zx_object_get_property ZX_PROP_PROCESS_DEBUG_ADDR, return status: "
<< zx_status_get_string(status);Use `ZX_LOG(ERROR, STATUS) << "zx_object_get_property ZX_PROP_PROCESS_DEBUG_ADDR";` instead.
if (!InitializeModules()) {
return false;Why should this block setting up `memory_map_`?
return false;Will ProcessReaderFuchsia::Modules have logged something in this case? If not, you should.
if (module->Initialize()) {This behaves contrary to the documentation you provided in the header.
What happens with your return value if ProcessReaderFuchsia::Modules gave you a module, but ModuleSnapshotElf::Initialize failed?
What happens with your return value if ModuleSnapshotElf::Initialize fails for all modules?
I’m not necessarily saying that one behavior is correct and another is incorrect, but your implementation and documentation need to agree.
Related details
- Alex Pankhurst
- Thomas Gales
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Gales (Gerrit)
Thomas Gales added 5 comments![]( "Open in Gerrit")
This belongs in the section above.
Done
LOG(ERROR)
<< "zx_object_get_property ZX_PROP_PROCESS_DEBUG_ADDR, return status: "
<< zx_status_get_string(status);Use `ZX_LOG(ERROR, STATUS) << "zx_object_get_property ZX_PROP_PROCESS_DEBUG_ADDR";` instead.
Done
if (!InitializeModules()) {
return false;Why should this block setting up `memory_map_`?
Our 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
Will ProcessReaderFuchsia::Modules have logged something in this case? If not, you should.
Done
This behaves contrary to the documentation you provided in the header.
What happens with your return value if ProcessReaderFuchsia::Modules gave you a module, but ModuleSnapshotElf::Initialize failed?
What happens with your return value if ModuleSnapshotElf::Initialize fails for all modules?
I’m not necessarily saying that one behavior is correct and another is incorrect, but your implementation and documentation need to agree.
Good point. Moved the check to the end of the function, so if for some reason ModuleSnapshotElf::Initialize fails for all modules we will still return false.
Related details
- Alex Pankhurst
- Mark Mentovai
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai 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.Commit messages should wrap at 72 columns.
if (!InitializeModules()) {
return false;Thomas GalesWhy should this block setting up `memory_map_`?
Our 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
Our 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?
Related details
- Alex Pankhurst
- Thomas Gales
Code-Review
No-Unresolved-Comments
| 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")
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
Our 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?
A 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
Related details
- Alex Pankhurst
- Mark Mentovai
Code-Review
No-Unresolved-Comments
| 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")
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?
A 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
Another consideration is if we don't plan on using the minidump, omitting it from the crash report will save on power and bandwidth
Related details
- Alex Pankhurst
- Mark Mentovai
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai added 1 comment![]( "Open in Gerrit")
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
Another consideration is if we don't plan on using the minidump, omitting it from the crash report will save on power and bandwidth
I’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.
Related details
- Alex Pankhurst
- Thomas Gales
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |