From 6b81f5a7608aadcc498f0e9ce343e1b3f8872c4e Mon Sep 17 00:00:00 2001 From: Greg Bolsinga Date: Thu, 28 Mar 2019 18:37:06 -0700 Subject: [PATCH] Fix retain cycle with transaction operations (#1429) Add unit tests that help find cycles. `-testWeakWithSingleOperation` fails without the code fix applied. --- AsyncDisplayKit.xcodeproj/project.pbxproj | 4 + .../_ASAsyncTransactionContainer.mm | 2 +- Tests/ASTransactionTests.mm | 84 +++++++++++++++++++ 3 files changed, 89 insertions(+), 1 deletion(-) create mode 100644 Tests/ASTransactionTests.mm diff --git a/AsyncDisplayKit.xcodeproj/project.pbxproj b/AsyncDisplayKit.xcodeproj/project.pbxproj index c3617ea67..2d9476e5f 100644 --- a/AsyncDisplayKit.xcodeproj/project.pbxproj +++ b/AsyncDisplayKit.xcodeproj/project.pbxproj @@ -455,6 +455,7 @@ CCEDDDD9200C518800FFCD0A /* ASConfigurationTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = CCEDDDD8200C518800FFCD0A /* ASConfigurationTests.mm */; }; CCF18FF41D2575E300DF5895 /* NSIndexSet+ASHelpers.h in Headers */ = {isa = PBXBuildFile; fileRef = CC4981BA1D1C7F65004E13CC /* NSIndexSet+ASHelpers.h */; settings = {ATTRIBUTES = (Private, ); }; }; CCF1FF5E20C4785000AAD8FC /* ASLocking.h in Headers */ = {isa = PBXBuildFile; fileRef = CCF1FF5D20C4785000AAD8FC /* ASLocking.h */; settings = {ATTRIBUTES = (Public, ); }; }; + D933F041224AD17F00FF495E /* ASTransactionTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = D933F040224AD17F00FF495E /* ASTransactionTests.mm */; }; DB55C2671C641AE4004EDCF5 /* ASContextTransitioning.h in Headers */ = {isa = PBXBuildFile; fileRef = DB55C2651C641AE4004EDCF5 /* ASContextTransitioning.h */; settings = {ATTRIBUTES = (Public, ); }; }; DB7121BCD50849C498C886FB /* libPods-AsyncDisplayKitTests.a in Frameworks */ = {isa = PBXBuildFile; fileRef = EFA731F0396842FF8AB635EE /* libPods-AsyncDisplayKitTests.a */; }; DB78412E1C6BCE1600A9E2B4 /* _ASTransitionContext.mm in Sources */ = {isa = PBXBuildFile; fileRef = DB55C2601C6408D6004EDCF5 /* _ASTransitionContext.mm */; }; @@ -1000,6 +1001,7 @@ D3779BCFF841AD3EB56537ED /* Pods-AsyncDisplayKitTests.release.xcconfig */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text.xcconfig; name = "Pods-AsyncDisplayKitTests.release.xcconfig"; path = "Pods/Target Support Files/Pods-AsyncDisplayKitTests/Pods-AsyncDisplayKitTests.release.xcconfig"; sourceTree = ""; }; D785F6601A74327E00291744 /* ASScrollNode.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = ASScrollNode.h; sourceTree = ""; }; D785F6611A74327E00291744 /* ASScrollNode.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = ASScrollNode.mm; sourceTree = ""; }; + D933F040224AD17F00FF495E /* ASTransactionTests.mm */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.objcpp; path = ASTransactionTests.mm; sourceTree = ""; }; DB55C25F1C6408D6004EDCF5 /* _ASTransitionContext.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = _ASTransitionContext.h; path = ../_ASTransitionContext.h; sourceTree = ""; }; DB55C2601C6408D6004EDCF5 /* _ASTransitionContext.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; name = _ASTransitionContext.mm; path = ../_ASTransitionContext.mm; sourceTree = ""; }; DB55C2651C641AE4004EDCF5 /* ASContextTransitioning.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = ASContextTransitioning.h; sourceTree = ""; }; @@ -1389,6 +1391,7 @@ 83A7D95D1D446A6E00BF333E /* ASWeakMapTests.mm */, CC3B208D1C3F7D0A00798563 /* ASWeakSetTests.mm */, 695BE2541DC1245C008E6EA5 /* ASWrapperSpecSnapshotTests.mm */, + D933F040224AD17F00FF495E /* ASTransactionTests.mm */, 057D02C01AC0A66700C7AC3C /* AsyncDisplayKitTestHost */, CC583ABF1EF9BAB400134156 /* Common */, 058D09C6195D04C000B7D73C /* Supporting Files */, @@ -2292,6 +2295,7 @@ isa = PBXSourcesBuildPhase; buildActionMask = 2147483647; files = ( + D933F041224AD17F00FF495E /* ASTransactionTests.mm in Sources */, CCEDDDD9200C518800FFCD0A /* ASConfigurationTests.mm in Sources */, AE440175210FB7CF00B36DA2 /* ASTextKitFontSizeAdjusterTests.mm in Sources */, E51B78BF1F028ABF00E32604 /* ASLayoutFlatteningTests.mm in Sources */, diff --git a/Source/Details/Transactions/_ASAsyncTransactionContainer.mm b/Source/Details/Transactions/_ASAsyncTransactionContainer.mm index ed44231ce..48dc6be44 100644 --- a/Source/Details/Transactions/_ASAsyncTransactionContainer.mm +++ b/Source/Details/Transactions/_ASAsyncTransactionContainer.mm @@ -59,7 +59,7 @@ - (_ASAsyncTransaction *)asyncdisplaykit_asyncTransaction if (self == nil) { return; } - [transactions removeObject:completedTransaction]; + [self.asyncdisplaykit_asyncLayerTransactions removeObject:completedTransaction]; [self asyncdisplaykit_asyncTransactionContainerDidCompleteTransaction:completedTransaction]; }]; [transactions addObject:transaction]; diff --git a/Tests/ASTransactionTests.mm b/Tests/ASTransactionTests.mm new file mode 100644 index 000000000..370f05135 --- /dev/null +++ b/Tests/ASTransactionTests.mm @@ -0,0 +1,84 @@ +// +// ASTransactionTests.m +// AsyncDisplayKitTests +// +// Created by Greg Bolsinga on 3/26/19. +// Copyright © 2019 Pinterest. All rights reserved. +// + +#import "ASTestCase.h" +#import + +@interface ASTransactionTests : ASTestCase + +@end + +@implementation ASTransactionTests + +- (void)testWeak +{ + __weak _ASAsyncTransaction* weakTransaction = nil; + @autoreleasepool { + CALayer *layer = [[CALayer alloc] init]; + _ASAsyncTransaction *transaction = layer.asyncdisplaykit_asyncTransaction; + + weakTransaction = transaction; + layer = nil; + } + + // held by main transaction group + XCTAssertNotNil(weakTransaction); + + // run so that transaction group drains. + static NSTimeInterval delay = 0.1; + [[NSRunLoop currentRunLoop] runUntilDate:[NSDate dateWithTimeIntervalSinceNow:delay]]; + + XCTAssertNil(weakTransaction); +} + +- (void)testWeakWhenCancelled +{ + __weak _ASAsyncTransaction* weakTransaction = nil; + @autoreleasepool { + CALayer *layer = [[CALayer alloc] init]; + _ASAsyncTransaction *transaction = layer.asyncdisplaykit_asyncTransaction; + + weakTransaction = transaction; + + [layer asyncdisplaykit_cancelAsyncTransactions]; + layer = nil; + } + + XCTAssertNil(weakTransaction); +} + +- (void)testWeakWithSingleOperation +{ + __weak _ASAsyncTransaction* weakTransaction = nil; + @autoreleasepool { + CALayer *layer = [[CALayer alloc] init]; + _ASAsyncTransaction *transaction = layer.asyncdisplaykit_asyncTransaction; + + [transaction addOperationWithBlock:^id _Nullable{ + return nil; + } priority:1 + queue:dispatch_get_main_queue() + completion:^(id _Nullable value, BOOL canceled) { + ; + }]; + + weakTransaction = transaction; + layer = nil; + } + + // held by main transaction group + XCTAssertNotNil(weakTransaction); + + // run so that transaction group drains. + static NSTimeInterval delay = 0.1; + [[NSRunLoop currentRunLoop] runUntilDate:[NSDate dateWithTimeIntervalSinceNow:delay]]; + + XCTAssertNil(weakTransaction); +} + +@end