From 41c3289a114f19e1f6040800f340c12459af776c Mon Sep 17 00:00:00 2001 From: Ian Cloutier Date: Thu, 9 Oct 2014 19:00:38 -0400 Subject: [PATCH] Fix retain cycles in ASDisplayNode and ASTableView --- AsyncDisplayKit.xcodeproj/project.pbxproj | 4 + AsyncDisplayKit/ASDisplayNode.h | 2 +- AsyncDisplayKit/ASTableView.m | 7 +- .../Private/ASDisplayNodeInternal.h | 2 +- AsyncDisplayKitTests/ASDisplayNodeTests.m | 26 ++++++ AsyncDisplayKitTests/ASTableViewTests.m | 86 +++++++++++++++++++ 6 files changed, 122 insertions(+), 5 deletions(-) create mode 100644 AsyncDisplayKitTests/ASTableViewTests.m diff --git a/AsyncDisplayKit.xcodeproj/project.pbxproj b/AsyncDisplayKit.xcodeproj/project.pbxproj index 6c7c967409..235966db87 100644 --- a/AsyncDisplayKit.xcodeproj/project.pbxproj +++ b/AsyncDisplayKit.xcodeproj/project.pbxproj @@ -120,6 +120,7 @@ 058D0A84195D060300B7D73C /* ASDisplayNodeExtraIvars.h in Headers */ = {isa = PBXBuildFile; fileRef = 058D0A45195D058D00B7D73C /* ASDisplayNodeExtraIvars.h */; settings = {ATTRIBUTES = (Public, ); }; }; 05A6D05A19D0EB64002DD95E /* ASDealloc2MainObject.h in Headers */ = {isa = PBXBuildFile; fileRef = 05A6D05819D0EB64002DD95E /* ASDealloc2MainObject.h */; settings = {ATTRIBUTES = (Public, ); }; }; 05A6D05B19D0EB64002DD95E /* ASDealloc2MainObject.m in Sources */ = {isa = PBXBuildFile; fileRef = 05A6D05919D0EB64002DD95E /* ASDealloc2MainObject.m */; settings = {COMPILER_FLAGS = "-fno-objc-arc"; }; }; + 3C9C128519E616EF00E942A0 /* ASTableViewTests.m in Sources */ = {isa = PBXBuildFile; fileRef = 3C9C128419E616EF00E942A0 /* ASTableViewTests.m */; settings = {COMPILER_FLAGS = "-fno-objc-arc"; }; }; 6BDC61F61979037800E50D21 /* AsyncDisplayKit.h in Headers */ = {isa = PBXBuildFile; fileRef = 6BDC61F51978FEA400E50D21 /* AsyncDisplayKit.h */; settings = {ATTRIBUTES = (Public, ); }; }; DB7121BCD50849C498C886FB /* libPods-AsyncDisplayKitTests.a in Frameworks */ = {isa = PBXBuildFile; fileRef = EFA731F0396842FF8AB635EE /* libPods-AsyncDisplayKitTests.a */; }; /* End PBXBuildFile section */ @@ -238,6 +239,7 @@ 058D0A45195D058D00B7D73C /* ASDisplayNodeExtraIvars.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = ASDisplayNodeExtraIvars.h; sourceTree = ""; }; 05A6D05819D0EB64002DD95E /* ASDealloc2MainObject.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = ASDealloc2MainObject.h; path = ../Details/ASDealloc2MainObject.h; sourceTree = ""; }; 05A6D05919D0EB64002DD95E /* ASDealloc2MainObject.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; name = ASDealloc2MainObject.m; path = ../Details/ASDealloc2MainObject.m; sourceTree = ""; }; + 3C9C128419E616EF00E942A0 /* ASTableViewTests.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = ASTableViewTests.m; sourceTree = ""; }; 6BDC61F51978FEA400E50D21 /* AsyncDisplayKit.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = AsyncDisplayKit.h; sourceTree = ""; }; EFA731F0396842FF8AB635EE /* libPods-AsyncDisplayKitTests.a */ = {isa = PBXFileReference; explicitFileType = archive.ar; includeInIndex = 0; path = "libPods-AsyncDisplayKitTests.a"; sourceTree = BUILT_PRODUCTS_DIR; }; FAD7085290B84183BD13BA1A /* Pods-AsyncDisplayKitTests.xcconfig */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text.xcconfig; name = "Pods-AsyncDisplayKitTests.xcconfig"; path = "Pods/Pods-AsyncDisplayKitTests.xcconfig"; sourceTree = ""; }; @@ -344,6 +346,7 @@ 058D0A30195D057000B7D73C /* ASDisplayNodeTestsHelper.h */, 058D0A31195D057000B7D73C /* ASDisplayNodeTestsHelper.m */, 058D0A32195D057000B7D73C /* ASMutableAttributedStringBuilderTests.m */, + 3C9C128419E616EF00E942A0 /* ASTableViewTests.m */, 058D0A33195D057000B7D73C /* ASTextNodeCoreTextAdditionsTests.m */, 058D0A34195D057000B7D73C /* ASTextNodeRendererTests.m */, 058D0A35195D057000B7D73C /* ASTextNodeShadowerTests.m */, @@ -688,6 +691,7 @@ 058D0A39195D057000B7D73C /* ASDisplayNodeAppearanceTests.m in Sources */, 058D0A41195D057000B7D73C /* ASTextNodeWordKernerTests.mm in Sources */, 058D0A40195D057000B7D73C /* ASTextNodeTests.m in Sources */, + 3C9C128519E616EF00E942A0 /* ASTableViewTests.m in Sources */, 058D0A38195D057000B7D73C /* ASDisplayLayerTests.m in Sources */, ); runOnlyForDeploymentPostprocessing = 0; diff --git a/AsyncDisplayKit/ASDisplayNode.h b/AsyncDisplayKit/ASDisplayNode.h index b58f21954b..8b0971f28b 100644 --- a/AsyncDisplayKit/ASDisplayNode.h +++ b/AsyncDisplayKit/ASDisplayNode.h @@ -236,7 +236,7 @@ /** * @abstract The receiver's supernode. */ -@property (nonatomic, readonly, assign) ASDisplayNode *supernode; +@property (nonatomic, readonly, weak) ASDisplayNode *supernode; /** @name Drawing and Updating the View */ diff --git a/AsyncDisplayKit/ASTableView.m b/AsyncDisplayKit/ASTableView.m index eb17dc58d3..c33feba12f 100644 --- a/AsyncDisplayKit/ASTableView.m +++ b/AsyncDisplayKit/ASTableView.m @@ -47,8 +47,8 @@ static BOOL _isInterceptedSelector(SEL sel) @end @implementation _ASTableViewProxy { - id _target; - ASTableView *_interceptor; + id __weak _target; + ASTableView * __weak _interceptor; } - (instancetype)initWithTarget:(id)target interceptor:(ASTableView *)interceptor @@ -140,7 +140,8 @@ static BOOL _isInterceptedSelector(SEL sel) - (void)setDelegate:(id)delegate { - ASDisplayNodeAssert(NO, @"ASTableView uses asyncDelegate, not UITableView's delegate property."); + // Our UIScrollView superclass sets its delegate to nil on dealloc. Only assert if we get a non-nil value here. + ASDisplayNodeAssert(delegate == nil, @"ASTableView uses asyncDelegate, not UITableView's delegate property."); } - (void)setAsyncDataSource:(id)asyncDataSource diff --git a/AsyncDisplayKit/Private/ASDisplayNodeInternal.h b/AsyncDisplayKit/Private/ASDisplayNodeInternal.h index 9085a060dd..f2d343b168 100644 --- a/AsyncDisplayKit/Private/ASDisplayNodeInternal.h +++ b/AsyncDisplayKit/Private/ASDisplayNodeInternal.h @@ -32,7 +32,7 @@ BOOL ASDisplayNodeSubclassOverridesSelector(Class subclass, SEL selector); @protected ASDN::RecursiveMutex _propertyLock; // Protects access to the _view, _pendingViewState, _subnodes, _supernode, _renderingSubnodes, and other properties which are accessed from multiple threads. - ASDisplayNode *_supernode; + ASDisplayNode * __weak _supernode; ASSentinel *_displaySentinel; ASSentinel *_replaceAsyncSentinel; diff --git a/AsyncDisplayKitTests/ASDisplayNodeTests.m b/AsyncDisplayKitTests/ASDisplayNodeTests.m index d7475a4878..10d805a747 100644 --- a/AsyncDisplayKitTests/ASDisplayNodeTests.m +++ b/AsyncDisplayKitTests/ASDisplayNodeTests.m @@ -783,6 +783,32 @@ static inline BOOL _CGPointEqualToPointWithEpsilon(CGPoint point1, CGPoint point [v release]; } +- (void)testAddingSubnodeDoesNotCreateRetainCycle +{ + ASTestDisplayNode *node = [[ASTestDisplayNode alloc] init]; + + __block BOOL didDealloc = NO; + node.willDeallocBlock = ^(ASDisplayNode *n){ + didDealloc = YES; + }; + + ASDisplayNode *subnode = [[ASDisplayNode alloc] init]; + + // verify initial + XCTAssertTrue(1 == node.retainCount, @"unexpected retain count:%tu", node.retainCount); + XCTAssertTrue(1 == subnode.retainCount, @"unexpected retain count:%tu", subnode.retainCount); + + [node addSubnode:subnode]; + XCTAssertTrue(2 == subnode.retainCount, @"node should retain subnode when added. retain count:%tu", node.retainCount); + XCTAssertTrue(1 == node.retainCount, @"subnode should not retain node when added. retain count:%tu", node.retainCount); + + [subnode release]; + XCTAssertTrue(1 == subnode.retainCount, @"subnode should be retained by node. retain count:%tu", subnode.retainCount); + + [node release]; + XCTAssertTrue(didDealloc, @"unexpected node lifetime:%@", node); +} + - (void)testMainThreadDealloc { __block BOOL didDealloc = NO; diff --git a/AsyncDisplayKitTests/ASTableViewTests.m b/AsyncDisplayKitTests/ASTableViewTests.m new file mode 100644 index 0000000000..8e44fdf880 --- /dev/null +++ b/AsyncDisplayKitTests/ASTableViewTests.m @@ -0,0 +1,86 @@ +/* Copyright (c) 2014-present, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + */ + +#import + +#import "ASTableView.h" + +@interface ASTestTableView : ASTableView +@property (atomic, copy) void (^willDeallocBlock)(ASTableView *tableView); +@end + +@implementation ASTestTableView + +- (void)dealloc +{ + if (_willDeallocBlock) { + _willDeallocBlock(self); + } + [super dealloc]; +} + +@end + +@interface ASTableViewTestDelegate : NSObject +@property (atomic, copy) void (^willDeallocBlock)(ASTableViewTestDelegate *delegate); +@end + +@implementation ASTableViewTestDelegate + +- (NSInteger)tableView:(UITableView *)tableView numberOfRowsInSection:(NSInteger)section +{ + return 0; +} + +- (ASCellNode *)tableView:(ASTableView *)tableView nodeForRowAtIndexPath:(NSIndexPath *)indexPath +{ + return nil; +} + +- (void)dealloc +{ + if (_willDeallocBlock) { + _willDeallocBlock(self); + } + [super dealloc]; +} + +@end + +@interface ASTableViewTests : XCTestCase +@end + +@implementation ASTableViewTests + +- (void)testTableViewDoesNotRetainItselfAndDelegate +{ + ASTestTableView *tableView = [[ASTestTableView alloc] initWithFrame:CGRectZero style:UITableViewStylePlain]; + + __block BOOL tableViewDidDealloc = NO; + tableView.willDeallocBlock = ^(ASTableView *v){ + tableViewDidDealloc = YES; + }; + + ASTableViewTestDelegate *delegate = [[ASTableViewTestDelegate alloc] init]; + + __block BOOL delegateDidDealloc = NO; + delegate.willDeallocBlock = ^(ASTableViewTestDelegate *d){ + delegateDidDealloc = YES; + }; + + tableView.asyncDataSource = delegate; + tableView.asyncDelegate = delegate; + + [delegate release]; + XCTAssertTrue(delegateDidDealloc, @"unexpected delegate lifetime:%@", delegate); + + XCTAssertNoThrow([tableView release], @"unexpected exception when deallocating table view:%@", tableView); + XCTAssertTrue(tableViewDidDealloc, @"unexpected table view lifetime:%@", tableView); +} + +@end