[PLAT-12346] Harmonize all calls to manual error reporting, and ensure proper frame stripping in all cases

Bugsnag/Bugsnag.m

@@ -32,6 +32,7 @@
 #import "BugsnagClient+Private.h"
 #import "BugsnagInternals.h"
 #import "BugsnagLogger.h"
+#import "BSGUtils.h"
 
 static BugsnagClient *bsg_g_bugsnag_client = NULL;
 
@@ -98,27 +99,38 @@ + (void)markLaunchCompleted {
     }
 }
 
+// Here, we pass all public notify APIs to a common handling method
+// (notifyErrorOrException) and then prevent the compiler from performing
+// any inlining or outlining that would change the number of Bugsnag handler
+// methods on the stack and break our stack stripping.
+// Note: Each BSGPreventInlining call site within a module MUST pass a different
+//       string to prevent outlining!
+
 + (void)notify:(NSException *)exception {
     if ([self bugsnagReadyForInternalCalls]) {
-        [self.client notify:exception];
+        BSGPreventInlining(@"Prevent");
+        [self.client notifyErrorOrException:exception stackStripDepth:2 block:nil];
     }
 }
 
 + (void)notify:(NSException *)exception block:(BugsnagOnErrorBlock)block {
     if ([self bugsnagReadyForInternalCalls]) {
-        [self.client notify:exception block:block];
+        BSGPreventInlining(@"inlining");
+        [self.client notifyErrorOrException:exception stackStripDepth:2 block:block];
     }
 }
 
 + (void)notifyError:(NSError *)error {
     if ([self bugsnagReadyForInternalCalls]) {
-        [self.client notifyError:error];
+        BSGPreventInlining(@"and");
+        [self.client notifyErrorOrException:error stackStripDepth:2 block:nil];
     }
 }
 
 + (void)notifyError:(NSError *)error block:(BugsnagOnErrorBlock)block {
     if ([self bugsnagReadyForInternalCalls]) {
-        [self.client notifyError:error block:block];
+        BSGPreventInlining(@"outlining");
+        [self.client notifyErrorOrException:error stackStripDepth:2 block:block];
     }
 }
 

Bugsnag/Client/BugsnagClient+Private.h

@@ -75,6 +75,26 @@ BSG_OBJC_DIRECT_MEMBERS
 
 - (void)start;
 
+/**
+ * Common entry point to notify an error or an exception.
+ * Bugsnag components MUST NOT call the regular notify methods in this class. ALWAYS call
+ * this method instead.
+ *
+ * You must provide the number of stack trace entries to strip from the top of the stack
+ * (INCLUDING this method) so that our own reporting methods don't show up in the reported stack trace.
+ *
+ * Example: stackStripDepth = 2 would strip the top two entries, which we would expect to be
+ * 1. +[Bugsnag notifyError:block:]
+ * 2. -[BugsnagClient notifyErrorOrException:stackStripDepth:block:]
+ *
+ * @param errorOrException the error or exception to report.
+ * @param stackStripDepth The number of stack trace entries to strip from the top of the stack.
+ * @param block Called after reporting.
+ */
+- (void)notifyErrorOrException:(id)errorOrException
+               stackStripDepth:(NSUInteger)stackStripDepth
+                         block:(_Nullable BugsnagOnErrorBlock)block;
+
 @end
 
 NS_ASSUME_NONNULL_END

Bugsnag/Client/BugsnagClient.m

