fix report size computation in prune condition

today the attachments are not taken into account, but should on Linux and Fuchsia

Bug: fuchsia:DX-1104

Tested:`fx run-test crashpad_test` for Fuchsia.

Change-Id: I022331bdb09c637f40ff2ba2d711e301e211e86a
Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/1518323
Reviewed-by: Scott Graham <scottmg@chromium.org>
Commit-Queue: Francois Rousseau <frousseau@google.com>

client/crash_report_database.cc

@@ -27,7 +27,8 @@ CrashReportDatabase::Report::Report()
       uploaded(false),
       last_upload_attempt_time(0),
       upload_attempts(0),
-      upload_explicitly_requested(false) {}
+      upload_explicitly_requested(false),
+      total_size(0u) {}
 
 CrashReportDatabase::NewReport::NewReport()
     : writer_(std::make_unique<FileWriter>()),

client/crash_report_database.h

@@ -15,6 +15,7 @@
 #ifndef CRASHPAD_CLIENT_CRASH_REPORT_DATABASE_H_
 #define CRASHPAD_CLIENT_CRASH_REPORT_DATABASE_H_
 
+#include <stdint.h>
 #include <time.h>
 
 #include <map>
@@ -98,6 +99,10 @@ class CrashReportDatabase {
     //! 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;
+
+    //! The total size in bytes taken by the report, including any potential
+    //! attachments.
+    uint64_t total_size;
   };
 
   //! \brief A crash report that is in the process of being written.

client/crash_report_database_generic.cc

@@ -15,6 +15,7 @@
 #include "client/crash_report_database.h"
 
 #include <stdint.h>
+#include <sys/stat.h>
 #include <sys/types.h>
 
 #include <utility>
@@ -160,6 +161,35 @@ class ScopedLockFile {
   DISALLOW_COPY_AND_ASSIGN(ScopedLockFile);
 };
 
