Merge pull request #357 from bugsnag/PLAT-13280-crosstalk

[PLAT-13280] More robust crosstalk API, and also a permissive proxy class

BugsnagPerformance.xcodeproj/project.pbxproj

@@ -65,7 +65,7 @@
 		09D59E1B2BDFE0D900199E1B /* NetworkHeaderInjector.mm in Sources */ = {isa = PBXBuildFile; fileRef = 09D59E192BDFE0D900199E1B /* NetworkHeaderInjector.mm */; };
 		09D807F52B9756B000D01DF5 /* BugsnagPerformanceSwiftUI.docc in Sources */ = {isa = PBXBuildFile; fileRef = 09D807F42B9756B000D01DF5 /* BugsnagPerformanceSwiftUI.docc */; };
 		09D807F62B9756B000D01DF5 /* BugsnagPerformanceSwiftUI.h in Headers */ = {isa = PBXBuildFile; fileRef = 09D807F32B9756B000D01DF5 /* BugsnagPerformanceSwiftUI.h */; settings = {ATTRIBUTES = (Public, ); }; };
-		09E313042BF363020081F219 /* CrossTalkTests.m in Sources */ = {isa = PBXBuildFile; fileRef = 09E313032BF363020081F219 /* CrossTalkTests.m */; };
+		09E313042BF363020081F219 /* CrossTalkTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = 09E313032BF363020081F219 /* CrossTalkTests.mm */; };
 		09F23A8C2CE351ED00F0D769 /* BugsnagSwiftTools.h in Headers */ = {isa = PBXBuildFile; fileRef = 09F23A8A2CE351ED00F0D769 /* BugsnagSwiftTools.h */; };
 		09F23A8D2CE351ED00F0D769 /* BugsnagSwiftTools.m in Sources */ = {isa = PBXBuildFile; fileRef = 09F23A8B2CE351ED00F0D769 /* BugsnagSwiftTools.m */; };
 		09F23A8F2CE3521D00F0D769 /* BugsnagSwiftToolsImpl.swift in Sources */ = {isa = PBXBuildFile; fileRef = 09F23A8E2CE3521D00F0D769 /* BugsnagSwiftToolsImpl.swift */; };
@@ -310,7 +310,7 @@
 		09D807F32B9756B000D01DF5 /* BugsnagPerformanceSwiftUI.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = BugsnagPerformanceSwiftUI.h; sourceTree = "<group>"; };
 		09D807F42B9756B000D01DF5 /* BugsnagPerformanceSwiftUI.docc */ = {isa = PBXFileReference; lastKnownFileType = folder.documentationcatalog; path = BugsnagPerformanceSwiftUI.docc; sourceTree = "<group>"; };
 		09DC62282C6A2EF6000AA8E1 /* BugsnagPerformanceSpanContext+Private.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = "BugsnagPerformanceSpanContext+Private.h"; sourceTree = "<group>"; };
-		09E313032BF363020081F219 /* CrossTalkTests.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = CrossTalkTests.m; sourceTree = "<group>"; };
+		09E313032BF363020081F219 /* CrossTalkTests.mm */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.objcpp; path = CrossTalkTests.mm; sourceTree = "<group>"; };
 		09F23A8A2CE351ED00F0D769 /* BugsnagSwiftTools.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = BugsnagSwiftTools.h; sourceTree = "<group>"; };
 		09F23A8B2CE351ED00F0D769 /* BugsnagSwiftTools.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = BugsnagSwiftTools.m; sourceTree = "<group>"; };
 		09F23A8E2CE3521D00F0D769 /* BugsnagSwiftToolsImpl.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = BugsnagSwiftToolsImpl.swift; sourceTree = "<group>"; };
@@ -662,7 +662,7 @@
 				CB0AD75C29641B59002A3FB6 /* BatchTests.mm */,
 				CB0AD76B296578D9002A3FB6 /* BugsnagPerformanceConfigurationTests.mm */,
 				0122C25F29019C05002D243C /* BugsnagPerformanceTests.mm */,
-				09E313032BF363020081F219 /* CrossTalkTests.m */,
+				09E313032BF363020081F219 /* CrossTalkTests.mm */,
 				CBEC51C8296ED98F009C0CE3 /* FileBasedTest.h */,
 				CBEC51C9296ED98F009C0CE3 /* FileBasedTest.m */,
 				CBEC51D82976D54B009C0CE3 /* FilesystemTests.m */,
@@ -1172,7 +1172,7 @@
 				CBEC51D92976D54B009C0CE3 /* FilesystemTests.m in Sources */,
 				CB0AD75D29641B59002A3FB6 /* BatchTests.mm in Sources */,
 				CB68FABC2A3C4208005B2CDB /* PersistentDeviceIDTest.mm in Sources */,
