From c45ba7920e01e89b8e9c00dbe588b4d73804d057 Mon Sep 17 00:00:00 2001 From: Joshua Peraza Date: Tue, 13 Feb 2018 17:28:11 -0800 Subject: [PATCH] Make NewReport objects own their associated database resources This change updates CrashReportDatbase::NewReport objects to own the file handle associated with the new report, now accessible via a FileWriter. NewReport's destructor closes its file handle and removes its new report unless disarmed with FinishedWritingCrashReport, eliminating the need for CallErrorWritingCrashReport. Bug: crashpad:206 Change-Id: Iccb5bbc0ebadb07a237ff8eb938389afcfeae2a5 Reviewed-on: https://chromium-review.googlesource.com/916941 Commit-Queue: Joshua Peraza Reviewed-by: Mark Mentovai Reviewed-by: Scott Graham --- client/crash_report_database.cc | 37 ++++--- client/crash_report_database.h | 104 +++++++----------- client/crash_report_database_mac.mm | 96 +++++----------- client/crash_report_database_test.cc | 46 +++----- client/crash_report_database_win.cc | 65 +++-------- client/prune_crash_reports_test.cc | 12 +- handler/mac/crash_report_exception_handler.cc | 17 +-- handler/win/crash_report_exception_handler.cc | 17 +-- tools/crashpad_database_util.cc | 12 +- util/misc/metrics.cc | 3 +- util/misc/metrics.h | 2 +- 11 files changed, 149 insertions(+), 262 deletions(-) diff --git a/client/crash_report_database.cc b/client/crash_report_database.cc index 8451e469..860807f7 100644 --- a/client/crash_report_database.cc +++ b/client/crash_report_database.cc @@ -14,6 +14,8 @@ #include "client/crash_report_database.h" +#include "build/build_config.h" + namespace crashpad { CrashReportDatabase::Report::Report() @@ -26,22 +28,31 @@ CrashReportDatabase::Report::Report() upload_attempts(0), upload_explicitly_requested(false) {} -CrashReportDatabase::CallErrorWritingCrashReport::CallErrorWritingCrashReport( - CrashReportDatabase* database, - NewReport* new_report) - : database_(database), - new_report_(new_report) { -} +CrashReportDatabase::NewReport::NewReport() + : writer_(std::make_unique()), uuid_(), file_remover_() {} -CrashReportDatabase::CallErrorWritingCrashReport:: - ~CallErrorWritingCrashReport() { - if (new_report_) { - database_->ErrorWritingCrashReport(new_report_); +CrashReportDatabase::NewReport::~NewReport() = default; + +bool CrashReportDatabase::NewReport::Initialize( + const base::FilePath& directory, + const base::FilePath::StringType& extension) { + if (!uuid_.InitializeWithNew()) { + return false; } -} -void CrashReportDatabase::CallErrorWritingCrashReport::Disarm() { - new_report_ = nullptr; +#if defined(OS_WIN) + const std::wstring uuid_string = uuid_.ToString16(); +#else + const std::string uuid_string = uuid_.ToString(); +#endif + + const base::FilePath path = directory.Append(uuid_string + extension); + if (!writer_->Open( + path, FileWriteMode::kCreateOrFail, FilePermissions::kOwnerOnly)) { + return false; + } + file_remover_.reset(path); + return true; } } // namespace crashpad diff --git a/client/crash_report_database.h b/client/crash_report_database.h index 62117899..a7d35ee1 100644 --- a/client/crash_report_database.h +++ b/client/crash_report_database.h @@ -24,6 +24,8 @@ #include "base/files/file_path.h" #include "base/macros.h" #include "util/file/file_io.h" +#include "util/file/file_writer.h" +#include "util/file/scoped_remove_file.h" #include "util/misc/metrics.h" #include "util/misc/uuid.h" @@ -98,44 +100,32 @@ class CrashReportDatabase { //! \brief A crash report that is in the process of being written. //! - //! An instance of this struct should be created via PrepareNewCrashReport() - //! and destroyed with FinishedWritingCrashReport(). - struct NewReport { - //! The file handle to which the report should be written. - FileHandle handle; + //! An instance of this struct should be created via PrepareNewCrashReport(). + class NewReport { + public: + NewReport(); + ~NewReport(); + + //! An open FileWriter with which to write the report. + FileWriter* Writer() const { return writer_.get(); } //! A unique identifier by which this report will always be known to the //! database. - UUID uuid; - - //! The path to the crash report being written. - base::FilePath path; - }; - - //! \brief A scoper to cleanly handle the interface requirement imposed by - //! PrepareNewCrashReport(). - //! - //! Calls ErrorWritingCrashReport() upon destruction unless disarmed by - //! calling Disarm(). Armed upon construction. - class CallErrorWritingCrashReport { - public: - //! \brief Arms the object to call ErrorWritingCrashReport() on \a database - //! with an argument of \a new_report on destruction. - CallErrorWritingCrashReport(CrashReportDatabase* database, - NewReport* new_report); - - //! \brief Calls ErrorWritingCrashReport() if the object is armed. - ~CallErrorWritingCrashReport(); - - //! \brief Disarms the object so that CallErrorWritingCrashReport() will not - //! be called upon destruction. - void Disarm(); + const UUID& ReportID() { return uuid_; } private: - CrashReportDatabase* database_; // weak - NewReport* new_report_; // weak + friend class CrashReportDatabase; + friend class CrashReportDatabaseMac; + friend class CrashReportDatabaseWin; - DISALLOW_COPY_AND_ASSIGN(CallErrorWritingCrashReport); + bool Initialize(const base::FilePath& directory, + const base::FilePath::StringType& extension); + + std::unique_ptr writer_; + UUID uuid_; + ScopedRemoveFile file_remover_; + + DISALLOW_COPY_AND_ASSIGN(NewReport); }; //! \brief The result code for operations performed on a database. @@ -217,49 +207,31 @@ class CrashReportDatabase { //! \brief Creates a record of a new crash report. //! - //! Callers can then write the crash report using the file handle provided. - //! The caller does not own the new crash report record or its file handle, - //! both of which must be explicitly disposed of by calling - //! FinishedWritingCrashReport() or ErrorWritingCrashReport(). + //! Callers should write the crash report using the FileWriter provided. + //! Callers should then call FinishedWritingCrashReport() to complete report + //! creation. If an error is encountered while writing the crash report, no + //! special action needs to be taken. If FinishedWritingCrashReport() is not + //! called, the report will be removed from the database when \a report is + //! destroyed. //! - //! To arrange to call ErrorWritingCrashReport() during any early return, use - //! CallErrorWritingCrashReport. - //! - //! \param[out] report A NewReport object containing a file handle to which - //! the crash report data should be written. Only valid if this returns - //! #kNoError. The caller must not delete the NewReport object or close - //! the file handle within. + //! \param[out] report A NewReport object containing a FileWriter with which + //! to write the report data. Only valid if this returns #kNoError. //! //! \return The operation status code. - virtual OperationStatus PrepareNewCrashReport(NewReport** report) = 0; + virtual OperationStatus PrepareNewCrashReport( + std::unique_ptr* report) = 0; - //! \brief Informs the database that a crash report has been written. - //! - //! After calling this method, the database is permitted to move and rename - //! the file at NewReport::path. + //! \brief Informs the database that a crash report has been successfully + //! written. //! //! \param[in] report A NewReport obtained with PrepareNewCrashReport(). The - //! NewReport object and file handle within will be invalidated as part of - //! this call. + //! NewReport object will be invalidated as part of this call. //! \param[out] uuid The UUID of this crash report. //! //! \return The operation status code. - virtual OperationStatus FinishedWritingCrashReport(NewReport* report, - UUID* uuid) = 0; - - //! \brief Informs the database that an error occurred while attempting to - //! write a crash report, and that any resources associated with it should - //! be cleaned up. - //! - //! After calling this method, the database is permitted to remove the file at - //! NewReport::path. - //! - //! \param[in] report A NewReport obtained with PrepareNewCrashReport(). The - //! NewReport object and file handle within will be invalidated as part of - //! this call. - //! - //! \return The operation status code. - virtual OperationStatus ErrorWritingCrashReport(NewReport* report) = 0; + virtual OperationStatus FinishedWritingCrashReport( + std::unique_ptr report, + UUID* uuid) = 0; //! \brief Returns the crash report record for the unique identifier. //! diff --git a/client/crash_report_database_mac.mm b/client/crash_report_database_mac.mm index 3bba2b63..011ff03e 100644 --- a/client/crash_report_database_mac.mm +++ b/client/crash_report_database_mac.mm @@ -107,6 +107,8 @@ std::string XattrNameInternal(const base::StringPiece& name, bool new_name) { name.data()); } +} // namespace + //! \brief A CrashReportDatabase that uses HFS+ extended attributes to store //! report metadata. //! @@ -130,10 +132,10 @@ class CrashReportDatabaseMac : public CrashReportDatabase { // CrashReportDatabase: Settings* GetSettings() override; - OperationStatus PrepareNewCrashReport(NewReport** report) override; - OperationStatus FinishedWritingCrashReport(NewReport* report, + OperationStatus PrepareNewCrashReport( + std::unique_ptr* report) override; + OperationStatus FinishedWritingCrashReport(std::unique_ptr report, UUID* uuid) override; - OperationStatus ErrorWritingCrashReport(NewReport* report) override; OperationStatus LookUpCrashReport(const UUID& uuid, Report* report) override; OperationStatus GetPendingReports(std::vector* reports) override; OperationStatus GetCompletedReports(std::vector* reports) override; @@ -301,103 +303,67 @@ Settings* CrashReportDatabaseMac::GetSettings() { } CrashReportDatabase::OperationStatus -CrashReportDatabaseMac::PrepareNewCrashReport(NewReport** out_report) { +CrashReportDatabaseMac::PrepareNewCrashReport( + std::unique_ptr* out_report) { INITIALIZATION_STATE_DCHECK_VALID(initialized_); std::unique_ptr report(new NewReport()); - - uuid_t uuid_gen; - uuid_generate(uuid_gen); - report->uuid.InitializeFromBytes(uuid_gen); - - report->path = - base_dir_.Append(kWriteDirectory) - .Append(report->uuid.ToString() + "." + kCrashReportFileExtension); - - report->handle = HANDLE_EINTR( - open(report->path.value().c_str(), - O_WRONLY | O_EXLOCK | O_CREAT | O_EXCL | O_NOCTTY | O_CLOEXEC, - 0600)); - if (report->handle < 0) { - PLOG(ERROR) << "open " << report->path.value(); + if (!report->Initialize(base_dir_.Append(kWriteDirectory), + std::string(".") + kCrashReportFileExtension)) { return kFileSystemError; } // TODO(rsesek): Potentially use an fsetxattr() here instead. - if (!WriteXattr( - report->path, XattrName(kXattrUUID), report->uuid.ToString())) { - PLOG_IF(ERROR, IGNORE_EINTR(close(report->handle)) != 0) << "close"; + if (!WriteXattr(report->file_remover_.get(), + XattrName(kXattrUUID), + report->ReportID().ToString())) { return kDatabaseError; } - *out_report = report.release(); - + out_report->reset(report.release()); return kNoError; } CrashReportDatabase::OperationStatus -CrashReportDatabaseMac::FinishedWritingCrashReport(NewReport* report, - UUID* uuid) { +CrashReportDatabaseMac::FinishedWritingCrashReport( + std::unique_ptr report, + UUID* uuid) { INITIALIZATION_STATE_DCHECK_VALID(initialized_); - // Takes ownership of the |handle| and the O_EXLOCK. - base::ScopedFD lock(report->handle); - - // Take ownership of the report. - std::unique_ptr scoped_report(report); + const base::FilePath& path = report->file_remover_.get(); // Get the report's UUID to return. std::string uuid_string; - if (ReadXattr(report->path, XattrName(kXattrUUID), - &uuid_string) != XattrStatus::kOK || + if (ReadXattr(path, XattrName(kXattrUUID), &uuid_string) != + XattrStatus::kOK || !uuid->InitializeFromString(uuid_string)) { - LOG(ERROR) << "Failed to read UUID for crash report " - << report->path.value(); + LOG(ERROR) << "Failed to read UUID for crash report " << path.value(); return kDatabaseError; } - if (*uuid != report->uuid) { - LOG(ERROR) << "UUID mismatch for crash report " << report->path.value(); + if (*uuid != report->ReportID()) { + LOG(ERROR) << "UUID mismatch for crash report " << path.value(); return kDatabaseError; } // Record the creation time of this report. - if (!WriteXattrTimeT(report->path, XattrName(kXattrCreationTime), - time(nullptr))) { + if (!WriteXattrTimeT(path, XattrName(kXattrCreationTime), time(nullptr))) { return kDatabaseError; } + FileOffset size = report->Writer()->Seek(0, SEEK_END); + // Move the report to its new location for uploading. base::FilePath new_path = - base_dir_.Append(kUploadPendingDirectory).Append(report->path.BaseName()); - if (rename(report->path.value().c_str(), new_path.value().c_str()) != 0) { - PLOG(ERROR) << "rename " << report->path.value() << " to " - << new_path.value(); + base_dir_.Append(kUploadPendingDirectory).Append(path.BaseName()); + if (rename(path.value().c_str(), new_path.value().c_str()) != 0) { + PLOG(ERROR) << "rename " << path.value() << " to " << new_path.value(); return kFileSystemError; } + ignore_result(report->file_remover_.release()); Metrics::CrashReportPending(Metrics::PendingReportReason::kNewlyCreated); - Metrics::CrashReportSize(report->handle); - - return kNoError; -} - -CrashReportDatabase::OperationStatus -CrashReportDatabaseMac::ErrorWritingCrashReport(NewReport* report) { - INITIALIZATION_STATE_DCHECK_VALID(initialized_); - - // Takes ownership of the |handle| and the O_EXLOCK. - base::ScopedFD lock(report->handle); - - // Take ownership of the report. - std::unique_ptr scoped_report(report); - - // Remove the file that the report would have been written to had no error - // occurred. - if (unlink(report->path.value().c_str()) != 0) { - PLOG(ERROR) << "unlink " << report->path.value(); - return kFileSystemError; - } + Metrics::CrashReportSize(size); return kNoError; } @@ -774,8 +740,6 @@ std::unique_ptr InitializeInternal( return std::unique_ptr(database_mac.release()); } -} // namespace - // static std::unique_ptr CrashReportDatabase::Initialize( const base::FilePath& path) { diff --git a/client/crash_report_database_test.cc b/client/crash_report_database_test.cc index c4266961..d84b23fc 100644 --- a/client/crash_report_database_test.cc +++ b/client/crash_report_database_test.cc @@ -14,13 +14,13 @@ #include "client/crash_report_database.h" -#include "build/build_config.h" #include "client/settings.h" #include "gtest/gtest.h" #include "test/errors.h" #include "test/file.h" #include "test/scoped_temp_dir.h" #include "util/file/file_io.h" +#include "util/file/filesystem.h" namespace crashpad { namespace test { @@ -48,20 +48,19 @@ class CrashReportDatabaseTest : public testing::Test { } void CreateCrashReport(CrashReportDatabase::Report* report) { - CrashReportDatabase::NewReport* new_report = nullptr; + std::unique_ptr new_report; ASSERT_EQ(db_->PrepareNewCrashReport(&new_report), CrashReportDatabase::kNoError); static constexpr char kTest[] = "test"; - ASSERT_TRUE(LoggingWriteFile(new_report->handle, kTest, sizeof(kTest))); + ASSERT_TRUE(new_report->Writer()->Write(kTest, sizeof(kTest))); UUID uuid; - EXPECT_EQ(db_->FinishedWritingCrashReport(new_report, &uuid), + EXPECT_EQ(db_->FinishedWritingCrashReport(std::move(new_report), &uuid), CrashReportDatabase::kNoError); EXPECT_EQ(db_->LookUpCrashReport(uuid, report), CrashReportDatabase::kNoError); ExpectPreparedCrashReport(*report); - ASSERT_TRUE(FileExists(report->file_path)); } void UploadReport(const UUID& uuid, bool successful, const std::string& id) { @@ -176,13 +175,12 @@ TEST_F(CrashReportDatabaseTest, Initialize) { } TEST_F(CrashReportDatabaseTest, NewCrashReport) { - CrashReportDatabase::NewReport* new_report; + std::unique_ptr new_report; EXPECT_EQ(db()->PrepareNewCrashReport(&new_report), CrashReportDatabase::kNoError); - UUID expect_uuid = new_report->uuid; - EXPECT_TRUE(FileExists(new_report->path)) << new_report->path.value(); + UUID expect_uuid = new_report->ReportID(); UUID uuid; - EXPECT_EQ(db()->FinishedWritingCrashReport(new_report, &uuid), + EXPECT_EQ(db()->FinishedWritingCrashReport(std::move(new_report), &uuid), CrashReportDatabase::kNoError); EXPECT_EQ(uuid, expect_uuid); @@ -201,17 +199,6 @@ TEST_F(CrashReportDatabaseTest, NewCrashReport) { EXPECT_TRUE(reports.empty()); } -TEST_F(CrashReportDatabaseTest, ErrorWritingCrashReport) { - CrashReportDatabase::NewReport* new_report = nullptr; - ASSERT_EQ(db()->PrepareNewCrashReport(&new_report), - CrashReportDatabase::kNoError); - base::FilePath new_report_path = new_report->path; - EXPECT_TRUE(FileExists(new_report_path)) << new_report_path.value(); - EXPECT_EQ(db()->ErrorWritingCrashReport(new_report), - CrashReportDatabase::kNoError); - EXPECT_FALSE(FileExists(new_report_path)) << new_report_path.value(); -} - TEST_F(CrashReportDatabaseTest, LookUpCrashReport) { UUID uuid; @@ -495,12 +482,11 @@ TEST_F(CrashReportDatabaseTest, UploadAlreadyUploaded) { } TEST_F(CrashReportDatabaseTest, MoveDatabase) { - CrashReportDatabase::NewReport* new_report; + std::unique_ptr new_report; EXPECT_EQ(db()->PrepareNewCrashReport(&new_report), CrashReportDatabase::kNoError); - EXPECT_TRUE(FileExists(new_report->path)) << new_report->path.value(); UUID uuid; - EXPECT_EQ(db()->FinishedWritingCrashReport(new_report, &uuid), + EXPECT_EQ(db()->FinishedWritingCrashReport(std::move(new_report), &uuid), CrashReportDatabase::kNoError); RelocateDatabase(); @@ -509,28 +495,22 @@ TEST_F(CrashReportDatabaseTest, MoveDatabase) { EXPECT_EQ(db()->LookUpCrashReport(uuid, &report), CrashReportDatabase::kNoError); ExpectPreparedCrashReport(report); - EXPECT_TRUE(FileExists(report.file_path)) << report.file_path.value(); } TEST_F(CrashReportDatabaseTest, ReportRemoved) { - CrashReportDatabase::NewReport* new_report; + std::unique_ptr new_report; EXPECT_EQ(db()->PrepareNewCrashReport(&new_report), CrashReportDatabase::kNoError); - EXPECT_TRUE(FileExists(new_report->path)) << new_report->path.value(); + UUID uuid; - EXPECT_EQ(db()->FinishedWritingCrashReport(new_report, &uuid), + EXPECT_EQ(db()->FinishedWritingCrashReport(std::move(new_report), &uuid), CrashReportDatabase::kNoError); CrashReportDatabase::Report report; EXPECT_EQ(db()->LookUpCrashReport(uuid, &report), CrashReportDatabase::kNoError); -#if defined(OS_WIN) - EXPECT_EQ(_wunlink(report.file_path.value().c_str()), 0); -#else - EXPECT_EQ(unlink(report.file_path.value().c_str()), 0) - << ErrnoMessage("unlink"); -#endif + EXPECT_TRUE(LoggingRemoveFile(report.file_path)); EXPECT_EQ(db()->LookUpCrashReport(uuid, &report), CrashReportDatabase::kReportNotFound); diff --git a/client/crash_report_database_win.cc b/client/crash_report_database_win.cc index d6699e1b..ee0832e3 100644 --- a/client/crash_report_database_win.cc +++ b/client/crash_report_database_win.cc @@ -571,6 +571,8 @@ bool CreateDirectoryIfNecessary(const base::FilePath& path) { return EnsureDirectory(path); } +} // namespace + // CrashReportDatabaseWin ------------------------------------------------------ class CrashReportDatabaseWin : public CrashReportDatabase { @@ -582,10 +584,10 @@ class CrashReportDatabaseWin : public CrashReportDatabase { // CrashReportDatabase: Settings* GetSettings() override; - OperationStatus PrepareNewCrashReport(NewReport** report) override; - OperationStatus FinishedWritingCrashReport(NewReport* report, + OperationStatus PrepareNewCrashReport( + std::unique_ptr* report) override; + OperationStatus FinishedWritingCrashReport(std::unique_ptr report, UUID* uuid) override; - OperationStatus ErrorWritingCrashReport(NewReport* report) override; OperationStatus LookUpCrashReport(const UUID& uuid, Report* report) override; OperationStatus GetPendingReports(std::vector* reports) override; OperationStatus GetCompletedReports(std::vector* reports) override; @@ -643,67 +645,38 @@ Settings* CrashReportDatabaseWin::GetSettings() { } OperationStatus CrashReportDatabaseWin::PrepareNewCrashReport( - NewReport** report) { + std::unique_ptr* report) { INITIALIZATION_STATE_DCHECK_VALID(initialized_); std::unique_ptr new_report(new NewReport()); - if (!new_report->uuid.InitializeWithNew()) - return kFileSystemError; - new_report->path = base_dir_.Append(kReportsDirectory) - .Append(new_report->uuid.ToString16() + L"." + - kCrashReportFileExtension); - new_report->handle = LoggingOpenFileForWrite(new_report->path, - FileWriteMode::kCreateOrFail, - FilePermissions::kOwnerOnly); - if (new_report->handle == INVALID_HANDLE_VALUE) + if (!new_report->Initialize(base_dir_.Append(kReportsDirectory), + std::wstring(L".") + kCrashReportFileExtension)) { return kFileSystemError; + } - *report = new_report.release(); + report->reset(new_report.release()); return kNoError; } OperationStatus CrashReportDatabaseWin::FinishedWritingCrashReport( - NewReport* report, + std::unique_ptr report, UUID* uuid) { INITIALIZATION_STATE_DCHECK_VALID(initialized_); - // Take ownership of the report. - std::unique_ptr scoped_report(report); - // Take ownership of the file handle. - ScopedFileHandle handle(report->handle); - std::unique_ptr metadata(AcquireMetadata()); if (!metadata) return kDatabaseError; - metadata->AddNewRecord(ReportDisk(scoped_report->uuid, - scoped_report->path, + metadata->AddNewRecord(ReportDisk(report->ReportID(), + report->file_remover_.get(), time(nullptr), ReportState::kPending)); - *uuid = scoped_report->uuid; + + ignore_result(report->file_remover_.release()); + + *uuid = report->ReportID(); Metrics::CrashReportPending(Metrics::PendingReportReason::kNewlyCreated); - Metrics::CrashReportSize(handle.get()); - - return kNoError; -} - -OperationStatus CrashReportDatabaseWin::ErrorWritingCrashReport( - NewReport* report) { - INITIALIZATION_STATE_DCHECK_VALID(initialized_); - - // Take ownership of the report. - std::unique_ptr scoped_report(report); - - // Close the outstanding handle. - LoggingCloseFile(report->handle); - - // We failed to write, so remove the dump file. There's no entry in the - // metadata table yet. - if (!DeleteFile(scoped_report->path.value().c_str())) { - PLOG(ERROR) << "DeleteFile " - << base::UTF16ToUTF8(scoped_report->path.value()); - return kFileSystemError; - } + Metrics::CrashReportSize(report->Writer()->Seek(0, SEEK_END)); return kNoError; } @@ -899,8 +872,6 @@ OperationStatus CrashReportDatabaseWin::RequestUpload(const UUID& uuid) { return kNoError; } -} // namespace - // static std::unique_ptr CrashReportDatabase::Initialize( const base::FilePath& path) { diff --git a/client/prune_crash_reports_test.cc b/client/prune_crash_reports_test.cc index 54d6941e..e5e5a41a 100644 --- a/client/prune_crash_reports_test.cc +++ b/client/prune_crash_reports_test.cc @@ -36,9 +36,8 @@ class MockDatabase : public CrashReportDatabase { public: // CrashReportDatabase: MOCK_METHOD0(GetSettings, Settings*()); - MOCK_METHOD1(PrepareNewCrashReport, OperationStatus(NewReport**)); - MOCK_METHOD2(FinishedWritingCrashReport, OperationStatus(NewReport*, UUID*)); - MOCK_METHOD1(ErrorWritingCrashReport, OperationStatus(NewReport*)); + MOCK_METHOD1(PrepareNewCrashReport, + OperationStatus(std::unique_ptr*)); MOCK_METHOD2(LookUpCrashReport, OperationStatus(const UUID&, Report*)); MOCK_METHOD1(GetPendingReports, OperationStatus(std::vector*)); MOCK_METHOD1(GetCompletedReports, OperationStatus(std::vector*)); @@ -50,6 +49,13 @@ class MockDatabase : public CrashReportDatabase { OperationStatus(const UUID&, Metrics::CrashSkippedReason)); MOCK_METHOD1(DeleteReport, OperationStatus(const UUID&)); MOCK_METHOD1(RequestUpload, OperationStatus(const UUID&)); + + // gmock doesn't support mocking methods with non-copyable types such as + // unique_ptr. + OperationStatus FinishedWritingCrashReport(std::unique_ptr report, + UUID* uuid) override { + return kNoError; + } }; time_t NDaysAgo(int num_days) { diff --git a/handler/mac/crash_report_exception_handler.cc b/handler/mac/crash_report_exception_handler.cc index 6f9cdbe6..2317df24 100644 --- a/handler/mac/crash_report_exception_handler.cc +++ b/handler/mac/crash_report_exception_handler.cc @@ -14,6 +14,7 @@ #include "handler/mac/crash_report_exception_handler.h" +#include #include #include "base/logging.h" @@ -155,7 +156,7 @@ kern_return_t CrashReportExceptionHandler::CatchMachException( process_snapshot.SetClientID(client_id); process_snapshot.SetAnnotationsSimpleMap(*process_annotations_); - CrashReportDatabase::NewReport* new_report; + std::unique_ptr new_report; CrashReportDatabase::OperationStatus database_status = database_->PrepareNewCrashReport(&new_report); if (database_status != CrashReportDatabase::kNoError) { @@ -164,28 +165,22 @@ kern_return_t CrashReportExceptionHandler::CatchMachException( return KERN_FAILURE; } - process_snapshot.SetReportID(new_report->uuid); - - CrashReportDatabase::CallErrorWritingCrashReport - call_error_writing_crash_report(database_, new_report); - - WeakFileHandleFileWriter file_writer(new_report->handle); + process_snapshot.SetReportID(new_report->ReportID()); MinidumpFileWriter minidump; minidump.InitializeFromSnapshot(&process_snapshot); AddUserExtensionStreams( user_stream_data_sources_, &process_snapshot, &minidump); - if (!minidump.WriteEverything(&file_writer)) { + if (!minidump.WriteEverything(new_report->Writer())) { Metrics::ExceptionCaptureResult( Metrics::CaptureResult::kMinidumpWriteFailed); return KERN_FAILURE; } - call_error_writing_crash_report.Disarm(); - UUID uuid; - database_status = database_->FinishedWritingCrashReport(new_report, &uuid); + database_status = + database_->FinishedWritingCrashReport(std::move(new_report), &uuid); if (database_status != CrashReportDatabase::kNoError) { Metrics::ExceptionCaptureResult( Metrics::CaptureResult::kFinishedWritingCrashReportFailed); diff --git a/handler/win/crash_report_exception_handler.cc b/handler/win/crash_report_exception_handler.cc index 0ab206c1..b1ea8446 100644 --- a/handler/win/crash_report_exception_handler.cc +++ b/handler/win/crash_report_exception_handler.cc @@ -15,6 +15,7 @@ #include "handler/win/crash_report_exception_handler.h" #include +#include #include "client/crash_report_database.h" #include "client/settings.h" @@ -90,7 +91,7 @@ unsigned int CrashReportExceptionHandler::ExceptionHandlerServerException( process_snapshot.SetClientID(client_id); process_snapshot.SetAnnotationsSimpleMap(*process_annotations_); - CrashReportDatabase::NewReport* new_report; + std::unique_ptr new_report; CrashReportDatabase::OperationStatus database_status = database_->PrepareNewCrashReport(&new_report); if (database_status != CrashReportDatabase::kNoError) { @@ -100,29 +101,23 @@ unsigned int CrashReportExceptionHandler::ExceptionHandlerServerException( return termination_code; } - process_snapshot.SetReportID(new_report->uuid); - - CrashReportDatabase::CallErrorWritingCrashReport - call_error_writing_crash_report(database_, new_report); - - WeakFileHandleFileWriter file_writer(new_report->handle); + process_snapshot.SetReportID(new_report->ReportID()); MinidumpFileWriter minidump; minidump.InitializeFromSnapshot(&process_snapshot); AddUserExtensionStreams( user_stream_data_sources_, &process_snapshot, &minidump); - if (!minidump.WriteEverything(&file_writer)) { + if (!minidump.WriteEverything(new_report->Writer())) { LOG(ERROR) << "WriteEverything failed"; Metrics::ExceptionCaptureResult( Metrics::CaptureResult::kMinidumpWriteFailed); return termination_code; } - call_error_writing_crash_report.Disarm(); - UUID uuid; - database_status = database_->FinishedWritingCrashReport(new_report, &uuid); + database_status = + database_->FinishedWritingCrashReport(std::move(new_report), &uuid); if (database_status != CrashReportDatabase::kNoError) { LOG(ERROR) << "FinishedWritingCrashReport failed"; Metrics::ExceptionCaptureResult( diff --git a/tools/crashpad_database_util.cc b/tools/crashpad_database_util.cc index cc21698b..b4c2a15b 100644 --- a/tools/crashpad_database_util.cc +++ b/tools/crashpad_database_util.cc @@ -584,16 +584,13 @@ int DatabaseUtilMain(int argc, char* argv[]) { file_reader = std::move(file_path_reader); } - CrashReportDatabase::NewReport* new_report; + std::unique_ptr new_report; CrashReportDatabase::OperationStatus status = database->PrepareNewCrashReport(&new_report); if (status != CrashReportDatabase::kNoError) { return EXIT_FAILURE; } - CrashReportDatabase::CallErrorWritingCrashReport - call_error_writing_crash_report(database.get(), new_report); - char buf[4096]; FileOperationResult read_result; do { @@ -601,16 +598,13 @@ int DatabaseUtilMain(int argc, char* argv[]) { if (read_result < 0) { return EXIT_FAILURE; } - if (read_result > 0 && - !LoggingWriteFile(new_report->handle, buf, read_result)) { + if (read_result > 0 && !new_report->Writer()->Write(buf, read_result)) { return EXIT_FAILURE; } } while (read_result > 0); - call_error_writing_crash_report.Disarm(); - UUID uuid; - status = database->FinishedWritingCrashReport(new_report, &uuid); + status = database->FinishedWritingCrashReport(std::move(new_report), &uuid); if (status != CrashReportDatabase::kNoError) { return EXIT_FAILURE; } diff --git a/util/misc/metrics.cc b/util/misc/metrics.cc index f4fb5882..7d191f71 100644 --- a/util/misc/metrics.cc +++ b/util/misc/metrics.cc @@ -61,8 +61,7 @@ void Metrics::CrashReportPending(PendingReportReason reason) { } // static -void Metrics::CrashReportSize(FileHandle file) { - const FileOffset size = LoggingFileSizeByHandle(file); +void Metrics::CrashReportSize(FileOffset size) { UMA_HISTOGRAM_CUSTOM_COUNTS( "Crashpad.CrashReportSize", size, 0, 20 * 1024 * 1024, 50); } diff --git a/util/misc/metrics.h b/util/misc/metrics.h index b4bea910..fbbf3ecb 100644 --- a/util/misc/metrics.h +++ b/util/misc/metrics.h @@ -50,7 +50,7 @@ class Metrics { //! \brief Reports the size of a crash report file in bytes. Should be called //! when a new report is written to disk. - static void CrashReportSize(FileHandle file); + static void CrashReportSize(FileOffset size); //! \brief Reports on a crash upload attempt, and if it succeeded. static void CrashUploadAttempted(bool successful);