From 3668f45286a56a87d200aea0299caee9d2e4a880 Mon Sep 17 00:00:00 2001 From: Michael Schneider Date: Tue, 22 Mar 2016 08:09:59 -0700 Subject: [PATCH 1/6] Prevent deallocation of asyncDataSource and asyncDelegate in ASCollectionView and ASTableView Grab a strong reference for asyncDataSource and asyncDelegate in ASCollectionView and ASTableView before executing the range update to be sure they are not going away while executing the range update. This can happen in range updates while going back in the view controller hierarchy --- AsyncDisplayKit/ASCollectionView.mm | 9 +++++++- AsyncDisplayKit/ASTableView.mm | 9 +++++++- AsyncDisplayKit/Details/ASRangeController.mm | 23 ++++++++++++++++++- ...SRangeControllerUpdateRangeProtocol+Beta.h | 11 ++++++++- 4 files changed, 48 insertions(+), 4 deletions(-) diff --git a/AsyncDisplayKit/ASCollectionView.mm b/AsyncDisplayKit/ASCollectionView.mm index ae1f579232..a91c84fef3 100644 --- a/AsyncDisplayKit/ASCollectionView.mm +++ b/AsyncDisplayKit/ASCollectionView.mm @@ -1139,7 +1139,14 @@ static NSString * const kCellReuseIdentifier = @"_ASCollectionViewCell"; // Updating the visible node index paths only for not range managed nodes. Range managed nodes will get their // their update in the layout pass if (![node supportsRangeManagedInterfaceState]) { - [_rangeController visibleNodeIndexPathsDidChangeWithScrollDirection:self.scrollDirection]; + // Grab a strong reference for data source and delegate to be sure they are not going away while executing + // the range update. This can happen in range updates while going back in the view controller hierarchy + __block id asyncDataSource = _asyncDataSource; + __block id asyncDelegate = _asyncDelegate; + [_rangeController scheduleRangeUpdateCompletion:^{ + asyncDataSource = nil; + asyncDelegate = nil; + }]; } } diff --git a/AsyncDisplayKit/ASTableView.mm b/AsyncDisplayKit/ASTableView.mm index d8d9037d36..9e5ad025c7 100644 --- a/AsyncDisplayKit/ASTableView.mm +++ b/AsyncDisplayKit/ASTableView.mm @@ -1072,7 +1072,14 @@ static NSString * const kCellReuseIdentifier = @"_ASTableViewCell"; // Updating the visible node index paths only for not range managed nodes. Range managed nodes will get their // their update in the layout pass if (![node supportsRangeManagedInterfaceState]) { - [_rangeController visibleNodeIndexPathsDidChangeWithScrollDirection:self.scrollDirection]; + // Grab a strong reference for data source and delegate to be sure they are not going away while executing + // the range update. This can happen in range updates while going back in the view controller hierarchy + __block id asyncDataSource = _asyncDataSource; + __block id asyncDelegate = _asyncDelegate; + [_rangeController scheduleRangeUpdateCompletion:^{ + asyncDataSource = nil; + asyncDelegate = nil; + }]; } } diff --git a/AsyncDisplayKit/Details/ASRangeController.mm b/AsyncDisplayKit/Details/ASRangeController.mm index 7935564a3d..c9fffb4cd0 100644 --- a/AsyncDisplayKit/Details/ASRangeController.mm +++ b/AsyncDisplayKit/Details/ASRangeController.mm @@ -27,6 +27,7 @@ BOOL _didUpdateCurrentRange; BOOL _didRegisterForNotifications; CFAbsoluteTime _pendingDisplayNodesTimestamp; + NSMutableArray *_scheduledRangeUpdateCompletionBlocks; } @end @@ -46,6 +47,7 @@ static UIApplicationState __ApplicationState = UIApplicationStateActive; _rangeIsValid = YES; _currentRangeMode = ASLayoutRangeModeInvalid; _didUpdateCurrentRange = NO; + _scheduledRangeUpdateCompletionBlocks = [NSMutableArray array]; [[[self class] allRangeControllersWeakSet] addObject:self]; @@ -111,6 +113,15 @@ static UIApplicationState __ApplicationState = UIApplicationStateActive; - (void)scheduleRangeUpdate { + [self scheduleRangeUpdateCompletion:nil]; +} + +- (void)scheduleRangeUpdateCompletion:(void (^)(void))completion +{ + if (completion) { + [_scheduledRangeUpdateCompletionBlocks addObject:completion]; + } + if (_queuedRangeUpdate) { return; } @@ -118,8 +129,13 @@ static UIApplicationState __ApplicationState = UIApplicationStateActive; // coalesce these events -- handling them multiple times per runloop is noisy and expensive _queuedRangeUpdate = YES; + __block id dataSource = _dataSource; + __block id delegate = _delegate; dispatch_async(dispatch_get_main_queue(), ^{ - [self performRangeUpdate]; + [self _updateVisibleNodeIndexPaths]; + + dataSource = nil; + delegate = nil; }); } @@ -320,6 +336,11 @@ static UIApplicationState __ApplicationState = UIApplicationStateActive; _rangeIsValid = YES; _queuedRangeUpdate = NO; + for (void (^completionBlock)(void) in _scheduledRangeUpdateCompletionBlocks) { + completionBlock(); + } + [_scheduledRangeUpdateCompletionBlocks removeAllObjects]; + #if ASRangeControllerLoggingEnabled // NSSet *visibleNodePathsSet = [NSSet setWithArray:visibleNodePaths]; // BOOL setsAreEqual = [visibleIndexPaths isEqualToSet:visibleNodePathsSet]; diff --git a/AsyncDisplayKit/Details/ASRangeControllerUpdateRangeProtocol+Beta.h b/AsyncDisplayKit/Details/ASRangeControllerUpdateRangeProtocol+Beta.h index 4f34cd1cef..f9fcbf77b7 100644 --- a/AsyncDisplayKit/Details/ASRangeControllerUpdateRangeProtocol+Beta.h +++ b/AsyncDisplayKit/Details/ASRangeControllerUpdateRangeProtocol+Beta.h @@ -49,6 +49,12 @@ */ - (void)updateCurrentRangeWithMode:(ASLayoutRangeMode)rangeMode; +/** + * Schedule a range update and call the completion block if finished. This drives updating the working + * ranges, and triggering their actions. + */ +- (void)scheduleRangeUpdateCompletion:(void (^)(void))completion; + @end @@ -64,7 +70,10 @@ @interface ASViewController (ASRangeControllerUpdateRangeProtocol) -/// Automatically adjust range mode based on view events if the containing node confirms to the ASRangeControllerUpdateRangeProtocol +/** + * Automatically adjust range mode based on view events if the containing node confirms to the + * ASRangeControllerUpdateRangeProtocol + */ @property (nonatomic, assign) BOOL automaticallyAdjustRangeModeBasedOnViewEvents; @end From 41362fca3942b71cb092376e5baaa78652d1e19b Mon Sep 17 00:00:00 2001 From: Michael Schneider Date: Tue, 22 Mar 2016 12:43:09 -0700 Subject: [PATCH 2/6] Move call of range update completion blocks to consider early returns in _updateVisibleNodeIndexPaths --- AsyncDisplayKit/Details/ASRangeController.mm | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/AsyncDisplayKit/Details/ASRangeController.mm b/AsyncDisplayKit/Details/ASRangeController.mm index c9fffb4cd0..10ea6c2a0f 100644 --- a/AsyncDisplayKit/Details/ASRangeController.mm +++ b/AsyncDisplayKit/Details/ASRangeController.mm @@ -134,6 +134,11 @@ static UIApplicationState __ApplicationState = UIApplicationStateActive; dispatch_async(dispatch_get_main_queue(), ^{ [self _updateVisibleNodeIndexPaths]; + for (void (^completionBlock)(void) in _scheduledRangeUpdateCompletionBlocks) { + completionBlock(); + } + [_scheduledRangeUpdateCompletionBlocks removeAllObjects]; + dataSource = nil; delegate = nil; }); @@ -336,11 +341,6 @@ static UIApplicationState __ApplicationState = UIApplicationStateActive; _rangeIsValid = YES; _queuedRangeUpdate = NO; - for (void (^completionBlock)(void) in _scheduledRangeUpdateCompletionBlocks) { - completionBlock(); - } - [_scheduledRangeUpdateCompletionBlocks removeAllObjects]; - #if ASRangeControllerLoggingEnabled // NSSet *visibleNodePathsSet = [NSSet setWithArray:visibleNodePaths]; // BOOL setsAreEqual = [visibleIndexPaths isEqualToSet:visibleNodePathsSet]; From 76b38cbe6e6bbd7aedf8550e717631236f43379a Mon Sep 17 00:00:00 2001 From: Scott Goodson Date: Tue, 22 Mar 2016 14:32:32 -0700 Subject: [PATCH 3/6] [ASInterfaceState] Clear the "Visible" bit immediately upon deallocation, rather than waiting for ASRangeController on the next runloop. --- AsyncDisplayKit/ASDisplayNode.mm | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-) diff --git a/AsyncDisplayKit/ASDisplayNode.mm b/AsyncDisplayKit/ASDisplayNode.mm index f1b6aa26f2..fa696f58ba 100644 --- a/AsyncDisplayKit/ASDisplayNode.mm +++ b/AsyncDisplayKit/ASDisplayNode.mm @@ -337,6 +337,8 @@ static ASDisplayNodeMethodOverrides GetASDisplayNodeMethodOverrides(Class c) - (void)dealloc { ASDisplayNodeAssertMainThread(); + + self.interfaceState &= ~ASInterfaceStateVisible; self.asyncLayer.asyncDelegate = nil; _view.asyncdisplaykit_node = nil; @@ -2076,13 +2078,7 @@ void recursivelyTriggerDisplayForLayer(CALayer *layer, BOOL shouldBlock) [self setDisplaySuspended:NO]; } else { [self setDisplaySuspended:YES]; - //schedule clear contents on next runloop - dispatch_async(dispatch_get_main_queue(), ^{ - ASDN::MutexLocker l(_propertyLock); - if (ASInterfaceStateIncludesDisplay(_interfaceState) == NO) { - [self clearContents]; - } - }); + [self clearContents]; } } else { // NOTE: This case isn't currently supported as setInterfaceState: isn't exposed externally, and all @@ -2094,13 +2090,7 @@ void recursivelyTriggerDisplayForLayer(CALayer *layer, BOOL shouldBlock) [ASDisplayNode scheduleNodeForRecursiveDisplay:self]; } else { [[self asyncLayer] cancelAsyncDisplay]; - //schedule clear contents on next runloop - dispatch_async(dispatch_get_main_queue(), ^{ - ASDN::MutexLocker l(_propertyLock); - if (ASInterfaceStateIncludesDisplay(_interfaceState) == NO) { - [self clearContents]; - } - }); + [self clearContents]; } } } From 953c0f51f05738dba96969c0c6774c18dc815b12 Mon Sep 17 00:00:00 2001 From: Scott Goodson Date: Tue, 22 Mar 2016 14:37:49 -0700 Subject: [PATCH 4/6] Revert "Move call of range update completion blocks to consider early returns in _updateVisibleNodeIndexPaths" This reverts commit 41362fca3942b71cb092376e5baaa78652d1e19b. --- AsyncDisplayKit/Details/ASRangeController.mm | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/AsyncDisplayKit/Details/ASRangeController.mm b/AsyncDisplayKit/Details/ASRangeController.mm index 10ea6c2a0f..c9fffb4cd0 100644 --- a/AsyncDisplayKit/Details/ASRangeController.mm +++ b/AsyncDisplayKit/Details/ASRangeController.mm @@ -134,11 +134,6 @@ static UIApplicationState __ApplicationState = UIApplicationStateActive; dispatch_async(dispatch_get_main_queue(), ^{ [self _updateVisibleNodeIndexPaths]; - for (void (^completionBlock)(void) in _scheduledRangeUpdateCompletionBlocks) { - completionBlock(); - } - [_scheduledRangeUpdateCompletionBlocks removeAllObjects]; - dataSource = nil; delegate = nil; }); @@ -341,6 +336,11 @@ static UIApplicationState __ApplicationState = UIApplicationStateActive; _rangeIsValid = YES; _queuedRangeUpdate = NO; + for (void (^completionBlock)(void) in _scheduledRangeUpdateCompletionBlocks) { + completionBlock(); + } + [_scheduledRangeUpdateCompletionBlocks removeAllObjects]; + #if ASRangeControllerLoggingEnabled // NSSet *visibleNodePathsSet = [NSSet setWithArray:visibleNodePaths]; // BOOL setsAreEqual = [visibleIndexPaths isEqualToSet:visibleNodePathsSet]; From 5b9302b6810a9ffaeb8060a4dd6bb928d2a81078 Mon Sep 17 00:00:00 2001 From: Scott Goodson Date: Tue, 22 Mar 2016 14:38:50 -0700 Subject: [PATCH 5/6] Revert "Prevent deallocation of asyncDataSource and asyncDelegate in ASCollectionView and ASTableView" This reverts commit 3668f45286a56a87d200aea0299caee9d2e4a880. --- AsyncDisplayKit/ASCollectionView.mm | 9 +------- AsyncDisplayKit/ASTableView.mm | 9 +------- AsyncDisplayKit/Details/ASRangeController.mm | 23 +------------------ ...SRangeControllerUpdateRangeProtocol+Beta.h | 11 +-------- 4 files changed, 4 insertions(+), 48 deletions(-) diff --git a/AsyncDisplayKit/ASCollectionView.mm b/AsyncDisplayKit/ASCollectionView.mm index a91c84fef3..ae1f579232 100644 --- a/AsyncDisplayKit/ASCollectionView.mm +++ b/AsyncDisplayKit/ASCollectionView.mm @@ -1139,14 +1139,7 @@ static NSString * const kCellReuseIdentifier = @"_ASCollectionViewCell"; // Updating the visible node index paths only for not range managed nodes. Range managed nodes will get their // their update in the layout pass if (![node supportsRangeManagedInterfaceState]) { - // Grab a strong reference for data source and delegate to be sure they are not going away while executing - // the range update. This can happen in range updates while going back in the view controller hierarchy - __block id asyncDataSource = _asyncDataSource; - __block id asyncDelegate = _asyncDelegate; - [_rangeController scheduleRangeUpdateCompletion:^{ - asyncDataSource = nil; - asyncDelegate = nil; - }]; + [_rangeController visibleNodeIndexPathsDidChangeWithScrollDirection:self.scrollDirection]; } } diff --git a/AsyncDisplayKit/ASTableView.mm b/AsyncDisplayKit/ASTableView.mm index 9e5ad025c7..d8d9037d36 100644 --- a/AsyncDisplayKit/ASTableView.mm +++ b/AsyncDisplayKit/ASTableView.mm @@ -1072,14 +1072,7 @@ static NSString * const kCellReuseIdentifier = @"_ASTableViewCell"; // Updating the visible node index paths only for not range managed nodes. Range managed nodes will get their // their update in the layout pass if (![node supportsRangeManagedInterfaceState]) { - // Grab a strong reference for data source and delegate to be sure they are not going away while executing - // the range update. This can happen in range updates while going back in the view controller hierarchy - __block id asyncDataSource = _asyncDataSource; - __block id asyncDelegate = _asyncDelegate; - [_rangeController scheduleRangeUpdateCompletion:^{ - asyncDataSource = nil; - asyncDelegate = nil; - }]; + [_rangeController visibleNodeIndexPathsDidChangeWithScrollDirection:self.scrollDirection]; } } diff --git a/AsyncDisplayKit/Details/ASRangeController.mm b/AsyncDisplayKit/Details/ASRangeController.mm index c9fffb4cd0..7935564a3d 100644 --- a/AsyncDisplayKit/Details/ASRangeController.mm +++ b/AsyncDisplayKit/Details/ASRangeController.mm @@ -27,7 +27,6 @@ BOOL _didUpdateCurrentRange; BOOL _didRegisterForNotifications; CFAbsoluteTime _pendingDisplayNodesTimestamp; - NSMutableArray *_scheduledRangeUpdateCompletionBlocks; } @end @@ -47,7 +46,6 @@ static UIApplicationState __ApplicationState = UIApplicationStateActive; _rangeIsValid = YES; _currentRangeMode = ASLayoutRangeModeInvalid; _didUpdateCurrentRange = NO; - _scheduledRangeUpdateCompletionBlocks = [NSMutableArray array]; [[[self class] allRangeControllersWeakSet] addObject:self]; @@ -113,15 +111,6 @@ static UIApplicationState __ApplicationState = UIApplicationStateActive; - (void)scheduleRangeUpdate { - [self scheduleRangeUpdateCompletion:nil]; -} - -- (void)scheduleRangeUpdateCompletion:(void (^)(void))completion -{ - if (completion) { - [_scheduledRangeUpdateCompletionBlocks addObject:completion]; - } - if (_queuedRangeUpdate) { return; } @@ -129,13 +118,8 @@ static UIApplicationState __ApplicationState = UIApplicationStateActive; // coalesce these events -- handling them multiple times per runloop is noisy and expensive _queuedRangeUpdate = YES; - __block id dataSource = _dataSource; - __block id delegate = _delegate; dispatch_async(dispatch_get_main_queue(), ^{ - [self _updateVisibleNodeIndexPaths]; - - dataSource = nil; - delegate = nil; + [self performRangeUpdate]; }); } @@ -336,11 +320,6 @@ static UIApplicationState __ApplicationState = UIApplicationStateActive; _rangeIsValid = YES; _queuedRangeUpdate = NO; - for (void (^completionBlock)(void) in _scheduledRangeUpdateCompletionBlocks) { - completionBlock(); - } - [_scheduledRangeUpdateCompletionBlocks removeAllObjects]; - #if ASRangeControllerLoggingEnabled // NSSet *visibleNodePathsSet = [NSSet setWithArray:visibleNodePaths]; // BOOL setsAreEqual = [visibleIndexPaths isEqualToSet:visibleNodePathsSet]; diff --git a/AsyncDisplayKit/Details/ASRangeControllerUpdateRangeProtocol+Beta.h b/AsyncDisplayKit/Details/ASRangeControllerUpdateRangeProtocol+Beta.h index f9fcbf77b7..4f34cd1cef 100644 --- a/AsyncDisplayKit/Details/ASRangeControllerUpdateRangeProtocol+Beta.h +++ b/AsyncDisplayKit/Details/ASRangeControllerUpdateRangeProtocol+Beta.h @@ -49,12 +49,6 @@ */ - (void)updateCurrentRangeWithMode:(ASLayoutRangeMode)rangeMode; -/** - * Schedule a range update and call the completion block if finished. This drives updating the working - * ranges, and triggering their actions. - */ -- (void)scheduleRangeUpdateCompletion:(void (^)(void))completion; - @end @@ -70,10 +64,7 @@ @interface ASViewController (ASRangeControllerUpdateRangeProtocol) -/** - * Automatically adjust range mode based on view events if the containing node confirms to the - * ASRangeControllerUpdateRangeProtocol - */ +/// Automatically adjust range mode based on view events if the containing node confirms to the ASRangeControllerUpdateRangeProtocol @property (nonatomic, assign) BOOL automaticallyAdjustRangeModeBasedOnViewEvents; @end From e59431e702a0d2564afefd7572542630385f56ae Mon Sep 17 00:00:00 2001 From: Scott Goodson Date: Tue, 22 Mar 2016 18:39:23 -0700 Subject: [PATCH 6/6] [ASDisplayNode] Restore dispatch_async for clearContents, to prevent discarding contents for nodes that are simply moving to a new container. --- AsyncDisplayKit/ASDisplayNode.mm | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/AsyncDisplayKit/ASDisplayNode.mm b/AsyncDisplayKit/ASDisplayNode.mm index fa696f58ba..e92f325283 100644 --- a/AsyncDisplayKit/ASDisplayNode.mm +++ b/AsyncDisplayKit/ASDisplayNode.mm @@ -2078,7 +2078,13 @@ void recursivelyTriggerDisplayForLayer(CALayer *layer, BOOL shouldBlock) [self setDisplaySuspended:NO]; } else { [self setDisplaySuspended:YES]; - [self clearContents]; + //schedule clear contents on next runloop + dispatch_async(dispatch_get_main_queue(), ^{ + ASDN::MutexLocker l(_propertyLock); + if (ASInterfaceStateIncludesDisplay(_interfaceState) == NO) { + [self clearContents]; + } + }); } } else { // NOTE: This case isn't currently supported as setInterfaceState: isn't exposed externally, and all @@ -2090,7 +2096,13 @@ void recursivelyTriggerDisplayForLayer(CALayer *layer, BOOL shouldBlock) [ASDisplayNode scheduleNodeForRecursiveDisplay:self]; } else { [[self asyncLayer] cancelAsyncDisplay]; - [self clearContents]; + //schedule clear contents on next runloop + dispatch_async(dispatch_get_main_queue(), ^{ + ASDN::MutexLocker l(_propertyLock); + if (ASInterfaceStateIncludesDisplay(_interfaceState) == NO) { + [self clearContents]; + } + }); } } }