From 1fc9cc0d0e1dfafb8d29dba8d01f09587d870026 Mon Sep 17 00:00:00 2001 From: Olivier Robin Date: Thu, 18 Apr 2019 18:06:31 +0200 Subject: [PATCH] [Breakpad iOS] Add a callback on report upload completion. This CL adds a result callback on report upload completion. On failure, Breakpad deletes the configuration file and does retry to upload a report. Using this callback, the client will be able to log some metrics and to act on upload failure. Bug: 954175 Change-Id: I95a3264b65d4c06ba5d8dde8377440d23f1e2081 Reviewed-on: https://chromium-review.googlesource.com/c/breakpad/breakpad/+/1572661 Reviewed-by: Mark Mentovai --- src/client/ios/Breakpad.h | 17 +++++++++--- src/client/ios/Breakpad.mm | 39 ++++++++++++++++++---------- src/client/ios/BreakpadController.h | 7 +++++ src/client/ios/BreakpadController.mm | 23 +++++++++++----- src/client/mac/sender/uploader.h | 17 ++++++++++++ src/client/mac/sender/uploader.mm | 8 ++++++ 6 files changed, 88 insertions(+), 23 deletions(-) diff --git a/src/client/ios/Breakpad.h b/src/client/ios/Breakpad.h index 30b13449..fa790f77 100644 --- a/src/client/ios/Breakpad.h +++ b/src/client/ios/Breakpad.h @@ -62,6 +62,14 @@ typedef bool (*BreakpadFilterCallback)(int exception_type, mach_port_t crashing_thread, void *context); +// Optional user-defined function that will be called after a network upload +// of a crash report. +// |report_id| will be the id returned by the server, or "ERR" if an error +// occurred. +// |error| will contain the error, or nil if no error occured. +typedef void (*BreakpadUploadCompletionCallback)(NSString *report_id, + NSError *error); + // Create a new BreakpadRef object and install it as an exception // handler. The |parameters| will typically be the contents of your // bundle's Info.plist. @@ -210,8 +218,10 @@ void BreakpadUploadNextReport(BreakpadRef ref); // Upload next report to the server. // |server_parameters| is additional server parameters to send. -void BreakpadUploadNextReportWithParameters(BreakpadRef ref, - NSDictionary *server_parameters); +void BreakpadUploadNextReportWithParameters( + BreakpadRef ref, + NSDictionary *server_parameters, + BreakpadUploadCompletionCallback callback); // Upload a report to the server. // |server_parameters| is additional server parameters to send. @@ -219,7 +229,8 @@ void BreakpadUploadNextReportWithParameters(BreakpadRef ref, void BreakpadUploadReportWithParametersAndConfiguration( BreakpadRef ref, NSDictionary *server_parameters, - NSDictionary *configuration); + NSDictionary *configuration, + BreakpadUploadCompletionCallback callback); // Handles the network response of a breakpad upload. This function is needed if // the actual upload is done by the Breakpad client. diff --git a/src/client/ios/Breakpad.mm b/src/client/ios/Breakpad.mm index c99ee4fd..2d8ba61e 100644 --- a/src/client/ios/Breakpad.mm +++ b/src/client/ios/Breakpad.mm @@ -164,7 +164,8 @@ class Breakpad { NSDate *DateOfMostRecentCrashReport(); void UploadNextReport(NSDictionary *server_parameters); void UploadReportWithConfiguration(NSDictionary *configuration, - NSDictionary *server_parameters); + NSDictionary *server_parameters, + BreakpadUploadCompletionCallback callback); void UploadData(NSData *data, NSString *name, NSDictionary *server_parameters); void HandleNetworkResponse(NSDictionary *configuration, @@ -502,8 +503,10 @@ void Breakpad::HandleNetworkResponse(NSDictionary *configuration, } //============================================================================= -void Breakpad::UploadReportWithConfiguration(NSDictionary *configuration, - NSDictionary *server_parameters) { +void Breakpad::UploadReportWithConfiguration( + NSDictionary *configuration, + NSDictionary *server_parameters, + BreakpadUploadCompletionCallback callback) { Uploader *uploader = [[[Uploader alloc] initWithConfig:configuration] autorelease]; if (!uploader) @@ -512,6 +515,13 @@ void Breakpad::UploadReportWithConfiguration(NSDictionary *configuration, [uploader addServerParameter:[server_parameters objectForKey:key] forKey:key]; } + if (callback) { + [uploader setUploadCompletionBlock:^(NSString *report_id, NSError *error) { + dispatch_async(dispatch_get_main_queue(), ^{ + callback(report_id, error); + }); + }]; + } [uploader report]; } @@ -519,7 +529,8 @@ void Breakpad::UploadReportWithConfiguration(NSDictionary *configuration, void Breakpad::UploadNextReport(NSDictionary *server_parameters) { NSDictionary *configuration = NextCrashReportConfiguration(); if (configuration) { - return UploadReportWithConfiguration(configuration, server_parameters); + return UploadReportWithConfiguration(configuration, server_parameters, + nullptr); } } @@ -850,7 +861,7 @@ int BreakpadGetCrashReportCount(BreakpadRef ref) { //============================================================================= void BreakpadUploadNextReport(BreakpadRef ref) { - BreakpadUploadNextReportWithParameters(ref, nil); + BreakpadUploadNextReportWithParameters(ref, nil, nullptr); } //============================================================================= @@ -882,22 +893,25 @@ NSDate *BreakpadGetDateOfMostRecentCrashReport(BreakpadRef ref) { void BreakpadUploadReportWithParametersAndConfiguration( BreakpadRef ref, NSDictionary *server_parameters, - NSDictionary *configuration) { + NSDictionary *configuration, + BreakpadUploadCompletionCallback callback) { try { Breakpad *breakpad = (Breakpad *)ref; if (!breakpad || !configuration) return; - breakpad->UploadReportWithConfiguration(configuration, server_parameters); + breakpad->UploadReportWithConfiguration(configuration, server_parameters, + callback); } catch(...) { // don't let exceptions leave this C API fprintf(stderr, "BreakpadUploadReportWithParametersAndConfiguration() : error\n"); } - } //============================================================================= -void BreakpadUploadNextReportWithParameters(BreakpadRef ref, - NSDictionary *server_parameters) { +void BreakpadUploadNextReportWithParameters( + BreakpadRef ref, + NSDictionary *server_parameters, + BreakpadUploadCompletionCallback callback) { try { Breakpad *breakpad = (Breakpad *)ref; if (!breakpad) @@ -905,9 +919,8 @@ void BreakpadUploadNextReportWithParameters(BreakpadRef ref, NSDictionary *configuration = breakpad->NextCrashReportConfiguration(); if (!configuration) return; - return BreakpadUploadReportWithParametersAndConfiguration(ref, - server_parameters, - configuration); + return BreakpadUploadReportWithParametersAndConfiguration( + ref, server_parameters, configuration, callback); } catch(...) { // don't let exceptions leave this C API fprintf(stderr, "BreakpadUploadNextReportWithParameters() : error\n"); } diff --git a/src/client/ios/BreakpadController.h b/src/client/ios/BreakpadController.h index 20868473..6c70c202 100644 --- a/src/client/ios/BreakpadController.h +++ b/src/client/ios/BreakpadController.h @@ -66,6 +66,9 @@ // The dictionary that contains additional server parameters to send when // uploading crash reports. NSDictionary* uploadTimeParameters_; + + // The callback to call on report upload completion. + BreakpadUploadCompletionCallback uploadCompleteCallback_; } // Singleton. @@ -95,6 +98,10 @@ // crash report is generated. See |BreakpadAddUploadParameter|. - (void)addUploadParameter:(NSString*)value forKey:(NSString*)key; +// Sets the callback to be called after uploading a crash report to the server. +// Only the latest callback registered will be called. +- (void)setUploadCallback:(BreakpadUploadCompletionCallback)callback; + // Remove a previously-added parameter from the upload parameter set. See // |BreakpadRemoveUploadParameter|. - (void)removeUploadParameterForKey:(NSString*)key; diff --git a/src/client/ios/BreakpadController.mm b/src/client/ios/BreakpadController.mm index 8d499c27..01fb5f13 100644 --- a/src/client/ios/BreakpadController.mm +++ b/src/client/ios/BreakpadController.mm @@ -166,9 +166,9 @@ NSString* GetPlatform() { NSAssert(started_, @"The controller must be started before " "threadUnsafeSendReportWithConfiguration is called"); if (breakpadRef_) { - BreakpadUploadReportWithParametersAndConfiguration(breakpadRef_, - uploadTimeParameters_, - configuration); + BreakpadUploadReportWithParametersAndConfiguration( + breakpadRef_, uploadTimeParameters_, configuration, + uploadCompleteCallback_); } } @@ -195,7 +195,7 @@ NSString* GetPlatform() { NSAssert(!started_, @"The controller must not be started when updateConfiguration is called"); [configuration_ addEntriesFromDictionary:configuration]; - NSString* uploadInterval = + NSString *uploadInterval = [configuration_ valueForKey:@BREAKPAD_REPORT_INTERVAL]; if (uploadInterval) [self setUploadInterval:[uploadInterval intValue]]; @@ -206,7 +206,7 @@ NSString* GetPlatform() { @"The controller must not be started when resetConfiguration is called"); [configuration_ autorelease]; configuration_ = [[[NSBundle mainBundle] infoDictionary] mutableCopy]; - NSString* uploadInterval = + NSString *uploadInterval = [configuration_ valueForKey:@BREAKPAD_REPORT_INTERVAL]; [self setUploadInterval:[uploadInterval intValue]]; [self setParametersToAddAtUploadTime:nil]; @@ -243,6 +243,15 @@ NSString* GetPlatform() { }); } +- (void)setUploadCallback:(BreakpadUploadCompletionCallback)callback { + NSAssert(started_, + @"The controller must not be started before setUploadCallback is " + "called"); + dispatch_async(queue_, ^{ + uploadCompleteCallback_ = callback; + }); +} + - (void)removeUploadParameterForKey:(NSString*)key { NSAssert(started_, @"The controller must be started before " "removeUploadParameterForKey is called"); @@ -345,8 +354,8 @@ NSString* GetPlatform() { // A report can be sent now. if (timeToWait == 0) { [self reportWillBeSent]; - BreakpadUploadNextReportWithParameters(breakpadRef_, - uploadTimeParameters_); + BreakpadUploadNextReportWithParameters(breakpadRef_, uploadTimeParameters_, + uploadCompleteCallback_); // If more reports must be sent, make sure this method is called again. if (BreakpadGetCrashReportCount(breakpadRef_) > 0) diff --git a/src/client/mac/sender/uploader.h b/src/client/mac/sender/uploader.h index 5f6aa464..0897dade 100644 --- a/src/client/mac/sender/uploader.h +++ b/src/client/mac/sender/uploader.h @@ -42,6 +42,13 @@ extern NSString *const kGoogleServerType; extern NSString *const kSocorroServerType; extern NSString *const kDefaultServerType; +// Optional user-defined function that will be called after a network upload +// of a crash report. +// |report_id| will be the id returned by the server, or "ERR" if an error +// occurred. +// |error| will contain the error, or nil if no error occured. +typedef void (^UploadCompletionBlock)(NSString *reportId, NSError *error); + @interface Uploader : NSObject { @private NSMutableDictionary *parameters_; // Key value pairs of data (STRONG) @@ -61,6 +68,12 @@ extern NSString *const kDefaultServerType; // that are uploaded to the // crash server with the // minidump. + UploadCompletionBlock uploadCompletion_; // A block called on network upload + // completion. Parameters are: + // The report ID returned by the + // server, + // the NSError triggered during + // upload. } - (id)initWithConfigFile:(const char *)configFile; @@ -86,4 +99,8 @@ extern NSString *const kDefaultServerType; // new ID. - (void)handleNetworkResponse:(NSData *)data withError:(NSError *)error; +// Sets the callback to be called after uploading a crash report to the server. +// Only the latest callback registered will be called. +- (void)setUploadCompletionBlock:(UploadCompletionBlock)uploadCompletion; + @end diff --git a/src/client/mac/sender/uploader.mm b/src/client/mac/sender/uploader.mm index c74c0583..08aecf76 100644 --- a/src/client/mac/sender/uploader.mm +++ b/src/client/mac/sender/uploader.mm @@ -511,6 +511,9 @@ NSDictionary *readConfigurationData(const char *configFile) { reportID = [[result stringByTrimmingCharactersInSet:trimSet] UTF8String]; [self logUploadWithID:reportID]; } + if (uploadCompletion_) { + uploadCompletion_([NSString stringWithUTF8String:reportID], error); + } // rename the minidump file according to the id returned from the server NSString *minidumpDir = @@ -547,6 +550,11 @@ NSDictionary *readConfigurationData(const char *configFile) { return [NSURLQueryItem queryItemWithName:queryItemName value:escapedValue]; } +//============================================================================= +- (void)setUploadCompletionBlock:(UploadCompletionBlock)uploadCompletion { + uploadCompletion_ = uploadCompletion; +} + //============================================================================= - (void)report { NSURL *url = [NSURL URLWithString:[parameters_ objectForKey:@BREAKPAD_URL]];