Skip to content

Commit

Permalink
Decouple stability file processing from crash report generation
Browse files Browse the repository at this point in the history 
This CL introduces processing of the stability files even when
postmortem crash report collection is disabled, thus enabling collection
of user metrics.

Bug: 691595
Change-Id: I0c843b61dc1d00a8b072f1f5f41c0237337df0b4
Reviewed-on: https://chromium-review.googlesource.com/619768
Commit-Queue: Pierre-Antoine Manzagol (departed) <manzagop@chromium.org>
Reviewed-by: Sigurður Ásgeirsson <siggi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#496336}
  • Loading branch information
@manzagop
manzagop authored and Commit Bot committed Aug 22, 2017
1 parent 6dd5aaa commit 81b0303
Show file tree
Hide file tree
Showing 4 changed files with 134 additions and 160 deletions.
114 changes: 61 additions & 53 deletions components/browser_watcher/postmortem_report_collector.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,16 +59,9 @@ void LogCollectionStatus(CollectionStatus status) {

} // namespace

void PostmortemDeleter::Process(
const std::vector<base::FilePath>& stability_files) {
for (const FilePath& file : stability_files) {
if (base::DeleteFile(file, false))
LogCollectionStatus(UNCLEAN_SHUTDOWN);
else
LogCollectionStatus(DEBUG_FILE_DELETION_FAILED);
}
}

