crash-reporter: keep ignoring chrome crashes even on dev builds
Change-Id: Idd8859958e626dc782e511f35fca5d45a2977f53
BUG=chromium-os:12911
TEST=unit tests and UserCrash
Review URL: http://codereview.chromium.org/6673002
diff --git a/crash_reporter/user_collector.cc b/crash_reporter/user_collector.cc
index c166e12..a038e69 100644
--- a/crash_reporter/user_collector.cc
+++ b/crash_reporter/user_collector.cc
@@ -415,6 +415,38 @@
return re.FullMatch(crash_attributes, pid, signal, kernel_supplied_name);
}
+bool UserCollector::ShouldDump(bool has_owner_consent,
+ bool is_developer,
+ bool is_crash_test_in_progress,
+ const std::string &exec,
+ std::string *reason) {
+ reason->clear();
+
+ // Treat Chrome crashes as if the user opted-out. We stop counting Chrome
+ // crashes towards user crashes, so user crashes really mean non-Chrome
+ // user-space crashes.
+ if (exec == "chrome" || exec == "supplied_chrome") {
+ *reason = "ignoring - chrome crash";
+ return false;
+ }
+
+ // For developer builds, we always want to keep the crash reports unless
+ // we're testing the crash facilities themselves. This overrides
+ // feedback. Crash sending still obeys consent.
+ if (is_developer && !is_crash_test_in_progress) {
+ *reason = "developer build - not testing - always dumping";
+ return true;
+ }
+
+ if (!has_owner_consent) {
+ *reason = "ignoring - no consent";
+ return false;
+ }
+
+ *reason = "handling";
+ return true;
+}
+
bool UserCollector::HandleCrash(const std::string &crash_attributes,
const char *force_exec) {
CHECK(initialized_);
@@ -450,31 +482,17 @@
return true;
}
- bool feedback = is_feedback_allowed_function_();
- const char *handling_string = "handling";
- if (!feedback) {
- handling_string = "ignoring - no consent";
- }
-
- // Treat Chrome crashes as if the user opted-out. We stop counting Chrome
- // crashes towards user crashes, so user crashes really mean non-Chrome
- // user-space crashes.
- if (exec == "chrome" || exec == "supplied_chrome") {
- feedback = false;
- handling_string = "ignoring - chrome crash";
- }
+ std::string reason;
+ bool dump = ShouldDump(is_feedback_allowed_function_(),
+ file_util::PathExists(FilePath(kLeaveCoreFile)),
+ IsCrashTestInProgress(),
+ exec,
+ &reason);
LOG(WARNING) << "Received crash notification for " << exec << "[" << pid
- << "] sig " << signal << " (" << handling_string << ")";
+ << "] sig " << signal << " (" << reason << ")";
- // For developer builds, we always want to keep the crash reports unless
- // we're testing the crash facilities themselves.
- if (file_util::PathExists(FilePath(kLeaveCoreFile)) &&
- !IsCrashTestInProgress()) {
- feedback = true;
- }
-
- if (feedback) {
+ if (dump) {
count_crash_function_();
if (generate_diagnostics_) {
diff --git a/crash_reporter/user_collector.h b/crash_reporter/user_collector.h
index e46382c..a231dd6 100644
--- a/crash_reporter/user_collector.h
+++ b/crash_reporter/user_collector.h
@@ -63,6 +63,9 @@
FRIEND_TEST(UserCollectorTest, GetSymlinkTarget);
FRIEND_TEST(UserCollectorTest, GetUserInfoFromName);
FRIEND_TEST(UserCollectorTest, ParseCrashAttributes);
+ FRIEND_TEST(UserCollectorTest, ShouldDumpChromeOverridesDeveloperImage);
+ FRIEND_TEST(UserCollectorTest, ShouldDumpDeveloperImageOverridesConsent);
+ FRIEND_TEST(UserCollectorTest, ShouldDumpUseConsentProductionImage);
// Enumeration to pass to GetIdFromStatus. Must match the order
// that the kernel lists IDs in the status file.
@@ -114,6 +117,11 @@
bool ParseCrashAttributes(const std::string &crash_attributes,
pid_t *pid, int *signal,
std::string *kernel_supplied_name);
+ bool ShouldDump(bool has_owner_consent,
+ bool is_developer,
+ bool is_crash_test_in_progress,
+ const std::string &exec,
+ std::string *reason);
bool generate_diagnostics_;
std::string core_pattern_file_;
diff --git a/crash_reporter/user_collector_test.cc b/crash_reporter/user_collector_test.cc
index 93a76ad..1a5c8c9 100644
--- a/crash_reporter/user_collector_test.cc
+++ b/crash_reporter/user_collector_test.cc
@@ -122,7 +122,37 @@
&pid, &signal, &exec_name));
}
-TEST_F(UserCollectorTest, HandleCrashWithoutMetrics) {
+TEST_F(UserCollectorTest, ShouldDumpDeveloperImageOverridesConsent) {
+ std::string reason;
+ EXPECT_TRUE(collector_.ShouldDump(false, true, false,
+ "chrome-wm", &reason));
+ EXPECT_EQ("developer build - not testing - always dumping", reason);
+
+ // When running a crash test, behave as normal.
+ EXPECT_FALSE(collector_.ShouldDump(false, true, true,
+ "chrome-wm", &reason));
+ EXPECT_EQ("ignoring - no consent", reason);
+}
+
+TEST_F(UserCollectorTest, ShouldDumpChromeOverridesDeveloperImage) {
+ std::string reason;
+ EXPECT_FALSE(collector_.ShouldDump(false, true, false,
+ "chrome", &reason));
+ EXPECT_EQ("ignoring - chrome crash", reason);
+}
+
+TEST_F(UserCollectorTest, ShouldDumpUseConsentProductionImage) {
+ std::string result;
+ EXPECT_FALSE(collector_.ShouldDump(false, false, false,
+ "chrome-wm", &result));
+ EXPECT_EQ("ignoring - no consent", result);
+
+ EXPECT_TRUE(collector_.ShouldDump(true, false, false,
+ "chrome-wm", &result));
+ EXPECT_EQ("handling", result);
+}
+
+TEST_F(UserCollectorTest, HandleCrashWithoutConsent) {
s_metrics = false;
collector_.HandleCrash("20:10:ignored", "foobar");
EXPECT_TRUE(FindLog(
@@ -130,7 +160,7 @@
ASSERT_EQ(s_crashes, 0);
}
-TEST_F(UserCollectorTest, HandleNonChromeCrashWithMetrics) {
+TEST_F(UserCollectorTest, HandleNonChromeCrashWithConsent) {
s_metrics = true;
collector_.HandleCrash("5:2:ignored", "chromeos-wm");
EXPECT_TRUE(FindLog(
@@ -138,7 +168,7 @@
ASSERT_EQ(s_crashes, 1);
}
-TEST_F(UserCollectorTest, HandleChromeCrashWithMetrics) {
+TEST_F(UserCollectorTest, HandleChromeCrashWithConsent) {
s_metrics = true;
collector_.HandleCrash("5:2:ignored", "chrome");
EXPECT_TRUE(FindLog(
@@ -147,7 +177,7 @@
ASSERT_EQ(s_crashes, 0);
}
-TEST_F(UserCollectorTest, HandleSuppliedChromeCrashWithMetrics) {
+TEST_F(UserCollectorTest, HandleSuppliedChromeCrashWithConsent) {
s_metrics = true;
collector_.HandleCrash("0:2:chrome", NULL);
EXPECT_TRUE(FindLog(