From 863b0ca956a48ecdf802c12102dd2dfac4ebb7e6 Mon Sep 17 00:00:00 2001 From: Michael Schneider Date: Tue, 15 Mar 2016 16:30:08 -0700 Subject: [PATCH 1/2] Remove the lock for accessing _pendingDisplayNodes and force methods regarding display to the main thread This should fix a deadlock in ASDisplayNode and it's caused by displayWillStart: and where one thread is recursing down the tree and another thread is recursing up the tree. We remove the lock in _pendingDisplayNodes for the property, but need to guarantee that we only modify the _pendingDisplayNodes state on the main thread. Furthermore add documentation on what thread displayWillStart and displayDidFinish will be called --- AsyncDisplayKit/ASDisplayNode+Subclasses.h | 4 ++ AsyncDisplayKit/ASDisplayNode.mm | 47 ++++++++++++---------- 2 files changed, 30 insertions(+), 21 deletions(-) diff --git a/AsyncDisplayKit/ASDisplayNode+Subclasses.h b/AsyncDisplayKit/ASDisplayNode+Subclasses.h index e369b5c18a..023567d9a2 100644 --- a/AsyncDisplayKit/ASDisplayNode+Subclasses.h +++ b/AsyncDisplayKit/ASDisplayNode+Subclasses.h @@ -206,6 +206,8 @@ NS_ASSUME_NONNULL_BEGIN * * @discussion Subclasses may override this method to be notified when display (asynchronous or synchronous) is * about to begin. + * + * @note Called on the main thread only */ - (void)displayWillStart ASDISPLAYNODE_REQUIRES_SUPER; @@ -214,6 +216,8 @@ NS_ASSUME_NONNULL_BEGIN * * @discussion Subclasses may override this method to be notified when display (asynchronous or synchronous) has * completed. + * + * @note Called on the main thread only */ - (void)displayDidFinish ASDISPLAYNODE_REQUIRES_SUPER; diff --git a/AsyncDisplayKit/ASDisplayNode.mm b/AsyncDisplayKit/ASDisplayNode.mm index d16479a5d5..256e9e7d03 100644 --- a/AsyncDisplayKit/ASDisplayNode.mm +++ b/AsyncDisplayKit/ASDisplayNode.mm @@ -1657,7 +1657,7 @@ static NSInteger incrementIfFound(NSInteger i) { // The node sending the message should usually be passed as the parameter, similar to the delegation pattern. - (void)_pendingNodeWillDisplay:(ASDisplayNode *)node { - ASDN::MutexLocker l(_propertyLock); + ASDisplayNodeAssertMainThread(); if (!_pendingDisplayNodes) { _pendingDisplayNodes = [[NSMutableSet alloc] init]; @@ -1670,27 +1670,25 @@ static NSInteger incrementIfFound(NSInteger i) { // The node sending the message should usually be passed as the parameter, similar to the delegation pattern. - (void)_pendingNodeDidDisplay:(ASDisplayNode *)node { - ASDN::MutexLocker l(_propertyLock); + ASDisplayNodeAssertMainThread(); [_pendingDisplayNodes removeObject:node]; // only trampoline if there is a placeholder and nodes are done displaying if ([self _pendingDisplayNodesHaveFinished] && _placeholderLayer.superlayer) { - dispatch_async(dispatch_get_main_queue(), ^{ - void (^cleanupBlock)() = ^{ - [self _tearDownPlaceholderLayer]; - }; + void (^cleanupBlock)() = ^{ + [self _tearDownPlaceholderLayer]; + }; - if (_placeholderFadeDuration > 0.0 && ASInterfaceStateIncludesVisible(self.interfaceState)) { - [CATransaction begin]; - [CATransaction setCompletionBlock:cleanupBlock]; - [CATransaction setAnimationDuration:_placeholderFadeDuration]; - _placeholderLayer.opacity = 0.0; - [CATransaction commit]; - } else { - cleanupBlock(); - } - }); + if (_placeholderFadeDuration > 0.0 && ASInterfaceStateIncludesVisible(self.interfaceState)) { + [CATransaction begin]; + [CATransaction setCompletionBlock:cleanupBlock]; + [CATransaction setAnimationDuration:_placeholderFadeDuration]; + _placeholderLayer.opacity = 0.0; + [CATransaction commit]; + } else { + cleanupBlock(); + } } } @@ -2260,6 +2258,8 @@ void recursivelyTriggerDisplayForLayer(CALayer *layer, BOOL shouldBlock) - (void)displayWillStart { + ASDisplayNodeAssertMainThread(); + // in case current node takes longer to display than it's subnodes, treat it as a dependent node [self _pendingNodeWillDisplay:self]; @@ -2288,6 +2288,8 @@ void recursivelyTriggerDisplayForLayer(CALayer *layer, BOOL shouldBlock) - (void)displayDidFinish { + ASDisplayNodeAssertMainThread(); + [self _pendingNodeDidDisplay:self]; [_supernode subnodeDisplayDidFinish:self]; @@ -2494,11 +2496,14 @@ static void _recursivelySetDisplaySuspended(ASDisplayNode *node, CALayer *layer, self.asyncLayer.displaySuspended = flag; if ([self __implementsDisplay]) { - if (flag) { - [_supernode subnodeDisplayDidFinish:self]; - } else { - [_supernode subnodeDisplayWillStart:self]; - } + // Display start and finish methods needs to happen on the main thread + ASPerformBlockOnMainThread(^{ + if (flag) { + [_supernode subnodeDisplayDidFinish:self]; + } else { + [_supernode subnodeDisplayWillStart:self]; + } + }); } } From b7a92b2947aaeccfdf49cc759aa31d5949cb281f Mon Sep 17 00:00:00 2001 From: Michael Schneider Date: Tue, 15 Mar 2016 16:30:23 -0700 Subject: [PATCH 2/2] Add documentation for visibilityDidChange: --- AsyncDisplayKit/ASDisplayNode+Subclasses.h | 5 +++++ AsyncDisplayKit/ASDisplayNode.mm | 1 + 2 files changed, 6 insertions(+) diff --git a/AsyncDisplayKit/ASDisplayNode+Subclasses.h b/AsyncDisplayKit/ASDisplayNode+Subclasses.h index 023567d9a2..d7dad4cbc8 100644 --- a/AsyncDisplayKit/ASDisplayNode+Subclasses.h +++ b/AsyncDisplayKit/ASDisplayNode+Subclasses.h @@ -231,6 +231,11 @@ NS_ASSUME_NONNULL_BEGIN */ - (void)interfaceStateDidChange:(ASInterfaceState)newState fromState:(ASInterfaceState)oldState ASDISPLAYNODE_REQUIRES_SUPER; +/** + * @abstract Called whenever the visiblity of the node changed. + * + * @discussion Subclasses may use this to monitor when they become visible. + */ - (void)visibilityDidChange:(BOOL)isVisible ASDISPLAYNODE_REQUIRES_SUPER; /** diff --git a/AsyncDisplayKit/ASDisplayNode.mm b/AsyncDisplayKit/ASDisplayNode.mm index 256e9e7d03..1421d8fa27 100644 --- a/AsyncDisplayKit/ASDisplayNode.mm +++ b/AsyncDisplayKit/ASDisplayNode.mm @@ -1982,6 +1982,7 @@ void recursivelyTriggerDisplayForLayer(CALayer *layer, BOOL shouldBlock) - (void)visibilityDidChange:(BOOL)isVisible { + // subclass override } /**