+off_t GetFileSize(const base::FilePath& filepath) {
+  struct stat statbuf;
+  if (stat(filepath.value().c_str(), &statbuf) == 0) {
+    return statbuf.st_size;
+  }
+  PLOG(ERROR) << "stat " << filepath.value();
+  return 0;
+}
+
+void AddAttachmentSize(const base::FilePath& attachments_dir, uint64_t* size) {
+  // Early return if the attachment directory does not exist.
+  struct stat statbuf;
+  if (stat(attachments_dir.value().c_str(), &statbuf) != 0) {
+    return;
+  }
+  DirectoryReader reader;
+  if (!reader.Open(attachments_dir)) {
+    return;
+  }
+  base::FilePath attachment_filename;
+  DirectoryReader::Result result;
+  while ((result = reader.NextFile(&attachment_filename)) ==
+         DirectoryReader::Result::kSuccess) {
+    const base::FilePath attachment_filepath(
+        attachments_dir.Append(attachment_filename));
+    *size += GetFileSize(attachment_filepath);
+  }
+}
+
 }  // namespace
 
 class CrashReportDatabaseGeneric : public CrashReportDatabase {
@@ -253,7 +283,7 @@ class CrashReportDatabaseGeneric : public CrashReportDatabase {
   void RemoveAttachmentsByUUID(const UUID& uuid);
 
   // Reads the metadata for a report from path and returns it in report.
-  static bool ReadMetadata(const base::FilePath& path, Report* report);
+  bool ReadMetadata(const base::FilePath& path, Report* report);
 
   // Wraps ReadMetadata and removes the report from the database on failure.
   bool CleaningReadMetadata(const base::FilePath& path, Report* report);
@@ -899,7 +929,6 @@ void CrashReportDatabaseGeneric::RemoveAttachmentsByUUID(const UUID& uuid) {
   LoggingRemoveDirectory(attachments_dir);
 }
 
-// static
 bool CrashReportDatabaseGeneric::ReadMetadata(const base::FilePath& path,
                                               Report* report) {
   const base::FilePath metadata_path(
@@ -910,7 +939,8 @@ bool CrashReportDatabaseGeneric::ReadMetadata(const base::FilePath& path,
     return false;
   }
 
-  if (!report->uuid.InitializeFromString(
+  UUID uuid;
+  if (!uuid.InitializeFromString(
           path.BaseName().RemoveFinalExtension().value())) {
     LOG(ERROR) << "Couldn't interpret report uuid";
     return false;
@@ -930,6 +960,12 @@ bool CrashReportDatabaseGeneric::ReadMetadata(const base::FilePath& path,
     return false;
   }
 
+  // Seed the total size with the main report size and then add the sizes of any
+  // potential attachments.
+  uint64_t total_size = GetFileSize(path);
+  AddAttachmentSize(AttachmentsPath(uuid), &total_size);
+
+  report->uuid = uuid;
   report->upload_attempts = metadata.upload_attempts;
   report->last_upload_attempt_time = metadata.last_upload_attempt_time;
   report->creation_time = metadata.creation_time;
@@ -937,6 +973,7 @@ bool CrashReportDatabaseGeneric::ReadMetadata(const base::FilePath& path,
   report->upload_explicitly_requested =
       (metadata.attributes & kAttributeUploadExplicitlyRequested) != 0;
   report->file_path = path;
+  report->total_size = total_size;
   return true;
 }
 

client/crash_report_database_mac.mm

@@ -682,6 +682,14 @@ bool CrashReportDatabaseMac::ReadReportMetadataLocked(
     return false;
   }
 
+  // There are no attachments on Mac so the total size is the main report size.
+  struct stat statbuf;
+  if (stat(path.value().c_str(), &statbuf) != 0) {
+    PLOG(ERROR) << "stat " << path.value();
+    return false;
+  }
+  report->total_size = statbuf.st_size;
+
   return true;
 }
 

client/crash_report_database_test.cc

@@ -31,8 +31,7 @@ namespace {
 
 class CrashReportDatabaseTest : public testing::Test {
  public:
-  CrashReportDatabaseTest() {
-  }
+  CrashReportDatabaseTest() {}
 
  protected:
   // testing::Test:
@@ -41,9 +40,7 @@ class CrashReportDatabaseTest : public testing::Test {
     ASSERT_TRUE(db_);
   }
 
-  void ResetDatabase() {
-    db_.reset();
-  }
+  void ResetDatabase() { db_.reset(); }
 
   CrashReportDatabase* db() { return db_.get(); }
   base::FilePath path() const {
@@ -101,6 +98,7 @@ class CrashReportDatabaseTest : public testing::Test {
     EXPECT_EQ(report.last_upload_attempt_time, 0);
     EXPECT_EQ(report.upload_attempts, 0);
     EXPECT_FALSE(report.upload_explicitly_requested);
+    EXPECT_GE(report.total_size, 0u);
   }
 
   void RelocateDatabase() {
@@ -835,6 +833,66 @@ TEST_F(CrashReportDatabaseTest, CleanBrokenDatabase) {
 }
 #endif  // !OS_MACOSX && !OS_WIN
 
+TEST_F(CrashReportDatabaseTest, TotalSize_MainReportOnly) {
+  std::unique_ptr<CrashReportDatabase::NewReport> new_report;
+  ASSERT_EQ(db()->PrepareNewCrashReport(&new_report),
+            CrashReportDatabase::kNoError);
+
+  // Main report.
+  static constexpr char main_report_data[] = "dlbvandslhb";
+  ASSERT_TRUE(
+      new_report->Writer()->Write(main_report_data, sizeof(main_report_data)));
+
+  UUID uuid;
+  ASSERT_EQ(db()->FinishedWritingCrashReport(std::move(new_report), &uuid),
+            CrashReportDatabase::kNoError);
+
+  CrashReportDatabase::Report report;
+  ASSERT_EQ(db()->LookUpCrashReport(uuid, &report),
+            CrashReportDatabase::kNoError);
+
+  EXPECT_EQ(report.total_size, sizeof(main_report_data));
+}
+
+TEST_F(CrashReportDatabaseTest, GetReportSize_RightSizeWithAttachments) {
+#if defined(OS_MACOSX) || defined(OS_WIN)
+  // Attachments aren't supported on Mac and Windows yet.
+  return;
+#else
+  std::unique_ptr<CrashReportDatabase::NewReport> new_report;
+  ASSERT_EQ(db()->PrepareNewCrashReport(&new_report),
+            CrashReportDatabase::kNoError);
+
+  // Main report.
+  static constexpr char main_report_data[] = "dlbvandslhb";
+  ASSERT_TRUE(
+      new_report->Writer()->Write(main_report_data, sizeof(main_report_data)));
+
+  // First attachment.
+  FileWriter* attachment_1 = new_report->AddAttachment("my_attachment_1");
+  ASSERT_NE(attachment_1, nullptr);
+  static constexpr char attachment_1_data[] = "vKDnidhvbiudshoihbvdsoiuh nhh";
+  attachment_1->Write(attachment_1_data, sizeof(attachment_1_data));
+
+  // Second attachment.
+  FileWriter* attachment_2 = new_report->AddAttachment("my_attachment_2");
+  ASSERT_NE(attachment_2, nullptr);
+  static constexpr char attachment_2_data[] = "sgvsvgusiyguysigfkhpmo-[";
+  attachment_2->Write(attachment_2_data, sizeof(attachment_2_data));
+
+  UUID uuid;
+  ASSERT_EQ(db()->FinishedWritingCrashReport(std::move(new_report), &uuid),
+            CrashReportDatabase::kNoError);
+
+  CrashReportDatabase::Report report;
+  ASSERT_EQ(db()->LookUpCrashReport(uuid, &report),
+            CrashReportDatabase::kNoError);
+  EXPECT_EQ(report.total_size,
+            sizeof(main_report_data) + sizeof(attachment_1_data) +
+                sizeof(attachment_2_data));
+#endif
+}
+
 }  // namespace
 }  // namespace test
 }  // namespace crashpad

client/crash_report_database_win.cc

@@ -458,7 +458,19 @@ void Metadata::Read() {
         LOG(ERROR) << "invalid string table index";
         return;
       }
-      reports.push_back(ReportDisk(record, report_dir_, string_table));
+      ReportDisk report_disk(record, report_dir_, string_table);
+
+      // There are no attachments on Windows so the total size is the main
+      // report size.
+      struct _stati64 statbuf;
+      if (_wstat64(report_disk.file_path.value().c_str(), &statbuf) == 0) {
+        report_disk.total_size = statbuf.st_size;
+      } else {
+        PLOG(ERROR) << "stat "
+                    << base::UTF16ToUTF8(report_disk.file_path.value());
+      }
+
+      reports.push_back(report_disk);
     }
   }
   reports_.swap(reports);

client/prune_crash_reports.cc

@@ -96,19 +96,9 @@ DatabaseSizePruneCondition::~DatabaseSizePruneCondition() {}
 
 bool DatabaseSizePruneCondition::ShouldPruneReport(
     const CrashReportDatabase::Report& report) {
-#if defined(OS_POSIX)
-  struct stat statbuf;
-  if (stat(report.file_path.value().c_str(), &statbuf) == 0) {
-#elif defined(OS_WIN)
-  struct _stati64 statbuf;
-  if (_wstat64(report.file_path.value().c_str(), &statbuf) == 0) {
-#else
-#error "Not implemented"
-#endif
   // Round up fractional KB to the next 1-KB boundary.
   measured_size_in_kb_ +=
-        static_cast<size_t>((statbuf.st_size + 1023) / 1024);
-  }
+      static_cast<size_t>((report.total_size + 1023) / 1024);
   return measured_size_in_kb_ > max_size_in_kb_;
 }
 

client/prune_crash_reports_test.cc

@@ -81,56 +81,49 @@ TEST(PruneCrashReports, AgeCondition) {
 }
 
 TEST(PruneCrashReports, SizeCondition) {
-  ScopedTempDir temp_dir;
-
   CrashReportDatabase::Report report_1k;
-  report_1k.file_path = temp_dir.path().Append(FILE_PATH_LITERAL("file1024"));
+  report_1k.total_size = 1024u;
   CrashReportDatabase::Report report_3k;
-  report_3k.file_path = temp_dir.path().Append(FILE_PATH_LITERAL("file3072"));
+  report_3k.total_size = 1024u * 3u;
+  CrashReportDatabase::Report report_unset_size;
 
   {
-    ScopedFileHandle scoped_file_1k(
-        LoggingOpenFileForWrite(report_1k.file_path,
-                                FileWriteMode::kCreateOrFail,
-                                FilePermissions::kOwnerOnly));
-    ASSERT_TRUE(scoped_file_1k.is_valid());
-
-    std::string string;
-    for (int i = 0; i < 128; ++i)
-      string.push_back(static_cast<char>(i));
-
-    for (size_t i = 0; i < 1024; i += string.size()) {
-      ASSERT_TRUE(LoggingWriteFile(scoped_file_1k.get(),
-                                   string.c_str(), string.length()));
-    }
-
-    ScopedFileHandle scoped_file_3k(
-        LoggingOpenFileForWrite(report_3k.file_path,
-                                FileWriteMode::kCreateOrFail,
-                                FilePermissions::kOwnerOnly));
-    ASSERT_TRUE(scoped_file_3k.is_valid());
-
-    for (size_t i = 0; i < 3072; i += string.size()) {
-      ASSERT_TRUE(LoggingWriteFile(scoped_file_3k.get(),
-                                   string.c_str(), string.length()));
-    }
-  }
-
-  {
-    DatabaseSizePruneCondition condition(1);
+    DatabaseSizePruneCondition condition(/*max_size_in_kb=*/1);
+    // |report_1k| should not be pruned as the cumulated size is not past 1kB
+    // yet.
     EXPECT_FALSE(condition.ShouldPruneReport(report_1k));
-    EXPECT_TRUE(condition.ShouldPruneReport(report_1k));
-  }
-
-  {
-    DatabaseSizePruneCondition condition(1);
+    // |report_3k| should be pruned as the cumulated size is now past 1kB.
     EXPECT_TRUE(condition.ShouldPruneReport(report_3k));
   }
 
   {
-    DatabaseSizePruneCondition condition(6);
+    DatabaseSizePruneCondition condition(/*max_size_in_kb=*/1);
+    // |report_3k| should be pruned as the cumulated size is already past 1kB.
+    EXPECT_TRUE(condition.ShouldPruneReport(report_3k));
+  }
+
+  {
+    DatabaseSizePruneCondition condition(/*max_size_in_kb=*/6);
+    // |report_3k| should not be pruned as the cumulated size is not past 6kB
+    // yet.
     EXPECT_FALSE(condition.ShouldPruneReport(report_3k));
+    // |report_3k| should not be pruned as the cumulated size is not past 6kB
+    // yet.
     EXPECT_FALSE(condition.ShouldPruneReport(report_3k));
+    // |report_1k| should be pruned as the cumulated size is now past 6kB.
+    EXPECT_TRUE(condition.ShouldPruneReport(report_1k));
+  }
+
+  {
+    DatabaseSizePruneCondition condition(/*max_size_in_kb=*/0);
+    // |report_unset_size| should not be pruned as its size is 0, regardless of
+    // how many times we try to prune it.
+    EXPECT_FALSE(condition.ShouldPruneReport(report_unset_size));
+    EXPECT_FALSE(condition.ShouldPruneReport(report_unset_size));
+    EXPECT_FALSE(condition.ShouldPruneReport(report_unset_size));
+    EXPECT_FALSE(condition.ShouldPruneReport(report_unset_size));
+    EXPECT_FALSE(condition.ShouldPruneReport(report_unset_size));
+    // |report_1k| should be pruned as the cumulated size is now past 0kB.
     EXPECT_TRUE(condition.ShouldPruneReport(report_1k));
   }
 }