mac: Prevent the same report from being uploaded multiple times

With multiple crashpad_handlers running out of the same database, it was
possible for more than one to attempt to upload the same report. Nothing
ensured that the reports remained pending between the calls to
CrashReportDatabaseMac::GetPendingReports() and
CrashReportDatabaseMac::GetReportForUploading().

The Windows equivalent did not share this bug, but it would return
kBusyError. kReportNotFound is a better code.

Test: crashpad_client_test CrashReportDatabaseTest.*
Change-Id: Ieaee7f94ca8e6f2606d000bd2ba508d3cfa2fe07
Reviewed-on: https://chromium-review.googlesource.com/473928
Reviewed-by: Robert Sesek <rsesek@chromium.org>

client/crash_report_database.h

@@ -144,6 +144,11 @@ class CrashReportDatabase {
     kNoError = 0,
 
     //! \brief The report that was requested could not be located.
+    //!
+    //! This may occur when the report is present in the database but not in a
+    //! state appropriate for the requested operation, for example, if
+    //! GetReportForUploading() is called to obtain report that’s already in the
+    //! completed state.
     kReportNotFound,
 
     //! \brief An error occured while performing a file operation on a crash

client/crash_report_database_mac.mm

@@ -17,6 +17,7 @@
 #include <errno.h>
 #include <fcntl.h>
 #import <Foundation/Foundation.h>
+#include <stdint.h>
 #include <stdio.h>
 #include <sys/stat.h>
 #include <sys/types.h>
@@ -146,6 +147,17 @@ class CrashReportDatabaseMac : public CrashReportDatabase {
   OperationStatus RequestUpload(const UUID& uuid) override;
 
  private:
+  //! \brief Report states for use with LocateCrashReport().
+  //!
+  //! ReportState may be considered to be a bitfield.
+  enum ReportState : uint8_t {
+    kReportStateWrite = 1 << 0,  // in kWriteDirectory
+    kReportStatePending = 1 << 1,  // in kUploadPendingDirectory
+    kReportStateCompleted = 1 << 2,  // in kCompletedDirectory
+    kReportStateAny =
+        kReportStateWrite | kReportStatePending | kReportStateCompleted,
+  };
+
   //! \brief A private extension of the Report class that maintains bookkeeping
   //!    information of the database.
   struct UploadReport : public Report {
@@ -157,10 +169,12 @@ class CrashReportDatabaseMac : public CrashReportDatabase {
   //! \brief Locates a crash report in the database by UUID.
   //!
   //! \param[in] uuid The UUID of the crash report to locate.
+  //! \param[in] desired_state The state of the report to locate, composed of
+  //!     ReportState values.
   //!
   //! \return The full path to the report file, or an empty path if it cannot be
   //!     found.
-  base::FilePath LocateCrashReport(const UUID& uuid);
+  base::FilePath LocateCrashReport(const UUID& uuid, uint8_t desired_state);
 
   //! \brief Obtains an exclusive advisory lock on a file.
   //!
@@ -392,7 +406,7 @@ CrashReportDatabaseMac::LookUpCrashReport(const UUID& uuid,
                                           CrashReportDatabase::Report* report) {
   INITIALIZATION_STATE_DCHECK_VALID(initialized_);
 
-  base::FilePath path = LocateCrashReport(uuid);
+  base::FilePath path = LocateCrashReport(uuid, kReportStateAny);
   if (path.empty())
     return kReportNotFound;
 
@@ -429,7 +443,7 @@ CrashReportDatabaseMac::GetReportForUploading(const UUID& uuid,
                                               const Report** report) {
   INITIALIZATION_STATE_DCHECK_VALID(initialized_);
 
-  base::FilePath report_path = LocateCrashReport(uuid);
+  base::FilePath report_path = LocateCrashReport(uuid, kReportStatePending);
   if (report_path.empty())
     return kReportNotFound;
 
@@ -459,7 +473,8 @@ CrashReportDatabaseMac::RecordUploadAttempt(const Report* report,
   DCHECK(report);
   DCHECK(successful || id.empty());
 
-  base::FilePath report_path = LocateCrashReport(report->uuid);
+  base::FilePath report_path =
+      LocateCrashReport(report->uuid, kReportStatePending);
   if (report_path.empty())
     return kReportNotFound;
 
@@ -513,7 +528,7 @@ CrashReportDatabase::OperationStatus CrashReportDatabaseMac::SkipReportUpload(
 
   Metrics::CrashUploadSkipped(reason);
 
-  base::FilePath report_path = LocateCrashReport(uuid);
+  base::FilePath report_path = LocateCrashReport(uuid, kReportStatePending);
   if (report_path.empty())
     return kReportNotFound;
 
@@ -528,7 +543,7 @@ CrashReportDatabase::OperationStatus CrashReportDatabaseMac::DeleteReport(
     const UUID& uuid) {
   INITIALIZATION_STATE_DCHECK_VALID(initialized_);
 
-  base::FilePath report_path = LocateCrashReport(uuid);
+  base::FilePath report_path = LocateCrashReport(uuid, kReportStateAny);
   if (report_path.empty())
     return kReportNotFound;
 
@@ -544,11 +559,25 @@ CrashReportDatabase::OperationStatus CrashReportDatabaseMac::DeleteReport(
   return kNoError;
 }
 
-base::FilePath CrashReportDatabaseMac::LocateCrashReport(const UUID& uuid) {
+base::FilePath CrashReportDatabaseMac::LocateCrashReport(
+    const UUID& uuid,
+    uint8_t desired_state) {
   const std::string target_uuid = uuid.ToString();
-  for (size_t i = 0; i < arraysize(kReportDirectories); ++i) {
+
+  std::vector<std::string> report_directories;
+  if (desired_state & kReportStateWrite) {
+    report_directories.push_back(kWriteDirectory);
+  }
+  if (desired_state & kReportStatePending) {
+    report_directories.push_back(kUploadPendingDirectory);
+  }
+  if (desired_state & kReportStateCompleted) {
+    report_directories.push_back(kCompletedDirectory);
+  }
+
+  for (const std::string& report_directory : report_directories) {
     base::FilePath path =
-        base_dir_.Append(kReportDirectories[i])
+        base_dir_.Append(report_directory)
                  .Append(target_uuid + "." + kCrashReportFileExtension);
 
     // Test if the path exists.
@@ -573,7 +602,8 @@ CrashReportDatabase::OperationStatus CrashReportDatabaseMac::RequestUpload(
     const UUID& uuid) {
   INITIALIZATION_STATE_DCHECK_VALID(initialized_);
 
-  base::FilePath report_path = LocateCrashReport(uuid);
+  base::FilePath report_path =
+      LocateCrashReport(uuid, kReportStatePending | kReportStateCompleted);
   if (report_path.empty())
     return kReportNotFound;
 

client/crash_report_database_test.cc

@@ -478,6 +478,22 @@ TEST_F(CrashReportDatabaseTest, DuelingUploads) {
             CrashReportDatabase::kNoError);
 }
 
+TEST_F(CrashReportDatabaseTest, UploadAlreadyUploaded) {
+  CrashReportDatabase::Report report;
+  CreateCrashReport(&report);
+
+  const CrashReportDatabase::Report* upload_report;
+  EXPECT_EQ(db()->GetReportForUploading(report.uuid, &upload_report),
+            CrashReportDatabase::kNoError);
+  EXPECT_EQ(db()->RecordUploadAttempt(upload_report, true, std::string()),
+            CrashReportDatabase::kNoError);
+
+  const CrashReportDatabase::Report* upload_report_2 = nullptr;
+  EXPECT_EQ(db()->GetReportForUploading(report.uuid, &upload_report_2),
+            CrashReportDatabase::kReportNotFound);
+  EXPECT_FALSE(upload_report_2);
+}
+
 TEST_F(CrashReportDatabaseTest, MoveDatabase) {
   CrashReportDatabase::NewReport* new_report;
   EXPECT_EQ(db()->PrepareNewCrashReport(&new_report),

client/crash_report_database_win.cc

@@ -242,8 +242,9 @@ class Metadata {
   //! written to disk via Write().
   //!
   //! \return #kNoError on success. #kReportNotFound if there was no report with
-  //!     the specified UUID. #kBusyError if the report was not in the specified
-  //!     state.
+  //!     the specified UUID, or if the report was not in the specified state
+  //!     and was not uploading. #kBusyError if the report was not in the
+  //!     specified state and was uploading.
   OperationStatus FindSingleReportAndMarkDirty(const UUID& uuid,
                                                ReportState desired_state,
                                                ReportDisk** report_disk);
@@ -530,9 +531,13 @@ OperationStatus Metadata::VerifyReportAnyState(const ReportDisk& report_disk) {
 // static
 OperationStatus Metadata::VerifyReport(const ReportDisk& report_disk,
                                        ReportState desired_state) {
-  return (report_disk.state == desired_state)
-             ? VerifyReportAnyState(report_disk)
-             : CrashReportDatabase::kBusyError;
+  if (report_disk.state == desired_state) {
+    return VerifyReportAnyState(report_disk);
+  }
+
+  return report_disk.state == ReportState::kUploading
+             ? CrashReportDatabase::kBusyError
+             : CrashReportDatabase::kReportNotFound;
 }
 
 bool EnsureDirectory(const base::FilePath& path) {
@@ -876,7 +881,7 @@ OperationStatus CrashReportDatabaseWin::RequestUpload(const UUID& uuid) {
   // TODO(gayane): Search for the report only once regardless of its state.
   OperationStatus os = metadata->FindSingleReportAndMarkDirty(
       uuid, ReportState::kCompleted, &report_disk);
-  if (os == kBusyError) {
+  if (os == kReportNotFound) {
     os = metadata->FindSingleReportAndMarkDirty(
         uuid, ReportState::kPending, &report_disk);
   }

handler/crash_report_upload_thread.cc

@@ -252,9 +252,12 @@ void CrashReportUploadThread::ProcessPendingReport(
       break;
 
     case CrashReportDatabase::kBusyError:
+    case CrashReportDatabase::kReportNotFound:
+      // Someone else may have gotten to it first. If they’re working on it now,
+      // this will be kBusyError. If they’ve already finished with it, it’ll be
+      // kReportNotFound.
       return;
 
-    case CrashReportDatabase::kReportNotFound:
     case CrashReportDatabase::kFileSystemError:
     case CrashReportDatabase::kDatabaseError:
       // In these cases, SkipReportUpload() might not work either, but it’s best