crash: Send UMA metric if crash_sender fails to rm
BUG=chromium:1060019
TEST=unit tests
Change-Id: I4091a1d5ae8ca09c3f9ff361a109a051bc961c3e
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2310887
Tested-by: Miriam Zimmerman <mutexlox@chromium.org>
Commit-Queue: Miriam Zimmerman <mutexlox@chromium.org>
Reviewed-by: Ian Barkley-Yeung <iby@chromium.org>
diff --git a/crash-reporter/crash_sender_util.cc b/crash-reporter/crash_sender_util.cc
index c63384e..c0f0d8b 100644
--- a/crash-reporter/crash_sender_util.cc
+++ b/crash-reporter/crash_sender_util.cc
@@ -85,6 +85,11 @@
constexpr char kTestModeSuccessful[] =
"Test Mode: Logging success and exiting instead of actually uploading";
+// UMA metrics to track crash removal attempts and failures.
+constexpr char kUMAFailedCrashRemoval[] = "Crash.Sender.FailedCrashRemoval";
+constexpr char kUMAAttemptedCrashRemoval[] =
+ "Crash.Sender.AttemptedCrashRemoval";
+
// Returns true if the given report kind is known.
// TODO(satorux): Move collector constants to a common file.
bool IsKnownKind(const std::string& kind) {
@@ -306,41 +311,6 @@
});
}
-void RemoveReportFiles(const base::FilePath& meta_file, bool delete_crashes) {
- if (meta_file.Extension() != ".meta") {
- LOG(ERROR) << "Not a meta file: " << meta_file.value();
- return;
- }
-
- const std::string pattern =
- meta_file.BaseName().RemoveExtension().value() + ".*";
-
- bool add_uploaded_file = !delete_crashes;
- if (delete_crashes) {
- base::FileEnumerator iter(meta_file.DirName(), false /* recursive */,
- base::FileEnumerator::FILES, pattern);
- for (base::FilePath file = iter.Next(); !file.empty(); file = iter.Next()) {
- if (!base::DeleteFile(file, false /* recursive */)) {
- PLOG(WARNING) << "Failed to remove " << file.value();
- // We may have failed to remove the file due to incorrect selinux config
- // on the directory. However, we may still be able to add files to it,
- // so mark the crash as uploaded to prevent uploading it again.
- // See https://crbug.com/1060019.
- if (file.Extension() == ".meta") {
- add_uploaded_file = true;
- }
- }
- }
- }
- if (add_uploaded_file) {
- base::File f(meta_file.ReplaceExtension(kAlreadyUploadedExt),
- base::File::FLAG_CREATE | base::File::FLAG_WRITE);
- if (!f.IsValid()) {
- LOG(ERROR) << "Failed to mark crash as uploaded";
- }
- }
-}
-
std::vector<base::FilePath> GetMetaFiles(const base::FilePath& crash_dir) {
std::vector<base::FilePath> meta_files;
if (!base::DirectoryExists(crash_dir)) {
@@ -483,7 +453,7 @@
current_bytes += previous_send.size();
} else {
if (!base::DeleteFile(file, false /* recursive */))
- PLOG(WARNING) << "Failed to remove " << file.value();
+ PLOG(WARNING) << "Failed to remove old report " << file.value();
}
}
LOG(INFO) << "Current send rate: " << current_rate << " sends and "
@@ -1077,6 +1047,48 @@
return form_data;
}
+void Sender::RemoveReportFiles(const base::FilePath& meta_file,
+ bool delete_crashes) {
+ if (meta_file.Extension() != ".meta") {
+ LOG(ERROR) << "Not a meta file: " << meta_file.value();
+ return;
+ }
+
+ const std::string pattern =
+ meta_file.BaseName().RemoveExtension().value() + ".*";
+
+ bool add_uploaded_file = !delete_crashes;
+ if (delete_crashes) {
+ if (!metrics_lib_->SendCrosEventToUMA(kUMAAttemptedCrashRemoval)) {
+ LOG(WARNING) << "Failed to record crash removal attempt in UMA";
+ }
+ base::FileEnumerator iter(meta_file.DirName(), false /* recursive */,
+ base::FileEnumerator::FILES, pattern);
+ for (base::FilePath file = iter.Next(); !file.empty(); file = iter.Next()) {
+ if (!base::DeleteFile(file, false /* recursive */)) {
+ PLOG(WARNING) << "Failed to remove " << file.value();
+ // We may have failed to remove the file due to incorrect selinux config
+ // on the directory. However, we may still be able to add files to it,
+ // so mark the crash as uploaded to prevent uploading it again.
+ // See https://crbug.com/1060019.
+ if (file.Extension() == ".meta") {
+ if (!metrics_lib_->SendCrosEventToUMA(kUMAFailedCrashRemoval)) {
+ LOG(WARNING) << "Further, couldn't record UMA event for failure";
+ }
+ add_uploaded_file = true;
+ }
+ }
+ }
+ }
+ if (add_uploaded_file) {
+ base::File f(meta_file.ReplaceExtension(kAlreadyUploadedExt),
+ base::File::FLAG_CREATE | base::File::FLAG_WRITE);
+ if (!f.IsValid()) {
+ LOG(ERROR) << "Failed to mark crash as uploaded";
+ }
+ }
+}
+
std::unique_ptr<base::Value> Sender::CreateJsonEntity(
const std::string& report_id,
const std::string& product_name,
diff --git a/crash-reporter/crash_sender_util.h b/crash-reporter/crash_sender_util.h
index 0f330c3..81bdb70 100644
--- a/crash-reporter/crash_sender_util.h
+++ b/crash-reporter/crash_sender_util.h
@@ -20,6 +20,7 @@
#include <brillo/http/http_form_data.h>
#include <brillo/key_value_store.h>
#include <brillo/osrelease_reader.h>
+#include <gtest/gtest_prod.h> // for FRIEND_TEST
#include <metrics/metrics_library.h>
#include <session_manager/dbus-proxies.h>
#include <shill/dbus-proxies.h>
@@ -126,10 +127,6 @@
// is at the front of the vector.
void SortReports(std::vector<MetaFile>* reports);
-// Removes report files associated with the given meta file.
-// More specifically, if "foo.meta" is given, "foo.*" will be removed.
-void RemoveReportFiles(const base::FilePath& meta_file, bool delete_crashes);
-
// Returns the list of meta data files (files with ".meta" suffix), sorted by
// the timestamp in the old-to-new order.
std::vector<base::FilePath> GetMetaFiles(const base::FilePath& crash_dir);
@@ -312,6 +309,12 @@
private:
friend class IsNetworkOnlineTest;
+ FRIEND_TEST(CrashSenderUtilTest, RemoveReportFiles);
+ FRIEND_TEST(CrashSenderUtilTest, FailRemoveReportFilesSendsMetric);
+
+ // Removes report files associated with the given meta file.
+ // More specifically, if "foo.meta" is given, "foo.*" will be removed.
+ void RemoveReportFiles(const base::FilePath& meta_file, bool delete_crashes);
// Creates a JSON entity with the required fields for uploads.log file.
std::unique_ptr<base::Value> CreateJsonEntity(const std::string& report_id,
diff --git a/crash-reporter/crash_sender_util_test.cc b/crash-reporter/crash_sender_util_test.cc
index 0993d3b..2f42bf8 100644
--- a/crash-reporter/crash_sender_util_test.cc
+++ b/crash-reporter/crash_sender_util_test.cc
@@ -1006,6 +1006,11 @@
}
TEST_F(CrashSenderUtilTest, RemoveReportFiles) {
+ Sender::Options options;
+ Sender sender(std::move(metrics_lib_),
+ std::make_unique<test_util::AdvancingClock>(), options);
+ ASSERT_TRUE(sender.Init());
+
const base::FilePath crash_directory =
paths::Get(paths::kSystemCrashDirectory);
ASSERT_TRUE(base::CreateDirectory(crash_directory));
@@ -1024,7 +1029,7 @@
EXPECT_FALSE(base::PathExists(foo_uploaded));
// This should create the alreadyuploaded file.
- RemoveReportFiles(foo_meta, false);
+ sender.RemoveReportFiles(foo_meta, false);
std::string contents;
ASSERT_TRUE(base::ReadFileToString(foo_meta, &contents));
EXPECT_TRUE(base::PathExists(foo_uploaded));
@@ -1033,7 +1038,7 @@
base::File::Info uploaded_info;
ASSERT_TRUE(base::GetFileInfo(foo_uploaded, &uploaded_info));
// This should not update the alreadyuploaded file, since it already exists
- RemoveReportFiles(foo_meta, false);
+ sender.RemoveReportFiles(foo_meta, false);
ASSERT_TRUE(base::ReadFileToString(foo_meta, &contents));
EXPECT_TRUE(base::PathExists(foo_uploaded));
base::File::Info new_uploaded_info;
@@ -1042,9 +1047,9 @@
EXPECT_EQ(new_uploaded_info.size, uploaded_info.size);
// This should remove foo.*.
- RemoveReportFiles(foo_meta, true);
+ sender.RemoveReportFiles(foo_meta, true);
// This should do nothing because the suffix is not ".meta".
- RemoveReportFiles(bar_log, true);
+ sender.RemoveReportFiles(bar_log, true);
// Check what files were removed.
EXPECT_FALSE(base::PathExists(foo_meta));
@@ -1053,6 +1058,38 @@
EXPECT_TRUE(base::PathExists(bar_log));
}
+TEST_F(CrashSenderUtilTest, FailRemoveReportFilesSendsMetric) {
+ Sender::Options options;
+ EXPECT_CALL(*metrics_lib_,
+ SendCrosEventToUMA("Crash.Sender.AttemptedCrashRemoval"))
+ .WillOnce(Return(true));
+ EXPECT_CALL(*metrics_lib_,
+ SendCrosEventToUMA("Crash.Sender.FailedCrashRemoval"))
+ .WillOnce(Return(true));
+
+ Sender sender(std::move(metrics_lib_),
+ std::make_unique<test_util::AdvancingClock>(), options);
+ ASSERT_TRUE(sender.Init());
+
+ const base::FilePath crash_directory =
+ paths::Get(paths::kSystemCrashDirectory);
+ ASSERT_TRUE(base::CreateDirectory(crash_directory));
+
+ const base::FilePath foo_meta = crash_directory.Append("foo.meta");
+ ASSERT_TRUE(test_util::CreateFile(foo_meta, ""));
+ const base::FilePath foo_log = crash_directory.Append("foo.log");
+ ASSERT_TRUE(test_util::CreateFile(foo_log, ""));
+
+ // chmod the file so RemoveReportFiles fails
+ ASSERT_EQ(chmod(crash_directory.value().c_str(), 0500), 0);
+
+ sender.RemoveReportFiles(foo_meta, true);
+
+ // Clean up after ourselves
+ EXPECT_EQ(chmod(crash_directory.value().c_str(), 0700), 0);
+ EXPECT_TRUE(base::DeleteFile(crash_directory, true));
+}
+
TEST_F(CrashSenderUtilTest, GetMetaFiles) {
const base::FilePath crash_directory =
paths::Get(paths::kSystemCrashDirectory);
diff --git a/metrics/metrics_library.cc b/metrics/metrics_library.cc
index 755401c..f3f7236 100644
--- a/metrics/metrics_library.cc
+++ b/metrics/metrics_library.cc
@@ -76,12 +76,14 @@
"Crash.Collector.CollectionCount", // 27
"Cryptohome.DoubleMountRequest", // 28
"SessionManager.SafeModeEnabled", // 29
+ "Crash.Sender.FailedCrashRemoval", // 30
+ "Crash.Sender.AttemptedCrashRemoval", // 31
};
// Update this to be last entry + 1 when you add new entries to the end. Checks
// that no one tries to remove entries from the middle or misnumbers during a
// merge conflict.
-static_assert(base::size(kCrosEventNames) == 30,
+static_assert(base::size(kCrosEventNames) == 32,
"CrosEvent enums not lining up properly");
} // namespace