-				09E313042BF363020081F219 /* CrossTalkTests.m in Sources */,
+				09E313042BF363020081F219 /* CrossTalkTests.mm in Sources */,
 				CB747D21299E5458003CA1B4 /* TimeTests.mm in Sources */,
 				0122C27129019CEF002D243C /* OtlpTraceEncodingTests.mm in Sources */,
 				09509B752ADFE9A900A358EC /* TracerTests.mm in Sources */,

Sources/BugsnagPerformance/Private/BugsnagPerformanceCrossTalkAPI.h

@@ -6,20 +6,61 @@
 //  Copyright © 2024 Bugsnag. All rights reserved.
 //
 
+// Bugsnag CrossTalk API
+//
+// CrossTalk is an Objective-C layer for sharing private APIs between Bugsnag libraries.
+// It allows client libraries to call internal functions of this one without the usual
+// worries of breaking downstream clients whenever internal code changes.
+//
+// This code should be duplicated and used as a template for any Bugsnag Objective-C
+// library that wants to expose its API to other Bugsnag libraries.
+//
+// NOTE: Your CrossTalk class name MUST be unique or else it will clash with another
+//       Bugsnag library's CrossTalk class name.
+//
+// See CrossTalkTests.mm for an example of how to use CrossTalk from a client library.
+// It contains a full example for how to set up a client library to call this one.
+
 #import <Foundation/Foundation.h>
-#import "SpanStackingHandler.h"
+#import <memory>
 
 NS_ASSUME_NONNULL_BEGIN
 
-@interface BugsnagPerformanceCrossTalkAPI : NSObject
+namespace bugsnag {
+class SpanStackingHandler;
+}
 
-#pragma mark Mandatory Methods
+@interface BugsnagPerformanceCrossTalkAPI : NSObject
 
 + (instancetype) sharedInstance;
 
-#pragma mark Configuration and Internal Functions
+/**
+ * Use the configure method to pass any information this CrossTalk API requires to function.
+ */
++ (void)configureWithSpanStackingHandler:(std::shared_ptr<bugsnag::SpanStackingHandler>) handler;
+
+@end
+
+/**
+ * A very permissive proxy that won't crash if a method or property doesn't exist.
+ *
+ * When returning instances of Bugsnag classes, wrap them in this proxy so that
+ * they don't crash when that class's API changes.
+ *
+ * WARNING: Returning internal classes is effectively creating a contract between Bugsnag libraries!
+ * Be VERY conservative about any internal class you expose, because its interfaces will effectively
+ * be "published", and changing a method's signature could break client libraries that use it.
+ *
+ * Adding/removing methods/properties is fine, but changing signatures WILL break things.
+ *
+ * Some ways to protect against breakage due to changed method signatures:
+ * - Convert to maps and arrays instead
+ * - Create custom classes designed specifically for library interop
+ * - Create versioned wrapper methods in the classes and access those instead (doStuffV1, doStuffV2, etc)
+ */
+@interface BugsnagPerformanceCrossTalkProxiedObject : NSProxy
 
-@property(nonatomic) std::shared_ptr<SpanStackingHandler> spanStackingHandler;
++ (instancetype) proxied:(id _Nullable)delegate;
 
 @end
 

Sources/BugsnagPerformance/Private/BugsnagPerformanceCrossTalkAPI.mm

@@ -7,12 +7,43 @@
 //
 
 #import "BugsnagPerformanceCrossTalkAPI.h"
+#import "SpanStackingHandler.h"
+#import "Utils.h"
 #import <objc/runtime.h>
 
+using namespace bugsnag;
+
+
+@interface BugsnagPerformanceCrossTalkAPI ()
+
+// Declare the things your API class needs here
+
+@property(nonatomic) std::shared_ptr<SpanStackingHandler> spanStackingHandler;
+
+@end
+
+
 @implementation BugsnagPerformanceCrossTalkAPI
 
+/**
+ * You'll call your configure method during start up.
+ */
++ (void)configureWithSpanStackingHandler:(std::shared_ptr<SpanStackingHandler>) handler {
+    BugsnagPerformanceCrossTalkAPI.sharedInstance.spanStackingHandler = handler;
+}
+
 #pragma mark Exposed API
 
