From 255682da3da5ff32a6910802383fe03af1dd3736 Mon Sep 17 00:00:00 2001 From: Garrett Moon Date: Tue, 31 Oct 2017 08:36:01 -0700 Subject: [PATCH] Fix a layout deadlock caused by holding the lock and going up the tree. (#638) * Fix a layout deadlock caused by holding the lock and going up the tree. * Add CHANGELOG message * Huy's comments --- CHANGELOG.md | 1 + Source/ASDisplayNode+Layout.mm | 17 +++++++++++------ Source/ASDisplayNode.mm | 4 +++- Source/Private/ASDisplayNode+FrameworkPrivate.h | 4 ++-- 4 files changed, 17 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ed264d511..6f0a0b1ff 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ - [Animated Image] Adds support for animated WebP as well as improves GIF handling. [#605](https://github.com/TextureGroup/Texture/pull/605) [Garrett Moon](https://github.com/garrettmoon) - [ASCollectionView] Check if batch fetching is needed if batch fetching parameter has been changed. [#624](https://github.com/TextureGroup/Texture/pull/624) [Garrett Moon](https://github.com/garrettmoon) - [ASNetworkImageNode] New delegate callback to tell the consumer whether the image was loaded from cache or download. [Adlai Holler](https://github.com/Adlai-Holler) +- [Layout] Fixes a deadlock in layout. [#638](https://github.com/TextureGroup/Texture/pull/638) [Garrett Moon](https://github.com/garrettmoon) ## 2.6 - [Xcode 9] Updated to require Xcode 9 (to fix warnings) [Garrett Moon](https://github.com/garrettmoon) diff --git a/Source/ASDisplayNode+Layout.mm b/Source/ASDisplayNode+Layout.mm index 6727cd23b..f91016ac4 100644 --- a/Source/ASDisplayNode+Layout.mm +++ b/Source/ASDisplayNode+Layout.mm @@ -211,8 +211,9 @@ @implementation ASDisplayNode (ASLayoutInternal) * @discussion The size of a root node is determined by each subnode. Calling invalidateSize will let the root node know * that the intrinsic size of the receiver node is no longer valid and a resizing of the root node needs to happen. */ -- (void)_setNeedsLayoutFromAbove +- (void)_u_setNeedsLayoutFromAbove { + ASDisplayNodeAssertLockUnownedByCurrentThread(__instanceLock); as_activity_create_for_scope("Set needs layout from above"); ASDisplayNodeAssertThreadAffinity(self); @@ -227,7 +228,7 @@ - (void)_setNeedsLayoutFromAbove if (supernode) { // Threading model requires that we unlock before calling a method on our parent. - [supernode _setNeedsLayoutFromAbove]; + [supernode _u_setNeedsLayoutFromAbove]; } else { // Let the root node method know that the size was invalidated [self _rootNodeDidInvalidateSize]; @@ -287,8 +288,10 @@ - (void)displayNodeDidInvalidateSizeNewSize:(CGSize)size } } -- (void)_locked_measureNodeWithBoundsIfNecessary:(CGRect)bounds +- (void)_u_measureNodeWithBoundsIfNecessary:(CGRect)bounds { + ASDisplayNodeAssertLockUnownedByCurrentThread(__instanceLock); + ASDN::MutexLocker l(__instanceLock__); // Check if we are a subnode in a layout transition. // In this case no measurement is needed as it's part of the layout transition if ([self _isLayoutTransitionInvalid]) { @@ -368,8 +371,10 @@ - (void)_locked_measureNodeWithBoundsIfNecessary:(CGRect)bounds // In this case, we need to detect that we've already asked to be resized to match this // particular ASLayout object, and shouldn't loop asking again unless we have a different ASLayout. nextLayout->requestedLayoutFromAbove = YES; - [self _setNeedsLayoutFromAbove]; - // Update the layout's version here because _setNeedsLayoutFromAbove calls __setNeedsLayout which in turn increases _layoutVersion + __instanceLock__.unlock(); + [self _u_setNeedsLayoutFromAbove]; + __instanceLock__.lock(); + // Update the layout's version here because _u_setNeedsLayoutFromAbove calls __setNeedsLayout which in turn increases _layoutVersion // Failing to do this will cause the layout to be invalid immediately nextLayout->version = _layoutVersion; } @@ -389,7 +394,7 @@ - (void)_locked_measureNodeWithBoundsIfNecessary:(CGRect)bounds - (ASSizeRange)_locked_constrainedSizeForLayoutPass { - // TODO: The logic in -_setNeedsLayoutFromAbove seems correct and doesn't use this method. + // TODO: The logic in -_u_setNeedsLayoutFromAbove seems correct and doesn't use this method. // logic seems correct. For what case does -this method need to do the CGSizeEqual checks? // IF WE CAN REMOVE BOUNDS CHECKS HERE, THEN WE CAN ALSO REMOVE "REQUESTED FROM ABOVE" CHECK diff --git a/Source/ASDisplayNode.mm b/Source/ASDisplayNode.mm index cde2f0030..3ef5f3629 100644 --- a/Source/ASDisplayNode.mm +++ b/Source/ASDisplayNode.mm @@ -911,7 +911,9 @@ - (void)__layout // This method will confirm that the layout is up to date (and update if needed). // Importantly, it will also APPLY the layout to all of our subnodes if (unless parent is transitioning). - [self _locked_measureNodeWithBoundsIfNecessary:bounds]; + __instanceLock__.unlock(); + [self _u_measureNodeWithBoundsIfNecessary:bounds]; + __instanceLock__.lock(); [self _locked_layoutPlaceholderIfNecessary]; } diff --git a/Source/Private/ASDisplayNode+FrameworkPrivate.h b/Source/Private/ASDisplayNode+FrameworkPrivate.h index ebcaf23c1..d4c18a6e9 100644 --- a/Source/Private/ASDisplayNode+FrameworkPrivate.h +++ b/Source/Private/ASDisplayNode+FrameworkPrivate.h @@ -225,7 +225,7 @@ __unused static NSString * _Nonnull NSStringFromASHierarchyState(ASHierarchyStat * @discussion The size of a root node is determined by each subnode. Calling invalidateSize will let the root node know * that the intrinsic size of the receiver node is no longer valid and a resizing of the root node needs to happen. */ -- (void)_setNeedsLayoutFromAbove; +- (void)_u_setNeedsLayoutFromAbove; /** * @abstract Subclass hook for nodes that are acting as root nodes. This method is called if one of the subnodes @@ -237,7 +237,7 @@ __unused static NSString * _Nonnull NSStringFromASHierarchyState(ASHierarchyStat * This method will confirm that the layout is up to date (and update if needed). * Importantly, it will also APPLY the layout to all of our subnodes if (unless parent is transitioning). */ -- (void)_locked_measureNodeWithBoundsIfNecessary:(CGRect)bounds; +- (void)_u_measureNodeWithBoundsIfNecessary:(CGRect)bounds; /** * Layout all of the subnodes based on the sublayouts