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.

CHANGELOG.md

@@ -5,6 +5,9 @@ Changelog
 
 ### Bug Fixes
 
+* Fix case where notify() causes an unhandled report
+  [#322](https://github.com/bugsnag/bugsnag-cocoa/pull/322)
+
 * Fix possible crash when fetching system info to append to a crash report
   [#321](https://github.com/bugsnag/bugsnag-cocoa/pull/321)
 

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

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];
 }
 

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;

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

Source/KSCrash/Source/KSCrash/Recording/BSG_KSCrash.m

@@ -347,30 +347,23 @@ - (void)deleteAllReports {
 
 - (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 {
     const char *cName = [name cStringUsingEncoding:NSUTF8StringEncoding];
     const char *cReason = [reason cStringUsingEncoding:NSUTF8StringEncoding];
-    const char *cLanguage =
-        [language cStringUsingEncoding:NSUTF8StringEncoding];
-    const char *cLineOfCode =
-        [lineOfCode cStringUsingEncoding:NSUTF8StringEncoding];
-    NSError *error = nil;
-    NSData *jsonData =
-        [BSG_KSJSONCodec encode:stackTrace 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.
-    }
-    NSString *jsonString =
-        [[NSString alloc] initWithData:jsonData encoding:NSUTF8StringEncoding];
-    const char *cStackTrace =
-        [jsonString cStringUsingEncoding:NSUTF8StringEncoding];
-
-    bsg_kscrash_reportUserException(cName, cReason, cLanguage, cLineOfCode,
-                                    cStackTrace, terminateProgram);
+    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.
@@ -474,6 +467,19 @@ - (NSMutableData *)nullTerminated:(NSData *)data {
     return mutable;
 }
 
+- (const char *)encodeAsJSONString:(id)object {
+    NSError *error = nil;
+    NSData *jsonData = [BSG_KSJSONCodec encode:object options:0 error:&error];
+    if (jsonData == nil || error != nil) {
+        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];
+    return [jsonString cStringUsingEncoding:NSUTF8StringEncoding];
+}
+
 // ============================================================================
 #pragma mark - Callbacks -
 // ============================================================================

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(

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

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);
         }

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 -