Refactor GULSwizzledObject to ARC to unblock SwiftPM support

GoogleUtilities.podspec

@@ -95,9 +95,6 @@ other Google CocoaPods. They're not intended for direct public usage.
   s.subspec 'ISASwizzler' do |iss|
     iss.source_files = 'GoogleUtilities/ISASwizzler/**/*.[mh]', 'GoogleUtilities/Common/*.h'
     iss.public_header_files = 'GoogleUtilities/ISASwizzler/Public/GoogleUtilities/*.h'
-
-    # Disable ARC for GULSwizzledObject.
-    iss.requires_arc = ['GoogleUtilities/Common/*.h', 'GoogleUtilities/ISASwizzler/GULObjectSwizzler*.[mh]']
   end
 
   s.subspec 'MethodSwizzler' do |mss|

GoogleUtilities/CHANGELOG.md

@@ -2,6 +2,7 @@
 - All APIs are now public. All CocoaPods private headers are transitioned to public. Note that
 - GoogleUtilities may have frequent breaking changes than Firebase. (#6588)
 - Fixed writing heartbeat to disk on tvOS devices. (#6658)
+- Refactor `GULSwizzledObject` to ARC to unblock SwiftPM support. (#5862)
 
 # 6.7.1
 - Fix import regression when mixing 6.7.0 with earlier Firebase versions. (#6047)

GoogleUtilities/ISASwizzler/GULObjectSwizzler.m

@@ -75,19 +75,25 @@ + (nullable id)getAssociatedObject:(id)object key:(NSString *)key {
  *  @return An instance of this class.
  */
 - (instancetype)initWithObject:(id)object {
+  if (object == nil) {
+    return nil;
+  }
+
+  GULObjectSwizzler *existingSwizzler =
+      [[self class] getAssociatedObject:object key:kSwizzlerAssociatedObjectKey];
+  if ([existingSwizzler isKindOfClass:[GULObjectSwizzler class]]) {
+    // The object has been swizzled already, no need to swizzle again.
+    return existingSwizzler;
+  }
+
   self = [super init];
   if (self) {
-    __strong id swizzledObject = object;
-    if (swizzledObject) {
-      _swizzledObject = swizzledObject;
-      _originalClass = object_getClass(object);
-      NSString *newClassName = [NSString stringWithFormat:@"fir_%@_%@", [[NSUUID UUID] UUIDString],
-                                                          NSStringFromClass(_originalClass)];
-      _generatedClass = objc_allocateClassPair(_originalClass, newClassName.UTF8String, 0);
-      NSAssert(_generatedClass, @"Wasn't able to allocate the class pair.");
-    } else {
-      return nil;
-    }
+    _swizzledObject = object;
+    _originalClass = object_getClass(object);
+    NSString *newClassName = [NSString stringWithFormat:@"fir_%@_%@", [[NSUUID UUID] UUIDString],
+                                                        NSStringFromClass(_originalClass)];
+    _generatedClass = objc_allocateClassPair(_originalClass, newClassName.UTF8String, 0);
+    NSAssert(_generatedClass, @"Wasn't able to allocate the class pair.");
   }
   return self;
 }
@@ -98,10 +104,9 @@ - (void)copySelector:(SEL)selector fromClass:(Class)aClass isClassSelector:(BOOL
                                   : class_getInstanceMethod(aClass, selector);
   Class targetClass = isClassSelector ? object_getClass(_generatedClass) : _generatedClass;
   IMP implementation = method_getImplementation(method);
+
   const char *typeEncoding = method_getTypeEncoding(method);
-  BOOL success __unused = class_addMethod(targetClass, selector, implementation, typeEncoding);
-  NSAssert(success, @"Unable to add selector %@ to class %@", NSStringFromSelector(selector),
-           NSStringFromClass(targetClass));
+  class_replaceMethod(targetClass, selector, implementation, typeEncoding);
 }
 
 - (void)setAssociatedObjectWithKey:(NSString *)key
@@ -123,11 +128,20 @@ - (nullable id)getAssociatedObjectForKey:(NSString *)key {
 
 - (void)swizzle {
   __strong id swizzledObject = _swizzledObject;
+
+  GULObjectSwizzler *existingSwizzler =
+      [[self class] getAssociatedObject:swizzledObject key:kSwizzlerAssociatedObjectKey];
+  if (existingSwizzler != nil) {
+    NSAssert(existingSwizzler == self, @"The swizzled object has a different swizzler.");
+    // The object has been swizzled already.
+    return;
+  }
+
   if (swizzledObject) {
     [GULObjectSwizzler setAssociatedObject:swizzledObject
                                        key:kSwizzlerAssociatedObjectKey
                                      value:self
-                               association:GUL_ASSOCIATION_RETAIN_NONATOMIC];
+                               association:GUL_ASSOCIATION_RETAIN];
 
     [GULSwizzledObject copyDonorSelectorsUsingObjectSwizzler:self];
 
@@ -144,16 +158,36 @@ - (void)swizzle {
   }
 }
 
-- (void)swizzledObjectHasBeenDeallocatedWithGeneratedSubclass:(BOOL)isInstanceOfGeneratedSubclass {
-  // If the swizzled object had a different class, it most likely indicates that the object was
-  // ISA swizzled one more time. In this case it is not safe to dispose the generated class. We
-  // will have to keep it to prevent a crash.
+- (void)dealloc {
+  if (_generatedClass) {
+    if (_swizzledObject == nil) {
+      // The swizzled object has been deallocated already, so the generated class can be disposed
+      // now.
+      objc_disposeClassPair(_generatedClass);
+      return;
+    }
+
+    // GULSwizzledObject is retained by the swizzled object which means that the swizzled object is
+    // being deallocated now. Let's see if we should schedule the generated class disposal.
+
+    // If the swizzled object has a different class, it most likely indicates that the object was
+    // ISA swizzled one more time. In this case it is not safe to dispose the generated class. We
+    // will have to keep it to prevent a crash.
 
-  // TODO: Consider adding a flag that can be set by the host application to dispose the class pair
-  // unconditionally. It may be used by apps that use ISA Swizzling themself and are confident in
-  // disposing their subclasses.
-  if (isInstanceOfGeneratedSubclass) {
-    objc_disposeClassPair(_generatedClass);
+    // TODO: Consider adding a flag that can be set by the host application to dispose the class
+    // pair unconditionally. It may be used by apps that use ISA Swizzling themself and are
+    // confident in disposing their subclasses.
+    BOOL isSwizzledObjectInstanceOfGeneratedClass =
+        object_getClass(_swizzledObject) == _generatedClass;
+
+    if (isSwizzledObjectInstanceOfGeneratedClass) {
+      Class generatedClass = _generatedClass;
+
+      // Schedule the generated class disposal after the swizzled object has been deallocated.
+      dispatch_async(dispatch_get_main_queue(), ^{
+        objc_disposeClassPair(generatedClass);
+      });
+    }
   }
 }
 

GoogleUtilities/ISASwizzler/GULSwizzledObject.m

@@ -28,7 +28,6 @@ @implementation GULSwizzledObject
 + (void)copyDonorSelectorsUsingObjectSwizzler:(GULObjectSwizzler *)objectSwizzler {
   [objectSwizzler copySelector:@selector(gul_objectSwizzler) fromClass:self isClassSelector:NO];
   [objectSwizzler copySelector:@selector(gul_class) fromClass:self isClassSelector:NO];
-  [objectSwizzler copySelector:@selector(dealloc) fromClass:self isClassSelector:NO];
 
   // This is needed because NSProxy objects usually override -[NSObjectProtocol respondsToSelector:]
   // and ask this question to the underlying object. Since we don't swizzle the underlying object
@@ -62,27 +61,4 @@ - (BOOL)respondsToSelector:(SEL)aSelector {
   return [gulClass instancesRespondToSelector:aSelector] || [super respondsToSelector:aSelector];
 }
 
-- (void)dealloc {
-  // We need to make sure the swizzler is deallocated after the swizzled object to do the clean up
-  // only when the swizzled object is not used.
-  GULObjectSwizzler *swizzler = nil;
-  BOOL isInstanceOfGeneratedClass = NO;
-
-  @autoreleasepool {
-    Class generatedClass = [self gul_class];
-    isInstanceOfGeneratedClass = object_getClass(self) == generatedClass;
-
-    swizzler = [[self gul_objectSwizzler] retain];
-    [GULObjectSwizzler setAssociatedObject:self
-                                       key:kSwizzlerAssociatedObjectKey
-                                     value:nil
-                               association:GUL_ASSOCIATION_RETAIN_NONATOMIC];
-  }
-
-  [super dealloc];
-
-  [swizzler swizzledObjectHasBeenDeallocatedWithGeneratedSubclass:isInstanceOfGeneratedClass];
-  [swizzler release];
-}
-
 @end

GoogleUtilities/Tests/Unit/Swizzler/GULObjectSwizzlerTest.m

@@ -324,12 +324,14 @@ - (void)testRespondsToSelectorWorksEvenIfSwizzledProxyIsKVOd {
  */
 - (void)testRespondsToSelectorWorksEvenIfSwizzledProxyISASwizzledBySomeoneElse {
   Class generatedClass = nil;
+  __weak GULObjectSwizzler *weakSwizzler;
 
   @autoreleasepool {
     NSObject *object = [[NSObject alloc] init];
     GULProxy *proxyObject = [GULProxy proxyWithDelegate:object];
 
     GULObjectSwizzler *swizzler = [[GULObjectSwizzler alloc] initWithObject:proxyObject];
+    weakSwizzler = swizzler;
     [swizzler copySelector:@selector(donorDescription)
                  fromClass:[GULObjectSwizzlerTest class]
            isClassSelector:NO];
@@ -348,27 +350,132 @@ - (void)testRespondsToSelectorWorksEvenIfSwizzledProxyISASwizzledBySomeoneElse {
                    @"SwizzledDonorDescription");
   }
 
-  // A class generated by GULObjectSwizzler must not be disposed if there is its subclass.
-  XCTAssertNoThrow([generatedClass description]);
+  // Clean up.
+  objc_disposeClassPair(generatedClass);
+}
+
+- (void)testSwizzlerDoesntDisposeGeneratedClassWhenObjectIsISASwizzledBySomeoneElse {
+  Class generatedClass = nil;
+  __weak GULObjectSwizzler *weakSwizzler;
+
+  XCTestExpectation *swizzlerDeallocatedExpectation =
+      [self expectationWithDescription:@"swizzlerDeallocatedExpectation"];
+
+  @autoreleasepool {
+    NSObject *object = [[NSObject alloc] init];
+
+    @autoreleasepool {
+      GULObjectSwizzler *swizzler = [[GULObjectSwizzler alloc] initWithObject:object];
+      weakSwizzler = swizzler;
+      [swizzler copySelector:@selector(donorDescription)
+                   fromClass:[GULObjectSwizzlerTest class]
+             isClassSelector:NO];
+      [swizzler swizzle];
+    }
+
+    // Someone else ISA Swizzles the same object after GULObjectSwizzler.
+    Class originalClass = object_getClass(object);
+    NSString *newClassName =
+        [NSString stringWithFormat:@"gul_test_%p_%@", object, NSStringFromClass(originalClass)];
+    generatedClass = objc_allocateClassPair(originalClass, newClassName.UTF8String, 0);
+    objc_registerClassPair(generatedClass);
+    object_setClass(object, generatedClass);
+
+    // Release GULObjectSwizzler
+    [GULObjectSwizzler setAssociatedObject:object
+                                       key:kSwizzlerAssociatedObjectKey
+                                     value:nil
+                               association:GUL_ASSOCIATION_RETAIN];
+
+    // Wait for a while
+    dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(1.0 * NSEC_PER_SEC)),
+                   dispatch_get_main_queue(), ^{
+                     [swizzlerDeallocatedExpectation fulfill];
+                   });
+
+    [self waitForExpectations:@[ swizzlerDeallocatedExpectation ] timeout:2];
+
+    XCTAssertNil(weakSwizzler);
+    // A class generated by GULObjectSwizzler must not be disposed if there is its subclass.
+    XCTAssertNoThrow([generatedClass description]);
+  }
 
   // Clean up.
   objc_disposeClassPair(generatedClass);
 }
 
+// The test is disabled because in the case of success it should crash with SIGABRT, so it is not
+// suitable for CI.
+- (void)disabledForCI_testSwizzlerDisposesGeneratedClass {
+  __weak GULObjectSwizzler *weakSwizzler;
+
+  XCTestExpectation *swizzlerDeallocatedExpectation =
+      [self expectationWithDescription:@"swizzlerDeallocatedExpectation"];
+
+  @autoreleasepool {
+    NSObject *object = [[NSObject alloc] init];
+
+    @autoreleasepool {
+      GULObjectSwizzler *swizzler = [[GULObjectSwizzler alloc] initWithObject:object];
+      weakSwizzler = swizzler;
+      [swizzler copySelector:@selector(donorDescription)
+                   fromClass:[GULObjectSwizzlerTest class]
+             isClassSelector:NO];
+      [swizzler swizzle];
+    }
+
+    // Release GULObjectSwizzler
+    [GULObjectSwizzler setAssociatedObject:object
+                                       key:kSwizzlerAssociatedObjectKey
+                                     value:nil
+                               association:GUL_ASSOCIATION_RETAIN];
+
+    // Wait for a while until GULObjectSwizzler has disposed the generated class.
+    dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(1.0 * NSEC_PER_SEC)),
+                   dispatch_get_main_queue(), ^{
+                     [swizzlerDeallocatedExpectation fulfill];
+                   });
+
+    [self waitForExpectations:@[ swizzlerDeallocatedExpectation ] timeout:2];
+
+    XCTAssertNil(weakSwizzler);
+
+    // Must crash here with SIGABRT.
+    XCTAssertThrows([object description]);
+    XCTFail(@"The test must have crashed on the previous line.");
+  }
+}
+
 - (void)testMultiSwizzling {
   NSObject *object = [[NSObject alloc] init];
 
-  NSInteger swizzleCount = 10;
-  for (NSInteger i = 0; i < swizzleCount; i++) {
-    GULObjectSwizzler *swizzler = [[GULObjectSwizzler alloc] initWithObject:object];
-    [swizzler copySelector:@selector(donorDescription)
-                 fromClass:[GULObjectSwizzlerTest class]
-           isClassSelector:NO];
-    [swizzler swizzle];
+  __weak GULObjectSwizzler *existingSwizzler;
+
+  // Use @autoreleasepool to make the memory management in the test more deterministic.
+  @autoreleasepool {
+    NSInteger swizzleCount = 10;
+    for (NSInteger i = 0; i < swizzleCount; i++) {
+      GULObjectSwizzler *swizzler = [[GULObjectSwizzler alloc] initWithObject:object];
+
+      if (i > 0) {
+        XCTAssertEqualObjects(swizzler, existingSwizzler,
+                              @"There must be a single swizzler per object.");
+      } else {
+        existingSwizzler = swizzler;
+      }
+
+      [swizzler copySelector:@selector(donorDescription)
+                   fromClass:[GULObjectSwizzlerTest class]
+             isClassSelector:NO];
+      [swizzler swizzle];
+    }
+
+    XCTAssertNoThrow([object performSelector:@selector(donorDescription)]);
+    object = nil;
   }
 
-  XCTAssertNoThrow([object performSelector:@selector(donorDescription)]);
-  object = nil;
+  XCTAssertNil(existingSwizzler,
+               @"GULObjectSwizzler must be deallocated after the object deallocation.");
 }
 
 @end