From c80bf96001ddaa69ffa910e61537a967eb1c3116 Mon Sep 17 00:00:00 2001 From: Joshua Peraza Date: Thu, 12 Apr 2018 19:37:06 -0700 Subject: [PATCH] Don't record reports as complete if there is no upload thread This allows clients to use the database to handle uploads themselves, e.g. on Android, where Crashpad does not yet provide an uploader. The handler does not launch an upload thread when no url is supplied. Previously, the handler would move these reports to completed and record the upload as skipped with kUploadsDisabled. With this change, these reports would remain pending until pruned, with no metrics recorded for them in regard to their upload. Bug: crashpad:30 Change-Id: I4167ab1531634b10e91d03229018ae6aab4103aa Reviewed-on: https://chromium-review.googlesource.com/1010970 Reviewed-by: Robert Sesek Commit-Queue: Joshua Peraza --- client/crashpad_client_linux_test.cc | 14 ++++++++------ handler/crashpad_handler_test.cc | 2 +- handler/linux/crash_report_exception_handler.cc | 3 --- handler/mac/crash_report_exception_handler.cc | 3 --- handler/win/crash_report_exception_handler.cc | 3 --- snapshot/win/end_to_end_test.py | 2 +- 6 files changed, 10 insertions(+), 17 deletions(-) diff --git a/client/crashpad_client_linux_test.cc b/client/crashpad_client_linux_test.cc index 9610cddf..edfb5112 100644 --- a/client/crashpad_client_linux_test.cc +++ b/client/crashpad_client_linux_test.cc @@ -85,12 +85,12 @@ TEST(CrashpadClient, SimulateCrash) { std::vector reports; ASSERT_EQ(database->GetPendingReports(&reports), CrashReportDatabase::kNoError); - EXPECT_EQ(reports.size(), 0u); + EXPECT_EQ(reports.size(), 1u); reports.clear(); ASSERT_EQ(database->GetCompletedReports(&reports), CrashReportDatabase::kNoError); - EXPECT_EQ(reports.size(), 1u); + EXPECT_EQ(reports.size(), 0u); } } @@ -147,11 +147,12 @@ class StartHandlerAtCrashTest : public MultiprocessExec { std::vector reports; ASSERT_EQ(database->GetPendingReports(&reports), CrashReportDatabase::kNoError); - EXPECT_EQ(reports.size(), 0u); + EXPECT_EQ(reports.size(), 1u); + reports.clear(); ASSERT_EQ(database->GetCompletedReports(&reports), CrashReportDatabase::kNoError); - EXPECT_EQ(reports.size(), 1u); + EXPECT_EQ(reports.size(), 0u); } DISALLOW_COPY_AND_ASSIGN(StartHandlerAtCrashTest); @@ -213,11 +214,12 @@ class StartHandlerForClientTest { std::vector reports; ASSERT_EQ(database->GetPendingReports(&reports), CrashReportDatabase::kNoError); - EXPECT_EQ(reports.size(), 0u); + EXPECT_EQ(reports.size(), 1u); + reports.clear(); ASSERT_EQ(database->GetCompletedReports(&reports), CrashReportDatabase::kNoError); - EXPECT_EQ(reports.size(), 1u); + EXPECT_EQ(reports.size(), 0u); } bool InstallHandler() { diff --git a/handler/crashpad_handler_test.cc b/handler/crashpad_handler_test.cc index 79076b86..9c468ddf 100644 --- a/handler/crashpad_handler_test.cc +++ b/handler/crashpad_handler_test.cc @@ -93,7 +93,7 @@ void CrashWithExtendedHandler::ValidateGeneratedDump() { ASSERT_TRUE(database); std::vector reports; - ASSERT_EQ(database->GetCompletedReports(&reports), + ASSERT_EQ(database->GetPendingReports(&reports), CrashReportDatabase::kNoError); ASSERT_EQ(reports.size(), 1u); diff --git a/handler/linux/crash_report_exception_handler.cc b/handler/linux/crash_report_exception_handler.cc index e72819cd..2f38f2c0 100644 --- a/handler/linux/crash_report_exception_handler.cc +++ b/handler/linux/crash_report_exception_handler.cc @@ -140,9 +140,6 @@ bool CrashReportExceptionHandler::HandleExceptionWithConnection( if (upload_thread_) { upload_thread_->ReportPending(uuid); - } else { - database_->SkipReportUpload( - uuid, Metrics::CrashSkippedReason::kUploadsDisabled); } } diff --git a/handler/mac/crash_report_exception_handler.cc b/handler/mac/crash_report_exception_handler.cc index edebf87a..9919e955 100644 --- a/handler/mac/crash_report_exception_handler.cc +++ b/handler/mac/crash_report_exception_handler.cc @@ -189,9 +189,6 @@ kern_return_t CrashReportExceptionHandler::CatchMachException( if (upload_thread_) { upload_thread_->ReportPending(uuid); - } else { - database_->SkipReportUpload( - uuid, Metrics::CrashSkippedReason::kUploadsDisabled); } } diff --git a/handler/win/crash_report_exception_handler.cc b/handler/win/crash_report_exception_handler.cc index 6d53d810..d845c446 100644 --- a/handler/win/crash_report_exception_handler.cc +++ b/handler/win/crash_report_exception_handler.cc @@ -126,9 +126,6 @@ unsigned int CrashReportExceptionHandler::ExceptionHandlerServerException( if (upload_thread_) { upload_thread_->ReportPending(uuid); - } else { - database_->SkipReportUpload( - uuid, Metrics::CrashSkippedReason::kUploadsDisabled); } } diff --git a/snapshot/win/end_to_end_test.py b/snapshot/win/end_to_end_test.py index a08cfd1a..fd602fbc 100755 --- a/snapshot/win/end_to_end_test.py +++ b/snapshot/win/end_to_end_test.py @@ -155,7 +155,7 @@ def GetDumpFromProgram( out = subprocess.check_output([ os.path.join(out_dir, 'crashpad_database_util.exe'), '--database=' + test_database, - '--show-completed-reports', + '--show-pending-reports', '--show-all-report-info', ]) for line in out.splitlines():