PostmortemReportCollector::PostmortemReportCollector(
SystemSessionAnalyzer* analyzer)
: report_database_(nullptr), system_session_analyzer_(analyzer) {}
PostmortemReportCollector::PostmortemReportCollector(
const std::string& product_name,
const std::string& version_number,
Expand All @@ -80,6 +73,9 @@ PostmortemReportCollector::PostmortemReportCollector(
channel_name_(channel_name),
report_database_(report_database),
system_session_analyzer_(analyzer) {
DCHECK(!product_name_.empty());
DCHECK(!version_number.empty());
DCHECK(!channel_name.empty());
DCHECK_NE(nullptr, report_database);
}

Expand All @@ -89,22 +85,23 @@ void PostmortemReportCollector::Process(
const std::vector<base::FilePath>& stability_files) {
// Determine the crashpad client id.
crashpad::UUID client_id;
crashpad::Settings* settings = report_database_->GetSettings();
if (settings) {
// If GetSettings() or GetClientID() fails client_id will be left at its
// default value, all zeroes, which is appropriate.
settings->GetClientID(&client_id);
if (report_database_) {
crashpad::Settings* settings = report_database_->GetSettings();
if (settings) {
// If GetSettings() or GetClientID() fails client_id will be left at its
// default value, all zeroes, which is appropriate.
settings->GetClientID(&client_id);
}
}

for (const FilePath& file : stability_files) {
CollectAndSubmitOneReport(client_id, file);
ProcessOneReport(client_id, file);
}
}

void PostmortemReportCollector::CollectAndSubmitOneReport(
void PostmortemReportCollector::ProcessOneReport(
const crashpad::UUID& client_id,
const FilePath& file) {
DCHECK_NE(nullptr, report_database_);
LogCollectionStatus(COLLECTION_ATTEMPT);

// Note: the code below involves two notions of report: chrome internal state
Expand All @@ -114,11 +111,12 @@ void PostmortemReportCollector::CollectAndSubmitOneReport(
StabilityReport report_proto;
CollectionStatus status = CollectOneReport(file, &report_proto);
if (status != SUCCESS) {
// The file was empty, or there was an error collecting the data. Detailed
// logging happens within the Collect function.
if (!base::DeleteFile(file, false))
DLOG(ERROR) << "Failed to delete " << file.value();
// The file was empty, or there was an error collecting the data. This is
// not deemed an unclean shutdown. Detailed logging happens within the
// Collect function.
LogCollectionStatus(status);
if (!base::DeleteFile(file, false))
LogCollectionStatus(DEBUG_FILE_DELETION_FAILED);
return;
}

Expand All @@ -138,37 +136,8 @@ void PostmortemReportCollector::CollectAndSubmitOneReport(
if (report_proto.system_state().session_state() == SystemState::UNCLEAN)
LogCollectionStatus(UNCLEAN_SESSION);

// Prepare a crashpad report.
CrashReportDatabase::NewReport* new_report = nullptr;
CrashReportDatabase::OperationStatus database_status =
report_database_->PrepareNewCrashReport(&new_report);
if (database_status != CrashReportDatabase::kNoError) {
LogCollectionStatus(PREPARE_NEW_CRASH_REPORT_FAILED);
return;
}
CrashReportDatabase::CallErrorWritingCrashReport
call_error_writing_crash_report(report_database_, new_report);

// Write the report to a minidump.
if (!WriteReportToMinidump(&report_proto, client_id, new_report->uuid,
reinterpret_cast<FILE*>(new_report->handle))) {
LogCollectionStatus(WRITE_TO_MINIDUMP_FAILED);
return;
}

// Finalize the report wrt the report database. Note that this doesn't trigger
// an immediate upload, but Crashpad will eventually upload the report (as of
// writing, the delay is on the order of up to 15 minutes).
call_error_writing_crash_report.Disarm();
crashpad::UUID unused_report_id;
database_status = report_database_->FinishedWritingCrashReport(
new_report, &unused_report_id);
if (database_status != CrashReportDatabase::kNoError) {
LogCollectionStatus(FINISHED_WRITING_CRASH_REPORT_FAILED);
return;
}

LogCollectionStatus(SUCCESS);
if (report_database_)
GenerateCrashReport(client_id, &report_proto);
}

CollectionStatus PostmortemReportCollector::CollectOneReport(
Expand Down Expand Up @@ -248,6 +217,45 @@ void PostmortemReportCollector::RecordSystemShutdownState(
SYSTEM_SESSION_ANALYSIS_STATUS_MAX);
}

void PostmortemReportCollector::GenerateCrashReport(
const crashpad::UUID& client_id,
StabilityReport* report_proto) {
DCHECK_NE(nullptr, report_database_);
DCHECK(report_proto);

// Prepare a crashpad report.
CrashReportDatabase::NewReport* new_report = nullptr;
CrashReportDatabase::OperationStatus database_status =
report_database_->PrepareNewCrashReport(&new_report);
if (database_status != CrashReportDatabase::kNoError) {
LogCollectionStatus(PREPARE_NEW_CRASH_REPORT_FAILED);
return;
}
CrashReportDatabase::CallErrorWritingCrashReport
call_error_writing_crash_report(report_database_, new_report);

// Write the report to a minidump.
if (!WriteReportToMinidump(report_proto, client_id, new_report->uuid,
reinterpret_cast<FILE*>(new_report->handle))) {
LogCollectionStatus(WRITE_TO_MINIDUMP_FAILED);
return;
}

// Finalize the report wrt the report database. Note that this doesn't trigger
// an immediate upload, but Crashpad will eventually upload the report (as of
// writing, the delay is on the order of up to 15 minutes).
call_error_writing_crash_report.Disarm();
crashpad::UUID unused_report_id;
database_status = report_database_->FinishedWritingCrashReport(
new_report, &unused_report_id);
if (database_status != CrashReportDatabase::kNoError) {
LogCollectionStatus(FINISHED_WRITING_CRASH_REPORT_FAILED);
return;
}

LogCollectionStatus(SUCCESS);
}

bool PostmortemReportCollector::WriteReportToMinidump(
StabilityReport* report,
const crashpad::UUID& client_id,
Expand Down
40 changes: 19 additions & 21 deletions components/browser_watcher/postmortem_report_collector.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,32 +28,27 @@

namespace browser_watcher {

// Deletes stability files.
class PostmortemDeleter {
public:
PostmortemDeleter() = default;
~PostmortemDeleter() = default;

void Process(const std::vector<base::FilePath>& stability_files);
};

// Handles postmortem report collection by establishing the set of stability
// files to collect, then for each file:
// - extracting a report protocol buffer
// - registering a crash report with the crash database
// - writing a minidump file for the report
// TODO(manzagop): throttling, graceful handling of accumulating data.
// Performs postmortem stability data collection and analysis. The data is then
// reported as user metrics (e.g. to estimate the number of unclean shutdowns,
// or those attributable to the system) and, optionally, as crash reports for
// a more detailed view.
class PostmortemReportCollector {
public:
// Creates a postmortem report collector. The |product_name|, |version_number|
// and |channel_name| are used to set reporter information in postmortem
// crash reports. If |report_database| is set, postmortem crash reports are
// generated and registered against it. If |analyzer| is set, it used to
// analyze the containing system session.
PostmortemReportCollector(SystemSessionAnalyzer* analyzer);
PostmortemReportCollector(const std::string& product_name,
const std::string& version_number,
const std::string& channel_name,
crashpad::CrashReportDatabase* report_database,
SystemSessionAnalyzer* analyzer);
~PostmortemReportCollector();

// Collects postmortem stability reports from |stability_files|. Reports are
// then wrapped in Crashpad reports and registered with the crash database.
// Analyzes |stability_files|, logs postmortem user metrics and optionally
// generates postmortem crash reports.
void Process(const std::vector<base::FilePath>& stability_files);

const std::string& product_name() const { return product_name_; }
Expand Down Expand Up @@ -83,10 +78,10 @@ class PostmortemReportCollector {
PostmortemReportCollectorCollectionFromGlobalTrackerTest,
SystemStateTest);

// Collects a stability file, generates a report and registers it with the
// database.
void CollectAndSubmitOneReport(const crashpad::UUID& client_id,
const base::FilePath& file);
// Processes a stability file, reports user metrics and optionally generates a
// crash report.
void ProcessOneReport(const crashpad::UUID& client_id,
const base::FilePath& file);

virtual CollectionStatus CollectOneReport(
const base::FilePath& stability_file,
Expand All @@ -96,6 +91,9 @@ class PostmortemReportCollector {

void RecordSystemShutdownState(StabilityReport* report) const;

void GenerateCrashReport(const crashpad::UUID& client_id,
StabilityReport* report_proto);

virtual bool WriteReportToMinidump(StabilityReport* report,
const crashpad::UUID& client_id,
const crashpad::UUID& report_id,
Expand Down
99 changes: 32 additions & 67 deletions components/browser_watcher/postmortem_report_collector_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,58 +55,6 @@ using testing::SetArgPointee;

namespace {

TEST(PostmortemDeleterTest, BasicTest) {
base::HistogramTester histogram_tester;
base::ScopedTempDir temp_dir;
ASSERT_TRUE(temp_dir.CreateUniqueTempDir());

// Create three files.
FilePath path_one = temp_dir.GetPath().AppendASCII("a.pma");
FilePath path_two = temp_dir.GetPath().AppendASCII("b.pma");
FilePath path_three = temp_dir.GetPath().AppendASCII("c.pma");

std::vector<FilePath> stability_files = {path_one, path_two, path_three};

for (const FilePath& path : stability_files) {
{
base::ScopedFILE file(base::OpenFile(path, "w"));
ASSERT_NE(file.get(), nullptr);
}
ASSERT_TRUE(base::PathExists(path));
}

// Open one stability file to prevent its deletion.
base::ScopedFILE file(base::OpenFile(path_two, "w"));
ASSERT_NE(file.get(), nullptr);

// Validate deletion and metrics.
PostmortemDeleter deleter;
deleter.Process(stability_files);

ASSERT_FALSE(base::PathExists(path_one));
ASSERT_FALSE(base::PathExists(path_three));
histogram_tester.ExpectBucketCount("ActivityTracker.Collect.Status",
UNCLEAN_SHUTDOWN, 2);

ASSERT_TRUE(base::PathExists(path_two));
histogram_tester.ExpectBucketCount("ActivityTracker.Collect.Status",
DEBUG_FILE_DELETION_FAILED, 1);

std::vector<CollectionStatus> unexpected_statuses = {
NONE,
SUCCESS,
ANALYZER_CREATION_FAILED,
DEBUG_FILE_NO_DATA,
PREPARE_NEW_CRASH_REPORT_FAILED,
WRITE_TO_MINIDUMP_FAILED,
FINISHED_WRITING_CRASH_REPORT_FAILED,
COLLECTION_ATTEMPT};
for (CollectionStatus status : unexpected_statuses) {
histogram_tester.ExpectBucketCount("ActivityTracker.Collect.Status", status,
0);
}
}

const char kProductName[] = "TestProduct";
const char kVersionNumber[] = "TestVersionNumber";
const char kChannelName[] = "TestChannel";
Expand Down Expand Up @@ -198,10 +146,14 @@ MATCHER_P(EqualsProto, message, "") {

} // namespace

class PostmortemReportCollectorProcessTest : public testing::Test {
class PostmortemReportCollectorProcessTest
: public ::testing::TestWithParam<bool> {
public:
void SetUpTest(bool system_session_clean, bool expect_write_dump) {
collector_.reset(new MockPostmortemReportCollector(&database_));
void SetUpTest(bool system_session_clean,
bool expect_write_dump,
bool provide_crash_db) {
collector_.reset(new MockPostmortemReportCollector(
provide_crash_db ? &database_ : nullptr));

// Create a dummy debug file.
ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
Expand All @@ -212,7 +164,8 @@ class PostmortemReportCollectorProcessTest : public testing::Test {
}
ASSERT_TRUE(base::PathExists(debug_file_));

EXPECT_CALL(database_, GetSettings()).Times(1).WillOnce(Return(nullptr));
if (provide_crash_db)
EXPECT_CALL(database_, GetSettings()).Times(1).WillOnce(Return(nullptr));

// Expect a single collection call.
StabilityReport report;
Expand Down Expand Up @@ -251,12 +204,14 @@ class PostmortemReportCollectorProcessTest : public testing::Test {
histogram_tester_.ExpectBucketCount("ActivityTracker.Collect.Status",
UNCLEAN_SESSION, unclean_system_cnt);
}
void CollectReports(bool is_session_clean) {
SetUpTest(is_session_clean, true);
void CollectReports(bool is_session_clean, bool provide_crash_db) {
SetUpTest(is_session_clean, provide_crash_db, provide_crash_db);

EXPECT_CALL(database_, FinishedWritingCrashReport(&crashpad_report_, _))
.Times(1)
.WillOnce(Return(CrashReportDatabase::kNoError));
if (provide_crash_db) {
EXPECT_CALL(database_, FinishedWritingCrashReport(&crashpad_report_, _))
.Times(1)
.WillOnce(Return(CrashReportDatabase::kNoError));
}

// Run the test.
std::vector<FilePath> debug_files{debug_file_};
Expand All @@ -273,24 +228,27 @@ class PostmortemReportCollectorProcessTest : public testing::Test {
CrashReportDatabase::NewReport crashpad_report_;
};

TEST_F(PostmortemReportCollectorProcessTest, ProcessCleanSession) {
CollectReports(true);
TEST_P(PostmortemReportCollectorProcessTest, ProcessCleanSession) {
bool provide_crash_db = GetParam();
CollectReports(true, provide_crash_db);
int expected_unclean = 1;
int expected_system_unclean = 0;
ValidateHistograms(expected_unclean, expected_system_unclean);
}

TEST_F(PostmortemReportCollectorProcessTest, ProcessUncleanSession) {
CollectReports(false);
TEST_P(PostmortemReportCollectorProcessTest, ProcessUncleanSession) {
bool provide_crash_db = GetParam();
CollectReports(false, provide_crash_db);
int expected_unclean = 1;
int expected_system_unclean = 1;
ValidateHistograms(expected_unclean, expected_system_unclean);
}

TEST_F(PostmortemReportCollectorProcessTest, ProcessStuckFile) {
TEST_P(PostmortemReportCollectorProcessTest, ProcessStuckFile) {
bool provide_crash_db = GetParam();
bool system_session_clean = true;
bool expect_write_dump = false;
SetUpTest(system_session_clean, expect_write_dump);
SetUpTest(system_session_clean, expect_write_dump, provide_crash_db);

// Open the stability debug file to prevent its deletion.
base::ScopedFILE file(base::OpenFile(debug_file_, "w"));
Expand All @@ -308,6 +266,13 @@ TEST_F(PostmortemReportCollectorProcessTest, ProcessStuckFile) {
ValidateHistograms(expected_unclean, expected_system_unclean);
}

INSTANTIATE_TEST_CASE_P(WithCrashDatabase,
PostmortemReportCollectorProcessTest,
::testing::Values(true));
INSTANTIATE_TEST_CASE_P(WithoutCrashDatabase,
PostmortemReportCollectorProcessTest,
::testing::Values(true));

TEST(PostmortemReportCollectorTest, CollectEmptyFile) {
// Create an empty file.
base::ScopedTempDir temp_dir;
Expand Down
Loading

0 comments on commit 81b0303

Please sign in to comment.