+// Implement internal functions you want to expose to a CrossTalk client library here.
+// NOTE: ALWAYS ALWAYS ALWAYS check the mapping of every single API with a unit test!!!
+
+/**
+ * For unit tests only.
+ */
+- (NSString *)returnStringTestV1 {
+    return @"test";
+}
+
 /**
  * Return the current trace and span IDs as strings in a 2-entry array, or return nil if no current span exists.
  *
@@ -36,30 +67,53 @@ - (NSArray * _Nullable)getCurrentTraceAndSpanIdV1 {
 
 #pragma mark Internal Functionality
 
-static NSString *BSGUserInfoKeyMapped = @"mapped";
-static NSString *BSGUserInfoValueMappedYes = @"YES";
-static NSString *BSGUserInfoValueMappedNo = @"NO";
+static NSString *BSGUserInfoKeyIsSafeToCall = @"isSafeToCall";
+static NSString *BSGUserInfoKeyWillNOOP = @"willNOOP";
+
+static bool classImplementsSelector(Class cls, SEL selector) {
+    bool selectorExists = false;
+    unsigned int methodCount = 0;
+    Method *methods = class_copyMethodList(cls, &methodCount);
+    for (unsigned int i = 0; i < methodCount; i++) {
+        if (method_getName(methods[i]) == selector) {
+            selectorExists = true;
+            break;
+        }
+    }
+    free(methods);
+    return selectorExists;
+}
 
 /**
  * Map a named API to a method with the specified selector.
- * If an error occurs, the user info dictionary of the error will contain a field "mapped".
- * If "mapped" is "YES", then the selector has been mapped to a null implementation (does nothing, returns nil).
- * If "mapped" is "NO", then no mapping has occurred, and the method doesn't exist (alling it will result in no such selector).
+ *
+ * If an error occurs, the user info dictionary will contain the following NSNumber (boolean) fields:
+ *  - "isSafeToCall": If @(YES), this method is safe to call (it has an implementation). Otherwise, calling it WILL throw a selector-not-found exception.
+ *  - "willNOOP": If @(YES), calling the mapped method will no-op.
+ *
+ * Common scenarios:
+ *  - The host library isn't linked in: isSafeToCall = YES, willNOOP = YES
+ *  - apiName doesn't exist: isSafeToCall = YES, willNOOP = YES
+ *  - toSelector already exists: isSafeToCall = YES, willNOOP = NO
+ *  - Tried to map the same thing twice: isSafeToCall = YES, willNOOP = NO
+ *  - Selector signature clash: isSafeToCall = NO, willNOOP = NO
  */
 + (NSError *)mapAPINamed:(NSString * _Nonnull)apiName toSelector:(SEL)toSelector {
     NSError *err = nil;
     // By default, we map to a "do nothing" implementation in case we don't find a real one.
     SEL fromSelector = @selector(internal_doNothing);
 
-    // Note: ALWAYS ALWAYS ALWAYS check every single API mapping with a unit test!!!
-    if ([apiName isEqualToString:@"getCurrentTraceAndSpanIdV1"]) {
-        fromSelector = @selector(getCurrentTraceAndSpanIdV1);
+    // apiName should map to an existing method in this API
+    SEL apiSelector = NSSelectorFromString(apiName);
+    if (classImplementsSelector(self.class, apiSelector)) {
+        fromSelector = apiSelector;
     } else {
         err = [NSError errorWithDomain:@"com.bugsnag.BugsnagCocoaPerformance"
                                   code:0
                               userInfo:@{
             NSLocalizedDescriptionKey:[NSString stringWithFormat:@"No such API: %@", apiName],
-            BSGUserInfoKeyMapped:BSGUserInfoValueMappedYes
+            BSGUserInfoKeyIsSafeToCall:@YES,
+            BSGUserInfoKeyWillNOOP:@YES
         }];
     }
 
@@ -73,7 +127,8 @@ + (NSError *)mapAPINamed:(NSString * _Nonnull)apiName toSelector:(SEL)toSelector
                                        apiName,
                                        NSStringFromSelector(fromSelector),
                                        self.class],
-            BSGUserInfoKeyMapped:BSGUserInfoValueMappedNo
+            BSGUserInfoKeyIsSafeToCall:@NO,
+            BSGUserInfoKeyWillNOOP:@NO
         }];
     }
 
@@ -87,7 +142,8 @@ + (NSError *)mapAPINamed:(NSString * _Nonnull)apiName toSelector:(SEL)toSelector
                                        apiName,
                                        NSStringFromSelector(fromSelector),
                                        self.class],
-            BSGUserInfoKeyMapped:BSGUserInfoValueMappedNo
+            BSGUserInfoKeyIsSafeToCall:@NO,
+            BSGUserInfoKeyWillNOOP:@NO
         }];
     }
 
@@ -101,7 +157,23 @@ + (NSError *)mapAPINamed:(NSString * _Nonnull)apiName toSelector:(SEL)toSelector
                                        apiName,
                                        NSStringFromSelector(fromSelector),
                                        self.class],
