Adding support for on-demand uploads.

In order to allow on-demand uploads for crash reports, adding a
upload_explicitly_requested bit on 'pending' state and necessary support
for it.

BUG=chromium:620762

Change-Id: Ida38e483fe8d0e48eb5cbe95e8b8bfd96a2f8f00
Reviewed-on: https://chromium-review.googlesource.com/367328
Reviewed-by: Mark Mentovai <mark@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>

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,

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() {}
 

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<CrashReportDatabase> InitializeInternal(
     const base::FilePath& path,
     bool may_create) {

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<CrashReportDatabase> 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<CrashReportDatabase::Report> 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<CrashReportDatabase::Report> 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

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<uint32_t>(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<ReportState>(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<Metadata> 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<CrashReportDatabase> InitializeInternal(
              : std::unique_ptr<CrashReportDatabaseWin>();
 }
 
+OperationStatus CrashReportDatabaseWin::RequestUpload(const UUID& uuid) {
+  INITIALIZATION_STATE_DCHECK_VALID(initialized_);
+
+  std::unique_ptr<Metadata> 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

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) {

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);

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

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_

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