Update firebase_performance_swizzle_denylist to honour classes in the list for ISA Swizzling, NSURLConnection and UIViewController

FirebasePerformance/CHANGELOG.md

@@ -3,6 +3,8 @@
   falsely classifying 60 FPS frames as slow on ProMotion devices,
   while preserving dynamic frame rate support for tvOS.
 - [fixed] Fixed a crash caused due to ISA swizzling weak ivars. (#16469)
+- [fixed] Honor `firebase_performance_swizzle_denylist` when registering objects,
+  proxies and additional classes for swizzling. (#16469)
 
 # 12.16.0
 - [fixed] Fixed a crash in `FPRMemoryGaugeCollector` by collecting memory usage

FirebasePerformance/Sources/Instrumentation/FPRInstrument.m

@@ -12,6 +12,8 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+#import <objc/runtime.h>
+
 #import "FirebasePerformance/Sources/Instrumentation/FPRInstrument.h"
 #import "FirebasePerformance/Sources/Instrumentation/FPRInstrument_Private.h"
 
@@ -49,22 +51,38 @@ - (void)registerInstrumentors {
 }
 
 - (BOOL)isObjectInstrumentable:(id)object {
-  if ([object isKindOfClass:[NSOperation class]]) {
+  if (!object || (![object isProxy] && [object isKindOfClass:[NSOperation class]])) {
+    return NO;
+  }
+  Class objectClass = [object isProxy] ? object_getClass(object) : [object class];
+  return [self isClassInstrumentable:objectClass];
+}
+
+- (BOOL)isClassInstrumentable:(Class)aClass {
+  NSString *className = NSStringFromClass(aClass);
+  if (!className) {
+    // If the className is nil, it should be a no-op.
     return NO;
   }
+
+  if ([[FPRConfigurations sharedInstance].swizzleClassDenylist containsObject:className]) {
+    FPRLogInfo(kFPRSwizzleClassDenylisted,
+               @"Skipped swizzling %@ because it is listed in "
+               @"firebase_performance_swizzle_denylist.",
+               className);
+    return NO;
+  }
+
   return YES;
 }
 
 - (BOOL)registerClassInstrumentor:(FPRClassInstrumentor *)instrumentor {
   @synchronized(self) {
-    NSString *className = NSStringFromClass(instrumentor.instrumentedClass);
-    if ([[FPRConfigurations sharedInstance].swizzleClassDenylist containsObject:className]) {
-      FPRLogInfo(kFPRSwizzleClassDenylisted,
-                 @"Skipped swizzling %@ because it is listed in "
-                 @"firebase_performance_swizzle_denylist.",
-                 className);
+    // Check if it's in the denylist.
+    if (![self isClassInstrumentable:instrumentor.instrumentedClass]) {
       return NO;
     }
+
     if ([_instrumentedClasses containsObject:instrumentor.instrumentedClass] ||
         [instrumentor.instrumentedClass instancesRespondToSelector:@selector(gul_class)]) {
       return NO;

FirebasePerformance/Sources/Instrumentation/FPRInstrument_Private.h

@@ -18,6 +18,14 @@ NS_ASSUME_NONNULL_BEGIN
 
 @interface FPRInstrument ()
 
+/** Verifies whether the class should be instrumented. The decision is based on
+ * `FPRConfigurations`.
+ *
+ *  @param aClass The class to verify if it's in the denylist.
+ *  @return NO if the class is in the denylist, YES otherwise.
+ */
+- (BOOL)isClassInstrumentable:(Class)aClass;
+
 /** Registers an instrumentor for a class. Should be called by subclasses.
  *
  *  @param instrumentor The instrumentor to register.

FirebasePerformance/Sources/Instrumentation/Network/Delegates/FPRNSURLSessionDelegateInstrument.m

@@ -237,9 +237,14 @@ - (void)registerClass:(Class)aClass {
 
 - (void)registerObject:(id)object {
   dispatch_sync(GetInstrumentationQueue(), ^{
+    if (![self isObjectInstrumentable:object]) {
+      return;
+    }
+
     if ([object respondsToSelector:@selector(gul_class)]) {
       return;
     }
+
     FPRObjectInstrumentor *instrumentor = [[FPRObjectInstrumentor alloc] initWithObject:object];
 
     // Register the non-swizzled versions of these methods.
@@ -266,6 +271,10 @@ - (void)registerObject:(id)object {
 }
 
 - (void)registerProxy:(id)proxy {
+  if (![self isObjectInstrumentable:proxy]) {
+    return;
+  }
+
   [FPRProxyObjectHelper registerProxyObject:proxy
                                 forProtocol:@protocol(NSURLSessionDelegate)
                             varFoundHandler:^(id ivar) {

FirebasePerformance/Sources/Instrumentation/Network/FPRNSURLConnectionInstrument.m

@@ -209,6 +209,12 @@ - (void)dealloc {
 
 - (void)registerInstrumentors {
   dispatch_sync(GetInstrumentationQueue(), ^{
+    // Check if it's in the denylist. This is needed because it's
+    // a top level class, otherwise the FPRAssert is wrongly trigerred.
+    if (![self isClassInstrumentable:[NSURLConnection class]]) {
+      return;
+    }
+
     FPRClassInstrumentor *instrumentor =
         [[FPRClassInstrumentor alloc] initWithClass:[NSURLConnection class]];
 

FirebasePerformance/Sources/Instrumentation/Network/FPRNSURLSessionInstrument.m

@@ -708,6 +708,10 @@ - (void)registerInstrumentorForClass:(Class)aClass {
 }
 
 - (void)registerProxyObject:(id)proxy {
+  if (![self isObjectInstrumentable:proxy]) {
+    return;
+  }
+
   [FPRProxyObjectHelper registerProxyObject:proxy
                               forSuperclass:[NSURLSession class]
                             varFoundHandler:^(id ivar) {

FirebasePerformance/Sources/Instrumentation/UIKit/FPRUIViewControllerInstrument.m

@@ -97,6 +97,12 @@ void InstrumentViewDidDisappear(FPRUIViewControllerInstrument *instrument,
 
 - (void)registerInstrumentors {
   dispatch_sync(GetInstrumentationQueue(), ^{
+    // Check if it's in the denylist. This is needed because it's
+    // a top level class, otherwise the FPRAssert is wrongly trigerred.
+    if (![self isClassInstrumentable:[UIViewController class]]) {
+      return;
+    }
+
     FPRClassInstrumentor *instrumentor =
         [[FPRClassInstrumentor alloc] initWithClass:[UIViewController class]];
 

FirebasePerformance/Tests/Unit/FPRInstrumentTest.m

@@ -14,6 +14,7 @@
 
 #import <XCTest/XCTest.h>
 
+#import "FirebasePerformance/Sources/Configurations/FPRConfigurations.h"
 #import "FirebasePerformance/Sources/Instrumentation/FPRClassInstrumentor.h"
 #import "FirebasePerformance/Sources/Instrumentation/FPRInstrument.h"
 #import "FirebasePerformance/Sources/Instrumentation/FPRInstrument_Private.h"
@@ -63,6 +64,45 @@
   [instrument deregisterInstrumentors];
 }
 
+- (void)testIsObjectInstrumentableWithValidObject {
+  FPRInstrument *instrument = [[FPRInstrument alloc] init];
+  NSObject *object = [[NSObject alloc] init];
+  XCTAssertTrue([instrument isObjectInstrumentable:object]);
+}
+
+- (void)testIsObjectInstrumentableWithNSOperation {
+  FPRInstrument *instrument = [[FPRInstrument alloc] init];
+  NSOperation *operation = [[NSOperation alloc] init];
+  XCTAssertFalse([instrument isObjectInstrumentable:operation]);
+}
+
+- (void)testIsObjectInstrumentableWithNil {
+  FPRInstrument *instrument = [[FPRInstrument alloc] init];
+  XCTAssertFalse([instrument isObjectInstrumentable:nil]);
+}
+
+- (void)testIsObjectInstrumentableWithDenylistedClass {
+  FPRInstrument *instrument = [[FPRInstrument alloc] init];
+  NSObject *object = [[NSObject alloc] init];
+  id mockConfig = [OCMockObject partialMockForObject:[FPRConfigurations sharedInstance]];
+  [[[mockConfig stub] andReturn:@[ NSStringFromClass([NSObject class]) ]] swizzleClassDenylist];
+  XCTAssertFalse([instrument isObjectInstrumentable:object]);
+  [mockConfig stopMocking];
+}
+
+- (void)testRegisterClassInstrumentorWithDenylistedClass {
+  FPRInstrument *instrument = [[FPRInstrument alloc] init];
+  FPRClassInstrumentor *instrumentor =
+      [[FPRClassInstrumentor alloc] initWithClass:[NSObject class]];
+  id mockConfig = [OCMockObject partialMockForObject:[FPRConfigurations sharedInstance]];
+  [[[mockConfig stub] andReturn:@[ NSStringFromClass([NSObject class]) ]] swizzleClassDenylist];
+  BOOL success = [instrument registerClassInstrumentor:instrumentor];
+  XCTAssertFalse(success);
+  XCTAssertEqual(instrument.classInstrumentors.count, 0);
+  XCTAssertEqual(instrument.instrumentedClasses.count, 0);
+  [mockConfig stopMocking];
+}
+
 #pragma mark - Unswizzle based tests
 
 #if !SWIFT_PACKAGE

FirebasePerformance/Tests/Unit/Instruments/FPRNSURLConnectionInstrumentTest.m

@@ -18,11 +18,13 @@
 
 #import "FirebasePerformance/Tests/Unit/Instruments/FPRNSURLConnectionInstrumentTestDelegates.h"
 
+#import <OCMock/OCMock.h>
 #import <XCTest/XCTest.h>
 
 #import "FirebasePerformance/Sources/Configurations/FPRConfigurations+Private.h"
 #import "FirebasePerformance/Sources/Configurations/FPRConfigurations.h"
 #import "FirebasePerformance/Sources/FPRClient.h"
+#import "FirebasePerformance/Sources/Instrumentation/Network/Delegates/FPRNSURLConnectionDelegateInstrument.h"
 #import "FirebasePerformance/Sources/Instrumentation/Network/FPRNSURLConnectionInstrument.h"
 #import "FirebasePerformance/Sources/Public/FirebasePerformance/FIRPerformance.h"
 
@@ -168,6 +170,24 @@
   [instrument deregisterInstrumentors];
 }
 
+/** Tests that registerObject: skips swizzling when the delegate class is in swizzleClassDenylist.
+ */
+- (void)testRegisterObjectSkippedWhenClassIsDenylisted {
+  FPRNSURLConnectionCompleteTestDelegate *delegate =
+      [[FPRNSURLConnectionCompleteTestDelegate alloc] init];
+  FPRNSURLConnectionDelegateInstrument *delegateInstrument =
+      [[FPRNSURLConnectionDelegateInstrument alloc] init];
+  id mockConfig = [OCMockObject partialMockForObject:[FPRConfigurations sharedInstance]];
+  [[[mockConfig stub]
+      andReturn:@[ NSStringFromClass([FPRNSURLConnectionCompleteTestDelegate class]) ]]
+      swizzleClassDenylist];
+
+  [delegateInstrument registerObject:delegate];
+  XCTAssertFalse([delegate respondsToSelector:@selector(gul_class)]);
+
+  [mockConfig stopMocking];
+}
+
 /** Tests calling -initWithRequest:delegate: is wrapped and calls through with nil delegate. */
 - (void)testInitWithRequestAndNilDelegate {
   FPRNSURLConnectionInstrument *instrument = [[FPRNSURLConnectionInstrument alloc] init];
@@ -202,8 +222,8 @@
   [connection start];
   XCTAssertNotNil([FPRNetworkTrace networkTraceFromObject:connection]);
   [[NSRunLoop currentRunLoop] runUntilDate:[NSDate dateWithTimeIntervalSinceNow:1.0]];
   XCTAssertTrue(delegate.connectionDidFailWithErrorCalled);
   XCTAssertNil([FPRNetworkTrace networkTraceFromObject:connection]);
   [self.testServer start];
   [instrument deregisterInstrumentors];
 }
@@ -293,8 +313,8 @@
   [connection start];
   XCTAssertNotNil([FPRNetworkTrace networkTraceFromObject:connection]);
   [[NSRunLoop currentRunLoop] runUntilDate:[NSDate dateWithTimeIntervalSinceNow:2.0]];
   XCTAssertTrue(delegate.connectionDidFailWithErrorCalled);
   XCTAssertNil([FPRNetworkTrace networkTraceFromObject:connection]);
   [self.testServer start];
   [instrument deregisterInstrumentors];
 }

FirebasePerformance/Tests/Unit/Instruments/FPRNSURLSessionInstrumentTest.m

@@ -18,6 +18,7 @@
 
 #import "FirebasePerformance/Tests/Unit/Instruments/FPRNSURLSessionInstrumentTestDelegates.h"
 
+#import <OCMock/OCMock.h>
 #import <XCTest/XCTest.h>
 #import <objc/runtime.h>
 
@@ -608,8 +609,8 @@
   NSURLSessionDownloadTask *downloadTask = [session downloadTaskWithURL:URL];
   [downloadTask resume];
   [[NSRunLoop currentRunLoop] runUntilDate:[NSDate dateWithTimeIntervalSinceNow:5.0]];
   XCTAssertNil([FPRNetworkTrace networkTraceFromObject:downloadTask]);
   XCTAssertTrue(delegate.URLSessionDownloadTaskDidResumeAtOffsetExpectedTotalBytesCalled);
   [instrument deregisterInstrumentors];
 }
 
@@ -671,8 +672,8 @@
       [[FPRNSURLSessionDelegateProxy alloc] initWithDelegate:delegate];
   NSURLSessionConfiguration *configuration =
       [NSURLSessionConfiguration defaultSessionConfiguration];
   [NSURLSession sessionWithConfiguration:configuration delegate:proxyDelegate delegateQueue:nil];
   [NSURLSession sessionWithConfiguration:configuration delegate:proxyDelegate delegateQueue:nil];
   XCTAssertEqual(instrument.delegateInstrument.classInstrumentors.count, 1);
   XCTAssertEqual(instrument.delegateInstrument.instrumentedClasses.count, 1);
   XCTAssertTrue(
@@ -690,13 +691,70 @@
       [[FPRNSURLSessionDelegateWeakProxy alloc] initWithDelegate:delegate];
   NSURLSessionConfiguration *configuration =
       [NSURLSessionConfiguration defaultSessionConfiguration];
   [NSURLSession sessionWithConfiguration:configuration delegate:proxyDelegate delegateQueue:nil];
   [NSURLSession sessionWithConfiguration:configuration delegate:proxyDelegate delegateQueue:nil];
   XCTAssertEqual(instrument.delegateInstrument.classInstrumentors.count, 0);
   XCTAssertEqual(instrument.delegateInstrument.instrumentedClasses.count, 0);
   [instrument deregisterInstrumentors];
 }
 
+/** Tests that registerObject: skips swizzling when the delegate class is in swizzleClassDenylist.
+ */
+- (void)testRegisterObjectSkippedWhenClassIsDenylisted {
+  FPRNSURLSessionTestDelegate *delegate = [[FPRNSURLSessionTestDelegate alloc] init];
+  FPRNSURLSessionDelegateInstrument *delegateInstrument =
+      [[FPRNSURLSessionDelegateInstrument alloc] init];
+  id mockConfig = [OCMockObject partialMockForObject:[FPRConfigurations sharedInstance]];
+  [[[mockConfig stub] andReturn:@[ NSStringFromClass([FPRNSURLSessionTestDelegate class]) ]]
+      swizzleClassDenylist];
+
+  XCTAssertFalse([delegate respondsToSelector:@selector(URLSession:task:didCompleteWithError:)]);
+  [delegateInstrument registerObject:delegate];
+  XCTAssertFalse([delegate respondsToSelector:@selector(URLSession:task:didCompleteWithError:)]);
+  XCTAssertFalse([delegate respondsToSelector:@selector(gul_class)]);
+
+  [mockConfig stopMocking];
+}
+
+/** Tests that registerProxy: skips swizzling when the proxy class is in swizzleClassDenylist. */
+- (void)testRegisterProxySkippedWhenProxyClassIsDenylisted {
+  FPRNSURLSessionTestDelegate *delegate = [[FPRNSURLSessionTestDelegate alloc] init];
+  FPRNSURLSessionDelegateProxy *proxyDelegate =
+      [[FPRNSURLSessionDelegateProxy alloc] initWithDelegate:delegate];
+  FPRNSURLSessionDelegateInstrument *delegateInstrument =
+      [[FPRNSURLSessionDelegateInstrument alloc] init];
+  id mockConfig = [OCMockObject partialMockForObject:[FPRConfigurations sharedInstance]];
+  [[[mockConfig stub] andReturn:@[ NSStringFromClass(object_getClass(proxyDelegate)) ]]
+      swizzleClassDenylist];
+
+  XCTAssertFalse([delegate respondsToSelector:@selector(URLSession:task:didCompleteWithError:)]);
+  [delegateInstrument registerProxy:proxyDelegate];
+  XCTAssertFalse([delegate respondsToSelector:@selector(URLSession:task:didCompleteWithError:)]);
+  XCTAssertEqual(delegateInstrument.classInstrumentors.count, 0);
+
+  [mockConfig stopMocking];
+}
+
+/** Tests that registerProxy: skips the wrapped delegate when its class is in swizzleClassDenylist.
+ */
+- (void)testRegisterProxySkipsWrappedDelegateWhenWrappedClassIsDenylisted {
+  FPRNSURLSessionTestDelegate *delegate = [[FPRNSURLSessionTestDelegate alloc] init];
+  FPRNSURLSessionDelegateProxy *proxyDelegate =
+      [[FPRNSURLSessionDelegateProxy alloc] initWithDelegate:delegate];
+  FPRNSURLSessionDelegateInstrument *delegateInstrument =
+      [[FPRNSURLSessionDelegateInstrument alloc] init];
+  id mockConfig = [OCMockObject partialMockForObject:[FPRConfigurations sharedInstance]];
+  [[[mockConfig stub] andReturn:@[ NSStringFromClass([FPRNSURLSessionTestDelegate class]) ]]
+      swizzleClassDenylist];
+
+  XCTAssertFalse([delegate respondsToSelector:@selector(URLSession:task:didCompleteWithError:)]);
+  [delegateInstrument registerProxy:proxyDelegate];
+  XCTAssertFalse([delegate respondsToSelector:@selector(URLSession:task:didCompleteWithError:)]);
+  XCTAssertEqual(delegateInstrument.classInstrumentors.count, 0);
+
+  [mockConfig stopMocking];
+}
+
 /** Tests that the called delegate selector is wrapped and calls through. */
 - (void)testProxyDelegateURLSessionTaskDidCompleteWithError {
   [self.testServer stop];
@@ -710,7 +768,7 @@
   NSURLSessionConfiguration *configuration =
       [NSURLSessionConfiguration defaultSessionConfiguration];
   NSURLSession *session = [NSURLSession sessionWithConfiguration:configuration
                                                         delegate:proxyDelegate
                                                    delegateQueue:nil];
   NSURLSessionTask *task;
   @autoreleasepool {
@@ -736,7 +794,7 @@
   NSURLSessionConfiguration *configuration =
       [NSURLSessionConfiguration defaultSessionConfiguration];
   NSURLSession *session = [NSURLSession sessionWithConfiguration:configuration
                                                         delegate:proxyDelegate
                                                    delegateQueue:nil];
   NSURL *URL = [self.testServer.serverURL URLByAppendingPathComponent:@"testUpload"];
   NSMutableURLRequest *request = [NSMutableURLRequest requestWithURL:URL];
@@ -773,7 +831,7 @@
   NSURL *URL = [self.testServer.serverURL URLByAppendingPathComponent:@"testRedirect"];
   NSURLRequest *request = [NSURLRequest requestWithURL:URL];
   NSURLSession *session = [NSURLSession sessionWithConfiguration:configuration
                                                         delegate:proxyDelegate
                                                    delegateQueue:nil];
   NSURLSessionTask *task = [session dataTaskWithRequest:request];
   [task resume];
@@ -800,7 +858,7 @@
     NSURLSessionConfiguration *configuration =
         [NSURLSessionConfiguration defaultSessionConfiguration];
     NSURLSession *session = [NSURLSession sessionWithConfiguration:configuration
                                                           delegate:proxyDelegate
                                                      delegateQueue:nil];
     NSURL *URL = [self.testServer.serverURL URLByAppendingPathComponent:@"testBigDownload"];
     dataTask = [session dataTaskWithURL:URL];