-            BSGUserInfoKeyMapped:BSGUserInfoValueMappedNo
+            BSGUserInfoKeyIsSafeToCall:@NO,
+            BSGUserInfoKeyWillNOOP:@NO
+        }];
+    }
+
+    // Don't add a method that already exists
+    if (classImplementsSelector(self.class, toSelector)) {
+        return [NSError errorWithDomain:@"com.bugsnag.BugsnagCocoaPerformance"
+                                  code:0
+                              userInfo:@{
+            NSLocalizedDescriptionKey:[NSString stringWithFormat:
+                                       @"class_addMethod (while mapping api %@): Instance method %@ already exists in class %@",
+                                       apiName,
+                                       NSStringFromSelector(fromSelector),
+                                       self.class],
+            BSGUserInfoKeyIsSafeToCall:@YES,
+            BSGUserInfoKeyWillNOOP:@NO
         }];
     }
 
@@ -114,7 +186,8 @@ + (NSError *)mapAPINamed:(NSString * _Nonnull)apiName toSelector:(SEL)toSelector
                                        apiName,
                                        NSStringFromSelector(fromSelector),
                                        self.class],
-            BSGUserInfoKeyMapped:BSGUserInfoValueMappedNo
+            BSGUserInfoKeyIsSafeToCall:@NO,
+            BSGUserInfoKeyWillNOOP:@NO
         }];
     }
 
@@ -133,3 +206,99 @@ + (instancetype) sharedInstance {
 }
 
 @end
+
+
+#pragma mark BugsnagPerformanceCrossTalkProxiedObject
+
+@interface BugsnagPerformanceCrossTalkProxiedObject ()
+
+@property(nonatomic,strong) id delegate;
+
+@end
+
+@implementation BugsnagPerformanceCrossTalkProxiedObject
+
++ (instancetype) proxied:(id _Nullable)delegate {
+    BugsnagPerformanceCrossTalkProxiedObject *proxy = [BugsnagPerformanceCrossTalkProxiedObject alloc];
+    proxy.delegate = delegate;
+    return proxy;
+}
+
+// Allow faster access to ivars in these special cases
+#pragma clang diagnostic ignored "-Wdirect-ivar-access"
+
+- (void)forwardInvocation:(NSInvocation *)anInvocation {
+    if ([_delegate respondsToSelector:anInvocation.selector]) {
+        [anInvocation setTarget:_delegate];
+        [anInvocation invoke];
+    }
+}
+
+-(NSMethodSignature*)methodSignatureForSelector:(SEL)aSelector {
+    NSMethodSignature *sig = [_delegate methodSignatureForSelector:aSelector];
+    if (sig) {
+        return sig;
+    }
+
+    BSGLogWarning(@"CrossTalk: Tried to invoke unimplemented selector [%@] on proxied object %@",
+                  NSStringFromSelector(aSelector), [_delegate debugDescription]);
+
+    // Return a no-arg signature that's guaranteed to exist
+    return [NSObject instanceMethodSignatureForSelector:@selector(init)];
+}
+
+#pragma mark NSObject protocol (BugsnagPerformanceCrossTalkProxiedObject)
+
+- (Class)class {
+    return [_delegate class];
+}
+
+- (Class)superclass {
+    return [_delegate superclass];
+}
+
+- (BOOL)isKindOfClass:(Class)aClass {
+    return [_delegate isKindOfClass:aClass];
+}
+
+- (BOOL)isMemberOfClass:(Class)aClass {
+    return [_delegate isMemberOfClass:aClass];
+}
+
+- (BOOL)respondsToSelector:(SEL)aSelector {
+    // Be truthful about this
+    return [_delegate respondsToSelector:aSelector];
+}
+
+- (BOOL)conformsToProtocol:(Protocol *)aProtocol {
+    // Be truthful about this
+    return [_delegate conformsToProtocol:aProtocol];
+}
+
+- (BOOL)isEqual:(id)object {
+    return [_delegate isEqual:object];
+}
+
+- (NSUInteger)hash {
+    return [_delegate hash];
+}
+
+- (BOOL)isProxy {
+    return YES;
+}
+
+- (NSString *)description {
+    if (_delegate) {
+        return [_delegate description];
+    }
+    return super.description;
+}
+
+- (NSString *)debugDescription {
+    if (_delegate) {
+        return [_delegate debugDescription];
+    }
+    return super.debugDescription;
+}
+
+@end

Sources/BugsnagPerformance/Private/BugsnagPerformanceImpl.mm

@@ -108,7 +108,7 @@
     [worker_ earlySetup];
     [frameMetricsCollector_ earlySetup];
 
-    BugsnagPerformanceCrossTalkAPI.sharedInstance.spanStackingHandler = spanStackingHandler_;
+    [BugsnagPerformanceCrossTalkAPI configureWithSpanStackingHandler:spanStackingHandler_];
 }
 
 void BugsnagPerformanceImpl::configure(BugsnagPerformanceConfiguration *config) noexcept {