Add support for attachments to generic crash report database [crashpad/crashpad : master]
Scott Graham (Gerrit)
How do you feel about this approach? I'm not too familiar with the _generic database, so I'm kind of just poking around.
crash 41d448fdbec02523 is an example report of what pops out, via a direct user of CrashReportExceptionHandler on Fuchsia.
To view, visit change 1060419. To unsubscribe, or for help writing mail filters, visit settings.
Joshua Peraza (Gerrit)
4 comments:
File client/crash_report_database.h:
//! \brief Obtains names and file readers for any attachments for the report.
//!
//! Callers should use this method after calling GetReportForUploading() and
//! and before calling RecordUploadComplete(), reading the contents of the
//! attachment using the given FileReader, and using the name in the upload.
//!
//! \note This method is only implemented for the generic database
//! implementation which is used on platforms other than macOS and Windows.
//!
//! \return A map of base filenames to file readers.
virtual std::map<std::string, std::unique_ptr<FileReader>>
GetAttachmentsForReport(const UUID& uuid) {
return std::map<std::string, std::unique_ptr<FileReader>>();Rather than requiring this to be called between GetReportForUploading() and RecordUploadComplete(), we could put this interface into UploadReport itself, guaranteeing it's called correctly.
File client/crash_report_database_generic.cc:
// Attempt to remove any associated attachments. There may not be any, so
// failing is not an error.We'll also want to remove attachments for corrupted reports in CleaningReadMetadata() and CleanReportsInState().
Probably a good idea to LocateAndLock() the report too.
File handler/fuchsia/crash_report_exception_handler.cc:
if (process_attachments_) {
// Note that attachments are read at this point each time rather than once
// so that if the contents of the file has changed it will be re-read for
// each upload (e.g. in the case of a log file).
for (const auto& it : *process_attachments_) {
std::string contents;
if (!LoggingReadEntireFile(it.second, &contents)) {
// Not being able to read the file isn't considered fatal, and should
// not prevent the report from being processed.
continue;
}
database_->WriteAttachment(uuid, it.first, contents);
}
}Probably not too big a deal since attachments are best effort and it'd probably be super rare, but there's a race here if CrashReportUploadThread has watch_pending_reports set. It could upload and delete the report while we're writing attachments. More reason to have WriteAttachment lock the report. If it can't find/lock the report, it should avoid writing the attachment.
To view, visit change 1060419. To unsubscribe, or for help writing mail filters, visit settings.
Scott Graham (Gerrit)
(no new patchset yet)
4 comments:
//! \brief Obtains names and file readers for any attachments for the report.
//!
//! Callers should use this method after calling GetReportForUploading() and
//! and before calling RecordUploadComplete(), reading the contents of the
//! attachment using the given FileReader, and using the name in the upload.
//!
//! \note This method is only implemented for the generic database
//! implementation which is used on platforms other than macOS and Windows.
//!
//! \return A map of base filenames to file readers.
virtual std::map<std::string, std::unique_ptr<FileReader>>
GetAttachmentsForReport(const UUID& uuid) {
return std::map<std::string, std::unique_ptr<FileReader>>();
Rather than requiring this to be called between GetReportForUploading() and RecordUploadComplete(), […]
That makes a lot more sense!
(It's a bit awkward because UploadReport needs to know the UUID in the _generic implementation, but not otherwise. I guess it's OK to add it to the cross-platform one, and then delegate back to a helper in _generic to actually do the work.)
File client/crash_report_database_generic.cc:
// Attempt to remove any associated attachments. There may not be any, so
// failing is not an error.
We'll also want to remove attachments for corrupted reports in CleaningReadMetadata() and CleanRepor […]
Done
Probably a good idea to LocateAndLock() the report too.
Done (but maybe obviated by next comment)
File handler/fuchsia/crash_report_exception_handler.cc:
if (process_attachments_) {
// Note that attachments are read at this point each time rather than once
// so that if the contents of the file has changed it will be re-read for
// each upload (e.g. in the case of a log file).
for (const auto& it : *process_attachments_) {
std::string contents;
if (!LoggingReadEntireFile(it.second, &contents)) {
// Not being able to read the file isn't considered fatal, and should
// not prevent the report from being processed.
continue;
}
database_->WriteAttachment(uuid, it.first, contents);
}
}
Probably not too big a deal since attachments are best effort and it'd probably be super rare, but t […]
Ah right, good catch. That kind of sucks. Maybe it'd be better to add an optional attachments map to FinishedWritingCrashReport() so that it can be done atomically?
That'd mean a bunch of nullptrs for all the non-using calls to that, and the drawback is that we'd be reading all the contents of all the attachments into memory here.
That's probably not a huge problem at the moment, but not great. (On the other hand, I believe the http builder will be doing that when it builds the request too.)
Yet another way to go would be to follow the way that the main report is written. i.e. have PrepareNewCrashReport() include a request for a list of FileWriters that the database creates in the attachments directory, and then store a map in NewReport for the report creator to write. That seems like probably the way to go. But then we get into the complexity of scoped file removers for the already created files on failure.
Hmm... I'll ponder for a while, but I'm leaning towards that last idea. WDYT?
To view, visit change 1060419. To unsubscribe, or for help writing mail filters, visit settings.
Joshua Peraza (Gerrit)
2 comments:
//! \brief Obtains names and file readers for any attachments for the report.
//!
//! Callers should use this method after calling GetReportForUploading() and
//! and before calling RecordUploadComplete(), reading the contents of the
//! attachment using the given FileReader, and using the name in the upload.
//!
//! \note This method is only implemented for the generic database
//! implementation which is used on platforms other than macOS and Windows.
//!
//! \return A map of base filenames to file readers.
virtual std::map<std::string, std::unique_ptr<FileReader>>
GetAttachmentsForReport(const UUID& uuid) {
return std::map<std::string, std::unique_ptr<FileReader>>();
That makes a lot more sense! […]
It already knows its UUID from Report. :)
File handler/fuchsia/crash_report_exception_handler.cc:
if (process_attachments_) {
// Note that attachments are read at this point each time rather than once
// so that if the contents of the file has changed it will be re-read for
// each upload (e.g. in the case of a log file).
for (const auto& it : *process_attachments_) {
std::string contents;
if (!LoggingReadEntireFile(it.second, &contents)) {
// Not being able to read the file isn't considered fatal, and should
// not prevent the report from being processed.
continue;
}
database_->WriteAttachment(uuid, it.first, contents);
}
}
Ah right, good catch. That kind of sucks. […]
Could also do something like:
FileWriter* NewReport::AddAttachment(std::string filename)and have NewReport keep unique_ptr vectors of FileWriters/ScopedRemoveFiles opened on the fly when AddAttachment is called.
To view, visit change 1060419. To unsubscribe, or for help writing mail filters, visit settings.
Scott Graham (Gerrit)
Thanks! PTAL at the current attempt and see what you think.
2 comments:
//! empty on entry. Only valid if this returns #kNoError.
//!
//! \return The operation status code.
virtual OperationStatus GetCompletedReports(std::vector<Report>* reports) = 0;
//! \brief Obtains and locks a report object for uploading to a collection
//! server.
//!
//! Callers should upload the crash report using the FileReader provided.
//! Callers should then call RecordUploadComplete() to record a successful
//! upload. If RecordUploadComplete() is not called, the upload attempt will
//! be recorded as unsuccessful and the report lock released when \a report is
//! destroyed.It already knows its UUID from Report. […]
Oh, of course. I kept getting mixed up between NewReport and UploadReport.
File handler/fuchsia/crash_report_exception_handler.cc:
std::string contents;
if (!LoggingReadEntireFile(it.second, &contents)) {
// Not being able to read the file isn't considered fatal, and
// should not prevent the report from being processed.
continue;
}
writer->Write(contents.data(), contents.size());
}
}
}
UUID uuid;
database_status =
database_->FinishedWritingCrashReport(std::move(new_report), &uuid);
iCould also do something like: […]
That setup seems to work pretty well.
To view, visit change 1060419. To unsubscribe, or for help writing mail filters, visit settings.
Joshua Peraza (Gerrit)
9 comments:
File client/crash_report_database.h:
std::vector<std::unique_ptr<FileWriter>> writers_;
std::vector<ScopedRemoveFile> removers_;I think including the writer/remover for the main report in these vectors doesn't get us a whole lot. In all cases, we're either just looking at the main report or just looking at the attachments.
File client/crash_report_database.cc:
: writers_(1), removers_(1), uuid_(), database_() {
writers_[kIndexOfMainReport] = std::make_unique<FileWriter>();
removers_[kIndexOfMainReport]and putting them all the in the same vector results in some funky situations. Defining kIndexOfMainReport suggests to me that it can be changed, but the hard-coded 1s here require that it remain 0.
DCHECK_GT(writers_.size(), kIndexOfMainReport);
DCHECK_GT(removers_.size(), kIndexOfMainReport);but if you do keep them in a shared vector, I think these would be more appropriate in NewReport's constructor, before assigning to writers_/removers_ for the main report. If it's going to fail, I think it will be there.
File client/crash_report_database_generic.cc:
Patch Set #15, Line 421:
for (size_t i = NewReport::kIndexOfMainReport + 1Things like this read funky in isolation. It makes me wonder what's kept in the vector before the main report.
Patch Set #15, Line 603:
return removed;I think we also need something here that purges attachments when both the metadata and report file are missing from the database.
File client/crash_report_database_test.cc:
Patch Set #15, Line 675:
supportsupported
Patch Set #15, Line 679:
EXPECTASSERT
Patch Set #15, Line 692:
EXPECThere too
File handler/crash_report_upload_thread.h:
Patch Set #15, Line 165:
const std::map<std::string, std::unique_ptr<FileReader>>& attachments,No need to pass this anymore, right? UploadReport() can just grab them itself.
To view, visit change 1060419. To unsubscribe, or for help writing mail filters, visit settings.
Scott Graham (Gerrit)
Thanks!
8 comments:
std::vector<ScopedRemoveFile> attachment_removers_;
UUID uuid_;I think including the writer/remover for the main report in these vectors doesn't get us a whole lot […]
Yeah, you're right. I was sort of hoping there would be some point where it which it was tidier, but it's just awkward. Separated.
File client/crash_report_database.cc:
attachment_writers_(),
attachment_removers_(),
uuid_(),and putting them all the in the same vector results in some funky situations. […]
Done
File client/crash_report_database_generic.cc:
Patch Set #15, Line 421:
for (auto& writer : report->attachment_writers_)Things like this read funky in isolation. […]
Done
Patch Set #15, Line 603:
return removed;
I think we also need something here that purges attachments when both the metadata and report file a […]
Tricky! So much state.
Just to confirm, you mean the case where the attachments have become completely, uh, detached, so it'd be a search across all attachments to see which attachment directory uuids have no corresponding report (in any state), is that right? Had a go at that.
File client/crash_report_database_test.cc:
Patch Set #15, Line 675:
supportsupported
Done
Patch Set #15, Line 679:
ASSERTASSERT
Done
Patch Set #15, Line 692:
ASSERThere too
Done
File handler/crash_report_upload_thread.h:
Patch Set #15, Line 165:
orkerThread::Delegate:
No need to pass this anymore, right? UploadReport() can just grab them itself.
Duh, of course.
To view, visit change 1060419. To unsubscribe, or for help writing mail filters, visit settings.
Joshua Peraza (Gerrit)
4 comments:
Patch Set #18, Line 19:
Cpp11I don't know if it's an option here, but we're up to c++14 now.
File client/crash_report_database.h:
Patch Set #18, Line 164:
std::map<std::string, std::unique_ptr<FileReader>>It's a little inconsistent that this returns the FileReaders in their own unique_ptrs, rather than keeping them owned by the UploadReport like the report file's reader. Maybe it's overkill to put the file reader's in a vector like new report did? I think we'll at least want a comment specifying that the attachment file readers shouldn't be kept alive longer than the UploadReport is.
File client/crash_report_database_generic.cc:
Patch Set #18, Line 859:
kSearchablekSearchable means kPending or kCompleted, which means there's a race here if there's a call to CleanDatabase() while a new report with attachments is being written.
You could:
1. Add a state which means kNew, kPending, or kCompleted. You wouldn't want to lock reports in new (they are treated like lock files themselves as long as they're in new since there's no interface for other database clients to see that they exist until they're moved to pending). You'd probably also want to search in a new -> pending -> completed order, just in case the report is being moved between states.2. Create the attachments in the new directory and move them to the attachments directory when calling FinishedWritingCrashReport(), just like we do for the report files. Lost attachments in the new directory could be purged using timestamps, just like for the report files.
3. Do nothing. Accept that in the unlikely event we are writing a new report with a file attachment when our timer for CleanDatabase() goes off, we might lose an attachment, which are best effort anyways. Maybe leave a comment.
Patch Set #18, Line 860:
os == kNoErrorProbably better to leave the attachments unless it specifically returns kReportNotFound.
To view, visit change 1060419. To unsubscribe, or for help writing mail filters, visit settings.
Scott Graham (Gerrit)
Thanks!
4 comments:
I don't know if it's an option here, but we're up to c++14 now.
Oops, didn't mean to include that in this CL. I'll add it separately (I tried using git cl format and it reformats some end-of-template->> incorrectly without this.
File client/crash_report_database.h:
Patch Set #18, Line 164:
std::map<std::string, FileReader*> GetAttachments(It's a little inconsistent that this returns the FileReaders in their own unique_ptrs, rather than k […]
Good point, makes sense to keep them the same. Had to reshuffle a little to keep GetAttachments() const (and UploadThread receives a const UploadReport, which I think makes sense).
File client/crash_report_database_generic.cc:
kSearchable means kPending or kCompleted, which means there's a race here if there's a call to Clean […]
Oh bummer. I tried to do #1.
Probably better to leave the attachments unless it specifically returns kReportNotFound.
Done
To view, visit change 1060419. To unsubscribe, or for help writing mail filters, visit settings.
Joshua Peraza (Gerrit)
Awesomesauce
Patch set 20:Code-Review +1
1 comment:
File client/crash_report_database_generic.cc:
Patch Set #20, Line 851:
uuid.InitializeFromString(filename.value());Maybe a good idea to skip the directory if this returns false.
To view, visit change 1060419. To unsubscribe, or for help writing mail filters, visit settings.
Scott Graham (Gerrit)
Thank you for your patience! :)
Patch set 21:Commit-Queue +2
1 comment:
File client/crash_report_database_generic.cc:
Patch Set #20, Line 851:
if (!uuid.InitializeFromString(filename.valu
Maybe a good idea to skip the directory if this returns false.
Done
To view, visit change 1060419. To unsubscribe, or for help writing mail filters, visit settings.
Commit Bot (Gerrit)
Commit Bot merged this change.
Add support for attachments to generic crash report database
This is the beginning of support for attachments at the process level
being stored alongside a report. Attachments will be uploaded by key as
part of the multipart http upload. There's no interface at the client
level yet to pass these through.
As this is intended for Fuchsia, this is not yet implemented for the
Mac/Windows database implementations.
Bug: crashpad:196
Change-Id: Ieaf580675164b631d313193f97375710979ba2a9
Reviewed-on: https://chromium-review.googlesource.com/1060419
Commit-Queue: Scott Graham <sco...@chromium.org>
Reviewed-by: Joshua Peraza <jpe...@chromium.org>
---
M client/crash_report_database.cc
M client/crash_report_database.h
M client/crash_report_database_generic.cc
M client/crash_report_database_mac.mm
M client/crash_report_database_test.cc
M client/crash_report_database_win.cc
M handler/crash_report_upload_thread.cc
M handler/fuchsia/crash_report_exception_handler.cc
M handler/fuchsia/crash_report_exception_handler.h
M handler/handler_main.cc
10 files changed, 380 insertions(+), 15 deletions(-)