From c406797ce62083d84cc072e133c5c48fd63445fd Mon Sep 17 00:00:00 2001 From: Joshua Peraza Date: Thu, 15 Feb 2018 08:17:12 -0800 Subject: [PATCH] Add UploadReport to manage database resources during upload This change adds CrashReportDatabase::UploadReport which owns the report's file handle during upload. An upload is recorded as a success by calling RecordUploadComplete(). If RecordUploadComplete() is not called, the operation is recorded as a failure when the UploadReport is destroyed. Bug: crashpad:206 Change-Id: I8385d08d52185ad30b06a3ed054de9812ae006a2 Reviewed-on: https://chromium-review.googlesource.com/917983 Commit-Queue: Joshua Peraza Reviewed-by: Mark Mentovai Reviewed-by: Robert Sesek --- client/crash_report_database.cc | 24 +++++++ client/crash_report_database.h | 91 +++++++++++++++++++-------- client/crash_report_database_mac.mm | 53 ++++++++-------- client/crash_report_database_test.cc | 24 ++++--- client/crash_report_database_win.cc | 42 ++++++------- client/prune_crash_reports_test.cc | 5 +- handler/crash_report_upload_thread.cc | 78 +++++------------------ handler/crash_report_upload_thread.h | 4 +- 8 files changed, 168 insertions(+), 153 deletions(-) diff --git a/client/crash_report_database.cc b/client/crash_report_database.cc index 860807f7..afd751d8 100644 --- a/client/crash_report_database.cc +++ b/client/crash_report_database.cc @@ -55,4 +55,28 @@ bool CrashReportDatabase::NewReport::Initialize( return true; } +CrashReportDatabase::UploadReport::UploadReport() + : Report(), reader_(std::make_unique()), database_(nullptr) {} + +CrashReportDatabase::UploadReport::~UploadReport() { + if (database_) { + database_->RecordUploadAttempt(this, false, std::string()); + } +} + +bool CrashReportDatabase::UploadReport::Initialize(const base::FilePath path, + CrashReportDatabase* db) { + database_ = db; + return reader_->Open(path); +} + +CrashReportDatabase::OperationStatus CrashReportDatabase::RecordUploadComplete( + std::unique_ptr report_in, + const std::string& id) { + UploadReport* report = const_cast(report_in.get()); + + report->database_ = nullptr; + return RecordUploadAttempt(report, true, id); +} + } // namespace crashpad diff --git a/client/crash_report_database.h b/client/crash_report_database.h index a7d35ee1..afde2817 100644 --- a/client/crash_report_database.h +++ b/client/crash_report_database.h @@ -24,6 +24,7 @@ #include "base/files/file_path.h" #include "base/macros.h" #include "util/file/file_io.h" +#include "util/file/file_reader.h" #include "util/file/file_writer.h" #include "util/file/scoped_remove_file.h" #include "util/misc/metrics.h" @@ -49,7 +50,7 @@ class Settings; //! processed, or it was has been brought back from 'Completed' state by //! user request. //! 3. Completed: The report has been locally processed, either by uploading -//! it to a collection server and calling RecordUploadAttempt(), or by +//! it to a collection server and calling RecordUploadComplete(), or by //! calling SkipReportUpload(). class CrashReportDatabase { public: @@ -100,7 +101,7 @@ class CrashReportDatabase { //! \brief A crash report that is in the process of being written. //! - //! An instance of this struct should be created via PrepareNewCrashReport(). + //! An instance of this class should be created via PrepareNewCrashReport(). class NewReport { public: NewReport(); @@ -128,6 +129,30 @@ class CrashReportDatabase { DISALLOW_COPY_AND_ASSIGN(NewReport); }; + //! \brief A crash report that is in the process of being uploaded. + //! + //! An instance of this class should be created via GetReportForUploading(). + class UploadReport : public Report { + public: + UploadReport(); + virtual ~UploadReport(); + + // An open FileReader with which to read the report. + FileReader* Reader() const { return reader_.get(); } + + private: + friend class CrashReportDatabase; + friend class CrashReportDatabaseMac; + friend class CrashReportDatabaseWin; + + bool Initialize(const base::FilePath path, CrashReportDatabase* database); + + std::unique_ptr reader_; + CrashReportDatabase* database_; + + DISALLOW_COPY_AND_ASSIGN(UploadReport); + }; + //! \brief The result code for operations performed on a database. enum OperationStatus { //! \brief No error occurred. @@ -260,42 +285,38 @@ class CrashReportDatabase { //! \return The operation status code. virtual OperationStatus GetCompletedReports(std::vector* reports) = 0; - //! \brief Obtains a report object for uploading to a collection server. + //! \brief Obtains and locks a report object for uploading to a collection + //! server. //! - //! The file at Report::file_path should be uploaded by the caller, and then - //! the returned Report object must be disposed of via a call to - //! RecordUploadAttempt(). - //! - //! A subsequent call to this method with the same \a uuid is illegal until - //! RecordUploadAttempt() has been called. + //! 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. //! //! \param[in] uuid The unique identifier for the crash report record. //! \param[out] report A crash report record for the report to be uploaded. - //! The caller does not own this object. Only valid if this returns - //! #kNoError. + //! Only valid if this returns #kNoError. //! //! \return The operation status code. - virtual OperationStatus GetReportForUploading(const UUID& uuid, - const Report** report) = 0; + virtual OperationStatus GetReportForUploading( + const UUID& uuid, + std::unique_ptr* report) = 0; - //! \brief Adjusts a crash report record’s metadata to account for an upload - //! attempt, and updates the last upload attempt time as returned by + //! \brief Records a successful upload for a report and updates the last + //! upload attempt time as returned by //! Settings::GetLastUploadAttemptTime(). //! - //! After calling this method, the database is permitted to move and rename - //! the file at Report::file_path. - //! - //! \param[in] report The report object obtained from - //! GetReportForUploading(). This object is invalidated after this call. - //! \param[in] successful Whether the upload attempt was successful. - //! \param[in] id The identifier assigned to this crash report by the - //! collection server. Must be empty if \a successful is `false`; may be - //! empty if it is `true`. + //! \param[in] report A UploadReport object obtained from + //! GetReportForUploading(). The UploadReport object will be invalidated + //! and the report unlocked as part of this call. + //! \param[in] id The possibly empty identifier assigned to this crash report + //! by the collection server. //! //! \return The operation status code. - virtual OperationStatus RecordUploadAttempt(const Report* report, - bool successful, - const std::string& id) = 0; + OperationStatus RecordUploadComplete( + std::unique_ptr report, + const std::string& id); //! \brief Moves a report from the pending state to the completed state, but //! without the report being uploaded. @@ -331,6 +352,22 @@ class CrashReportDatabase { CrashReportDatabase() {} private: + //! \brief Adjusts a crash report record’s metadata to account for an upload + //! attempt, and updates the last upload attempt time as returned by + //! Settings::GetLastUploadAttemptTime(). + //! + //! \param[in] report The report object obtained from + //! GetReportForUploading(). + //! \param[in] successful Whether the upload attempt was successful. + //! \param[in] id The identifier assigned to this crash report by the + //! collection server. Must be empty if \a successful is `false`; may be + //! empty if it is `true`. + //! + //! \return The operation status code. + virtual OperationStatus RecordUploadAttempt(UploadReport* report, + bool successful, + const std::string& id) = 0; + DISALLOW_COPY_AND_ASSIGN(CrashReportDatabase); }; diff --git a/client/crash_report_database_mac.mm b/client/crash_report_database_mac.mm index 011ff03e..d0197fce 100644 --- a/client/crash_report_database_mac.mm +++ b/client/crash_report_database_mac.mm @@ -139,17 +139,20 @@ class CrashReportDatabaseMac : public CrashReportDatabase { OperationStatus LookUpCrashReport(const UUID& uuid, Report* report) override; OperationStatus GetPendingReports(std::vector* reports) override; OperationStatus GetCompletedReports(std::vector* reports) override; - OperationStatus GetReportForUploading(const UUID& uuid, - const Report** report) override; - OperationStatus RecordUploadAttempt(const Report* report, - bool successful, - const std::string& id) override; + OperationStatus GetReportForUploading( + const UUID& uuid, + std::unique_ptr* report) override; OperationStatus SkipReportUpload(const UUID& uuid, Metrics::CrashSkippedReason reason) override; OperationStatus DeleteReport(const UUID& uuid) override; OperationStatus RequestUpload(const UUID& uuid) override; private: + // CrashReportDatabase: + OperationStatus RecordUploadAttempt(UploadReport* report, + bool successful, + const std::string& id) override; + //! \brief Report states for use with LocateCrashReport(). //! //! ReportState may be considered to be a bitfield. @@ -163,10 +166,10 @@ class CrashReportDatabaseMac : public CrashReportDatabase { //! \brief A private extension of the Report class that maintains bookkeeping //! information of the database. - struct UploadReport : public Report { + struct UploadReportMac : public UploadReport { //! \brief Stores the flock of the file for the duration of //! GetReportForUploading() and RecordUploadAttempt(). - int lock_fd; + base::ScopedFD lock_fd; }; //! \brief Locates a crash report in the database by UUID. @@ -406,31 +409,36 @@ CrashReportDatabaseMac::GetCompletedReports( } CrashReportDatabase::OperationStatus -CrashReportDatabaseMac::GetReportForUploading(const UUID& uuid, - const Report** report) { +CrashReportDatabaseMac::GetReportForUploading( + const UUID& uuid, + std::unique_ptr* report) { INITIALIZATION_STATE_DCHECK_VALID(initialized_); - base::FilePath report_path = LocateCrashReport(uuid, kReportStatePending); - if (report_path.empty()) + auto upload_report = std::make_unique(); + + upload_report->file_path = LocateCrashReport(uuid, kReportStatePending); + if (upload_report->file_path.empty()) return kReportNotFound; - std::unique_ptr upload_report(new UploadReport()); - upload_report->file_path = report_path; - - base::ScopedFD lock(ObtainReportLock(report_path)); + base::ScopedFD lock(ObtainReportLock(upload_report->file_path)); if (!lock.is_valid()) return kBusyError; - if (!ReadReportMetadataLocked(report_path, upload_report.get())) + if (!ReadReportMetadataLocked(upload_report->file_path, upload_report.get())) return kDatabaseError; - upload_report->lock_fd = lock.release(); - *report = upload_report.release(); + if (!upload_report->reader_->Open(upload_report->file_path)) { + return kFileSystemError; + } + + upload_report->database_ = this; + upload_report->lock_fd.reset(lock.release()); + report->reset(upload_report.release()); return kNoError; } CrashReportDatabase::OperationStatus -CrashReportDatabaseMac::RecordUploadAttempt(const Report* report, +CrashReportDatabaseMac::RecordUploadAttempt(UploadReport* report, bool successful, const std::string& id) { INITIALIZATION_STATE_DCHECK_VALID(initialized_); @@ -445,13 +453,6 @@ CrashReportDatabaseMac::RecordUploadAttempt(const Report* report, if (report_path.empty()) return kReportNotFound; - std::unique_ptr upload_report( - static_cast(report)); - - base::ScopedFD lock(upload_report->lock_fd); - if (!lock.is_valid()) - return kBusyError; - if (successful) { CrashReportDatabase::OperationStatus os = MarkReportCompletedLocked(report_path, &report_path); diff --git a/client/crash_report_database_test.cc b/client/crash_report_database_test.cc index d84b23fc..31087b93 100644 --- a/client/crash_report_database_test.cc +++ b/client/crash_report_database_test.cc @@ -69,15 +69,19 @@ class CrashReportDatabaseTest : public testing::Test { time_t times[2]; ASSERT_TRUE(settings->GetLastUploadAttemptTime(×[0])); - const CrashReportDatabase::Report* report = nullptr; + std::unique_ptr report; ASSERT_EQ(db_->GetReportForUploading(uuid, &report), CrashReportDatabase::kNoError); EXPECT_NE(report->uuid, UUID()); EXPECT_FALSE(report->file_path.empty()); EXPECT_TRUE(FileExists(report->file_path)) << report->file_path.value(); EXPECT_GT(report->creation_time, 0); - EXPECT_EQ(db_->RecordUploadAttempt(report, successful, id), - CrashReportDatabase::kNoError); + if (successful) { + EXPECT_EQ(db_->RecordUploadComplete(std::move(report), id), + CrashReportDatabase::kNoError); + } else { + report.reset(); + } ASSERT_TRUE(settings->GetLastUploadAttemptTime(×[1])); EXPECT_NE(times[1], 0); @@ -452,16 +456,16 @@ TEST_F(CrashReportDatabaseTest, DuelingUploads) { CrashReportDatabase::Report report; CreateCrashReport(&report); - const CrashReportDatabase::Report* upload_report; + std::unique_ptr upload_report; EXPECT_EQ(db()->GetReportForUploading(report.uuid, &upload_report), CrashReportDatabase::kNoError); - const CrashReportDatabase::Report* upload_report_2 = nullptr; + std::unique_ptr upload_report_2; EXPECT_EQ(db()->GetReportForUploading(report.uuid, &upload_report_2), CrashReportDatabase::kBusyError); EXPECT_FALSE(upload_report_2); - EXPECT_EQ(db()->RecordUploadAttempt(upload_report, true, std::string()), + EXPECT_EQ(db()->RecordUploadComplete(std::move(upload_report), std::string()), CrashReportDatabase::kNoError); } @@ -469,16 +473,16 @@ TEST_F(CrashReportDatabaseTest, UploadAlreadyUploaded) { CrashReportDatabase::Report report; CreateCrashReport(&report); - const CrashReportDatabase::Report* upload_report; + std::unique_ptr upload_report; EXPECT_EQ(db()->GetReportForUploading(report.uuid, &upload_report), CrashReportDatabase::kNoError); - EXPECT_EQ(db()->RecordUploadAttempt(upload_report, true, std::string()), + EXPECT_EQ(db()->RecordUploadComplete(std::move(upload_report), std::string()), CrashReportDatabase::kNoError); - const CrashReportDatabase::Report* upload_report_2 = nullptr; + std::unique_ptr upload_report_2; EXPECT_EQ(db()->GetReportForUploading(report.uuid, &upload_report_2), CrashReportDatabase::kReportNotFound); - EXPECT_FALSE(upload_report_2); + EXPECT_FALSE(upload_report_2.get()); } TEST_F(CrashReportDatabaseTest, MoveDatabase) { diff --git a/client/crash_report_database_win.cc b/client/crash_report_database_win.cc index ee0832e3..fb3cd8f7 100644 --- a/client/crash_report_database_win.cc +++ b/client/crash_report_database_win.cc @@ -29,6 +29,7 @@ #include "base/strings/stringprintf.h" #include "base/strings/utf_string_conversions.h" #include "client/settings.h" +#include "util/misc/implicit_cast.h" #include "util/misc/initialization_state_dcheck.h" #include "util/misc/metrics.h" @@ -591,17 +592,20 @@ class CrashReportDatabaseWin : public CrashReportDatabase { OperationStatus LookUpCrashReport(const UUID& uuid, Report* report) override; OperationStatus GetPendingReports(std::vector* reports) override; OperationStatus GetCompletedReports(std::vector* reports) override; - OperationStatus GetReportForUploading(const UUID& uuid, - const Report** report) override; - OperationStatus RecordUploadAttempt(const Report* report, - bool successful, - const std::string& id) override; + OperationStatus GetReportForUploading( + const UUID& uuid, + std::unique_ptr* report) override; OperationStatus SkipReportUpload(const UUID& uuid, Metrics::CrashSkippedReason reason) override; OperationStatus DeleteReport(const UUID& uuid) override; OperationStatus RequestUpload(const UUID& uuid) override; private: + // CrashReportDatabase: + OperationStatus RecordUploadAttempt(UploadReport* report, + bool successful, + const std::string& id) override; + std::unique_ptr AcquireMetadata(); base::FilePath base_dir_; @@ -716,44 +720,38 @@ OperationStatus CrashReportDatabaseWin::GetCompletedReports( OperationStatus CrashReportDatabaseWin::GetReportForUploading( const UUID& uuid, - const Report** report) { + std::unique_ptr* report) { INITIALIZATION_STATE_DCHECK_VALID(initialized_); std::unique_ptr metadata(AcquireMetadata()); if (!metadata) return kDatabaseError; - // TODO(scottmg): After returning this report to the client, there is no way - // to reap this report if the uploader fails to call RecordUploadAttempt() or - // SkipReportUpload() (if it crashed or was otherwise buggy). To resolve this, - // one possibility would be to change the interface to be FileHandle based, so - // that instead of giving the file_path back to the client and changing state - // to kUploading, we return an exclusive access handle, and use that as the - // signal that the upload is pending, rather than an update to state in the - // metadata. Alternatively, there could be a "garbage collection" at startup - // where any reports that are orphaned in the kUploading state are either - // reset to kPending to retry, or discarded. + ReportDisk* report_disk; OperationStatus os = metadata->FindSingleReportAndMarkDirty( uuid, ReportState::kPending, &report_disk); if (os == kNoError) { report_disk->state = ReportState::kUploading; - // Create a copy for passing back to client. This will be freed in - // RecordUploadAttempt. - *report = new Report(*report_disk); + auto upload_report = std::make_unique(); + *implicit_cast(upload_report.get()) = *report_disk; + + if (!upload_report->Initialize(upload_report->file_path, this)) { + return kFileSystemError; + } + + report->reset(upload_report.release()); } return os; } OperationStatus CrashReportDatabaseWin::RecordUploadAttempt( - const Report* report, + UploadReport* report, bool successful, const std::string& id) { INITIALIZATION_STATE_DCHECK_VALID(initialized_); Metrics::CrashUploadAttempted(successful); - // Take ownership, allocated in GetReportForUploading. - std::unique_ptr upload_report(report); std::unique_ptr metadata(AcquireMetadata()); if (!metadata) return kDatabaseError; diff --git a/client/prune_crash_reports_test.cc b/client/prune_crash_reports_test.cc index e5e5a41a..2648dee2 100644 --- a/client/prune_crash_reports_test.cc +++ b/client/prune_crash_reports_test.cc @@ -42,9 +42,10 @@ class MockDatabase : public CrashReportDatabase { MOCK_METHOD1(GetPendingReports, OperationStatus(std::vector*)); MOCK_METHOD1(GetCompletedReports, OperationStatus(std::vector*)); MOCK_METHOD2(GetReportForUploading, - OperationStatus(const UUID&, const Report**)); + OperationStatus(const UUID&, + std::unique_ptr*)); MOCK_METHOD3(RecordUploadAttempt, - OperationStatus(const Report*, bool, const std::string&)); + OperationStatus(UploadReport*, bool, const std::string&)); MOCK_METHOD2(SkipReportUpload, OperationStatus(const UUID&, Metrics::CrashSkippedReason)); MOCK_METHOD1(DeleteReport, OperationStatus(const UUID&)); diff --git a/handler/crash_report_upload_thread.cc b/handler/crash_report_upload_thread.cc index 7505524b..8d7149ea 100644 --- a/handler/crash_report_upload_thread.cc +++ b/handler/crash_report_upload_thread.cc @@ -45,44 +45,6 @@ namespace crashpad { -namespace { - -// Calls CrashReportDatabase::RecordUploadAttempt() with |successful| set to -// false upon destruction unless disarmed by calling Fire() or Disarm(). Fire() -// triggers an immediate call. Armed upon construction. -class CallRecordUploadAttempt { - public: - CallRecordUploadAttempt(CrashReportDatabase* database, - const CrashReportDatabase::Report* report) - : database_(database), - report_(report) { - } - - ~CallRecordUploadAttempt() { - Fire(); - } - - void Fire() { - if (report_) { - database_->RecordUploadAttempt(report_, false, std::string()); - } - - Disarm(); - } - - void Disarm() { - report_ = nullptr; - } - - private: - CrashReportDatabase* database_; // weak - const CrashReportDatabase::Report* report_; // weak - - DISALLOW_COPY_AND_ASSIGN(CallRecordUploadAttempt); -}; - -} // namespace - CrashReportUploadThread::CrashReportUploadThread(CrashReportDatabase* database, const std::string& url, const Options& options) @@ -229,7 +191,7 @@ void CrashReportUploadThread::ProcessPendingReport( } } - const CrashReportDatabase::Report* upload_report; + std::unique_ptr upload_report; CrashReportDatabase::OperationStatus status = database_->GetReportForUploading(report.uuid, &upload_report); switch (status) { @@ -256,18 +218,16 @@ void CrashReportUploadThread::ProcessPendingReport( return; } - CallRecordUploadAttempt call_record_upload_attempt(database_, upload_report); - std::string response_body; - UploadResult upload_result = UploadReport(upload_report, &response_body); + UploadResult upload_result = + UploadReport(upload_report.get(), &response_body); switch (upload_result) { case UploadResult::kSuccess: - call_record_upload_attempt.Disarm(); - database_->RecordUploadAttempt(upload_report, true, response_body); + database_->RecordUploadComplete(std::move(upload_report), response_body); break; case UploadResult::kPermanentFailure: case UploadResult::kRetry: - call_record_upload_attempt.Fire(); + upload_report.reset(); // TODO(mark): Deal with retries properly: don’t call SkipReportUplaod() // if the result was kRetry and the report hasn’t already been retried @@ -279,17 +239,12 @@ void CrashReportUploadThread::ProcessPendingReport( } CrashReportUploadThread::UploadResult CrashReportUploadThread::UploadReport( - const CrashReportDatabase::Report* report, + const CrashReportDatabase::UploadReport* report, std::string* response_body) { std::map parameters; - FileReader minidump_file_reader; - if (!minidump_file_reader.Open(report->file_path)) { - // If the minidump file can’t be opened, all hope is lost. - return UploadResult::kPermanentFailure; - } - - FileOffset start_offset = minidump_file_reader.SeekGet(); + FileReader* reader = report->Reader(); + FileOffset start_offset = reader->SeekGet(); if (start_offset < 0) { return UploadResult::kPermanentFailure; } @@ -299,12 +254,12 @@ CrashReportUploadThread::UploadResult CrashReportUploadThread::UploadReport( // parameters, but as long as there’s a dump file, the server can decide what // to do with it. ProcessSnapshotMinidump minidump_process_snapshot; - if (minidump_process_snapshot.Initialize(&minidump_file_reader)) { + if (minidump_process_snapshot.Initialize(reader)) { parameters = BreakpadHTTPFormParametersFromMinidump(&minidump_process_snapshot); } - if (!minidump_file_reader.SeekSet(start_offset)) { + if (!reader->SeekSet(start_offset)) { return UploadResult::kPermanentFailure; } @@ -322,15 +277,10 @@ CrashReportUploadThread::UploadResult CrashReportUploadThread::UploadReport( } } - http_multipart_builder.SetFileAttachment( - kMinidumpKey, -#if defined(OS_WIN) - base::UTF16ToUTF8(report->file_path.BaseName().value()), -#else - report->file_path.BaseName().value(), -#endif - &minidump_file_reader, - "application/octet-stream"); + http_multipart_builder.SetFileAttachment(kMinidumpKey, + report->uuid.ToString() + ".dmp", + reader, + "application/octet-stream"); std::unique_ptr http_transport(HTTPTransport::Create()); HTTPHeaders content_headers; diff --git a/handler/crash_report_upload_thread.h b/handler/crash_report_upload_thread.h index cdd1502b..69e7a3c2 100644 --- a/handler/crash_report_upload_thread.h +++ b/handler/crash_report_upload_thread.h @@ -148,14 +148,14 @@ class CrashReportUploadThread : public WorkerThread::Delegate { //! \param[in] report The report to upload. The caller is responsible for //! calling CrashReportDatabase::GetReportForUploading() before calling //! this method, and for calling - //! CrashReportDatabase::RecordUploadAttempt() after calling this method. + //! CrashReportDatabase::RecordUploadComplete() after calling this method. //! \param[out] response_body If the upload attempt is successful, this will //! be set to the response body sent by the server. Breakpad-type servers //! provide the crash ID assigned by the server in the response body. //! //! \return A member of UploadResult indicating the result of the upload //! attempt. - UploadResult UploadReport(const CrashReportDatabase::Report* report, + UploadResult UploadReport(const CrashReportDatabase::UploadReport* report, std::string* response_body); // WorkerThread::Delegate: