diff --git a/client/crash_report_database.cc b/client/crash_report_database.cc index b758ee56..8451e469 100644 --- a/client/crash_report_database.cc +++ b/client/crash_report_database.cc @@ -23,8 +23,8 @@ CrashReportDatabase::Report::Report() creation_time(0), uploaded(false), last_upload_attempt_time(0), - upload_attempts(0) { -} + upload_attempts(0), + upload_explicitly_requested(false) {} CrashReportDatabase::CallErrorWritingCrashReport::CallErrorWritingCrashReport( CrashReportDatabase* database, diff --git a/client/crash_report_database.h b/client/crash_report_database.h index 63501e0c..7fef8a2e 100644 --- a/client/crash_report_database.h +++ b/client/crash_report_database.h @@ -43,7 +43,8 @@ class Settings; //! the client then writes the report, and then calls //! FinishedWritingCrashReport() to make the report Pending. //! 2. Pending: The report has been written but has not been locally -//! processed. +//! 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 //! calling SkipReportUpload(). @@ -88,6 +89,10 @@ class CrashReportDatabase { //! #last_upload_attempt_time will be set to the timestamp of the most //! recent attempt. int upload_attempts; + + //! Whether this crash report was explicitly requested by user to be + //! uploaded. This can be true only if report is in the 'pending' state. + bool upload_explicitly_requested; }; //! \brief A crash report that is in the process of being written. @@ -161,6 +166,10 @@ class CrashReportDatabase { //! \brief The operation could not be completed because a concurrent //! operation affecting the report is occurring. kBusyError, + + //! \brief The report cannot be uploaded by user request as it has already + //! been uploaded. + kCannotRequestUpload, }; virtual ~CrashReportDatabase() {} @@ -328,6 +337,14 @@ class CrashReportDatabase { //! \return The operation status code. virtual OperationStatus DeleteReport(const UUID& uuid) = 0; + //! \brief Marks a crash report as explicitly requested to be uploaded by the + //! user and moves it to 'pending' state. + //! + //! \param[in] uuid The unique identifier for the crash report record. + //! + //! \return The operation status code. + virtual OperationStatus RequestUpload(const UUID& uuid) = 0; + protected: CrashReportDatabase() {} diff --git a/client/crash_report_database_mac.mm b/client/crash_report_database_mac.mm index 42220781..9799bb0f 100644 --- a/client/crash_report_database_mac.mm +++ b/client/crash_report_database_mac.mm @@ -47,9 +47,9 @@ const char kCompletedDirectory[] = "completed"; const char kSettings[] = "settings.dat"; const char* const kReportDirectories[] = { - kWriteDirectory, - kUploadPendingDirectory, - kCompletedDirectory, + kWriteDirectory, + kUploadPendingDirectory, + kCompletedDirectory, }; const char kCrashReportFileExtension[] = "dmp"; @@ -60,6 +60,7 @@ const char kXattrCreationTime[] = "creation_time"; const char kXattrIsUploaded[] = "uploaded"; const char kXattrLastUploadTime[] = "last_upload_time"; const char kXattrUploadAttemptCount[] = "upload_count"; +const char kXattrIsUploadExplicitlyRequested[] = "upload_explicitly_requested"; const char kXattrDatabaseInitialized[] = "initialized"; @@ -140,6 +141,7 @@ class CrashReportDatabaseMac : public CrashReportDatabase { const std::string& id) override; OperationStatus SkipReportUpload(const UUID& uuid) override; OperationStatus DeleteReport(const UUID& uuid) override; + OperationStatus RequestUpload(const UUID& uuid) override; private: //! \brief A private extension of the Report class that maintains bookkeeping @@ -201,6 +203,18 @@ class CrashReportDatabaseMac : public CrashReportDatabase { //! \return The long name of the extended attribute. std::string XattrName(const base::StringPiece& name); + //! \brief Marks a report with a given path as completed. + //! + //! Assumes that the report is locked. + //! + //! \param[in] report_path The path of the file to mark completed. + //! \param[out] out_path The path of the new file. This parameter is optional. + //! + //! \return The operation status code. + CrashReportDatabase::OperationStatus MarkReportCompletedLocked( + const base::FilePath& report_path, + base::FilePath* out_path); + base::FilePath base_dir_; Settings settings_; bool xattr_new_names_; @@ -449,14 +463,10 @@ CrashReportDatabaseMac::RecordUploadAttempt(const Report* report, return kBusyError; if (successful) { - base::FilePath new_path = - base_dir_.Append(kCompletedDirectory).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(); - return kFileSystemError; - } - report_path = new_path; + CrashReportDatabase::OperationStatus os = + MarkReportCompletedLocked(report_path, &report_path); + if (os != kNoError) + return os; } if (!WriteXattrBool(report_path, XattrName(kXattrIsUploaded), successful)) { @@ -500,15 +510,7 @@ CrashReportDatabase::OperationStatus CrashReportDatabaseMac::SkipReportUpload( if (!lock.is_valid()) return kBusyError; - base::FilePath new_path = - base_dir_.Append(kCompletedDirectory).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(); - return kFileSystemError; - } - - return kNoError; + return MarkReportCompletedLocked(report_path, nullptr); } CrashReportDatabase::OperationStatus CrashReportDatabaseMac::DeleteReport( @@ -556,6 +558,45 @@ base::FilePath CrashReportDatabaseMac::LocateCrashReport(const UUID& uuid) { return base::FilePath(); } +CrashReportDatabase::OperationStatus CrashReportDatabaseMac::RequestUpload( + const UUID& uuid) { + INITIALIZATION_STATE_DCHECK_VALID(initialized_); + + base::FilePath report_path = LocateCrashReport(uuid); + if (report_path.empty()) + return kReportNotFound; + + base::ScopedFD lock(ObtainReportLock(report_path)); + if (!lock.is_valid()) + return kBusyError; + + // If the crash report has already been uploaded, don't request new upload. + bool uploaded = false; + XattrStatus status = + ReadXattrBool(report_path, XattrName(kXattrIsUploaded), &uploaded); + if (status == XattrStatus::kOtherError) + return kDatabaseError; + if (uploaded) + return kCannotRequestUpload; + + // Mark the crash report as having upload explicitly requested by the user, + // and move it to the pending state. + if (!WriteXattrBool( + report_path, XattrName(kXattrIsUploadExplicitlyRequested), true)) { + return kDatabaseError; + } + + 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(); + return kFileSystemError; + } + + return kNoError; +} + // static base::ScopedFD CrashReportDatabaseMac::ObtainReportLock( const base::FilePath& path) { @@ -604,6 +645,14 @@ bool CrashReportDatabaseMac::ReadReportMetadataLocked( return false; } + report->upload_explicitly_requested = false; + if (ReadXattrBool(path, + XattrName(kXattrIsUploadExplicitlyRequested), + &report->upload_explicitly_requested) == + XattrStatus::kOtherError) { + return false; + } + return true; } @@ -647,6 +696,28 @@ std::string CrashReportDatabaseMac::XattrName(const base::StringPiece& name) { return XattrNameInternal(name, xattr_new_names_); } +CrashReportDatabase::OperationStatus +CrashReportDatabaseMac::MarkReportCompletedLocked( + const base::FilePath& report_path, + base::FilePath* out_path) { + if (RemoveXattr(report_path, XattrName(kXattrIsUploadExplicitlyRequested)) == + XattrStatus::kOtherError) { + return kDatabaseError; + } + + base::FilePath new_path = + base_dir_.Append(kCompletedDirectory).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(); + return kFileSystemError; + } + + if (out_path) + *out_path = new_path; + return kNoError; +} + std::unique_ptr InitializeInternal( const base::FilePath& path, bool may_create) { diff --git a/client/crash_report_database_test.cc b/client/crash_report_database_test.cc index 90a95aa4..200619d2 100644 --- a/client/crash_report_database_test.cc +++ b/client/crash_report_database_test.cc @@ -94,6 +94,7 @@ class CrashReportDatabaseTest : public testing::Test { EXPECT_FALSE(report.uploaded); EXPECT_EQ(0, report.last_upload_attempt_time); EXPECT_EQ(0, report.upload_attempts); + EXPECT_FALSE(report.upload_explicitly_requested); } void RelocateDatabase() { @@ -102,6 +103,16 @@ class CrashReportDatabaseTest : public testing::Test { SetUp(); } + CrashReportDatabase::OperationStatus RequestUpload(const UUID& uuid) { + CrashReportDatabase::OperationStatus os = db()->RequestUpload(uuid); + + CrashReportDatabase::Report report; + EXPECT_EQ(CrashReportDatabase::kNoError, + db_->LookUpCrashReport(uuid, &report)); + + return os; + } + private: ScopedTempDir temp_dir_; std::unique_ptr db_; @@ -220,6 +231,7 @@ TEST_F(CrashReportDatabaseTest, LookUpCrashReport) { EXPECT_FALSE(report.uploaded); EXPECT_EQ(0, report.last_upload_attempt_time); EXPECT_EQ(0, report.upload_attempts); + EXPECT_FALSE(report.upload_explicitly_requested); } UploadReport(uuid, true, "test"); @@ -234,6 +246,7 @@ TEST_F(CrashReportDatabaseTest, LookUpCrashReport) { EXPECT_TRUE(report.uploaded); EXPECT_NE(0, report.last_upload_attempt_time); EXPECT_EQ(1, report.upload_attempts); + EXPECT_FALSE(report.upload_explicitly_requested); } } @@ -585,6 +598,72 @@ TEST_F(CrashReportDatabaseTest, ReadEmptyDatabase) { CreateCrashReport(&report2); } +TEST_F(CrashReportDatabaseTest, RequestUpload) { + std::vector reports(2); + CreateCrashReport(&reports[0]); + CreateCrashReport(&reports[1]); + + const UUID& report_0_uuid = reports[0].uuid; + const UUID& report_1_uuid = reports[1].uuid; + + // Skipped report gets back to pending state after RequestUpload is called. + EXPECT_EQ(CrashReportDatabase::kNoError, + db()->SkipReportUpload(report_1_uuid)); + + std::vector pending_reports; + CrashReportDatabase::OperationStatus os = + db()->GetPendingReports(&pending_reports); + EXPECT_EQ(CrashReportDatabase::kNoError, os); + ASSERT_EQ(1u, pending_reports.size()); + EXPECT_EQ(pending_reports[0].uuid, report_0_uuid); + + pending_reports.clear(); + EXPECT_EQ(CrashReportDatabase::kNoError, RequestUpload(report_1_uuid)); + os = db()->GetPendingReports(&pending_reports); + EXPECT_EQ(CrashReportDatabase::kNoError, os); + ASSERT_EQ(2u, pending_reports.size()); + + // Check individual reports. + const CrashReportDatabase::Report* expicitly_requested_report; + const CrashReportDatabase::Report* pending_report; + if (pending_reports[0].uuid == report_0_uuid) { + pending_report = &pending_reports[0]; + expicitly_requested_report = &pending_reports[1]; + } else { + pending_report = &pending_reports[1]; + expicitly_requested_report = &pending_reports[0]; + } + + EXPECT_EQ(report_0_uuid, pending_report->uuid); + EXPECT_FALSE(pending_report->upload_explicitly_requested); + + EXPECT_EQ(report_1_uuid, expicitly_requested_report->uuid); + EXPECT_TRUE(expicitly_requested_report->upload_explicitly_requested); + + // Explicitly requested reports will not have upload_explicitly_requested bit + // after getting skipped. + EXPECT_EQ(CrashReportDatabase::kNoError, + db()->SkipReportUpload(report_1_uuid)); + CrashReportDatabase::Report report; + EXPECT_EQ(CrashReportDatabase::kNoError, + db()->LookUpCrashReport(report_1_uuid, &report)); + EXPECT_FALSE(report.upload_explicitly_requested); + + // Pending report gets correctly affected after RequestUpload is called. + pending_reports.clear(); + EXPECT_EQ(CrashReportDatabase::kNoError, RequestUpload(report_0_uuid)); + os = db()->GetPendingReports(&pending_reports); + EXPECT_EQ(CrashReportDatabase::kNoError, os); + EXPECT_EQ(1u, pending_reports.size()); + EXPECT_EQ(pending_reports[0].uuid, report_0_uuid); + EXPECT_TRUE(pending_reports[0].upload_explicitly_requested); + + // Already uploaded report cannot be requested for the new upload. + UploadReport(report_0_uuid, true, "1"); + EXPECT_EQ(CrashReportDatabase::kCannotRequestUpload, + RequestUpload(report_0_uuid)); +} + } // namespace } // namespace test } // namespace crashpad diff --git a/client/crash_report_database_win.cc b/client/crash_report_database_win.cc index c3869c58..c0bc42b8 100644 --- a/client/crash_report_database_win.cc +++ b/client/crash_report_database_win.cc @@ -103,6 +103,14 @@ enum class ReportState { kCompleted, }; +enum { + //! \brief Corresponds to uploaded bit of the report state. + kAttributeUploaded = 1 << 0, + + //! \brief Corresponds to upload_explicity_requested bit of the report state. + kAttributeUploadExplicitlyRequested = 1 << 1, +}; + struct MetadataFileReportRecord { // Note that this default constructor does no initialization. It is used only // to create an array of records that are immediately initialized by reading @@ -121,7 +129,7 @@ struct MetadataFileReportRecord { int64_t last_upload_attempt_time; // Holds a time_t. int32_t upload_attempts; int32_t state; // A ReportState. - uint8_t uploaded; // Boolean, 0 or 1. + uint8_t attributes; // Bitfield of kAttribute*. uint8_t padding[7]; }; @@ -151,7 +159,10 @@ MetadataFileReportRecord::MetadataFileReportRecord(const ReportDisk& report, last_upload_attempt_time(report.last_upload_attempt_time), upload_attempts(report.upload_attempts), state(static_cast(report.state)), - uploaded(report.uploaded) { + attributes((report.uploaded ? kAttributeUploaded : 0) | + (report.upload_explicitly_requested + ? kAttributeUploadExplicitlyRequested + : 0)) { memset(&padding, 0, sizeof(padding)); } @@ -164,10 +175,12 @@ ReportDisk::ReportDisk(const MetadataFileReportRecord& record, base::UTF8ToUTF16(&string_table[record.file_path_index])); id = &string_table[record.id_index]; creation_time = record.creation_time; - uploaded = record.uploaded != 0; last_upload_attempt_time = record.last_upload_attempt_time; upload_attempts = record.upload_attempts; state = static_cast(record.state); + uploaded = (record.attributes & kAttributeUploaded) != 0; + upload_explicitly_requested = + (record.attributes & kAttributeUploadExplicitlyRequested) != 0; } ReportDisk::ReportDisk(const UUID& uuid, @@ -577,6 +590,7 @@ class CrashReportDatabaseWin : public CrashReportDatabase { const std::string& id) override; OperationStatus SkipReportUpload(const UUID& uuid) override; OperationStatus DeleteReport(const UUID& uuid) override; + OperationStatus RequestUpload(const UUID& uuid) override; private: std::unique_ptr AcquireMetadata(); @@ -773,8 +787,14 @@ OperationStatus CrashReportDatabaseWin::RecordUploadAttempt( report_disk->id = id; report_disk->last_upload_attempt_time = now; report_disk->upload_attempts++; - report_disk->state = - successful ? ReportState::kCompleted : ReportState::kPending; + if (successful) { + report_disk->state = ReportState::kCompleted; + report_disk->upload_explicitly_requested = false; + } else { + report_disk->state = ReportState::kPending; + report_disk->upload_explicitly_requested = + report->upload_explicitly_requested; + } if (!settings_.SetLastUploadAttemptTime(now)) return kDatabaseError; @@ -811,8 +831,10 @@ OperationStatus CrashReportDatabaseWin::SkipReportUpload(const UUID& uuid) { ReportDisk* report_disk; OperationStatus os = metadata->FindSingleReportAndMarkDirty( uuid, ReportState::kPending, &report_disk); - if (os == kNoError) + if (os == kNoError) { report_disk->state = ReportState::kCompleted; + report_disk->upload_explicitly_requested = false; + } return os; } @@ -831,6 +853,37 @@ std::unique_ptr InitializeInternal( : std::unique_ptr(); } +OperationStatus CrashReportDatabaseWin::RequestUpload(const UUID& uuid) { + INITIALIZATION_STATE_DCHECK_VALID(initialized_); + + std::unique_ptr metadata(AcquireMetadata()); + if (!metadata) + return kDatabaseError; + + ReportDisk* report_disk; + // TODO(gayane): Search for the report only once regardless of its state. + OperationStatus os = metadata->FindSingleReportAndMarkDirty( + uuid, ReportState::kCompleted, &report_disk); + if (os == kBusyError) { + os = metadata->FindSingleReportAndMarkDirty( + uuid, ReportState::kPending, &report_disk); + } + + if (os != kNoError) + return os; + + // If the crash report has already been uploaded, don't request new upload. + if (report_disk->uploaded) + return kCannotRequestUpload; + + // Mark the crash report as having upload explicitly requested by the user, + // and move it to the pending state. + report_disk->upload_explicitly_requested = true; + report_disk->state = ReportState::kPending; + + return kNoError; +} + } // namespace // static diff --git a/client/prune_crash_reports_test.cc b/client/prune_crash_reports_test.cc index bb9ea8e2..4e7c3da5 100644 --- a/client/prune_crash_reports_test.cc +++ b/client/prune_crash_reports_test.cc @@ -46,6 +46,7 @@ class MockDatabase : public CrashReportDatabase { OperationStatus(const Report*, bool, const std::string&)); MOCK_METHOD1(SkipReportUpload, OperationStatus(const UUID&)); MOCK_METHOD1(DeleteReport, OperationStatus(const UUID&)); + MOCK_METHOD1(RequestUpload, OperationStatus(const UUID&)); }; time_t NDaysAgo(int num_days) { diff --git a/handler/crash_report_upload_thread.cc b/handler/crash_report_upload_thread.cc index 00a12f7f..99c76b77 100644 --- a/handler/crash_report_upload_thread.cc +++ b/handler/crash_report_upload_thread.cc @@ -190,11 +190,12 @@ void CrashReportUploadThread::ProcessPendingReport( Settings* const settings = database_->GetSettings(); bool uploads_enabled; - if (!settings->GetUploadsEnabled(&uploads_enabled) || - !uploads_enabled || - url_.empty()) { - // If the upload-enabled state can’t be determined, uploads are disabled, or - // there’s no URL to upload to, don’t attempt to upload the new report. + if (url_.empty() || + (!report.upload_explicitly_requested && + (!settings->GetUploadsEnabled(&uploads_enabled) || !uploads_enabled))) { + // Don’t attempt an upload if there’s no URL to upload to. Allow upload if + // it has been explicitly requested by the user, otherwise, respect the + // upload-enabled state stored in the database’s settings. database_->SkipReportUpload(report.uuid); return; } @@ -251,6 +252,10 @@ void CrashReportUploadThread::ProcessPendingReport( // to at least try to get the report out of the way. database_->SkipReportUpload(report.uuid); return; + + case CrashReportDatabase::kCannotRequestUpload: + NOTREACHED(); + return; } CallRecordUploadAttempt call_record_upload_attempt(database_, upload_report); diff --git a/util/mac/xattr.cc b/util/mac/xattr.cc index 36dfda83..138de3e4 100644 --- a/util/mac/xattr.cc +++ b/util/mac/xattr.cc @@ -148,4 +148,16 @@ bool WriteXattrTimeT(const base::FilePath& file, return WriteXattr(file, name, tmp); } +XattrStatus RemoveXattr(const base::FilePath& file, + const base::StringPiece& name) { + int rv = removexattr(file.value().c_str(), name.data(), 0); + if (rv != 0) { + if (errno == ENOATTR) + return XattrStatus::kNoAttribute; + PLOG(ERROR) << "removexattr " << name << " on file " << file.value(); + return XattrStatus::kOtherError; + } + return XattrStatus::kOK; +} + } // namespace crashpad diff --git a/util/mac/xattr.h b/util/mac/xattr.h index 3e14f672..ebe13df5 100644 --- a/util/mac/xattr.h +++ b/util/mac/xattr.h @@ -92,6 +92,15 @@ bool WriteXattrTimeT(const base::FilePath& file, const base::StringPiece& name, time_t value); +//! \brief Removes an extended attribute from a file. +//! +//! \param[in] file The path to the file. +//! \param[in] name The name of the extended attribute to remove. +//! +//! \return XattrStatus +XattrStatus RemoveXattr(const base::FilePath& file, + const base::StringPiece& name); + } // namespace crashpad #endif // CRASHPAD_UTIL_MAC_XATTR_H_ diff --git a/util/mac/xattr_test.cc b/util/mac/xattr_test.cc index a156a0a7..daaba307 100644 --- a/util/mac/xattr_test.cc +++ b/util/mac/xattr_test.cc @@ -117,6 +117,20 @@ TEST_F(Xattr, WriteAndReadTimeT) { EXPECT_EQ(expected, actual); } +TEST_F(Xattr, RemoveAndRead) { + const std::string value = "hello world"; + EXPECT_TRUE(WriteXattr(path(), kKey, value)); + + std::string actual; + EXPECT_EQ(XattrStatus::kOK, ReadXattr(path(), kKey, &actual)); + EXPECT_EQ(value, actual); + + EXPECT_EQ(XattrStatus::kOK, RemoveXattr(path(), kKey)); + EXPECT_EQ(XattrStatus::kNoAttribute, ReadXattr(path(), kKey, &actual)); + + EXPECT_EQ(XattrStatus::kNoAttribute, RemoveXattr(path(), kKey)); +} + } // namespace } // namespace test } // namespace crashpad