ios: Prevent duplicate uploads and watchdog kills with slow uploads.

On iOS, holding a lock during a slow upload can lead to watchdog kills
if the app is suspended mid-upload. Instead, if the client can obtain
the lock, the database sets a lock-time file attribute and releases the
flock. The file attribute is cleared when the upload is completed. The
lock-time attribute can be used to prevent file access from other
processes, or to discard reports that likely were terminated mid-upload.

Bug:chromium:1342051
Change-Id: Ib878f6ade8eae467ee39acb52288296759c84582
Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/3739019
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Commit-Queue: Justin Cohen <justincohen@chromium.org>
Reviewed-by: Mark Mentovai <mark@chromium.org>

client/crash_report_database.h

@@ -328,7 +328,8 @@ class CrashReportDatabase {
   virtual OperationStatus GetCompletedReports(std::vector<Report>* reports) = 0;
 
   //! \brief Obtains and locks a report object for uploading to a collection
-  //!     server.
+  //!     server. On iOS the file lock is released and mutual-exclusion is kept
+  //!     via a file attribute.
   //!
   //! Callers should upload the crash report using the FileReader provided.
   //! Callers should then call RecordUploadComplete() to record a successful
@@ -336,6 +337,13 @@ class CrashReportDatabase {
   //! be recorded as unsuccessful and the report lock released when \a report is
   //! destroyed.
   //!
+  //! On iOS, holding a lock during a slow upload can lead to watchdog kills if
+  //! the app is suspended mid-upload. Instead, if the client can obtain the
+  //! lock, the database sets a lock-time file attribute and releases the lock.
+  //! The attribute is cleared when the upload is completed. The lock-time
+  //! attribute can be used to prevent file access from other processes, or to
+  //! discard reports that likely were terminated mid-upload.
+  //!
   //! \param[in] uuid The unique identifier for the crash report record.
   //! \param[out] report A crash report record for the report to be uploaded.
   //!     Only valid if this returns #kNoError.

client/crash_report_database_mac.mm

@@ -71,6 +71,9 @@ constexpr char kXattrUUID[] = "uuid";
 constexpr char kXattrCollectorID[] = "id";
 constexpr char kXattrCreationTime[] = "creation_time";
 constexpr char kXattrIsUploaded[] = "uploaded";
+#if BUILDFLAG(IS_IOS)
+constexpr char kXattrUploadStartTime[] = "upload_start_time";
+#endif
 constexpr char kXattrLastUploadTime[] = "last_upload_time";
 constexpr char kXattrUploadAttemptCount[] = "upload_count";
 constexpr char kXattrIsUploadExplicitlyRequested[] =
@@ -189,10 +192,11 @@ class CrashReportDatabaseMac : public CrashReportDatabase {
     //! \brief Obtain a background task assertion while a flock is in use.
     //!     Ensure this is defined first so it is destroyed last.
     internal::ScopedBackgroundTask ios_background_task{"UploadReportMac"};
-#endif  // BUILDFLAG(IS_IOS)
+#else
     //! \brief Stores the flock of the file for the duration of
     //!     GetReportForUploading() and RecordUploadAttempt().
     base::ScopedFD lock_fd;
+#endif  // BUILDFLAG(IS_IOS)
   };
 
   //! \brief Locates a crash report in the database by UUID.
@@ -474,12 +478,50 @@ CrashReportDatabaseMac::GetReportForUploading(
   if (!ReadReportMetadataLocked(upload_report->file_path, upload_report.get()))
     return kDatabaseError;
 
+#if BUILDFLAG(IS_IOS)
+  time_t upload_start_time = 0;
+  if (ReadXattrTimeT(upload_report->file_path,
+                     XattrName(kXattrUploadStartTime),
+                     &upload_start_time) == XattrStatus::kOtherError) {
+    return kDatabaseError;
+  }
+
+  time_t now = time(nullptr);
+  if (upload_start_time) {
+    // If we were able to ObtainReportLock but kXattrUploadStartTime is set,
+    // either another client is uploading this report or a client was terminated
+    // during an upload. CrashReportUploadThread sets the timeout to 20 seconds
+    // for iOS. If kXattrUploadStartTime is less than  5 minutes ago, consider
+    // the report locked and return kBusyError. Otherwise, consider the upload a
+    // failure and skip the report.
+    if (upload_start_time > now - 15 * internal::kUploadReportTimeoutSeconds) {
+      return kBusyError;
+    } else {
+      // SkipReportUpload expects an unlocked report.
+      lock.reset();
+      CrashReportDatabase::OperationStatus os = SkipReportUpload(
+          upload_report->uuid, Metrics::CrashSkippedReason::kUploadFailed);
+      if (os != kNoError) {
+        return kDatabaseError;
+      }
+      return kReportNotFound;
+    }
+  }
+
+  if (!WriteXattrTimeT(
+          upload_report->file_path, XattrName(kXattrUploadStartTime), now)) {
+    return kDatabaseError;
+  }
+#endif
+
   if (!upload_report->Initialize(upload_report->file_path, this)) {
     return kFileSystemError;
   }
 
   upload_report->database_ = this;
+#if !BUILDFLAG(IS_IOS)
   upload_report->lock_fd.reset(lock.release());
+#endif
   upload_report->report_metrics_ = report_metrics;
   report->reset(upload_report.release());
   return kNoError;
@@ -510,6 +552,13 @@ CrashReportDatabaseMac::RecordUploadAttempt(UploadReport* report,
       return os;
   }
 
+#if BUILDFLAG(IS_IOS)
+  if (RemoveXattr(report_path, XattrName(kXattrUploadStartTime)) ==
+      XattrStatus::kOtherError) {
+    return kDatabaseError;
+  }
+#endif
+
   if (!WriteXattrBool(report_path, XattrName(kXattrIsUploaded), successful)) {
     return kDatabaseError;
   }
@@ -554,6 +603,13 @@ CrashReportDatabase::OperationStatus CrashReportDatabaseMac::SkipReportUpload(
   if (!lock.is_valid())
     return kBusyError;
 
+#if BUILDFLAG(IS_IOS)
+  if (RemoveXattr(report_path, XattrName(kXattrUploadStartTime)) ==
+      XattrStatus::kOtherError) {
+    return kDatabaseError;
+  }
+#endif
+
   return MarkReportCompletedLocked(report_path, nullptr);
 }
 

client/crash_report_database_test.cc

@@ -24,6 +24,10 @@
 #include "util/file/file_io.h"
 #include "util/file/filesystem.h"
 
+#if BUILDFLAG(IS_IOS)
+#include "util/mac/xattr.h"
+#endif
+
 namespace crashpad {
 namespace test {
 namespace {
@@ -511,6 +515,46 @@ TEST_F(CrashReportDatabaseTest, DuelingUploads) {
             CrashReportDatabase::kNoError);
 }
 
+#if BUILDFLAG(IS_IOS)
+TEST_F(CrashReportDatabaseTest, InterruptedIOSUploads) {
+  CrashReportDatabase::Report report;
+  CreateCrashReport(&report);
+
+  std::unique_ptr<const CrashReportDatabase::UploadReport> upload_report;
+  EXPECT_EQ(db()->GetReportForUploading(report.uuid, &upload_report),
+            CrashReportDatabase::kNoError);
+
+  // Set upload_start_time to 10 minutes ago.
+  time_t ten_minutes_ago = time(nullptr) - 10 * 60;
+  ASSERT_TRUE(
+      WriteXattrTimeT(report.file_path,
+                      "org.chromium.crashpad.database.upload_start_time",
+                      ten_minutes_ago));
+
+  std::vector<CrashReportDatabase::Report> reports;
+  EXPECT_EQ(db()->GetPendingReports(&reports), CrashReportDatabase::kNoError);
+  ASSERT_EQ(reports.size(), 1u);
+  reports.clear();
+  EXPECT_EQ(db()->GetCompletedReports(&reports), CrashReportDatabase::kNoError);
+  EXPECT_TRUE(reports.empty());
+
+  // Getting a stale report will automatically skip it.
+  std::unique_ptr<const CrashReportDatabase::UploadReport> upload_report_2;
+  EXPECT_EQ(db()->GetReportForUploading(report.uuid, &upload_report_2),
+            CrashReportDatabase::kReportNotFound);
+  EXPECT_FALSE(upload_report_2);
+
+  // Confirm report was moved from pending to completed.
+  EXPECT_EQ(db()->GetPendingReports(&reports), CrashReportDatabase::kNoError);
+  EXPECT_TRUE(reports.empty());
+  EXPECT_EQ(db()->GetCompletedReports(&reports), CrashReportDatabase::kNoError);
+  ASSERT_EQ(reports.size(), 1u);
+
+  EXPECT_EQ(db()->RecordUploadComplete(std::move(upload_report), std::string()),
+            CrashReportDatabase::kReportNotFound);
+}
+#endif
+
 TEST_F(CrashReportDatabaseTest, UploadAlreadyUploaded) {
   CrashReportDatabase::Report report;
   CreateCrashReport(&report);

client/settings.h

@@ -37,6 +37,14 @@ struct ScopedLockedFileHandleTraits {
   static void Free(FileHandle handle);
 };
 
+// TODO(mark): The timeout should be configurable by the client.
+#if BUILDFLAG(IS_IOS)
+// iOS background assertions only last 30 seconds, keep the timeout shorter.
+constexpr double kUploadReportTimeoutSeconds = 20;
+#else
+constexpr double kUploadReportTimeoutSeconds = 60;
+#endif
+
 }  // namespace internal
 
 //! \brief An interface for accessing and modifying the settings of a

handler/crash_report_upload_thread.cc

@@ -311,13 +311,7 @@ CrashReportUploadThread::UploadResult CrashReportUploadThread::UploadReport(
   }
   http_transport->SetBodyStream(http_multipart_builder.GetBodyStream());
   // TODO(mark): The timeout should be configurable by the client.
-#if BUILDFLAG(IS_IOS)
-  // iOS background assertions only last 30 seconds, keep the timeout shorter.
-  double timeout_seconds = 20;
-#else
-  double timeout_seconds = 60;
-#endif
-  http_transport->SetTimeout(timeout_seconds);
+  http_transport->SetTimeout(internal::kUploadReportTimeoutSeconds);
 
   std::string url = url_;
   if (options_.identify_client_via_url) {

util/misc/metrics.cc

@@ -78,6 +78,13 @@ void Metrics::CrashUploadAttempted(bool successful) {
   UMA_HISTOGRAM_BOOLEAN("Crashpad.CrashUpload.AttemptSuccessful", successful);
 }
 
+#if BUILDFLAG(IS_APPLE)
+// static
+void Metrics::CrashUploadErrorCode(int error_code) {
+  base::UmaHistogramSparse("Crashpad.CrashUpload.ErrorCode", error_code);
+}
+#endif
+
 // static
 void Metrics::CrashUploadSkipped(CrashSkippedReason reason) {
   UMA_HISTOGRAM_ENUMERATION(

util/misc/metrics.h

@@ -63,6 +63,12 @@ class Metrics {
   //! \brief Reports on a crash upload attempt, and if it succeeded.
   static void CrashUploadAttempted(bool successful);
 
+#if BUILDFLAG(IS_APPLE) || DOXYGEN
+  //! \brief Records error codes from
+  //!     `+[NSURLConnection sendSynchronousRequest:returningResponse:error:]`.
+  static void CrashUploadErrorCode(int error_code);
+#endif
+
   //! \brief Values for CrashUploadSkipped().
   //!
   //! \note These are used as metrics enumeration values, so new values should

util/net/http_transport_mac.mm

@@ -25,6 +25,7 @@
 #include "package.h"
 #include "util/file/file_io.h"
 #include "util/misc/implicit_cast.h"
+#include "util/misc/metrics.h"
 #include "util/net/http_body.h"
 
 // An implementation of NSInputStream that reads from a
@@ -257,6 +258,7 @@ bool HTTPTransportMac::ExecuteSynchronously(std::string* response_body) {
 #pragma clang diagnostic pop
 
     if (error) {
+      Metrics::CrashUploadErrorCode(error.code);
       LOG(ERROR) << [[error localizedDescription] UTF8String] << " ("
                  << [[error domain] UTF8String] << " " << [error code] << ")";
       return false;