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 9610cddf0b..edfb5112df 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 79076b865d..9c468ddf21 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 e72819cdf2..2f38f2c09c 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 edebf87aa2..9919e9557b 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 6d53d81057..d845c446ec 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 a08cfd1ac7..fd602fbcf6 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():