From 6354abb94adb3b270e50968e0199b1f0f89a7c72 Mon Sep 17 00:00:00 2001 From: Delisa Mason Date: Thu, 13 Dec 2018 08:43:52 -0800 Subject: [PATCH] fix: Record handled report info during initial write Before this change, information which is not part of the standard KS report was serialized in the onCrash callback, which is shared between notify() and fatal reports, and used simultaneously in the case that two reports are generated at once. This change serializes information which applies to only a single report into the initial report, reserving the onCrash handler for shared global state which does not become null once set, like session information, and handlers which are only run in the event of a crash (by definition, occurring only once). Fixes a case where handled state can be reported incorrect for notify() requests as the shared handled state was cleared/set to null by a concurrent request. --- Source/BugsnagCrashSentry.h | 8 ++- Source/BugsnagCrashSentry.m | 17 +++-- Source/BugsnagNotifier.m | 67 ++++++------------- .../Source/KSCrash/Recording/BSG_KSCrash.h | 27 ++++---- .../Source/KSCrash/Recording/BSG_KSCrash.m | 48 +++++++------ .../Source/KSCrash/Recording/BSG_KSCrashC.c | 14 ++-- .../Source/KSCrash/Recording/BSG_KSCrashC.h | 26 +++---- .../KSCrash/Recording/BSG_KSCrashReport.c | 39 +++++++---- .../Recording/BSG_KSCrashReportFields.h | 6 ++ .../Recording/Sentry/BSG_KSCrashSentry.h | 15 ++--- .../Recording/Sentry/BSG_KSCrashSentry_User.c | 18 +++-- .../Recording/Sentry/BSG_KSCrashSentry_User.h | 27 ++++---- 12 files changed, 167 insertions(+), 145 deletions(-) diff --git a/Source/BugsnagCrashSentry.h b/Source/BugsnagCrashSentry.h index 2b72ad403..0913fb022 100644 --- a/Source/BugsnagCrashSentry.h +++ b/Source/BugsnagCrashSentry.h @@ -19,6 +19,12 @@ onCrash:(BSGReportCallback)onCrash; - (void)reportUserException:(NSString *)reportName - reason:(NSString *)reportMessage; + reason:(NSString *)reportMessage + handledState:(NSDictionary *)handledState + appState:(NSDictionary *)appState + callbackOverrides:(NSDictionary *)overrides + metadata:(NSDictionary *)metadata + config:(NSDictionary *)config + discardDepth:(int)depth; @end diff --git a/Source/BugsnagCrashSentry.m b/Source/BugsnagCrashSentry.m index 3bdb2a920..3a64a1707 100644 --- a/Source/BugsnagCrashSentry.m +++ b/Source/BugsnagCrashSentry.m @@ -42,13 +42,22 @@ - (void)install:(BugsnagConfiguration *)config } - (void)reportUserException:(NSString *)reportName - reason:(NSString *)reportMessage { + reason:(NSString *)reportMessage + handledState:(NSDictionary *)handledState + appState:(NSDictionary *)appState + callbackOverrides:(NSDictionary *)overrides + metadata:(NSDictionary *)metadata + config:(NSDictionary *)config + discardDepth:(int)depth { [[BSG_KSCrash sharedInstance] reportUserException:reportName reason:reportMessage - language:NULL - lineOfCode:@"" - stackTrace:@[] + handledState:handledState + appState:appState + callbackOverrides:overrides + metadata:metadata + config:config + discardDepth:depth terminateProgram:NO]; } diff --git a/Source/BugsnagNotifier.m b/Source/BugsnagNotifier.m index 804757fa2..a7caad8bc 100644 --- a/Source/BugsnagNotifier.m +++ b/Source/BugsnagNotifier.m @@ -82,37 +82,23 @@ * @param writer report writer which will receive updated metadata */ void BSSerializeDataCrashHandler(const BSG_KSCrashReportWriter *writer, int type) { - BOOL userReported = BSG_KSCrashTypeUserReported == type; - + BOOL isCrash = BSG_KSCrashTypeUserReported != type; if (hasRecordedSessions) { // a session is available // persist session info writer->addStringElement(writer, "id", (const char *) sessionId); writer->addStringElement(writer, "startedAt", (const char *) sessionStartDate); writer->addUIntegerElement(writer, "handledCount", handledCount); - - if (!userReported) { - writer->addUIntegerElement(writer, "unhandledCount", 1); - } else { - writer->addUIntegerElement(writer, "unhandledCount", 0); - } - } - if (bsg_g_bugsnag_data.configJSON) { - writer->addJSONElement(writer, "config", bsg_g_bugsnag_data.configJSON); - } - if (bsg_g_bugsnag_data.stateJSON) { - writer->addJSONElement(writer, "state", bsg_g_bugsnag_data.stateJSON); - } - if (bsg_g_bugsnag_data.metaDataJSON) { - writer->addJSONElement(writer, "metaData", bsg_g_bugsnag_data.metaDataJSON); + writer->addUIntegerElement(writer, "unhandledCount", isCrash ? 1 : 0); } - - // write additional user-supplied metadata - if (userReported) { - if (bsg_g_bugsnag_data.handledState) { - writer->addJSONElement(writer, "handledState", bsg_g_bugsnag_data.handledState); + if (isCrash) { + if (bsg_g_bugsnag_data.configJSON) { + writer->addJSONElement(writer, "config", bsg_g_bugsnag_data.configJSON); + } + if (bsg_g_bugsnag_data.stateJSON) { + writer->addJSONElement(writer, "state", bsg_g_bugsnag_data.stateJSON); } - if (bsg_g_bugsnag_data.userOverridesJSON) { - writer->addJSONElement(writer, "overrides", bsg_g_bugsnag_data.userOverridesJSON); + if (bsg_g_bugsnag_data.metaDataJSON) { + writer->addJSONElement(writer, "metaData", bsg_g_bugsnag_data.metaDataJSON); } } @@ -537,19 +523,6 @@ - (void)notify:(NSString *)exceptionName if (block) { block(report); } - - [self.metaDataLock lock]; - BSSerializeJSONDictionary([report.handledState toJson], - &bsg_g_bugsnag_data.handledState); - BSSerializeJSONDictionary(report.metaData, - &bsg_g_bugsnag_data.metaDataJSON); - BSSerializeJSONDictionary(report.overrides, - &bsg_g_bugsnag_data.userOverridesJSON); - - [self.state addAttribute:BSGKeySeverity - withValue:BSGFormatSeverity(report.severity) - toTabWithName:BSTabCrash]; - // We discard 5 stack frames (including this one) by default, // and sum that with the number specified by report.depth: // @@ -560,23 +533,21 @@ - (void)notify:(NSString *)exceptionName // 3 -[BugsnagCrashSentry reportUserException:reason:] // 4 -[BugsnagNotifier notify:message:block:] - NSNumber *depth = @(BSGNotifierStackFrameCount + report.depth); - [self.state addAttribute:BSAttributeDepth - withValue:depth - toTabWithName:BSTabCrash]; + int depth = (int)(BSGNotifierStackFrameCount + report.depth); NSString *reportName = report.errorClass ?: NSStringFromClass([NSException class]); NSString *reportMessage = report.errorMessage ?: @""; - [self.crashSentry reportUserException:reportName reason:reportMessage]; - bsg_g_bugsnag_data.userOverridesJSON = NULL; - bsg_g_bugsnag_data.handledState = NULL; + [self.crashSentry reportUserException:reportName + reason:reportMessage + handledState:[handledState toJson] + appState:[self.state toDictionary] + callbackOverrides:report.overrides + metadata:[report.metaData copy] + config:[self.configuration.config toDictionary] + discardDepth:depth]; - // Restore metaData to pre-crash state. - [self.metaDataLock unlock]; - [self metaDataChanged:self.configuration.metaData]; - [[self state] clearTab:BSTabCrash]; [self addBreadcrumbWithBlock:^(BugsnagBreadcrumb *_Nonnull crumb) { crumb.type = BSGBreadcrumbTypeError; crumb.name = reportName; diff --git a/Source/KSCrash/Source/KSCrash/Recording/BSG_KSCrash.h b/Source/KSCrash/Source/KSCrash/Recording/BSG_KSCrash.h index 764cbe884..91fefa031 100644 --- a/Source/KSCrash/Source/KSCrash/Recording/BSG_KSCrash.h +++ b/Source/KSCrash/Source/KSCrash/Recording/BSG_KSCrash.h @@ -172,24 +172,25 @@ typedef enum { * application will terminate with an abort(). * * @param name The exception name (for namespacing exception types). - * - * @param reason A description of why the exception occurred. - * - * @param language A unique language identifier. - * - * @param lineOfCode A copy of the offending line of code (nil = ignore). - * - * @param stackTrace An array of frames (dictionaries or strings) representing - * the call stack leading to the exception (nil = ignore). - * + * @param reason A description of why the exception occurred + * @param handledState The severity, reason, and handled-ness of the report + * @param appState breadcrumbs and other app environmental info + * @param overrides Report fields overridden by callbacks, collated in the + * final report + * @param metadata additional information to attach to the report + * @param config delivery options + * @param depth The number of frames to discard from the top of the stacktrace * @param terminateProgram If true, do not return from this function call. * Terminate the program instead. */ - (void)reportUserException:(NSString *)name reason:(NSString *)reason - language:(NSString *)language - lineOfCode:(NSString *)lineOfCode - stackTrace:(NSArray *)stackTrace + handledState:(NSDictionary *)handledState + appState:(NSDictionary *)appState + callbackOverrides:(NSDictionary *)overrides + metadata:(NSDictionary *)metadata + config:(NSDictionary *)config + discardDepth:(int)depth terminateProgram:(BOOL)terminateProgram; /** If YES, user reported exceptions will suspend all threads during report diff --git a/Source/KSCrash/Source/KSCrash/Recording/BSG_KSCrash.m b/Source/KSCrash/Source/KSCrash/Recording/BSG_KSCrash.m index 508d37ab8..dc595732a 100644 --- a/Source/KSCrash/Source/KSCrash/Recording/BSG_KSCrash.m +++ b/Source/KSCrash/Source/KSCrash/Recording/BSG_KSCrash.m @@ -345,32 +345,38 @@ - (void)deleteAllReports { [self.crashReportStore deleteAllFiles]; } -- (void)reportUserException:(NSString *)name - reason:(NSString *)reason - language:(NSString *)language - lineOfCode:(NSString *)lineOfCode - stackTrace:(NSArray *)stackTrace - terminateProgram:(BOOL)terminateProgram { - const char *cName = [name cStringUsingEncoding:NSUTF8StringEncoding]; - const char *cReason = [reason cStringUsingEncoding:NSUTF8StringEncoding]; - const char *cLanguage = - [language cStringUsingEncoding:NSUTF8StringEncoding]; - const char *cLineOfCode = - [lineOfCode cStringUsingEncoding:NSUTF8StringEncoding]; +- (const char *)encodeAsJSONString:(id)object { NSError *error = nil; - NSData *jsonData = - [BSG_KSJSONCodec encode:stackTrace options:0 error:&error]; + NSData *jsonData = [BSG_KSJSONCodec encode:object options:0 error:&error]; if (jsonData == nil || error != nil) { - BSG_KSLOG_ERROR(@"Error encoding stack trace to JSON: %@", error); - // Don't return, since we can still record other useful information. + BSG_KSLOG_ERROR(@"Error encoding object to JSON: %@", error); + // we can still record other useful information from the report + return NULL; } NSString *jsonString = - [[NSString alloc] initWithData:jsonData encoding:NSUTF8StringEncoding]; - const char *cStackTrace = - [jsonString cStringUsingEncoding:NSUTF8StringEncoding]; + [[NSString alloc] initWithData:jsonData encoding:NSUTF8StringEncoding]; + return [jsonString cStringUsingEncoding:NSUTF8StringEncoding]; +} - bsg_kscrash_reportUserException(cName, cReason, cLanguage, cLineOfCode, - cStackTrace, terminateProgram); +- (void)reportUserException:(NSString *)name + reason:(NSString *)reason + handledState:(NSDictionary *)handledState + appState:(NSDictionary *)appState + callbackOverrides:(NSDictionary *)overrides + metadata:(NSDictionary *)metadata + config:(NSDictionary *)config + discardDepth:(int)depth + terminateProgram:(BOOL)terminateProgram { + const char *cName = [name cStringUsingEncoding:NSUTF8StringEncoding]; + const char *cReason = [reason cStringUsingEncoding:NSUTF8StringEncoding]; + bsg_kscrash_reportUserException(cName, cReason, + [self encodeAsJSONString:handledState], + [self encodeAsJSONString:overrides], + [self encodeAsJSONString:metadata], + [self encodeAsJSONString:appState], + [self encodeAsJSONString:config], + depth, + terminateProgram); // If bsg_kscrash_reportUserException() returns, we did not terminate. // Set up IDs and paths for the next crash. diff --git a/Source/KSCrash/Source/KSCrash/Recording/BSG_KSCrashC.c b/Source/KSCrash/Source/KSCrash/Recording/BSG_KSCrashC.c index c66ea766a..1ebc3a93b 100644 --- a/Source/KSCrash/Source/KSCrash/Recording/BSG_KSCrashC.c +++ b/Source/KSCrash/Source/KSCrash/Recording/BSG_KSCrashC.c @@ -242,12 +242,16 @@ void bsg_kscrash_setCrashNotifyCallback( } void bsg_kscrash_reportUserException(const char *name, const char *reason, - const char *language, - const char *lineOfCode, - const char *stackTrace, + const char *handledState, + const char *overrides, + const char *metadata, + const char *appState, + const char *config, + int discardDepth, bool terminateProgram) { - bsg_kscrashsentry_reportUserException(name, reason, language, lineOfCode, - stackTrace, terminateProgram); + bsg_kscrashsentry_reportUserException(name, reason, handledState, overrides, + metadata, appState, config, discardDepth, + terminateProgram); } void bsg_kscrash_setSuspendThreadsForUserReported( diff --git a/Source/KSCrash/Source/KSCrash/Recording/BSG_KSCrashC.h b/Source/KSCrash/Source/KSCrash/Recording/BSG_KSCrashC.h index 4ac563b6b..9df924549 100644 --- a/Source/KSCrash/Source/KSCrash/Recording/BSG_KSCrashC.h +++ b/Source/KSCrash/Source/KSCrash/Recording/BSG_KSCrashC.h @@ -175,24 +175,24 @@ void bsg_kscrash_setCrashNotifyCallback( * application will terminate with an abort(). * * @param name The exception name (for namespacing exception types). - * * @param reason A description of why the exception occurred. - * - * @param language A unique language identifier. - * - * @param lineOfCode A copy of the offending line of code (NULL = ignore). - * - * @param stackTrace JSON encoded array containing stack trace information (one - * frame per array entry). The frame structure can be anything you want, - * including bare strings. - * + * @param handledState The severity, reason, and handled-ness of the report + * @param appState breadcrumbs and other app environmental info + * @param overrides Report fields overridden by callbacks, collated in the + * final report + * @param metadata additional information to attach to the report + * @param discardDepth The number of frames to discard from the top of the + * stacktrace * @param terminateProgram If true, do not return from this function call. * Terminate the program instead. */ void bsg_kscrash_reportUserException(const char *name, const char *reason, - const char *language, - const char *lineOfCode, - const char *stackTrace, + const char *handledState, + const char *overrides, + const char *metadata, + const char *appState, + const char *config, + int discardDepth, bool terminateProgram); /** If YES, user reported exceptions will suspend all threads during report diff --git a/Source/KSCrash/Source/KSCrash/Recording/BSG_KSCrashReport.c b/Source/KSCrash/Source/KSCrash/Recording/BSG_KSCrashReport.c index 55bae15b5..b416b4ef0 100644 --- a/Source/KSCrash/Source/KSCrash/Recording/BSG_KSCrashReport.c +++ b/Source/KSCrash/Source/KSCrash/Recording/BSG_KSCrashReport.c @@ -1789,20 +1789,6 @@ void bsg_kscrw_i_writeError(const BSG_KSCrashReportWriter *const writer, { writer->addStringElement(writer, BSG_KSCrashField_Name, crash->userException.name); - if (crash->userException.language != NULL) { - writer->addStringElement(writer, BSG_KSCrashField_Language, - crash->userException.language); - } - if (crash->userException.lineOfCode != NULL) { - writer->addStringElement(writer, - BSG_KSCrashField_LineOfCode, - crash->userException.lineOfCode); - } - if (crash->userException.customStackTrace != NULL) { - writer->addJSONElement( - writer, BSG_KSCrashField_Backtrace, - crash->userException.customStackTrace); - } } writer->endContainer(writer); break; @@ -2103,7 +2089,32 @@ void bsg_kscrashreport_writeStandardReport( writer->endContainer(writer); if (crashContext->config.onCrashNotify != NULL) { + // Write handled exception report info writer->beginObject(writer, BSG_KSCrashField_UserAtCrash); + if (crashContext->crash.crashType == BSG_KSCrashTypeUserReported) { + if (crashContext->crash.userException.overrides != NULL) { + writer->addJSONElement(writer, BSG_KSCrashField_Overrides, + crashContext->crash.userException.overrides); + } + if (crashContext->crash.userException.handledState != NULL) { + writer->addJSONElement(writer, BSG_KSCrashField_HandledState, + crashContext->crash.userException.handledState); + } + if (crashContext->crash.userException.metadata != NULL) { + writer->addJSONElement(writer, BSG_KSCrashField_Metadata, + crashContext->crash.userException.metadata); + } + if (crashContext->crash.userException.state != NULL) { + writer->addJSONElement(writer, BSG_KSCrashField_State, + crashContext->crash.userException.state); + } + if (crashContext->crash.userException.config != NULL) { + writer->addJSONElement(writer, BSG_KSCrashField_Config, + crashContext->crash.userException.config); + } + writer->addIntegerElement(writer, BSG_KSCrashField_DiscardDepth, + crashContext->crash.userException.discardDepth); + } { bsg_kscrw_i_callUserCrashHandler(crashContext, writer); } writer->endContainer(writer); } diff --git a/Source/KSCrash/Source/KSCrash/Recording/BSG_KSCrashReportFields.h b/Source/KSCrash/Source/KSCrash/Recording/BSG_KSCrashReportFields.h index 77389f0aa..b58590256 100644 --- a/Source/KSCrash/Source/KSCrash/Recording/BSG_KSCrashReportFields.h +++ b/Source/KSCrash/Source/KSCrash/Recording/BSG_KSCrashReportFields.h @@ -129,6 +129,12 @@ #define BSG_KSCrashField_Signal "signal" #define BSG_KSCrashField_Subcode "subcode" #define BSG_KSCrashField_UserReported "user_reported" +#define BSG_KSCrashField_Overrides "overrides" +#define BSG_KSCrashField_HandledState "handledState" +#define BSG_KSCrashField_Metadata "metaData" +#define BSG_KSCrashField_State "state" +#define BSG_KSCrashField_Config "config" +#define BSG_KSCrashField_DiscardDepth "depth" #pragma mark - Process State - diff --git a/Source/KSCrash/Source/KSCrash/Recording/Sentry/BSG_KSCrashSentry.h b/Source/KSCrash/Source/KSCrash/Recording/Sentry/BSG_KSCrashSentry.h index 0d619f03a..1e8e73311 100644 --- a/Source/KSCrash/Source/KSCrash/Recording/Sentry/BSG_KSCrashSentry.h +++ b/Source/KSCrash/Source/KSCrash/Recording/Sentry/BSG_KSCrashSentry.h @@ -138,14 +138,13 @@ typedef struct BSG_KSCrash_SentryContext { /** The exception name. */ const char *name; - /** The language the exception occured in. */ - const char *language; - - /** The line of code where the exception occurred. Can be NULL. */ - const char *lineOfCode; - - /** The user-supplied JSON encoded stack trace. */ - const char *customStackTrace; + /** Handled exception report info: */ + const char *overrides; // info set in callbacks + const char *handledState; + const char *metadata; + const char *state; // breadcrumbs, other shared app state + const char *config; // config options which affect report delivery + int discardDepth; // number of frames from the top to remove } userException; } BSG_KSCrash_SentryContext; diff --git a/Source/KSCrash/Source/KSCrash/Recording/Sentry/BSG_KSCrashSentry_User.c b/Source/KSCrash/Source/KSCrash/Recording/Sentry/BSG_KSCrashSentry_User.c index ef9e17d50..fddd69917 100644 --- a/Source/KSCrash/Source/KSCrash/Recording/Sentry/BSG_KSCrashSentry_User.c +++ b/Source/KSCrash/Source/KSCrash/Recording/Sentry/BSG_KSCrashSentry_User.c @@ -48,9 +48,12 @@ void bsg_kscrashsentry_uninstallUserExceptionHandler(void) { } void bsg_kscrashsentry_reportUserException(const char *name, const char *reason, - const char *language, - const char *lineOfCode, - const char *stackTrace, + const char *handledState, + const char *overrides, + const char *metadata, + const char *appState, + const char *config, + int discardDepth, bool terminateProgram) { if (bsg_g_context == NULL) { BSG_KSLOG_WARN("User-reported exception sentry is not installed. " @@ -81,9 +84,12 @@ void bsg_kscrashsentry_reportUserException(const char *name, const char *reason, bsg_g_context->stackTrace = callstack; bsg_g_context->stackTraceLength = callstackCount; bsg_g_context->userException.name = name; - bsg_g_context->userException.language = language; - bsg_g_context->userException.lineOfCode = lineOfCode; - bsg_g_context->userException.customStackTrace = stackTrace; + bsg_g_context->userException.handledState = handledState; + bsg_g_context->userException.overrides = overrides; + bsg_g_context->userException.config = config; + bsg_g_context->userException.discardDepth = discardDepth; + bsg_g_context->userException.metadata = metadata; + bsg_g_context->userException.state = appState; BSG_KSLOG_DEBUG("Calling main crash handler."); bsg_g_context->onCrash(); diff --git a/Source/KSCrash/Source/KSCrash/Recording/Sentry/BSG_KSCrashSentry_User.h b/Source/KSCrash/Source/KSCrash/Recording/Sentry/BSG_KSCrashSentry_User.h index 3e5383015..103f33823 100644 --- a/Source/KSCrash/Source/KSCrash/Recording/Sentry/BSG_KSCrashSentry_User.h +++ b/Source/KSCrash/Source/KSCrash/Recording/Sentry/BSG_KSCrashSentry_User.h @@ -55,22 +55,25 @@ void bsg_kscrashsentry_uninstallUserExceptionHandler(void); * * @param reason A description of why the exception occurred. * - * @param language A unique language identifier. - * - * @param lineOfCode A copy of the offending line of code (NULL = ignore). - * - * @param stackTrace JSON encoded array containing stack trace information (one - * frame per array entry). The frame structure can be anything you want, - * including bare strings. + * @param handledState The severity, reason, and handled-ness of the report + * @param appState breadcrumbs and other app environmental info + * @param overrides Report fields overridden by callbacks, collated in the + * final report + * @param metadata additional information to attach to the report + * @param discardDepth The number of frames to discard from the top of the + * stacktrace * * @param terminateProgram If true, do not return from this function call. * Terminate the program instead. */ -void bsg_kscrashsentry_reportUserException(const char *name, const char *reason, - const char *language, - const char *lineOfCode, - const char *stackTrace, - bool terminateProgram); + void bsg_kscrashsentry_reportUserException(const char *name, const char *reason, + const char *handledState, + const char *overrides, + const char *metadata, + const char *appState, + const char *config, + int discardDepth, + bool terminateProgram); #ifdef __cplusplus }