GULObjectSwizzler fix dealloc thread safety (#3300)

Author: maksymmalyhin

GoogleUtilities.podspec

@@ -78,7 +78,11 @@ 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/Private/*.h'
     iss.private_header_files = 'GoogleUtilities/ISASwizzler/Private/*.h'
+
+    # Disable ARC for GULSwizzledObject.
+    iss.requires_arc = ['GoogleUtilities/Common/*.h', 'GoogleUtilities/ISASwizzler/GULObjectSwizzler*.[mh]']
   end
 
   s.subspec 'MethodSwizzler' do |mss|

GoogleUtilities/Example/Tests/Swizzler/GULObjectSwizzlerTest.m

@@ -323,30 +323,52 @@ - (void)testRespondsToSelectorWorksEvenIfSwizzledProxyIsKVOd {
  *  swizzles a proxy object that we've also ISA Swizzled.
  */
 - (void)testRespondsToSelectorWorksEvenIfSwizzledProxyISASwizzledBySomeoneElse {
-  NSObject *object = [[NSObject alloc] init];
-  GULProxy *proxyObject = [GULProxy proxyWithDelegate:object];
+  Class generatedClass = nil;
+
+  @autoreleasepool {
+    NSObject *object = [[NSObject alloc] init];
+    GULProxy *proxyObject = [GULProxy proxyWithDelegate:object];
+
+    GULObjectSwizzler *swizzler = [[GULObjectSwizzler alloc] initWithObject:proxyObject];
+    [swizzler copySelector:@selector(donorDescription)
+                 fromClass:[GULObjectSwizzlerTest class]
+           isClassSelector:NO];
+    [swizzler swizzle];
+
+    // Someone else ISA Swizzles the same object after GULObjectSwizzler.
+    Class originalClass = object_getClass(proxyObject);
+    NSString *newClassName = [NSString
+        stringWithFormat:@"gul_test_%p_%@", proxyObject, NSStringFromClass(originalClass)];
+    generatedClass = objc_allocateClassPair(originalClass, newClassName.UTF8String, 0);
+    objc_registerClassPair(generatedClass);
+    object_setClass(proxyObject, generatedClass);
+
+    XCTAssertTrue([proxyObject respondsToSelector:@selector(donorDescription)]);
+    XCTAssertEqual([proxyObject performSelector:@selector(donorDescription)],
+                   @"SwizzledDonorDescription");
+  }
+
+  // A class generated by GULObjectSwizzler must not be disposed if there is its subclass.
+  XCTAssertNoThrow([generatedClass description]);
 
-  GULObjectSwizzler *swizzler = [[GULObjectSwizzler alloc] initWithObject:proxyObject];
-  [swizzler copySelector:@selector(donorDescription)
-               fromClass:[GULObjectSwizzlerTest class]
-         isClassSelector:NO];
-  [swizzler swizzle];
+  // Clean up.
+  objc_disposeClassPair(generatedClass);
+}
 
-  // Someone else ISA Swizzles the same object after GULObjectSwizzler.
-  Class originalClass = object_getClass(proxyObject);
-  NSString *newClassName =
-      [NSString stringWithFormat:@"gul_test_%p_%@", proxyObject, NSStringFromClass(originalClass)];
-  Class generatedClass = objc_allocateClassPair(originalClass, newClassName.UTF8String, 0);
-  objc_registerClassPair(generatedClass);
-  object_setClass(proxyObject, generatedClass);
+- (void)testMultiSwizzling {
+  NSObject *object = [[NSObject alloc] init];
 
-  XCTAssertTrue([proxyObject respondsToSelector:@selector(donorDescription)]);
-  XCTAssertEqual([proxyObject performSelector:@selector(donorDescription)],
-                 @"SwizzledDonorDescription");
+  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];
+  }
 
-  // Clean up.
-  object_setClass(proxyObject, originalClass);
-  objc_disposeClassPair(generatedClass);
+  XCTAssertNoThrow([object performSelector:@selector(donorDescription)]);
+  object = nil;
 }
 
 @end

GoogleUtilities/ISASwizzler/GULObjectSwizzler+Internal.h

@@ -0,0 +1,21 @@
+// Copyright 2019 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//      http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#import <GoogleUtilities/GULObjectSwizzler.h>
+
+@interface GULObjectSwizzler (Internal)
+
+- (void)swizzledObjectHasBeenDeallocatedWithGeneratedSubclass:(BOOL)isInstanceOfGeneratedSubclass;
+
+@end

GoogleUtilities/ISASwizzler/GULObjectSwizzler.m

@@ -12,7 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-#import "Private/GULObjectSwizzler.h"
+#import <GoogleUtilities/GULObjectSwizzler.h>
 
 #import <objc/runtime.h>
 
@@ -81,8 +81,8 @@ - (instancetype)initWithObject:(id)object {
     if (swizzledObject) {
       _swizzledObject = swizzledObject;
       _originalClass = object_getClass(object);
-      NSString *newClassName = [NSString
-          stringWithFormat:@"fir_%p_%@", swizzledObject, NSStringFromClass(_originalClass)];
+      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 {
@@ -150,8 +150,17 @@ - (void)swizzle {
   }
 }
 
-- (void)dealloc {
-  objc_disposeClassPair(_generatedClass);
+- (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.
+
+  // 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);
+  }
 }
 
 - (BOOL)isSwizzlingProxyObject {

GoogleUtilities/ISASwizzler/GULSwizzledObject.m

@@ -14,7 +14,7 @@
 
 #import <objc/runtime.h>
 
-#import "Private/GULObjectSwizzler.h"
+#import "GULObjectSwizzler+Internal.h"
 #import "Private/GULSwizzledObject.h"
 
 NSString *kSwizzlerAssociatedObjectKey = @"gul_objectSwizzler";
@@ -28,6 +28,7 @@ @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
@@ -61,4 +62,27 @@ - (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