From 785cb10e80924f936eb32497248745a4726a540e Mon Sep 17 00:00:00 2001 From: Justin Cohen Date: Thu, 10 Mar 2022 14:12:57 -0500 Subject: [PATCH] ios: Move IOSSystemDataCollector to InProcessHandler. The IOSSystemDataCollector was previously owned by the iOS CrashHandler and passed in to the iOS InProcessHandler in each method. Move ownership to iOS InProcessHandler to simplify. Change-Id: Ifa41304cb1e3e3825a211e6cce5aa730d0edcc95 Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/3517965 Reviewed-by: Joshua Peraza Commit-Queue: Justin Cohen --- client/crashpad_client_ios.cc | 25 ++++++------- client/ios_handler/in_process_handler.cc | 33 +++++++---------- client/ios_handler/in_process_handler.h | 35 +++++-------------- client/ios_handler/in_process_handler_test.cc | 3 +- 4 files changed, 32 insertions(+), 64 deletions(-) diff --git a/client/crashpad_client_ios.cc b/client/crashpad_client_ios.cc index 7a4f2a2d..e98a2320 100644 --- a/client/crashpad_client_ios.cc +++ b/client/crashpad_client_ios.cc @@ -24,7 +24,6 @@ #include "base/mac/scoped_mach_port.h" #include "client/ios_handler/exception_processor.h" #include "client/ios_handler/in_process_handler.h" -#include "util/ios/ios_system_data_collector.h" #include "util/mach/exc_server_variants.h" #include "util/mach/exception_ports.h" #include "util/mach/mach_extensions.h" @@ -74,8 +73,7 @@ class CrashHandler : public Thread, const std::string& url, const std::map& annotations) { INITIALIZATION_STATE_SET_INITIALIZING(initialized_); - if (!in_process_handler_.Initialize( - database, url, annotations, system_data_) || + if (!in_process_handler_.Initialize(database, url, annotations) || !InstallMachExceptionHandler() || // xnu turns hardware faults into Mach exceptions, so the only signal // left to register is SIGABRT, which never starts off as a hardware @@ -128,8 +126,8 @@ class CrashHandler : public Thread, void DumpWithoutCrash(NativeCPUContext* context, bool process_dump) { INITIALIZATION_STATE_DCHECK_VALID(initialized_); base::FilePath path; - if (!in_process_handler_.DumpExceptionFromSimulatedMachException( - system_data_, context, &path)) { + if (!in_process_handler_.DumpExceptionFromSimulatedMachException(context, + &path)) { return; } @@ -140,8 +138,8 @@ class CrashHandler : public Thread, void DumpWithoutCrashAtPath(NativeCPUContext* context, const base::FilePath& path) { - in_process_handler_.DumpExceptionFromSimulatedMachExceptionAtPath( - system_data_, context, path); + in_process_handler_.DumpExceptionFromSimulatedMachExceptionAtPath(context, + path); } void StartProcessingPendingReports() { @@ -278,8 +276,7 @@ class CrashHandler : public Thread, thread_state_flavor_t flavor, ConstThreadState old_state, mach_msg_type_number_t old_state_count) { - in_process_handler_.DumpExceptionFromMachException(system_data_, - behavior, + in_process_handler_.DumpExceptionFromMachException(behavior, thread, exception, code, @@ -291,8 +288,8 @@ class CrashHandler : public Thread, void HandleUncaughtNSException(const uint64_t* frames, const size_t num_frames) override { - in_process_handler_.DumpExceptionFromNSExceptionWithFrames( - system_data_, frames, num_frames); + in_process_handler_.DumpExceptionFromNSExceptionWithFrames(frames, + num_frames); // After uncaught exceptions are reported, the system immediately triggers a // call to std::terminate()/abort(). Remove the abort handler so a second // dump isn't generated. @@ -301,8 +298,7 @@ class CrashHandler : public Thread, void HandleUncaughtNSExceptionWithContext( NativeCPUContext* context) override { - in_process_handler_.DumpExceptionFromNSExceptionWithContext(system_data_, - context); + in_process_handler_.DumpExceptionFromNSExceptionWithContext(context); // After uncaught exceptions are reported, the system immediately triggers a // call to std::terminate()/abort(). Remove the abort handler so a second @@ -331,7 +327,7 @@ class CrashHandler : public Thread, siginfo_t* siginfo, ucontext_t* context, struct sigaction* old_action) { - in_process_handler_.DumpExceptionFromSignal(system_data_, siginfo, context); + in_process_handler_.DumpExceptionFromSignal(siginfo, context); // Always call system handler. Signals::RestoreHandlerAndReraiseSignalOnReturn(siginfo, old_action); @@ -341,7 +337,6 @@ class CrashHandler : public Thread, ExceptionPorts::ExceptionHandlerVector original_handlers_; struct sigaction old_action_ = {}; internal::InProcessHandler in_process_handler_; - internal::IOSSystemDataCollector system_data_; static CrashHandler* instance_; bool mach_handler_running_ = false; InitializationStateDcheck initialized_; diff --git a/client/ios_handler/in_process_handler.cc b/client/ios_handler/in_process_handler.cc index 2ed31131..ffcbcebc 100644 --- a/client/ios_handler/in_process_handler.cc +++ b/client/ios_handler/in_process_handler.cc @@ -71,8 +71,7 @@ InProcessHandler::~InProcessHandler() { bool InProcessHandler::Initialize( const base::FilePath& database, const std::string& url, - const std::map& annotations, - const IOSSystemDataCollector& system_data) { + const std::map& annotations) { INITIALIZATION_STATE_SET_INITIALIZING(initialized_); annotations_ = annotations; database_ = CrashReportDatabase::Initialize(database); @@ -80,7 +79,7 @@ bool InProcessHandler::Initialize( return false; } bundle_identifier_and_seperator_ = - system_data.BundleIdentifier() + kBundleSeperator; + system_data_.BundleIdentifier() + kBundleSeperator; if (!url.empty()) { // TODO(scottmg): options.rate_limit should be removed when we have a @@ -109,7 +108,7 @@ bool InProcessHandler::Initialize( PruneCondition::GetDefault(), base_dir_, bundle_identifier_and_seperator_, - system_data.IsExtension())); + system_data_.IsExtension())); prune_thread_->Start(); base::FilePath cached_writer_path = NewLockedFilePath(); @@ -126,10 +125,8 @@ bool InProcessHandler::Initialize( return true; } -void InProcessHandler::DumpExceptionFromSignal( - const IOSSystemDataCollector& system_data, - siginfo_t* siginfo, - ucontext_t* context) { +void InProcessHandler::DumpExceptionFromSignal(siginfo_t* siginfo, + ucontext_t* context) { INITIALIZATION_STATE_DCHECK_VALID(initialized_); ScopedLockedWriter writer(GetCachedWriter(), cached_writer_path_.c_str(), @@ -138,13 +135,12 @@ void InProcessHandler::DumpExceptionFromSignal( CRASHPAD_RAW_LOG("Cannot DumpExceptionFromSignal without writer"); return; } - ScopedReport report(writer.GetWriter(), system_data, annotations_); + ScopedReport report(writer.GetWriter(), system_data_, annotations_); InProcessIntermediateDumpHandler::WriteExceptionFromSignal( - writer.GetWriter(), system_data, siginfo, context); + writer.GetWriter(), system_data_, siginfo, context); } void InProcessHandler::DumpExceptionFromMachException( - const IOSSystemDataCollector& system_data, exception_behavior_t behavior, thread_t thread, exception_type_t exception, @@ -166,7 +162,7 @@ void InProcessHandler::DumpExceptionFromMachException( mach_exception_callback_for_testing_(); } - ScopedReport report(writer.GetWriter(), system_data, annotations_); + ScopedReport report(writer.GetWriter(), system_data_, annotations_); InProcessIntermediateDumpHandler::WriteExceptionFromMachException( writer.GetWriter(), behavior, @@ -180,7 +176,6 @@ void InProcessHandler::DumpExceptionFromMachException( } void InProcessHandler::DumpExceptionFromNSExceptionWithContext( - const IOSSystemDataCollector& system_data, NativeCPUContext* context) { INITIALIZATION_STATE_DCHECK_VALID(initialized_); // This does not use the cached writer. NSExceptionWithContext comes from @@ -198,7 +193,7 @@ void InProcessHandler::DumpExceptionFromNSExceptionWithContext( return; } - ScopedReport report(writer.GetWriter(), system_data, annotations_); + ScopedReport report(writer.GetWriter(), system_data_, annotations_); InProcessIntermediateDumpHandler::WriteExceptionFromMachException( writer.GetWriter(), MACH_EXCEPTION_CODES, @@ -212,7 +207,6 @@ void InProcessHandler::DumpExceptionFromNSExceptionWithContext( } void InProcessHandler::DumpExceptionFromNSExceptionWithFrames( - const IOSSystemDataCollector& system_data, const uint64_t* frames, const size_t num_frames) { INITIALIZATION_STATE_DCHECK_VALID(initialized_); @@ -225,23 +219,20 @@ void InProcessHandler::DumpExceptionFromNSExceptionWithFrames( return; } ScopedReport report( - writer.GetWriter(), system_data, annotations_, frames, num_frames); + writer.GetWriter(), system_data_, annotations_, frames, num_frames); InProcessIntermediateDumpHandler::WriteExceptionFromNSException( writer.GetWriter()); } bool InProcessHandler::DumpExceptionFromSimulatedMachException( - const IOSSystemDataCollector& system_data, const NativeCPUContext* context, base::FilePath* path) { base::FilePath locked_path = NewLockedFilePath(); *path = locked_path.RemoveFinalExtension(); - return DumpExceptionFromSimulatedMachExceptionAtPath( - system_data, context, locked_path); + return DumpExceptionFromSimulatedMachExceptionAtPath(context, locked_path); } bool InProcessHandler::DumpExceptionFromSimulatedMachExceptionAtPath( - const IOSSystemDataCollector& system_data, const NativeCPUContext* context, const base::FilePath& path) { // This does not use the cached writer. It's expected that simulated @@ -259,7 +250,7 @@ bool InProcessHandler::DumpExceptionFromSimulatedMachExceptionAtPath( "Cannot DumpExceptionFromSimulatedMachExceptionAtPath without writer"); return false; } - ScopedReport report(writer.GetWriter(), system_data, annotations_); + ScopedReport report(writer.GetWriter(), system_data_, annotations_); InProcessIntermediateDumpHandler::WriteExceptionFromMachException( writer.GetWriter(), MACH_EXCEPTION_CODES, diff --git a/client/ios_handler/in_process_handler.h b/client/ios_handler/in_process_handler.h index fe638452..096e477c 100644 --- a/client/ios_handler/in_process_handler.h +++ b/client/ios_handler/in_process_handler.h @@ -49,34 +49,28 @@ class InProcessHandler { //! \param[in] database The path to a Crashpad database. //! \param[in] url The URL of an upload server. //! \param[in] annotations Process annotations to set in each crash report. - //! \param[in] system_data An object containing various system data points. //! \return `true` if a handler to a pending intermediate dump could be //! opened. bool Initialize(const base::FilePath& database, const std::string& url, - const std::map& annotations, - const IOSSystemDataCollector& system_data); + const std::map& annotations); //! \brief Generate an intermediate dump from a signal handler exception. //! Writes the dump with the cached writer does not allow concurrent //! exceptions to be written. It is expected the system will terminate //! the application after this call. //! - //! \param[in] system_data An object containing various system data points. //! \param[in] siginfo A pointer to a `siginfo_t` object received by a signal //! handler. //! \param[in] context A pointer to a `ucontext_t` object received by a //! signal. - void DumpExceptionFromSignal(const IOSSystemDataCollector& system_data, - siginfo_t* siginfo, - ucontext_t* context); + void DumpExceptionFromSignal(siginfo_t* siginfo, ucontext_t* context); //! \brief Generate an intermediate dump from a mach exception. Writes the //! dump with the cached writer does not allow concurrent exceptions to be //! written. It is expected the system will terminate the application //! after this call. //! - //! \param[in] system_data An object containing various system data points. //! \param[in] behavior //! \param[in] thread //! \param[in] exception @@ -85,8 +79,7 @@ class InProcessHandler { //! \param[in,out] flavor //! \param[in] old_state //! \param[in] old_state_count - void DumpExceptionFromMachException(const IOSSystemDataCollector& system_data, - exception_behavior_t behavior, + void DumpExceptionFromMachException(exception_behavior_t behavior, thread_t thread, exception_type_t exception, const mach_exception_data_type_t* code, @@ -100,11 +93,8 @@ class InProcessHandler { //! exceptions is imperfect, uses a new writer for the intermediate dump, //! as it is possible for further exceptions to happen. //! - //! \param[in] system_data An object containing various system data points. //! \param[in] context - void DumpExceptionFromNSExceptionWithContext( - const IOSSystemDataCollector& system_data, - NativeCPUContext* context); + void DumpExceptionFromNSExceptionWithContext(NativeCPUContext* context); //! \brief Generate an intermediate dump from an uncaught NSException. //! @@ -116,38 +106,30 @@ class InProcessHandler { //! the system will terminate the application after this call. //! - //! \param[in] system_data An object containing various system data points. //! \param[in] frames An array of call stack frame addresses. //! \param[in] num_frames The number of frames in |frames|. - void DumpExceptionFromNSExceptionWithFrames( - const IOSSystemDataCollector& system_data, - const uint64_t* frames, - const size_t num_frames); + void DumpExceptionFromNSExceptionWithFrames(const uint64_t* frames, + const size_t num_frames); //! \brief Generate a simulated intermediate dump similar to a Mach exception //! in the same base directory as other exceptions. Does not use the //! cached writer. //! - //! \param[in] system_data An object containing various system data points. //! \param[in] context A pointer to a NativeCPUContext object for this //! simulated exception. //! \param[out] path The path of the intermediate dump generated. //! \return `true` if the pending intermediate dump could be written. - bool DumpExceptionFromSimulatedMachException( - const IOSSystemDataCollector& system_data, - const NativeCPUContext* context, - base::FilePath* path); + bool DumpExceptionFromSimulatedMachException(const NativeCPUContext* context, + base::FilePath* path); //! \brief Generate a simulated intermediate dump similar to a Mach exception //! at a specific path. Does not use the cached writer. //! - //! \param[in] system_data An object containing various system data points. //! \param[in] context A pointer to a NativeCPUContext object for this //! simulated exception. //! \param[in] path Path to where the intermediate dump should be written. //! \return `true` if the pending intermediate dump could be written. bool DumpExceptionFromSimulatedMachExceptionAtPath( - const IOSSystemDataCollector& system_data, const NativeCPUContext* context, const base::FilePath& path); @@ -259,6 +241,7 @@ class InProcessHandler { std::unique_ptr prune_thread_; std::unique_ptr database_; std::string bundle_identifier_and_seperator_; + IOSSystemDataCollector system_data_; InitializationStateDcheck initialized_; }; diff --git a/client/ios_handler/in_process_handler_test.cc b/client/ios_handler/in_process_handler_test.cc index d7dfc053..110d86c7 100644 --- a/client/ios_handler/in_process_handler_test.cc +++ b/client/ios_handler/in_process_handler_test.cc @@ -36,8 +36,7 @@ class InProcessHandlerTest : public testing::Test { // testing::Test: void SetUp() override { - ASSERT_TRUE( - in_process_handler_.Initialize(temp_dir_.path(), "", {}, system_data_)); + ASSERT_TRUE(in_process_handler_.Initialize(temp_dir_.path(), "", {})); pending_dir_ = temp_dir_.path().Append("pending-serialized-ios-dump"); bundle_identifier_and_seperator_ = system_data_.BundleIdentifier() + "@"; }