@@ -641,37 +641,35 @@ - (NSString *)context {
 
 // MARK: - Notify
 
-// Prevent the compiler from inlining or optimizing, which would reduce
-// the number of bugsnag-only stack entries and mess up our pruning.
-// We have to do it this way because you can't mark Objective-C methods noinline or optnone.
-// We leave it externable to further dissuade the optimizer.
-__attribute__((optnone))
-void bsg_notifyErrorOrException(BugsnagClient *self, id errorOrException, BugsnagOnErrorBlock block) {
-    [self notifyErrorOrException:errorOrException block:block];
-}
-
-// note - some duplication between notifyError calls is required to ensure
-// the same number of stackframes are used for each call.
-// see notify:handledState:block for further info
+// Here, we pass all public notify APIs to a common handling method
+// (notifyErrorOrException) and then prevent the compiler from performing
+// any inlining or outlining that would change the number of Bugsnag handler
+// methods on the stack and break our stack stripping.
+// Note: Each BSGPreventInlining call site within a module MUST pass a different
+//       string to prevent outlining!
 
 - (void)notifyError:(NSError *)error {
     bsg_log_debug(@"%s %@", __PRETTY_FUNCTION__, error);
-    bsg_notifyErrorOrException(self, error, nil);
+    BSGPreventInlining(@"Prevent");
+    [self notifyErrorOrException:error stackStripDepth:2 block:nil];
 }
 
 - (void)notifyError:(NSError *)error block:(BugsnagOnErrorBlock)block {
     bsg_log_debug(@"%s %@", __PRETTY_FUNCTION__, error);
-    bsg_notifyErrorOrException(self, error, block);
+    BSGPreventInlining(@"inlining");
+    [self notifyErrorOrException:error stackStripDepth:2 block:block];
 }
 
 - (void)notify:(NSException *)exception {
     bsg_log_debug(@"%s %@", __PRETTY_FUNCTION__, exception);
-    bsg_notifyErrorOrException(self, exception, nil);
+    BSGPreventInlining(@"and");
+    [self notifyErrorOrException:exception stackStripDepth:2 block:nil];
 }
 
 - (void)notify:(NSException *)exception block:(BugsnagOnErrorBlock)block {
     bsg_log_debug(@"%s %@", __PRETTY_FUNCTION__, exception);
-    bsg_notifyErrorOrException(self, exception, block);
+    BSGPreventInlining(@"outlining");
+    [self notifyErrorOrException:exception stackStripDepth:2 block:block];
 }
 
 // MARK: - Notify (Internal)
@@ -686,7 +684,9 @@ - (BugsnagCorrelation *)getCurrentCorrelation {
     return [[BugsnagCorrelation alloc] initWithTraceId:traceId spanId:spanId];
 }
 
-- (void)notifyErrorOrException:(id)errorOrException block:(BugsnagOnErrorBlock)block {
+- (void)notifyErrorOrException:(id)errorOrException
+               stackStripDepth:(NSUInteger)stackStripDepth
+                         block:(_Nullable BugsnagOnErrorBlock)block {
     BugsnagCorrelation *correlation = [self getCurrentCorrelation];
     NSDictionary *systemInfo = [BSG_KSSystemInfo systemInfo];
     BugsnagMetadata *metadata = [self.metadata copy];
@@ -730,25 +730,10 @@ - (void)notifyErrorOrException:(id)errorOrException block:(BugsnagOnErrorBlock)b
         return;
     }
 
-    /**
-     * Stack frames starting from this one are removed by setting the depth.
-     * This helps remove bugsnag frames from showing in NSErrors as their
-     * trace is synthesized.
-     *
-     * For example, for [Bugsnag notifyError:block:], bugsnag adds the following
-     * frames which must be removed:
-     *
-     * 1. +[Bugsnag notifyError:block:]
-     * 2. -[BugsnagClient notifyError:block:]
-     * 3. bsg_notifyErrorOrException()
-     * 4. -[BugsnagClient notifyErrorOrException:block:]
-     */
-    NSUInteger depth = 4;
-
     if (!callStack.count) {
         // If the NSException was not raised by the Objective-C runtime, it will be missing a call stack.
         // Use the current call stack instead.
-        callStack = BSGArraySubarrayFromIndex(NSThread.callStackReturnAddresses, depth);
+        callStack = BSGArraySubarrayFromIndex(NSThread.callStackReturnAddresses, stackStripDepth);
     }
 
 #if BSG_HAVE_MACH_THREADS

Bugsnag/Helpers/BSGUtils.h

@@ -45,6 +45,8 @@ static inline NSString * _Nullable BSGStringFromClass(Class _Nullable cls) {
  */
 void bsg_safe_strncpy(char *dst, const char *src, size_t length);
 
+NSString * _Nullable BSGPreventInlining(NSString * _Nullable someValue);
+
 NS_ASSUME_NONNULL_END
 
 __END_DECLS

Bugsnag/Helpers/BSGUtils.m

@@ -89,3 +89,10 @@ dispatch_queue_t BSGGetFileSystemQueue(void) {
     }
     return nil;
 }
+
+NSString * _Nullable BSGPreventInlining(NSString * _Nullable someValue) {
+    static NSString *lastValue = nil;
+    NSString *returnValue = lastValue;
+    lastValue = someValue;
+    return returnValue;
+}

Tests/BugsnagTests/BugsnagClientMirrorTest.m

@@ -132,6 +132,7 @@ - (void)setUp {
             @"systemState @16@0:8",
             @"thermalStateDidChange: v24@0:8@16",
             @"updateSession: v24@0:8@?16",
+            @"notifyErrorOrException:stackStripDepth:block: v40@0:8@16Q24@?32",
     ]];
 
     // the following methods are implemented on Bugsnag but do not need to