From c037799e1d814478caa9270c078530bb7a9011b9 Mon Sep 17 00:00:00 2001 From: Huy Nguyen Date: Wed, 11 Jul 2018 18:29:23 -0700 Subject: [PATCH 1/7] Stricter locking assertions - Rename `ASDisplayNodeAssertLockUnownedByCurrentThread` to `ASDisplayNodeAssertLockNotHeld`, and `ASDisplayNodeAssertLockOwnedByCurrentThread` to `ASDisplayNodeAssertLockHeld` -> shorter and hopefully easier to distinguish between the two. - Add assertions to `_locked_` and `_u_` (i.e "unlocked") methods. - Turn `CHECK_LOCKING_SAFETY` flag on by default. After #1022 and #1023, we're in a good shape to actually enforce locked/unlocked requirements of internal methods. Our test suite passed, and we'll test more at Pinterest after the sync this week. - Fix ASVideoNode to avoid calling `play` while holding the lock. That method inserts a subnode and must be called lock free. - Other minor changes. --- CHANGELOG.md | 1 + Source/ASDisplayNode+Layout.mm | 106 ++++++++++-------- Source/ASDisplayNode.mm | 76 ++++++++----- Source/ASImageNode+AnimatedImage.mm | 12 ++ Source/ASImageNode.mm | 1 + Source/ASNetworkImageNode.mm | 8 ++ Source/ASTextNode.mm | 8 +- Source/ASTextNode2.mm | 7 +- Source/ASVideoNode.mm | 3 + Source/ASVideoPlayerNode.mm | 13 +++ Source/Details/ASThread.h | 14 +-- .../Private/ASDisplayNode+FrameworkPrivate.h | 5 + Source/Private/ASDisplayNode+UIViewBridge.mm | 1 + 13 files changed, 173 insertions(+), 82 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2002cc0ad..9635c8e42 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,7 @@ - Properly consider node for responder methods [Michael Schneider](https://github.com/maicki) - [IGListKit] Adds missing UIScrollViewDelegate method to DataSource proxy [Sergey Pronin](https://github.com/wannabehero) - Fix misleading/scary stack trace shown when an assertion occurs during node measurement [Huy Nguyen](https://github.com/nguyenhuy) [#1022](https://github.com/TextureGroup/Texture/pull/1022) +- Enable locking assertions (and add some more) to improve and enforce locking safety within the framework [Huy Nguyen](https://github.com/nguyenhuy) [#1024](https://github.com/TextureGroup/Texture/pull/1024) ## 2.7 - Fix pager node for interface coalescing. [Max Wang](https://github.com/wsdwsd0829) [#877](https://github.com/TextureGroup/Texture/pull/877) diff --git a/Source/ASDisplayNode+Layout.mm b/Source/ASDisplayNode+Layout.mm index e77c354ab..1d6448904 100644 --- a/Source/ASDisplayNode+Layout.mm +++ b/Source/ASDisplayNode+Layout.mm @@ -184,6 +184,7 @@ - (ASSizeRange)constrainedSizeForCalculatedLayout - (ASSizeRange)_locked_constrainedSizeForCalculatedLayout { + ASDisplayNodeAssertLockHeld(__instanceLock__); if (_pendingDisplayNodeLayout != nullptr && _pendingDisplayNodeLayout->isValid(_layoutVersion)) { return _pendingDisplayNodeLayout->constrainedSize; } @@ -218,9 +219,10 @@ @implementation ASDisplayNode (ASLayoutInternal) */ - (void)_u_setNeedsLayoutFromAbove { - ASDisplayNodeAssertLockUnownedByCurrentThread(__instanceLock); - as_activity_create_for_scope("Set needs layout from above"); ASDisplayNodeAssertThreadAffinity(self); + ASDisplayNodeAssertLockNotHeld(__instanceLock__); + + as_activity_create_for_scope("Set needs layout from above"); // Mark the node for layout in the next layout pass [self setNeedsLayout]; @@ -243,7 +245,7 @@ - (void)_u_setNeedsLayoutFromAbove - (void)_rootNodeDidInvalidateSize { ASDisplayNodeAssertThreadAffinity(self); - ASDisplayNodeAssertLockUnownedByCurrentThread(__instanceLock__); + ASDisplayNodeAssertLockNotHeld(__instanceLock__); __instanceLock__.lock(); @@ -273,7 +275,7 @@ - (void)_rootNodeDidInvalidateSize - (void)displayNodeDidInvalidateSizeNewSize:(CGSize)size { ASDisplayNodeAssertThreadAffinity(self); - ASDisplayNodeAssertLockUnownedByCurrentThread(__instanceLock__); + ASDisplayNodeAssertLockNotHeld(__instanceLock__); // The default implementation of display node changes the size of itself to the new size CGRect oldBounds = self.bounds; @@ -295,19 +297,19 @@ - (void)displayNodeDidInvalidateSizeNewSize:(CGSize)size - (void)_u_measureNodeWithBoundsIfNecessary:(CGRect)bounds { - ASDisplayNodeAssertLockUnownedByCurrentThread(__instanceLock); + ASDisplayNodeAssertLockNotHeld(__instanceLock__); BOOL isInLayoutPendingState = NO; { 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]) { + if ([self _locked_isLayoutTransitionInvalid]) { return; } - + CGSize boundsSizeForLayout = ASCeilSizeValues(bounds.size); - + // Prefer a newer and not yet applied _pendingDisplayNodeLayout over _calculatedDisplayNodeLayout // If there is no such _pending, check if _calculated is valid to reuse (avoiding recalculation below). BOOL pendingLayoutIsPreferred = NO; @@ -328,12 +330,16 @@ - (void)_u_measureNodeWithBoundsIfNecessary:(CGRect)bounds if (!pendingLayoutIsPreferred && calculatedLayoutIsReusable) { return; } - + as_activity_create_for_scope("Update node layout for current bounds"); - as_log_verbose(ASLayoutLog(), "Node %@, bounds size %@, calculatedSize %@, calculatedIsDirty %d", self, NSStringFromCGSize(boundsSizeForLayout), NSStringFromCGSize(_calculatedDisplayNodeLayout->layout.size), _calculatedDisplayNodeLayout->version < _layoutVersion.load()); + as_log_verbose(ASLayoutLog(), "Node %@, bounds size %@, calculatedSize %@, calculatedIsDirty %d", + self, + NSStringFromCGSize(boundsSizeForLayout), + NSStringFromCGSize(_calculatedDisplayNodeLayout->layout.size), + _calculatedDisplayNodeLayout->version < _layoutVersion); // _calculatedDisplayNodeLayout is not reusable we need to transition to a new one [self cancelLayoutTransition]; - + BOOL didCreateNewContext = NO; ASLayoutElementContext *context = ASLayoutElementGetCurrentContext(); if (context == nil) { @@ -341,17 +347,17 @@ - (void)_u_measureNodeWithBoundsIfNecessary:(CGRect)bounds ASLayoutElementPushContext(context); didCreateNewContext = YES; } - + // Figure out previous and pending layouts for layout transition std::shared_ptr nextLayout = _pendingDisplayNodeLayout; -#define layoutSizeDifferentFromBounds !CGSizeEqualToSize(nextLayout->layout.size, boundsSizeForLayout) - + #define layoutSizeDifferentFromBounds !CGSizeEqualToSize(nextLayout->layout.size, boundsSizeForLayout) + // nextLayout was likely created by a call to layoutThatFits:, check if it is valid and can be applied. // If our bounds size is different than it, or invalid, recalculate. Use #define to avoid nullptr-> BOOL pendingLayoutApplicable = NO; if (nextLayout == nullptr) { as_log_verbose(ASLayoutLog(), "No pending layout."); - } else if (nextLayout->version < _layoutVersion) { + } else if (nextLayout->isValid(_layoutVersion) == NO) { as_log_verbose(ASLayoutLog(), "Pending layout is stale."); } else if (layoutSizeDifferentFromBounds) { as_log_verbose(ASLayoutLog(), "Pending layout size %@ doesn't match bounds size.", NSStringFromCGSize(nextLayout->layout.size)); @@ -359,7 +365,7 @@ - (void)_u_measureNodeWithBoundsIfNecessary:(CGRect)bounds as_log_verbose(ASLayoutLog(), "Using pending layout %@.", nextLayout->layout); pendingLayoutApplicable = YES; } - + if (!pendingLayoutApplicable) { as_log_verbose(ASLayoutLog(), "Measuring with previous constrained size."); // Use the last known constrainedSize passed from a parent during layout (if never, use bounds). @@ -373,11 +379,11 @@ - (void)_u_measureNodeWithBoundsIfNecessary:(CGRect)bounds // Release it and any orphaned subnodes it retains _pendingDisplayNodeLayout = nullptr; } - + if (didCreateNewContext) { ASLayoutElementPopContext(); } - + // If our new layout's desired size for self doesn't match current size, ask our parent to update it. // This can occur for either pre-calculated or newly-calculated layouts. if (nextLayout->requestedLayoutFromAbove == NO @@ -390,14 +396,17 @@ - (void)_u_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; - __instanceLock__.unlock(); - [self _u_setNeedsLayoutFromAbove]; - __instanceLock__.lock(); + + { + ASDN::MutexUnlocker u(__instanceLock__); + [self _u_setNeedsLayoutFromAbove]; + } + // 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; } - + // Prepare to transition to nextLayout ASDisplayNodeAssertNotNil(nextLayout->layout, @"nextLayout->layout should not be nil! %@", self); _pendingLayoutTransition = [[ASLayoutTransition alloc] initWithNode:self @@ -405,7 +414,7 @@ - (void)_u_measureNodeWithBoundsIfNecessary:(CGRect)bounds previousLayout:_calculatedDisplayNodeLayout]; isInLayoutPendingState = ASHierarchyStateIncludesLayoutPending(_hierarchyState); } - + // If a parent is currently executing a layout transition, perform our layout application after it. if (isInLayoutPendingState == NO) { // If no transition, apply our new layout immediately (common case). @@ -413,26 +422,34 @@ - (void)_u_measureNodeWithBoundsIfNecessary:(CGRect)bounds } } +- (ASSizeRange)_constrainedSizeForLayoutPass +{ + ASDN::MutexLocker l(__instanceLock__); + return [self _locked_constrainedSizeForLayoutPass]; +} + - (ASSizeRange)_locked_constrainedSizeForLayoutPass { // 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 - + + ASDisplayNodeAssertLockHeld(__instanceLock__); + CGSize boundsSizeForLayout = ASCeilSizeValues(self.threadSafeBounds.size); + std::shared_ptr pendingLayout = _pendingDisplayNodeLayout; + std::shared_ptr calculatedLayout = _calculatedDisplayNodeLayout; // Checkout if constrained size of pending or calculated display node layout can be used - if (_pendingDisplayNodeLayout != nullptr - && (_pendingDisplayNodeLayout->requestedLayoutFromAbove - || CGSizeEqualToSize(_pendingDisplayNodeLayout->layout.size, boundsSizeForLayout))) { + if (pendingLayout != nullptr + && (pendingLayout->requestedLayoutFromAbove || CGSizeEqualToSize(pendingLayout->layout.size, boundsSizeForLayout))) { // We assume the size from the last returned layoutThatFits: layout was applied so use the pending display node // layout constrained size - return _pendingDisplayNodeLayout->constrainedSize; - } else if (_calculatedDisplayNodeLayout->layout != nil - && (_calculatedDisplayNodeLayout->requestedLayoutFromAbove - || CGSizeEqualToSize(_calculatedDisplayNodeLayout->layout.size, boundsSizeForLayout))) { + return pendingLayout->constrainedSize; + } else if (calculatedLayout->layout != nil + && (calculatedLayout->requestedLayoutFromAbove || CGSizeEqualToSize(calculatedLayout->layout.size, boundsSizeForLayout))) { // We assume the _calculatedDisplayNodeLayout is still valid and the frame is not different - return _calculatedDisplayNodeLayout->constrainedSize; + return calculatedLayout->constrainedSize; } else { // In this case neither the _pendingDisplayNodeLayout or the _calculatedDisplayNodeLayout constrained size can // be reused, so the current bounds is used. This is usual the case if a frame was set manually that differs to @@ -444,7 +461,7 @@ - (ASSizeRange)_locked_constrainedSizeForLayoutPass - (void)_layoutSublayouts { ASDisplayNodeAssertThreadAffinity(self); - ASDisplayNodeAssertLockUnownedByCurrentThread(__instanceLock__); + ASDisplayNodeAssertLockNotHeld(__instanceLock__); ASLayout *layout; { @@ -503,6 +520,7 @@ - (BOOL)_isLayoutTransitionInvalid - (BOOL)_locked_isLayoutTransitionInvalid { + ASDisplayNodeAssertLockHeld(__instanceLock__); if (ASHierarchyStateIncludesLayoutPending(_hierarchyState)) { ASLayoutElementContext *context = ASLayoutElementGetCurrentContext(); if (context == nil || _pendingTransitionID != context.transitionID) { @@ -535,18 +553,10 @@ - (void)transitionLayoutWithAnimation:(BOOL)animated measurementCompletion:(void(^)())completion { ASDisplayNodeAssertMainThread(); - - ASSizeRange sizeRange; - { - ASDN::MutexLocker l(__instanceLock__); - sizeRange = [self _locked_constrainedSizeForLayoutPass]; - } - - [self transitionLayoutWithSizeRange:sizeRange + [self transitionLayoutWithSizeRange:[self _constrainedSizeForLayoutPass] animated:animated shouldMeasureAsync:shouldMeasureAsync measurementCompletion:completion]; - } - (void)transitionLayoutWithSizeRange:(ASSizeRange)constrainedSize @@ -865,8 +875,8 @@ - (void)didCompleteLayoutTransition:(id)context */ - (void)_completePendingLayoutTransition { - ASDisplayNodeAssertLockUnownedByCurrentThread(__instanceLock); - + ASDisplayNodeAssertLockNotHeld(__instanceLock__); + ASLayoutTransition *pendingLayoutTransition = nil; { ASDN::MutexLocker l(__instanceLock__); @@ -875,7 +885,7 @@ - (void)_completePendingLayoutTransition [self _locked_setCalculatedDisplayNodeLayout:pendingLayoutTransition.pendingLayout]; } } - + if (pendingLayoutTransition != nil) { [self _completeLayoutTransition:pendingLayoutTransition]; [self _pendingLayoutTransitionDidComplete]; @@ -895,6 +905,8 @@ - (void)_completeLayoutTransition:(ASLayoutTransition *)layoutTransition // Trampoline to the main thread if necessary if (ASDisplayNodeThreadIsMain() || layoutTransition.isSynchronous == NO) { + // Committing the layout transition will result in subnode insertions and removals, both of which must be called without the lock held + ASDisplayNodeAssertLockNotHeld(__instanceLock__); [layoutTransition commitTransition]; } else { // Subnode insertions and removals need to happen always on the main thread if at least one subnode is already loaded @@ -948,12 +960,13 @@ - (void)_assertSubnodeState - (void)_pendingLayoutTransitionDidComplete { // This assertion introduces a breaking behavior for nodes that has ASM enabled but also manually manage some subnodes. - // Let's gate it behind YOGA flag and remove it right after a branch cut. + // Let's gate it behind YOGA flag. #if YOGA [self _assertSubnodeState]; #endif // Subclass hook + ASDisplayNodeAssertLockNotHeld(__instanceLock__); [self calculatedLayoutDidChange]; // Grab lock after calling out to subclass @@ -998,6 +1011,7 @@ - (void)_setCalculatedDisplayNodeLayout:(std::shared_ptr)di - (void)_locked_setCalculatedDisplayNodeLayout:(std::shared_ptr)displayNodeLayout { + ASDisplayNodeAssertLockHeld(__instanceLock__); ASDisplayNodeAssertTrue(displayNodeLayout->layout.layoutElement == self); ASDisplayNodeAssertTrue(displayNodeLayout->layout.size.width >= 0.0); ASDisplayNodeAssertTrue(displayNodeLayout->layout.size.height >= 0.0); diff --git a/Source/ASDisplayNode.mm b/Source/ASDisplayNode.mm index 2b251adbc..9d5d07411 100644 --- a/Source/ASDisplayNode.mm +++ b/Source/ASDisplayNode.mm @@ -460,11 +460,14 @@ - (void)dealloc - (BOOL)_locked_shouldLoadViewOrLayer { + ASDisplayNodeAssertLockHeld(__instanceLock__); return !_flags.isDeallocating && !(_hierarchyState & ASHierarchyStateRasterized); } - (UIView *)_locked_viewToLoad { + ASDisplayNodeAssertLockHeld(__instanceLock__); + UIView *view = nil; if (_viewBlock) { view = _viewBlock(); @@ -503,6 +506,7 @@ - (UIView *)_locked_viewToLoad - (CALayer *)_locked_layerToLoad { + ASDisplayNodeAssertLockHeld(__instanceLock__); ASDisplayNodeAssert(_flags.layerBacked, @"_layerToLoad is only for layer-backed nodes"); CALayer *layer = nil; @@ -521,6 +525,8 @@ - (CALayer *)_locked_layerToLoad - (void)_locked_loadViewOrLayer { + ASDisplayNodeAssertLockHeld(__instanceLock__); + if (_flags.layerBacked) { TIME_SCOPED(_debugTimeToCreateView); _layer = [self _locked_layerToLoad]; @@ -549,7 +555,7 @@ - (void)_locked_loadViewOrLayer - (void)_didLoad { ASDisplayNodeAssertMainThread(); - ASDisplayNodeAssertLockUnownedByCurrentThread(__instanceLock__); + ASDisplayNodeAssertLockNotHeld(__instanceLock__); ASDisplayNodeLogEvent(self, @"didLoad"); as_log_verbose(ASNodeLog(), "didLoad %@", self); TIME_SCOPED(_debugTimeForDidLoad); @@ -584,7 +590,7 @@ - (BOOL)isNodeLoaded if (ASDisplayNodeThreadIsMain()) { // Because the view and layer can only be created and destroyed on Main, that is also the only thread // where the state of this property can change. As an optimization, we can avoid locking. - return [self _locked_isNodeLoaded]; + return [self _isNodeLoaded]; } else { ASDN::MutexLocker l(__instanceLock__); return [self _locked_isNodeLoaded]; @@ -592,6 +598,12 @@ - (BOOL)isNodeLoaded } - (BOOL)_locked_isNodeLoaded +{ + ASDisplayNodeAssertLockHeld(__instanceLock__); + return (_view != nil || (_layer != nil && _flags.layerBacked)); +} + +- (BOOL)_isNodeLoaded { return (_view != nil || (_layer != nil && _flags.layerBacked)); } @@ -696,6 +708,7 @@ - (_ASDisplayLayer *)asyncLayer - (_ASDisplayLayer *)_locked_asyncLayer { + ASDisplayNodeAssertLockHeld(__instanceLock__); return [_layer isKindOfClass:[_ASDisplayLayer class]] ? (_ASDisplayLayer *)_layer : nil; } @@ -754,6 +767,7 @@ - (CGRect)threadSafeBounds - (CGRect)_locked_threadSafeBounds { + ASDisplayNodeAssertLockHeld(__instanceLock__); return _threadSafeBounds; } @@ -1014,7 +1028,7 @@ - (void)invalidateCalculatedLayout - (void)__layout { ASDisplayNodeAssertThreadAffinity(self); - ASDisplayNodeAssertLockUnownedByCurrentThread(__instanceLock__); + ASDisplayNodeAssertLockNotHeld(__instanceLock__); BOOL loaded = NO; { @@ -1066,7 +1080,7 @@ - (void)layoutDidFinish { // Hook for subclasses ASDisplayNodeAssertMainThread(); - ASDisplayNodeAssertLockUnownedByCurrentThread(__instanceLock__); + ASDisplayNodeAssertLockNotHeld(__instanceLock__); ASDisplayNodeAssertTrue(self.isNodeLoaded); } @@ -1219,6 +1233,7 @@ - (CGSize)calculateSizeThatFits:(CGSize)constrainedSize - (id)_locked_layoutElementThatFits:(ASSizeRange)constrainedSize { + ASDisplayNodeAssertLockHeld(__instanceLock__); __ASDisplayNodeCheckForLayoutMethodOverrides; BOOL measureLayoutSpec = _measurementOptions & ASDisplayNodePerformanceMeasurementOptionLayoutSpec; @@ -1249,7 +1264,7 @@ - (void)layout { // Hook for subclasses ASDisplayNodeAssertMainThread(); - ASDisplayNodeAssertLockUnownedByCurrentThread(__instanceLock__); + ASDisplayNodeAssertLockNotHeld(__instanceLock__); ASDisplayNodeAssertTrue(self.isNodeLoaded); for (id delegate in _interfaceStateDelegates) { if ([delegate respondsToSelector:@selector(nodeDidLayout)]) { @@ -1303,6 +1318,7 @@ - (BOOL)displaysAsynchronously */ - (BOOL)_locked_displaysAsynchronously { + ASDisplayNodeAssertLockHeld(__instanceLock__); return checkFlag(Synchronous) == NO && _flags.displaysAsynchronously; } @@ -2086,7 +2102,7 @@ ASDISPLAYNODE_INLINE BOOL subtreeIsRasterized(ASDisplayNode *node) { - (ASDisplayNode *)supernode { #if CHECK_LOCKING_SAFETY - if (__instanceLock__.ownedByCurrentThread()) { + if (__instanceLock__.locked()) { NSLog(@"WARNING: Accessing supernode while holding recursive instance lock of this node is worrisome. It's likely that you will soon try to acquire the supernode's lock, and this can easily cause deadlocks."); } #endif @@ -2180,7 +2196,7 @@ - (NSArray *)subnodes /* * Central private helper method that should eventually be called if submethods add, insert or replace subnodes - * This method is called with thread affinity. + * This method is called with thread affinity and without lock held. * * @param subnode The subnode to insert * @param subnodeIndex The index in _subnodes to insert it @@ -2190,7 +2206,9 @@ - (NSArray *)subnodes */ - (void)_insertSubnode:(ASDisplayNode *)subnode atSubnodeIndex:(NSInteger)subnodeIndex sublayerIndex:(NSInteger)sublayerIndex andRemoveSubnode:(ASDisplayNode *)oldSubnode { - ASDisplayNodeAssertLockUnownedByCurrentThread(__instanceLock__); + ASDisplayNodeAssertThreadAffinity(self); + ASDisplayNodeAssertLockNotHeld(__instanceLock__); + as_log_verbose(ASNodeLog(), "Insert subnode %@ at index %zd of %@ and remove subnode %@", subnode, subnodeIndex, self, oldSubnode); if (subnode == nil || subnode == self) { @@ -2398,6 +2416,7 @@ - (void)insertSubnode:(ASDisplayNode *)subnode belowSubnode:(ASDisplayNode *)bel - (void)_insertSubnode:(ASDisplayNode *)subnode belowSubnode:(ASDisplayNode *)below { ASDisplayNodeAssertThreadAffinity(self); + ASDisplayNodeAssertLockNotHeld(__instanceLock__); if (subnode == nil) { ASDisplayNodeFailAssert(@"Cannot insert a nil subnode"); @@ -2461,6 +2480,7 @@ - (void)insertSubnode:(ASDisplayNode *)subnode aboveSubnode:(ASDisplayNode *)abo - (void)_insertSubnode:(ASDisplayNode *)subnode aboveSubnode:(ASDisplayNode *)above { ASDisplayNodeAssertThreadAffinity(self); + ASDisplayNodeAssertLockNotHeld(__instanceLock__); if (subnode == nil) { ASDisplayNodeFailAssert(@"Cannot insert a nil subnode"); @@ -2522,6 +2542,7 @@ - (void)insertSubnode:(ASDisplayNode *)subnode atIndex:(NSInteger)idx - (void)_insertSubnode:(ASDisplayNode *)subnode atIndex:(NSInteger)idx { ASDisplayNodeAssertThreadAffinity(self); + ASDisplayNodeAssertLockNotHeld(__instanceLock__); if (subnode == nil) { ASDisplayNodeFailAssert(@"Cannot insert a nil subnode"); @@ -2558,7 +2579,7 @@ - (void)_insertSubnode:(ASDisplayNode *)subnode atIndex:(NSInteger)idx - (void)_removeSubnode:(ASDisplayNode *)subnode { ASDisplayNodeAssertThreadAffinity(self); - ASDisplayNodeAssertLockUnownedByCurrentThread(__instanceLock__); + ASDisplayNodeAssertLockNotHeld(__instanceLock__); // Don't call self.supernode here because that will retain/autorelease the supernode. This method -_removeSupernode: is often called while tearing down a node hierarchy, and the supernode in question might be in the middle of its -dealloc. The supernode is never messaged, only compared by value, so this is safe. // The particular issue that triggers this edge case is when a node calls -removeFromSupernode on a subnode from within its own -dealloc method. @@ -2584,7 +2605,7 @@ - (void)removeFromSupernode - (void)_removeFromSupernode { ASDisplayNodeAssertThreadAffinity(self); - ASDisplayNodeAssertLockUnownedByCurrentThread(__instanceLock__); + ASDisplayNodeAssertLockNotHeld(__instanceLock__); __instanceLock__.lock(); __weak ASDisplayNode *supernode = _supernode; @@ -2598,7 +2619,7 @@ - (void)_removeFromSupernode - (void)_removeFromSupernodeIfEqualTo:(ASDisplayNode *)supernode { ASDisplayNodeAssertThreadAffinity(self); - ASDisplayNodeAssertLockUnownedByCurrentThread(__instanceLock__); + ASDisplayNodeAssertLockNotHeld(__instanceLock__); __instanceLock__.lock(); @@ -2691,6 +2712,7 @@ - (void)__decrementVisibilityNotificationsDisabled - (void)_locked_layoutPlaceholderIfNecessary { + ASDisplayNodeAssertLockHeld(__instanceLock__); if ([self _locked_shouldHavePlaceholderLayer]) { [self _locked_setupPlaceholderLayerIfNeeded]; } @@ -2700,12 +2722,14 @@ - (void)_locked_layoutPlaceholderIfNecessary - (BOOL)_locked_shouldHavePlaceholderLayer { + ASDisplayNodeAssertLockHeld(__instanceLock__); return (_placeholderEnabled && [self _implementsDisplay]); } - (void)_locked_setupPlaceholderLayerIfNeeded { ASDisplayNodeAssertMainThread(); + ASDisplayNodeAssertLockHeld(__instanceLock__); if (!_placeholderLayer) { _placeholderLayer = [CALayer layer]; @@ -2753,7 +2777,7 @@ - (void)__enterHierarchy { ASDisplayNodeAssertMainThread(); ASDisplayNodeAssert(!_flags.isEnteringHierarchy, @"Should not cause recursive __enterHierarchy"); - ASDisplayNodeAssertLockUnownedByCurrentThread(__instanceLock__); + ASDisplayNodeAssertLockNotHeld(__instanceLock__); ASDisplayNodeLogEvent(self, @"enterHierarchy"); // Profiling has shown that locking this method is beneficial, so each of the property accesses don't have to lock and unlock. @@ -2802,7 +2826,7 @@ - (void)__exitHierarchy { ASDisplayNodeAssertMainThread(); ASDisplayNodeAssert(!_flags.isExitingHierarchy, @"Should not cause recursive __exitHierarchy"); - ASDisplayNodeAssertLockUnownedByCurrentThread(__instanceLock__); + ASDisplayNodeAssertLockNotHeld(__instanceLock__); ASDisplayNodeLogEvent(self, @"exitHierarchy"); // Profiling has shown that locking this method is beneficial, so each of the property accesses don't have to lock and unlock. @@ -2909,7 +2933,7 @@ - (void)willEnterHierarchy ASDisplayNodeAssertMainThread(); ASDisplayNodeAssert(_flags.isEnteringHierarchy, @"You should never call -willEnterHierarchy directly. Appearance is automatically managed by ASDisplayNode"); ASDisplayNodeAssert(!_flags.isExitingHierarchy, @"ASDisplayNode inconsistency. __enterHierarchy and __exitHierarchy are mutually exclusive"); - ASDisplayNodeAssertLockUnownedByCurrentThread(__instanceLock__); + ASDisplayNodeAssertLockNotHeld(__instanceLock__); if (![self supportsRangeManagedInterfaceState]) { self.interfaceState = ASInterfaceStateInHierarchy; @@ -2921,7 +2945,7 @@ - (void)didEnterHierarchy { ASDisplayNodeAssert(!_flags.isEnteringHierarchy, @"You should never call -didEnterHierarchy directly. Appearance is automatically managed by ASDisplayNode"); ASDisplayNodeAssert(!_flags.isExitingHierarchy, @"ASDisplayNode inconsistency. __enterHierarchy and __exitHierarchy are mutually exclusive"); ASDisplayNodeAssert(_flags.isInHierarchy, @"ASDisplayNode inconsistency. __enterHierarchy and __exitHierarchy are mutually exclusive"); - ASDisplayNodeAssertLockUnownedByCurrentThread(__instanceLock__); + ASDisplayNodeAssertLockNotHeld(__instanceLock__); } - (void)didExitHierarchy @@ -2929,7 +2953,7 @@ - (void)didExitHierarchy ASDisplayNodeAssertMainThread(); ASDisplayNodeAssert(_flags.isExitingHierarchy, @"You should never call -didExitHierarchy directly. Appearance is automatically managed by ASDisplayNode"); ASDisplayNodeAssert(!_flags.isEnteringHierarchy, @"ASDisplayNode inconsistency. __enterHierarchy and __exitHierarchy are mutually exclusive"); - ASDisplayNodeAssertLockUnownedByCurrentThread(__instanceLock__); + ASDisplayNodeAssertLockNotHeld(__instanceLock__); // This case is important when tearing down hierarchies. We must deliver a visibileStateDidChange:NO callback, as part our API guarantee that this method can be used for // things like data analytics about user content viewing. We cannot call the method in the dealloc as any incidental retain operations in client code would fail. @@ -3051,7 +3075,7 @@ - (void)applyPendingInterfaceState:(ASInterfaceState)newPendingState ASDisplayNodeAssertMainThread(); // This method manages __instanceLock__ itself, to ensure the lock is not held while didEnter/Exit(.*)State methods are called, thus avoid potential deadlocks - ASDisplayNodeAssertLockUnownedByCurrentThread(__instanceLock__); + ASDisplayNodeAssertLockNotHeld(__instanceLock__); ASInterfaceState oldState = ASInterfaceStateNone; ASInterfaceState newState = ASInterfaceStateNone; @@ -3178,7 +3202,7 @@ - (void)prepareForCATransactionCommit - (void)interfaceStateDidChange:(ASInterfaceState)newState fromState:(ASInterfaceState)oldState { // Subclass hook - ASDisplayNodeAssertLockUnownedByCurrentThread(__instanceLock__); + ASDisplayNodeAssertLockNotHeld(__instanceLock__); ASDisplayNodeAssertMainThread(); for (id delegate in _interfaceStateDelegates) { if ([delegate respondsToSelector:@selector(interfaceStateDidChange:fromState:)]) { @@ -3222,7 +3246,7 @@ - (void)didEnterVisibleState { // subclass override ASDisplayNodeAssertMainThread(); - ASDisplayNodeAssertLockUnownedByCurrentThread(__instanceLock__); + ASDisplayNodeAssertLockNotHeld(__instanceLock__); for (id delegate in _interfaceStateDelegates) { if ([delegate respondsToSelector:@selector(didEnterVisibleState)]) { [delegate didEnterVisibleState]; @@ -3237,7 +3261,7 @@ - (void)didExitVisibleState { // subclass override ASDisplayNodeAssertMainThread(); - ASDisplayNodeAssertLockUnownedByCurrentThread(__instanceLock__); + ASDisplayNodeAssertLockNotHeld(__instanceLock__); for (id delegate in _interfaceStateDelegates) { if ([delegate respondsToSelector:@selector(didExitVisibleState)]) { [delegate didExitVisibleState]; @@ -3255,7 +3279,7 @@ - (void)didEnterDisplayState { // subclass override ASDisplayNodeAssertMainThread(); - ASDisplayNodeAssertLockUnownedByCurrentThread(__instanceLock__); + ASDisplayNodeAssertLockNotHeld(__instanceLock__); for (id delegate in _interfaceStateDelegates) { if ([delegate respondsToSelector:@selector(didEnterDisplayState)]) { [delegate didEnterDisplayState]; @@ -3267,7 +3291,7 @@ - (void)didExitDisplayState { // subclass override ASDisplayNodeAssertMainThread(); - ASDisplayNodeAssertLockUnownedByCurrentThread(__instanceLock__); + ASDisplayNodeAssertLockNotHeld(__instanceLock__); for (id delegate in _interfaceStateDelegates) { if ([delegate respondsToSelector:@selector(didExitDisplayState)]) { [delegate didExitDisplayState]; @@ -3309,7 +3333,7 @@ - (void)recursivelyClearPreloadedData - (void)didEnterPreloadState { ASDisplayNodeAssertMainThread(); - ASDisplayNodeAssertLockUnownedByCurrentThread(__instanceLock__); + ASDisplayNodeAssertLockNotHeld(__instanceLock__); // If this node has ASM enabled and is not yet visible, force a layout pass to apply its applicable pending layout, if any, // so that its subnodes are inserted/deleted and start preloading right away. @@ -3333,7 +3357,7 @@ - (void)didEnterPreloadState - (void)didExitPreloadState { ASDisplayNodeAssertMainThread(); - ASDisplayNodeAssertLockUnownedByCurrentThread(__instanceLock__); + ASDisplayNodeAssertLockNotHeld(__instanceLock__); for (id delegate in _interfaceStateDelegates) { if ([delegate respondsToSelector:@selector(didExitPreloadState)]) { [delegate didExitPreloadState]; @@ -3438,6 +3462,7 @@ - (BOOL)pointInside:(CGPoint)point withEvent:(UIEvent *)event - (void)_locked_applyPendingStateToViewOrLayer { ASDisplayNodeAssertMainThread(); + ASDisplayNodeAssertLockHeld(__instanceLock__); ASDisplayNodeAssert(self.nodeLoaded, @"must have a view or layer"); TIME_SCOPED(_debugTimeToApplyPendingState); @@ -3457,7 +3482,7 @@ - (void)_locked_applyPendingStateToViewOrLayer - (void)applyPendingViewState { ASDisplayNodeAssertMainThread(); - ASDisplayNodeAssertLockUnownedByCurrentThread(__instanceLock__); + ASDisplayNodeAssertLockNotHeld(__instanceLock__); ASDN::MutexLocker l(__instanceLock__); // FIXME: Ideally we'd call this as soon as the node receives -setNeedsLayout @@ -3476,6 +3501,7 @@ - (void)applyPendingViewState - (void)_locked_applyPendingViewState { ASDisplayNodeAssertMainThread(); + ASDisplayNodeAssertLockHeld(__instanceLock__); ASDisplayNodeAssert([self _locked_isNodeLoaded], @"Expected node to be loaded before applying pending state."); if (_flags.layerBacked) { diff --git a/Source/ASImageNode+AnimatedImage.mm b/Source/ASImageNode+AnimatedImage.mm index febbb725e..9597090cd 100644 --- a/Source/ASImageNode+AnimatedImage.mm +++ b/Source/ASImageNode+AnimatedImage.mm @@ -21,6 +21,7 @@ #import #import #import +#import #import #import #import @@ -50,6 +51,8 @@ - (void)setAnimatedImage:(id )animatedImage - (void)_locked_setAnimatedImage:(id )animatedImage { + ASDisplayNodeAssertLockHeld(__instanceLock__); + if (ASObjectIsEqual(_animatedImage, animatedImage) && (animatedImage == nil || animatedImage.playbackReady)) { return; } @@ -124,6 +127,8 @@ - (void)setCoverImageCompleted:(UIImage *)coverImage - (void)_locked_setCoverImageCompleted:(UIImage *)coverImage { + ASDisplayNodeAssertLockHeld(__instanceLock__); + _displayLinkLock.lock(); BOOL setCoverImage = (_displayLink == nil) || _displayLink.paused; _displayLinkLock.unlock(); @@ -141,6 +146,8 @@ - (void)setCoverImage:(UIImage *)coverImage - (void)_locked_setCoverImage:(UIImage *)coverImage { + ASDisplayNodeAssertLockHeld(__instanceLock__); + //If we're a network image node, we want to set the default image so //that it will correctly be restored if it exits the range. #if ASAnimatedImageDebug @@ -182,6 +189,8 @@ - (void)setShouldAnimate:(BOOL)shouldAnimate - (void)_locked_setShouldAnimate:(BOOL)shouldAnimate { + ASDisplayNodeAssertLockHeld(__instanceLock__); + // This test is explicitly done and not ASPerformBlockOnMainThread as this would perform the block immediately // on main if called on main thread and we have to call methods locked or unlocked based on which thread we are on if (ASDisplayNodeThreadIsMain()) { @@ -215,6 +224,8 @@ - (void)startAnimating - (void)_locked_startAnimating { + ASDisplayNodeAssertLockHeld(__instanceLock__); + // It should be safe to call self.interfaceState in this case as it will only grab the lock of the superclass if (!ASInterfaceStateIncludesVisible(self.interfaceState)) { return; @@ -258,6 +269,7 @@ - (void)stopAnimating - (void)_locked_stopAnimating { ASDisplayNodeAssertMainThread(); + ASDisplayNodeAssertLockHeld(__instanceLock__); #if ASAnimatedImageDebug NSLog(@"stopping animation: %p", self); diff --git a/Source/ASImageNode.mm b/Source/ASImageNode.mm index b572c9d97..3a7ef5500 100644 --- a/Source/ASImageNode.mm +++ b/Source/ASImageNode.mm @@ -244,6 +244,7 @@ - (void)setImage:(UIImage *)image - (void)_locked_setImage:(UIImage *)image { + ASDisplayNodeAssertLockHeld(__instanceLock__); if (ASObjectIsEqual(_image, image)) { return; } diff --git a/Source/ASNetworkImageNode.mm b/Source/ASNetworkImageNode.mm index be5bf1df4..499c29a1e 100755 --- a/Source/ASNetworkImageNode.mm +++ b/Source/ASNetworkImageNode.mm @@ -20,6 +20,7 @@ #import #import #import +#import #import #import #import @@ -149,6 +150,8 @@ - (void)setImage:(UIImage *)image - (void)_locked_setImage:(UIImage *)image { + ASDisplayNodeAssertLockHeld(__instanceLock__); + BOOL imageWasSetExternally = (image != nil); BOOL shouldCancelAndClear = imageWasSetExternally && (imageWasSetExternally != _imageWasSetExternally); _imageWasSetExternally = imageWasSetExternally; @@ -175,6 +178,7 @@ - (void)_setImage:(UIImage *)image - (void)_locked__setImage:(UIImage *)image { + ASDisplayNodeAssertLockHeld(__instanceLock__); [super _locked_setImage:image]; } @@ -508,6 +512,8 @@ - (void)_cancelDownloadAndClearImageWithResumePossibility:(BOOL)storeResume - (void)_locked_cancelDownloadAndClearImageWithResumePossibility:(BOOL)storeResume { + ASDisplayNodeAssertLockHeld(__instanceLock__); + [self _locked_cancelImageDownloadWithResumePossibility:storeResume]; [self _locked_setAnimatedImage:nil]; @@ -532,6 +538,8 @@ - (void)_cancelImageDownloadWithResumePossibility:(BOOL)storeResume - (void)_locked_cancelImageDownloadWithResumePossibility:(BOOL)storeResume { + ASDisplayNodeAssertLockHeld(__instanceLock__); + if (!_downloadIdentifier) { return; } diff --git a/Source/ASTextNode.mm b/Source/ASTextNode.mm index 75507a994..7df3cc7ee 100644 --- a/Source/ASTextNode.mm +++ b/Source/ASTextNode.mm @@ -26,9 +26,10 @@ #import #import #import +#import +#import #import #import -#import #import #import @@ -343,17 +344,20 @@ - (ASTextKitRenderer *)_rendererWithBounds:(CGRect)bounds - (ASTextKitRenderer *)_locked_renderer { + ASDisplayNodeAssertLockHeld(__instanceLock__); return [self _locked_rendererWithBounds:[self _locked_threadSafeBounds]]; } - (ASTextKitRenderer *)_locked_rendererWithBounds:(CGRect)bounds { + ASDisplayNodeAssertLockHeld(__instanceLock__); bounds = UIEdgeInsetsInsetRect(bounds, _textContainerInset); return rendererForAttributes([self _locked_rendererAttributes], bounds.size); } - (ASTextKitAttributes)_locked_rendererAttributes { + ASDisplayNodeAssertLockHeld(__instanceLock__); return { .attributedString = _attributedText, .truncationAttributedString = [self _locked_composedTruncationText], @@ -1272,6 +1276,7 @@ - (NSRange)_additionalTruncationMessageRangeWithVisibleRange:(NSRange)visibleRan */ - (NSAttributedString *)_locked_composedTruncationText { + ASDisplayNodeAssertLockHeld(__instanceLock__); if (_composedTruncationText == nil) { if (_truncationAttributedText != nil && _additionalTruncationMessage != nil) { NSMutableAttributedString *newComposedTruncationString = [[NSMutableAttributedString alloc] initWithAttributedString:_truncationAttributedText]; @@ -1297,6 +1302,7 @@ - (NSAttributedString *)_locked_composedTruncationText */ - (NSAttributedString *)_locked_prepareTruncationStringForDrawing:(NSAttributedString *)truncationString { + ASDisplayNodeAssertLockHeld(__instanceLock__); truncationString = ASCleanseAttributedStringOfCoreTextAttributes(truncationString); NSMutableAttributedString *truncationMutableString = [truncationString mutableCopy]; // Grab the attributes from the full string diff --git a/Source/ASTextNode2.mm b/Source/ASTextNode2.mm index 92dc243b2..fa6ca9808 100644 --- a/Source/ASTextNode2.mm +++ b/Source/ASTextNode2.mm @@ -19,8 +19,9 @@ #import #import #import -#import #import +#import +#import #import #import @@ -1098,7 +1099,7 @@ - (NSRange)_additionalTruncationMessageRangeWithVisibleRange:(NSRange)visibleRan */ - (NSAttributedString *)_locked_composedTruncationText { - ASDisplayNodeAssertLockOwnedByCurrentThread(__instanceLock__); + ASDisplayNodeAssertLockHeld(__instanceLock__); if (_composedTruncationText == nil) { if (_truncationAttributedText != nil && _additionalTruncationMessage != nil) { NSMutableAttributedString *newComposedTruncationString = [[NSMutableAttributedString alloc] initWithAttributedString:_truncationAttributedText]; @@ -1124,7 +1125,7 @@ - (NSAttributedString *)_locked_composedTruncationText */ - (NSAttributedString *)_locked_prepareTruncationStringForDrawing:(NSAttributedString *)truncationString { - ASDisplayNodeAssertLockOwnedByCurrentThread(__instanceLock__); + ASDisplayNodeAssertLockHeld(__instanceLock__); NSMutableAttributedString *truncationMutableString = [truncationString mutableCopy]; // Grab the attributes from the full string if (_attributedText.length > 0) { diff --git a/Source/ASVideoNode.mm b/Source/ASVideoNode.mm index d67c72ab0..3b8c9bd37 100644 --- a/Source/ASVideoNode.mm +++ b/Source/ASVideoNode.mm @@ -338,6 +338,7 @@ - (void)observeValueForKeyPath:(NSString *)keyPath ofObject:(id)object change:(N if (self.playerState != ASVideoNodePlayerStatePlaying) { self.playerState = ASVideoNodePlayerStateReadyToPlay; if (_shouldBePlaying && ASInterfaceStateIncludesVisible(self.interfaceState)) { + ASUnlockScope(self); [self play]; } } @@ -360,6 +361,8 @@ - (void)observeValueForKeyPath:(NSString *)keyPath ofObject:(id)object change:(N if (self.playerState == ASVideoNodePlayerStateLoading && _delegateFlags.delegateVideoNodeDidRecoverFromStall) { [self.delegate videoNodeDidRecoverFromStall:self]; } + + ASUnlockScope(self); [self play]; // autoresume after buffer catches up } } else if ([keyPath isEqualToString:kplaybackBufferEmpty]) { diff --git a/Source/ASVideoPlayerNode.mm b/Source/ASVideoPlayerNode.mm index 7859448a6..f74bec992 100644 --- a/Source/ASVideoPlayerNode.mm +++ b/Source/ASVideoPlayerNode.mm @@ -26,6 +26,7 @@ #import #import #import +#import #import static void *ASVideoPlayerNodeContext = &ASVideoPlayerNodeContext; @@ -334,6 +335,8 @@ - (void)cleanCachedControls - (void)_locked_createPlaybackButton { + ASDisplayNodeAssertLockHeld(__instanceLock__); + if (_playbackButtonNode == nil) { _playbackButtonNode = [[ASDefaultPlaybackButton alloc] init]; _playbackButtonNode.style.preferredSize = CGSizeMake(16.0, 22.0); @@ -357,6 +360,8 @@ - (void)_locked_createPlaybackButton - (void)_locked_createFullScreenButton { + ASDisplayNodeAssertLockHeld(__instanceLock__); + if (_fullScreenButtonNode == nil) { _fullScreenButtonNode = [[ASButtonNode alloc] init]; _fullScreenButtonNode.style.preferredSize = CGSizeMake(16.0, 22.0); @@ -374,6 +379,8 @@ - (void)_locked_createFullScreenButton - (void)_locked_createElapsedTextField { + ASDisplayNodeAssertLockHeld(__instanceLock__); + if (_elapsedTextNode == nil) { _elapsedTextNode = [[ASTextNode alloc] init]; _elapsedTextNode.attributedText = [self timeLabelAttributedStringForString:@"00:00" @@ -387,6 +394,8 @@ - (void)_locked_createElapsedTextField - (void)_locked_createDurationTextField { + ASDisplayNodeAssertLockHeld(__instanceLock__); + if (_durationTextNode == nil) { _durationTextNode = [[ASTextNode alloc] init]; _durationTextNode.attributedText = [self timeLabelAttributedStringForString:@"00:00" @@ -401,6 +410,8 @@ - (void)_locked_createDurationTextField - (void)_locked_createScrubber { + ASDisplayNodeAssertLockHeld(__instanceLock__); + if (_scrubberNode == nil) { __weak __typeof__(self) weakSelf = self; _scrubberNode = [[ASDisplayNode alloc] initWithViewBlock:^UIView * _Nonnull { @@ -445,6 +456,8 @@ - (void)_locked_createScrubber - (void)_locked_createControlFlexGrowSpacer { + ASDisplayNodeAssertLockHeld(__instanceLock__); + if (_controlFlexGrowSpacerSpec == nil) { _controlFlexGrowSpacerSpec = [[ASStackLayoutSpec alloc] init]; _controlFlexGrowSpacerSpec.style.flexGrow = 1.0; diff --git a/Source/Details/ASThread.h b/Source/Details/ASThread.h index 5c61aa783..e077532cb 100644 --- a/Source/Details/ASThread.h +++ b/Source/Details/ASThread.h @@ -108,13 +108,13 @@ ASDISPLAYNODE_INLINE void _ASUnlockScopeCleanup(id __strong *lockPtr) * Enable this flag to collect information on the owning thread and ownership level of a mutex. * These properties are useful to determine if a mutext has been acquired and in case of a recursive mutex, how many times that happened. * - * This flag also enable locking assertions (e.g ASDisplayNodeAssertLockUnownedByCurrentThread(node)). + * This flag also enable locking assertions (e.g ASDisplayNodeAssertLockNotHeld(node)). * The assertions are useful when you want to indicate and enforce the locking policy/expectation of methods. * To determine when and which methods acquired a (recursive) mutex (to debug deadlocks, for example), * put breakpoints at some assertions. When the breakpoints hit, walk through stack trace frames * and check ownership count of the mutex. */ -#define CHECK_LOCKING_SAFETY 0 +#define CHECK_LOCKING_SAFETY 1 #if TIME_LOCKER #import @@ -138,11 +138,11 @@ ASDISPLAYNODE_INLINE void _ASUnlockScopeCleanup(id __strong *lockPtr) * and check ownership count of the mutex. */ #if CHECK_LOCKING_SAFETY -#define ASDisplayNodeAssertLockUnownedByCurrentThread(lock) ASDisplayNodeAssertFalse(lock.ownedByCurrentThread()) -#define ASDisplayNodeAssertLockOwnedByCurrentThread(lock) ASDisplayNodeAssert(lock.ownedByCurrentThread()) +#define ASDisplayNodeAssertLockNotHeld(lock) ASDisplayNodeAssertFalse(lock.locked()) +#define ASDisplayNodeAssertLockHeld(lock) ASDisplayNodeAssert(lock.locked(), @"Lock must be held by current thread") #else -#define ASDisplayNodeAssertLockUnownedByCurrentThread(lock) -#define ASDisplayNodeAssertLockOwnedByCurrentThread(lock) +#define ASDisplayNodeAssertLockNotHeld(lock) +#define ASDisplayNodeAssertLockHeld(lock) #endif namespace ASDN { @@ -346,7 +346,7 @@ namespace ASDN { pthread_mutex_t *mutex () { return &_m; } #if CHECK_LOCKING_SAFETY - bool ownedByCurrentThread() { + bool locked() { return _count > 0 && pthread_mach_thread_np(pthread_self()) == _owner; } #endif diff --git a/Source/Private/ASDisplayNode+FrameworkPrivate.h b/Source/Private/ASDisplayNode+FrameworkPrivate.h index 245d9f382..95a1e8dc3 100644 --- a/Source/Private/ASDisplayNode+FrameworkPrivate.h +++ b/Source/Private/ASDisplayNode+FrameworkPrivate.h @@ -297,6 +297,11 @@ __unused static NSString * _Nonnull NSStringFromASHierarchyStateChange(ASHierarc */ - (BOOL)_isLayoutTransitionInvalid; +/** + * Same as @c -_isLayoutTransitionInvalid but must be called with the node's instance lock held. + */ +- (BOOL)_locked_isLayoutTransitionInvalid; + /** * Internal method that can be overriden by subclasses to add specific behavior after the measurement of a layout * transition did finish. diff --git a/Source/Private/ASDisplayNode+UIViewBridge.mm b/Source/Private/ASDisplayNode+UIViewBridge.mm index 939ed642c..9c46a5ceb 100644 --- a/Source/Private/ASDisplayNode+UIViewBridge.mm +++ b/Source/Private/ASDisplayNode+UIViewBridge.mm @@ -975,6 +975,7 @@ - (void)setLayerCornerRadius:(CGFloat)newLayerCornerRadius - (BOOL)_locked_insetsLayoutMarginsFromSafeArea { + ASDisplayNodeAssertLockHeld(__instanceLock__); if (AS_AVAILABLE_IOS(11.0)) { if (!_flags.layerBacked) { return _getFromViewOnly(insetsLayoutMarginsFromSafeArea); From c2bdd1a439e3670ecb6dce21b1457dcbd079e1ab Mon Sep 17 00:00:00 2001 From: Huy Nguyen Date: Thu, 12 Jul 2018 13:53:42 -0700 Subject: [PATCH 2/7] Rename to ASAssertLocked and ASAssertUnlocked --- Source/ASDisplayNode+Layout.mm | 24 +++--- Source/ASDisplayNode.mm | 78 ++++++++++---------- Source/ASImageNode+AnimatedImage.mm | 12 +-- Source/ASImageNode.mm | 2 +- Source/ASNetworkImageNode.mm | 8 +- Source/ASTextNode.mm | 10 +-- Source/ASTextNode2.mm | 4 +- Source/ASVideoPlayerNode.mm | 12 +-- Source/Details/ASThread.h | 10 +-- Source/Private/ASDisplayNode+UIViewBridge.mm | 2 +- 10 files changed, 81 insertions(+), 81 deletions(-) diff --git a/Source/ASDisplayNode+Layout.mm b/Source/ASDisplayNode+Layout.mm index 1d6448904..8232c66c0 100644 --- a/Source/ASDisplayNode+Layout.mm +++ b/Source/ASDisplayNode+Layout.mm @@ -184,7 +184,7 @@ - (ASSizeRange)constrainedSizeForCalculatedLayout - (ASSizeRange)_locked_constrainedSizeForCalculatedLayout { - ASDisplayNodeAssertLockHeld(__instanceLock__); + ASAssertLocked(__instanceLock__); if (_pendingDisplayNodeLayout != nullptr && _pendingDisplayNodeLayout->isValid(_layoutVersion)) { return _pendingDisplayNodeLayout->constrainedSize; } @@ -220,7 +220,7 @@ @implementation ASDisplayNode (ASLayoutInternal) - (void)_u_setNeedsLayoutFromAbove { ASDisplayNodeAssertThreadAffinity(self); - ASDisplayNodeAssertLockNotHeld(__instanceLock__); + ASAssertUnlocked(__instanceLock__); as_activity_create_for_scope("Set needs layout from above"); @@ -245,7 +245,7 @@ - (void)_u_setNeedsLayoutFromAbove - (void)_rootNodeDidInvalidateSize { ASDisplayNodeAssertThreadAffinity(self); - ASDisplayNodeAssertLockNotHeld(__instanceLock__); + ASAssertUnlocked(__instanceLock__); __instanceLock__.lock(); @@ -275,7 +275,7 @@ - (void)_rootNodeDidInvalidateSize - (void)displayNodeDidInvalidateSizeNewSize:(CGSize)size { ASDisplayNodeAssertThreadAffinity(self); - ASDisplayNodeAssertLockNotHeld(__instanceLock__); + ASAssertUnlocked(__instanceLock__); // The default implementation of display node changes the size of itself to the new size CGRect oldBounds = self.bounds; @@ -297,7 +297,7 @@ - (void)displayNodeDidInvalidateSizeNewSize:(CGSize)size - (void)_u_measureNodeWithBoundsIfNecessary:(CGRect)bounds { - ASDisplayNodeAssertLockNotHeld(__instanceLock__); + ASAssertUnlocked(__instanceLock__); BOOL isInLayoutPendingState = NO; { @@ -434,7 +434,7 @@ - (ASSizeRange)_locked_constrainedSizeForLayoutPass // 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 - ASDisplayNodeAssertLockHeld(__instanceLock__); + ASAssertLocked(__instanceLock__); CGSize boundsSizeForLayout = ASCeilSizeValues(self.threadSafeBounds.size); std::shared_ptr pendingLayout = _pendingDisplayNodeLayout; @@ -461,7 +461,7 @@ - (ASSizeRange)_locked_constrainedSizeForLayoutPass - (void)_layoutSublayouts { ASDisplayNodeAssertThreadAffinity(self); - ASDisplayNodeAssertLockNotHeld(__instanceLock__); + ASAssertUnlocked(__instanceLock__); ASLayout *layout; { @@ -520,7 +520,7 @@ - (BOOL)_isLayoutTransitionInvalid - (BOOL)_locked_isLayoutTransitionInvalid { - ASDisplayNodeAssertLockHeld(__instanceLock__); + ASAssertLocked(__instanceLock__); if (ASHierarchyStateIncludesLayoutPending(_hierarchyState)) { ASLayoutElementContext *context = ASLayoutElementGetCurrentContext(); if (context == nil || _pendingTransitionID != context.transitionID) { @@ -875,7 +875,7 @@ - (void)didCompleteLayoutTransition:(id)context */ - (void)_completePendingLayoutTransition { - ASDisplayNodeAssertLockNotHeld(__instanceLock__); + ASAssertUnlocked(__instanceLock__); ASLayoutTransition *pendingLayoutTransition = nil; { @@ -906,7 +906,7 @@ - (void)_completeLayoutTransition:(ASLayoutTransition *)layoutTransition // Trampoline to the main thread if necessary if (ASDisplayNodeThreadIsMain() || layoutTransition.isSynchronous == NO) { // Committing the layout transition will result in subnode insertions and removals, both of which must be called without the lock held - ASDisplayNodeAssertLockNotHeld(__instanceLock__); + ASAssertUnlocked(__instanceLock__); [layoutTransition commitTransition]; } else { // Subnode insertions and removals need to happen always on the main thread if at least one subnode is already loaded @@ -966,7 +966,7 @@ - (void)_pendingLayoutTransitionDidComplete #endif // Subclass hook - ASDisplayNodeAssertLockNotHeld(__instanceLock__); + ASAssertUnlocked(__instanceLock__); [self calculatedLayoutDidChange]; // Grab lock after calling out to subclass @@ -1011,7 +1011,7 @@ - (void)_setCalculatedDisplayNodeLayout:(std::shared_ptr)di - (void)_locked_setCalculatedDisplayNodeLayout:(std::shared_ptr)displayNodeLayout { - ASDisplayNodeAssertLockHeld(__instanceLock__); + ASAssertLocked(__instanceLock__); ASDisplayNodeAssertTrue(displayNodeLayout->layout.layoutElement == self); ASDisplayNodeAssertTrue(displayNodeLayout->layout.size.width >= 0.0); ASDisplayNodeAssertTrue(displayNodeLayout->layout.size.height >= 0.0); diff --git a/Source/ASDisplayNode.mm b/Source/ASDisplayNode.mm index 9d5d07411..36a1f4441 100644 --- a/Source/ASDisplayNode.mm +++ b/Source/ASDisplayNode.mm @@ -460,13 +460,13 @@ - (void)dealloc - (BOOL)_locked_shouldLoadViewOrLayer { - ASDisplayNodeAssertLockHeld(__instanceLock__); + ASAssertLocked(__instanceLock__); return !_flags.isDeallocating && !(_hierarchyState & ASHierarchyStateRasterized); } - (UIView *)_locked_viewToLoad { - ASDisplayNodeAssertLockHeld(__instanceLock__); + ASAssertLocked(__instanceLock__); UIView *view = nil; if (_viewBlock) { @@ -506,7 +506,7 @@ - (UIView *)_locked_viewToLoad - (CALayer *)_locked_layerToLoad { - ASDisplayNodeAssertLockHeld(__instanceLock__); + ASAssertLocked(__instanceLock__); ASDisplayNodeAssert(_flags.layerBacked, @"_layerToLoad is only for layer-backed nodes"); CALayer *layer = nil; @@ -525,7 +525,7 @@ - (CALayer *)_locked_layerToLoad - (void)_locked_loadViewOrLayer { - ASDisplayNodeAssertLockHeld(__instanceLock__); + ASAssertLocked(__instanceLock__); if (_flags.layerBacked) { TIME_SCOPED(_debugTimeToCreateView); @@ -555,7 +555,7 @@ - (void)_locked_loadViewOrLayer - (void)_didLoad { ASDisplayNodeAssertMainThread(); - ASDisplayNodeAssertLockNotHeld(__instanceLock__); + ASAssertUnlocked(__instanceLock__); ASDisplayNodeLogEvent(self, @"didLoad"); as_log_verbose(ASNodeLog(), "didLoad %@", self); TIME_SCOPED(_debugTimeForDidLoad); @@ -599,7 +599,7 @@ - (BOOL)isNodeLoaded - (BOOL)_locked_isNodeLoaded { - ASDisplayNodeAssertLockHeld(__instanceLock__); + ASAssertLocked(__instanceLock__); return (_view != nil || (_layer != nil && _flags.layerBacked)); } @@ -708,7 +708,7 @@ - (_ASDisplayLayer *)asyncLayer - (_ASDisplayLayer *)_locked_asyncLayer { - ASDisplayNodeAssertLockHeld(__instanceLock__); + ASAssertLocked(__instanceLock__); return [_layer isKindOfClass:[_ASDisplayLayer class]] ? (_ASDisplayLayer *)_layer : nil; } @@ -767,7 +767,7 @@ - (CGRect)threadSafeBounds - (CGRect)_locked_threadSafeBounds { - ASDisplayNodeAssertLockHeld(__instanceLock__); + ASAssertLocked(__instanceLock__); return _threadSafeBounds; } @@ -1028,7 +1028,7 @@ - (void)invalidateCalculatedLayout - (void)__layout { ASDisplayNodeAssertThreadAffinity(self); - ASDisplayNodeAssertLockNotHeld(__instanceLock__); + ASAssertUnlocked(__instanceLock__); BOOL loaded = NO; { @@ -1080,7 +1080,7 @@ - (void)layoutDidFinish { // Hook for subclasses ASDisplayNodeAssertMainThread(); - ASDisplayNodeAssertLockNotHeld(__instanceLock__); + ASAssertUnlocked(__instanceLock__); ASDisplayNodeAssertTrue(self.isNodeLoaded); } @@ -1233,7 +1233,7 @@ - (CGSize)calculateSizeThatFits:(CGSize)constrainedSize - (id)_locked_layoutElementThatFits:(ASSizeRange)constrainedSize { - ASDisplayNodeAssertLockHeld(__instanceLock__); + ASAssertLocked(__instanceLock__); __ASDisplayNodeCheckForLayoutMethodOverrides; BOOL measureLayoutSpec = _measurementOptions & ASDisplayNodePerformanceMeasurementOptionLayoutSpec; @@ -1264,7 +1264,7 @@ - (void)layout { // Hook for subclasses ASDisplayNodeAssertMainThread(); - ASDisplayNodeAssertLockNotHeld(__instanceLock__); + ASAssertUnlocked(__instanceLock__); ASDisplayNodeAssertTrue(self.isNodeLoaded); for (id delegate in _interfaceStateDelegates) { if ([delegate respondsToSelector:@selector(nodeDidLayout)]) { @@ -1318,7 +1318,7 @@ - (BOOL)displaysAsynchronously */ - (BOOL)_locked_displaysAsynchronously { - ASDisplayNodeAssertLockHeld(__instanceLock__); + ASAssertLocked(__instanceLock__); return checkFlag(Synchronous) == NO && _flags.displaysAsynchronously; } @@ -2207,7 +2207,7 @@ - (NSArray *)subnodes - (void)_insertSubnode:(ASDisplayNode *)subnode atSubnodeIndex:(NSInteger)subnodeIndex sublayerIndex:(NSInteger)sublayerIndex andRemoveSubnode:(ASDisplayNode *)oldSubnode { ASDisplayNodeAssertThreadAffinity(self); - ASDisplayNodeAssertLockNotHeld(__instanceLock__); + ASAssertUnlocked(__instanceLock__); as_log_verbose(ASNodeLog(), "Insert subnode %@ at index %zd of %@ and remove subnode %@", subnode, subnodeIndex, self, oldSubnode); @@ -2416,7 +2416,7 @@ - (void)insertSubnode:(ASDisplayNode *)subnode belowSubnode:(ASDisplayNode *)bel - (void)_insertSubnode:(ASDisplayNode *)subnode belowSubnode:(ASDisplayNode *)below { ASDisplayNodeAssertThreadAffinity(self); - ASDisplayNodeAssertLockNotHeld(__instanceLock__); + ASAssertUnlocked(__instanceLock__); if (subnode == nil) { ASDisplayNodeFailAssert(@"Cannot insert a nil subnode"); @@ -2480,7 +2480,7 @@ - (void)insertSubnode:(ASDisplayNode *)subnode aboveSubnode:(ASDisplayNode *)abo - (void)_insertSubnode:(ASDisplayNode *)subnode aboveSubnode:(ASDisplayNode *)above { ASDisplayNodeAssertThreadAffinity(self); - ASDisplayNodeAssertLockNotHeld(__instanceLock__); + ASAssertUnlocked(__instanceLock__); if (subnode == nil) { ASDisplayNodeFailAssert(@"Cannot insert a nil subnode"); @@ -2542,7 +2542,7 @@ - (void)insertSubnode:(ASDisplayNode *)subnode atIndex:(NSInteger)idx - (void)_insertSubnode:(ASDisplayNode *)subnode atIndex:(NSInteger)idx { ASDisplayNodeAssertThreadAffinity(self); - ASDisplayNodeAssertLockNotHeld(__instanceLock__); + ASAssertUnlocked(__instanceLock__); if (subnode == nil) { ASDisplayNodeFailAssert(@"Cannot insert a nil subnode"); @@ -2579,7 +2579,7 @@ - (void)_insertSubnode:(ASDisplayNode *)subnode atIndex:(NSInteger)idx - (void)_removeSubnode:(ASDisplayNode *)subnode { ASDisplayNodeAssertThreadAffinity(self); - ASDisplayNodeAssertLockNotHeld(__instanceLock__); + ASAssertUnlocked(__instanceLock__); // Don't call self.supernode here because that will retain/autorelease the supernode. This method -_removeSupernode: is often called while tearing down a node hierarchy, and the supernode in question might be in the middle of its -dealloc. The supernode is never messaged, only compared by value, so this is safe. // The particular issue that triggers this edge case is when a node calls -removeFromSupernode on a subnode from within its own -dealloc method. @@ -2605,7 +2605,7 @@ - (void)removeFromSupernode - (void)_removeFromSupernode { ASDisplayNodeAssertThreadAffinity(self); - ASDisplayNodeAssertLockNotHeld(__instanceLock__); + ASAssertUnlocked(__instanceLock__); __instanceLock__.lock(); __weak ASDisplayNode *supernode = _supernode; @@ -2619,7 +2619,7 @@ - (void)_removeFromSupernode - (void)_removeFromSupernodeIfEqualTo:(ASDisplayNode *)supernode { ASDisplayNodeAssertThreadAffinity(self); - ASDisplayNodeAssertLockNotHeld(__instanceLock__); + ASAssertUnlocked(__instanceLock__); __instanceLock__.lock(); @@ -2712,7 +2712,7 @@ - (void)__decrementVisibilityNotificationsDisabled - (void)_locked_layoutPlaceholderIfNecessary { - ASDisplayNodeAssertLockHeld(__instanceLock__); + ASAssertLocked(__instanceLock__); if ([self _locked_shouldHavePlaceholderLayer]) { [self _locked_setupPlaceholderLayerIfNeeded]; } @@ -2722,14 +2722,14 @@ - (void)_locked_layoutPlaceholderIfNecessary - (BOOL)_locked_shouldHavePlaceholderLayer { - ASDisplayNodeAssertLockHeld(__instanceLock__); + ASAssertLocked(__instanceLock__); return (_placeholderEnabled && [self _implementsDisplay]); } - (void)_locked_setupPlaceholderLayerIfNeeded { ASDisplayNodeAssertMainThread(); - ASDisplayNodeAssertLockHeld(__instanceLock__); + ASAssertLocked(__instanceLock__); if (!_placeholderLayer) { _placeholderLayer = [CALayer layer]; @@ -2777,7 +2777,7 @@ - (void)__enterHierarchy { ASDisplayNodeAssertMainThread(); ASDisplayNodeAssert(!_flags.isEnteringHierarchy, @"Should not cause recursive __enterHierarchy"); - ASDisplayNodeAssertLockNotHeld(__instanceLock__); + ASAssertUnlocked(__instanceLock__); ASDisplayNodeLogEvent(self, @"enterHierarchy"); // Profiling has shown that locking this method is beneficial, so each of the property accesses don't have to lock and unlock. @@ -2826,7 +2826,7 @@ - (void)__exitHierarchy { ASDisplayNodeAssertMainThread(); ASDisplayNodeAssert(!_flags.isExitingHierarchy, @"Should not cause recursive __exitHierarchy"); - ASDisplayNodeAssertLockNotHeld(__instanceLock__); + ASAssertUnlocked(__instanceLock__); ASDisplayNodeLogEvent(self, @"exitHierarchy"); // Profiling has shown that locking this method is beneficial, so each of the property accesses don't have to lock and unlock. @@ -2933,7 +2933,7 @@ - (void)willEnterHierarchy ASDisplayNodeAssertMainThread(); ASDisplayNodeAssert(_flags.isEnteringHierarchy, @"You should never call -willEnterHierarchy directly. Appearance is automatically managed by ASDisplayNode"); ASDisplayNodeAssert(!_flags.isExitingHierarchy, @"ASDisplayNode inconsistency. __enterHierarchy and __exitHierarchy are mutually exclusive"); - ASDisplayNodeAssertLockNotHeld(__instanceLock__); + ASAssertUnlocked(__instanceLock__); if (![self supportsRangeManagedInterfaceState]) { self.interfaceState = ASInterfaceStateInHierarchy; @@ -2945,7 +2945,7 @@ - (void)didEnterHierarchy { ASDisplayNodeAssert(!_flags.isEnteringHierarchy, @"You should never call -didEnterHierarchy directly. Appearance is automatically managed by ASDisplayNode"); ASDisplayNodeAssert(!_flags.isExitingHierarchy, @"ASDisplayNode inconsistency. __enterHierarchy and __exitHierarchy are mutually exclusive"); ASDisplayNodeAssert(_flags.isInHierarchy, @"ASDisplayNode inconsistency. __enterHierarchy and __exitHierarchy are mutually exclusive"); - ASDisplayNodeAssertLockNotHeld(__instanceLock__); + ASAssertUnlocked(__instanceLock__); } - (void)didExitHierarchy @@ -2953,7 +2953,7 @@ - (void)didExitHierarchy ASDisplayNodeAssertMainThread(); ASDisplayNodeAssert(_flags.isExitingHierarchy, @"You should never call -didExitHierarchy directly. Appearance is automatically managed by ASDisplayNode"); ASDisplayNodeAssert(!_flags.isEnteringHierarchy, @"ASDisplayNode inconsistency. __enterHierarchy and __exitHierarchy are mutually exclusive"); - ASDisplayNodeAssertLockNotHeld(__instanceLock__); + ASAssertUnlocked(__instanceLock__); // This case is important when tearing down hierarchies. We must deliver a visibileStateDidChange:NO callback, as part our API guarantee that this method can be used for // things like data analytics about user content viewing. We cannot call the method in the dealloc as any incidental retain operations in client code would fail. @@ -3075,7 +3075,7 @@ - (void)applyPendingInterfaceState:(ASInterfaceState)newPendingState ASDisplayNodeAssertMainThread(); // This method manages __instanceLock__ itself, to ensure the lock is not held while didEnter/Exit(.*)State methods are called, thus avoid potential deadlocks - ASDisplayNodeAssertLockNotHeld(__instanceLock__); + ASAssertUnlocked(__instanceLock__); ASInterfaceState oldState = ASInterfaceStateNone; ASInterfaceState newState = ASInterfaceStateNone; @@ -3202,7 +3202,7 @@ - (void)prepareForCATransactionCommit - (void)interfaceStateDidChange:(ASInterfaceState)newState fromState:(ASInterfaceState)oldState { // Subclass hook - ASDisplayNodeAssertLockNotHeld(__instanceLock__); + ASAssertUnlocked(__instanceLock__); ASDisplayNodeAssertMainThread(); for (id delegate in _interfaceStateDelegates) { if ([delegate respondsToSelector:@selector(interfaceStateDidChange:fromState:)]) { @@ -3246,7 +3246,7 @@ - (void)didEnterVisibleState { // subclass override ASDisplayNodeAssertMainThread(); - ASDisplayNodeAssertLockNotHeld(__instanceLock__); + ASAssertUnlocked(__instanceLock__); for (id delegate in _interfaceStateDelegates) { if ([delegate respondsToSelector:@selector(didEnterVisibleState)]) { [delegate didEnterVisibleState]; @@ -3261,7 +3261,7 @@ - (void)didExitVisibleState { // subclass override ASDisplayNodeAssertMainThread(); - ASDisplayNodeAssertLockNotHeld(__instanceLock__); + ASAssertUnlocked(__instanceLock__); for (id delegate in _interfaceStateDelegates) { if ([delegate respondsToSelector:@selector(didExitVisibleState)]) { [delegate didExitVisibleState]; @@ -3279,7 +3279,7 @@ - (void)didEnterDisplayState { // subclass override ASDisplayNodeAssertMainThread(); - ASDisplayNodeAssertLockNotHeld(__instanceLock__); + ASAssertUnlocked(__instanceLock__); for (id delegate in _interfaceStateDelegates) { if ([delegate respondsToSelector:@selector(didEnterDisplayState)]) { [delegate didEnterDisplayState]; @@ -3291,7 +3291,7 @@ - (void)didExitDisplayState { // subclass override ASDisplayNodeAssertMainThread(); - ASDisplayNodeAssertLockNotHeld(__instanceLock__); + ASAssertUnlocked(__instanceLock__); for (id delegate in _interfaceStateDelegates) { if ([delegate respondsToSelector:@selector(didExitDisplayState)]) { [delegate didExitDisplayState]; @@ -3333,7 +3333,7 @@ - (void)recursivelyClearPreloadedData - (void)didEnterPreloadState { ASDisplayNodeAssertMainThread(); - ASDisplayNodeAssertLockNotHeld(__instanceLock__); + ASAssertUnlocked(__instanceLock__); // If this node has ASM enabled and is not yet visible, force a layout pass to apply its applicable pending layout, if any, // so that its subnodes are inserted/deleted and start preloading right away. @@ -3357,7 +3357,7 @@ - (void)didEnterPreloadState - (void)didExitPreloadState { ASDisplayNodeAssertMainThread(); - ASDisplayNodeAssertLockNotHeld(__instanceLock__); + ASAssertUnlocked(__instanceLock__); for (id delegate in _interfaceStateDelegates) { if ([delegate respondsToSelector:@selector(didExitPreloadState)]) { [delegate didExitPreloadState]; @@ -3462,7 +3462,7 @@ - (BOOL)pointInside:(CGPoint)point withEvent:(UIEvent *)event - (void)_locked_applyPendingStateToViewOrLayer { ASDisplayNodeAssertMainThread(); - ASDisplayNodeAssertLockHeld(__instanceLock__); + ASAssertLocked(__instanceLock__); ASDisplayNodeAssert(self.nodeLoaded, @"must have a view or layer"); TIME_SCOPED(_debugTimeToApplyPendingState); @@ -3482,7 +3482,7 @@ - (void)_locked_applyPendingStateToViewOrLayer - (void)applyPendingViewState { ASDisplayNodeAssertMainThread(); - ASDisplayNodeAssertLockNotHeld(__instanceLock__); + ASAssertUnlocked(__instanceLock__); ASDN::MutexLocker l(__instanceLock__); // FIXME: Ideally we'd call this as soon as the node receives -setNeedsLayout @@ -3501,7 +3501,7 @@ - (void)applyPendingViewState - (void)_locked_applyPendingViewState { ASDisplayNodeAssertMainThread(); - ASDisplayNodeAssertLockHeld(__instanceLock__); + ASAssertLocked(__instanceLock__); ASDisplayNodeAssert([self _locked_isNodeLoaded], @"Expected node to be loaded before applying pending state."); if (_flags.layerBacked) { diff --git a/Source/ASImageNode+AnimatedImage.mm b/Source/ASImageNode+AnimatedImage.mm index 9597090cd..bd085eabe 100644 --- a/Source/ASImageNode+AnimatedImage.mm +++ b/Source/ASImageNode+AnimatedImage.mm @@ -51,7 +51,7 @@ - (void)setAnimatedImage:(id )animatedImage - (void)_locked_setAnimatedImage:(id )animatedImage { - ASDisplayNodeAssertLockHeld(__instanceLock__); + ASAssertLocked(__instanceLock__); if (ASObjectIsEqual(_animatedImage, animatedImage) && (animatedImage == nil || animatedImage.playbackReady)) { return; @@ -127,7 +127,7 @@ - (void)setCoverImageCompleted:(UIImage *)coverImage - (void)_locked_setCoverImageCompleted:(UIImage *)coverImage { - ASDisplayNodeAssertLockHeld(__instanceLock__); + ASAssertLocked(__instanceLock__); _displayLinkLock.lock(); BOOL setCoverImage = (_displayLink == nil) || _displayLink.paused; @@ -146,7 +146,7 @@ - (void)setCoverImage:(UIImage *)coverImage - (void)_locked_setCoverImage:(UIImage *)coverImage { - ASDisplayNodeAssertLockHeld(__instanceLock__); + ASAssertLocked(__instanceLock__); //If we're a network image node, we want to set the default image so //that it will correctly be restored if it exits the range. @@ -189,7 +189,7 @@ - (void)setShouldAnimate:(BOOL)shouldAnimate - (void)_locked_setShouldAnimate:(BOOL)shouldAnimate { - ASDisplayNodeAssertLockHeld(__instanceLock__); + ASAssertLocked(__instanceLock__); // This test is explicitly done and not ASPerformBlockOnMainThread as this would perform the block immediately // on main if called on main thread and we have to call methods locked or unlocked based on which thread we are on @@ -224,7 +224,7 @@ - (void)startAnimating - (void)_locked_startAnimating { - ASDisplayNodeAssertLockHeld(__instanceLock__); + ASAssertLocked(__instanceLock__); // It should be safe to call self.interfaceState in this case as it will only grab the lock of the superclass if (!ASInterfaceStateIncludesVisible(self.interfaceState)) { @@ -269,7 +269,7 @@ - (void)stopAnimating - (void)_locked_stopAnimating { ASDisplayNodeAssertMainThread(); - ASDisplayNodeAssertLockHeld(__instanceLock__); + ASAssertLocked(__instanceLock__); #if ASAnimatedImageDebug NSLog(@"stopping animation: %p", self); diff --git a/Source/ASImageNode.mm b/Source/ASImageNode.mm index 3a7ef5500..d060d8993 100644 --- a/Source/ASImageNode.mm +++ b/Source/ASImageNode.mm @@ -244,7 +244,7 @@ - (void)setImage:(UIImage *)image - (void)_locked_setImage:(UIImage *)image { - ASDisplayNodeAssertLockHeld(__instanceLock__); + ASAssertLocked(__instanceLock__); if (ASObjectIsEqual(_image, image)) { return; } diff --git a/Source/ASNetworkImageNode.mm b/Source/ASNetworkImageNode.mm index 499c29a1e..cace5498e 100755 --- a/Source/ASNetworkImageNode.mm +++ b/Source/ASNetworkImageNode.mm @@ -150,7 +150,7 @@ - (void)setImage:(UIImage *)image - (void)_locked_setImage:(UIImage *)image { - ASDisplayNodeAssertLockHeld(__instanceLock__); + ASAssertLocked(__instanceLock__); BOOL imageWasSetExternally = (image != nil); BOOL shouldCancelAndClear = imageWasSetExternally && (imageWasSetExternally != _imageWasSetExternally); @@ -178,7 +178,7 @@ - (void)_setImage:(UIImage *)image - (void)_locked__setImage:(UIImage *)image { - ASDisplayNodeAssertLockHeld(__instanceLock__); + ASAssertLocked(__instanceLock__); [super _locked_setImage:image]; } @@ -512,7 +512,7 @@ - (void)_cancelDownloadAndClearImageWithResumePossibility:(BOOL)storeResume - (void)_locked_cancelDownloadAndClearImageWithResumePossibility:(BOOL)storeResume { - ASDisplayNodeAssertLockHeld(__instanceLock__); + ASAssertLocked(__instanceLock__); [self _locked_cancelImageDownloadWithResumePossibility:storeResume]; @@ -538,7 +538,7 @@ - (void)_cancelImageDownloadWithResumePossibility:(BOOL)storeResume - (void)_locked_cancelImageDownloadWithResumePossibility:(BOOL)storeResume { - ASDisplayNodeAssertLockHeld(__instanceLock__); + ASAssertLocked(__instanceLock__); if (!_downloadIdentifier) { return; diff --git a/Source/ASTextNode.mm b/Source/ASTextNode.mm index 7df3cc7ee..3ffcfe569 100644 --- a/Source/ASTextNode.mm +++ b/Source/ASTextNode.mm @@ -344,20 +344,20 @@ - (ASTextKitRenderer *)_rendererWithBounds:(CGRect)bounds - (ASTextKitRenderer *)_locked_renderer { - ASDisplayNodeAssertLockHeld(__instanceLock__); + ASAssertLocked(__instanceLock__); return [self _locked_rendererWithBounds:[self _locked_threadSafeBounds]]; } - (ASTextKitRenderer *)_locked_rendererWithBounds:(CGRect)bounds { - ASDisplayNodeAssertLockHeld(__instanceLock__); + ASAssertLocked(__instanceLock__); bounds = UIEdgeInsetsInsetRect(bounds, _textContainerInset); return rendererForAttributes([self _locked_rendererAttributes], bounds.size); } - (ASTextKitAttributes)_locked_rendererAttributes { - ASDisplayNodeAssertLockHeld(__instanceLock__); + ASAssertLocked(__instanceLock__); return { .attributedString = _attributedText, .truncationAttributedString = [self _locked_composedTruncationText], @@ -1276,7 +1276,7 @@ - (NSRange)_additionalTruncationMessageRangeWithVisibleRange:(NSRange)visibleRan */ - (NSAttributedString *)_locked_composedTruncationText { - ASDisplayNodeAssertLockHeld(__instanceLock__); + ASAssertLocked(__instanceLock__); if (_composedTruncationText == nil) { if (_truncationAttributedText != nil && _additionalTruncationMessage != nil) { NSMutableAttributedString *newComposedTruncationString = [[NSMutableAttributedString alloc] initWithAttributedString:_truncationAttributedText]; @@ -1302,7 +1302,7 @@ - (NSAttributedString *)_locked_composedTruncationText */ - (NSAttributedString *)_locked_prepareTruncationStringForDrawing:(NSAttributedString *)truncationString { - ASDisplayNodeAssertLockHeld(__instanceLock__); + ASAssertLocked(__instanceLock__); truncationString = ASCleanseAttributedStringOfCoreTextAttributes(truncationString); NSMutableAttributedString *truncationMutableString = [truncationString mutableCopy]; // Grab the attributes from the full string diff --git a/Source/ASTextNode2.mm b/Source/ASTextNode2.mm index fa6ca9808..7d3d8d7b3 100644 --- a/Source/ASTextNode2.mm +++ b/Source/ASTextNode2.mm @@ -1099,7 +1099,7 @@ - (NSRange)_additionalTruncationMessageRangeWithVisibleRange:(NSRange)visibleRan */ - (NSAttributedString *)_locked_composedTruncationText { - ASDisplayNodeAssertLockHeld(__instanceLock__); + ASAssertLocked(__instanceLock__); if (_composedTruncationText == nil) { if (_truncationAttributedText != nil && _additionalTruncationMessage != nil) { NSMutableAttributedString *newComposedTruncationString = [[NSMutableAttributedString alloc] initWithAttributedString:_truncationAttributedText]; @@ -1125,7 +1125,7 @@ - (NSAttributedString *)_locked_composedTruncationText */ - (NSAttributedString *)_locked_prepareTruncationStringForDrawing:(NSAttributedString *)truncationString { - ASDisplayNodeAssertLockHeld(__instanceLock__); + ASAssertLocked(__instanceLock__); NSMutableAttributedString *truncationMutableString = [truncationString mutableCopy]; // Grab the attributes from the full string if (_attributedText.length > 0) { diff --git a/Source/ASVideoPlayerNode.mm b/Source/ASVideoPlayerNode.mm index f74bec992..c864c10a9 100644 --- a/Source/ASVideoPlayerNode.mm +++ b/Source/ASVideoPlayerNode.mm @@ -335,7 +335,7 @@ - (void)cleanCachedControls - (void)_locked_createPlaybackButton { - ASDisplayNodeAssertLockHeld(__instanceLock__); + ASAssertLocked(__instanceLock__); if (_playbackButtonNode == nil) { _playbackButtonNode = [[ASDefaultPlaybackButton alloc] init]; @@ -360,7 +360,7 @@ - (void)_locked_createPlaybackButton - (void)_locked_createFullScreenButton { - ASDisplayNodeAssertLockHeld(__instanceLock__); + ASAssertLocked(__instanceLock__); if (_fullScreenButtonNode == nil) { _fullScreenButtonNode = [[ASButtonNode alloc] init]; @@ -379,7 +379,7 @@ - (void)_locked_createFullScreenButton - (void)_locked_createElapsedTextField { - ASDisplayNodeAssertLockHeld(__instanceLock__); + ASAssertLocked(__instanceLock__); if (_elapsedTextNode == nil) { _elapsedTextNode = [[ASTextNode alloc] init]; @@ -394,7 +394,7 @@ - (void)_locked_createElapsedTextField - (void)_locked_createDurationTextField { - ASDisplayNodeAssertLockHeld(__instanceLock__); + ASAssertLocked(__instanceLock__); if (_durationTextNode == nil) { _durationTextNode = [[ASTextNode alloc] init]; @@ -410,7 +410,7 @@ - (void)_locked_createDurationTextField - (void)_locked_createScrubber { - ASDisplayNodeAssertLockHeld(__instanceLock__); + ASAssertLocked(__instanceLock__); if (_scrubberNode == nil) { __weak __typeof__(self) weakSelf = self; @@ -456,7 +456,7 @@ - (void)_locked_createScrubber - (void)_locked_createControlFlexGrowSpacer { - ASDisplayNodeAssertLockHeld(__instanceLock__); + ASAssertLocked(__instanceLock__); if (_controlFlexGrowSpacerSpec == nil) { _controlFlexGrowSpacerSpec = [[ASStackLayoutSpec alloc] init]; diff --git a/Source/Details/ASThread.h b/Source/Details/ASThread.h index e077532cb..362f96c5b 100644 --- a/Source/Details/ASThread.h +++ b/Source/Details/ASThread.h @@ -108,7 +108,7 @@ ASDISPLAYNODE_INLINE void _ASUnlockScopeCleanup(id __strong *lockPtr) * Enable this flag to collect information on the owning thread and ownership level of a mutex. * These properties are useful to determine if a mutext has been acquired and in case of a recursive mutex, how many times that happened. * - * This flag also enable locking assertions (e.g ASDisplayNodeAssertLockNotHeld(node)). + * This flag also enable locking assertions (e.g ASAssertUnlocked(node)). * The assertions are useful when you want to indicate and enforce the locking policy/expectation of methods. * To determine when and which methods acquired a (recursive) mutex (to debug deadlocks, for example), * put breakpoints at some assertions. When the breakpoints hit, walk through stack trace frames @@ -138,11 +138,11 @@ ASDISPLAYNODE_INLINE void _ASUnlockScopeCleanup(id __strong *lockPtr) * and check ownership count of the mutex. */ #if CHECK_LOCKING_SAFETY -#define ASDisplayNodeAssertLockNotHeld(lock) ASDisplayNodeAssertFalse(lock.locked()) -#define ASDisplayNodeAssertLockHeld(lock) ASDisplayNodeAssert(lock.locked(), @"Lock must be held by current thread") +#define ASAssertUnlocked(lock) ASDisplayNodeAssertFalse(lock.locked()) +#define ASAssertLocked(lock) ASDisplayNodeAssert(lock.locked(), @"Lock must be held by current thread") #else -#define ASDisplayNodeAssertLockNotHeld(lock) -#define ASDisplayNodeAssertLockHeld(lock) +#define ASAssertUnlocked(lock) +#define ASAssertLocked(lock) #endif namespace ASDN { diff --git a/Source/Private/ASDisplayNode+UIViewBridge.mm b/Source/Private/ASDisplayNode+UIViewBridge.mm index 9c46a5ceb..3d99f425e 100644 --- a/Source/Private/ASDisplayNode+UIViewBridge.mm +++ b/Source/Private/ASDisplayNode+UIViewBridge.mm @@ -975,7 +975,7 @@ - (void)setLayerCornerRadius:(CGFloat)newLayerCornerRadius - (BOOL)_locked_insetsLayoutMarginsFromSafeArea { - ASDisplayNodeAssertLockHeld(__instanceLock__); + ASAssertLocked(__instanceLock__); if (AS_AVAILABLE_IOS(11.0)) { if (!_flags.layerBacked) { return _getFromViewOnly(insetsLayoutMarginsFromSafeArea); From 48b7fa1a0c5b7162709b1603a954afbfab27d7a0 Mon Sep 17 00:00:00 2001 From: Huy Nguyen Date: Thu, 12 Jul 2018 14:17:06 -0700 Subject: [PATCH 3/7] Revert unnecessary change --- Source/ASDisplayNode+Layout.mm | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/Source/ASDisplayNode+Layout.mm b/Source/ASDisplayNode+Layout.mm index 8232c66c0..f6179260d 100644 --- a/Source/ASDisplayNode+Layout.mm +++ b/Source/ASDisplayNode+Layout.mm @@ -437,19 +437,19 @@ - (ASSizeRange)_locked_constrainedSizeForLayoutPass ASAssertLocked(__instanceLock__); CGSize boundsSizeForLayout = ASCeilSizeValues(self.threadSafeBounds.size); - std::shared_ptr pendingLayout = _pendingDisplayNodeLayout; - std::shared_ptr calculatedLayout = _calculatedDisplayNodeLayout; - + // Checkout if constrained size of pending or calculated display node layout can be used - if (pendingLayout != nullptr - && (pendingLayout->requestedLayoutFromAbove || CGSizeEqualToSize(pendingLayout->layout.size, boundsSizeForLayout))) { + if (_pendingDisplayNodeLayout != nullptr + && (_pendingDisplayNodeLayout->requestedLayoutFromAbove + || CGSizeEqualToSize(_pendingDisplayNodeLayout->layout.size, boundsSizeForLayout))) { // We assume the size from the last returned layoutThatFits: layout was applied so use the pending display node // layout constrained size - return pendingLayout->constrainedSize; - } else if (calculatedLayout->layout != nil - && (calculatedLayout->requestedLayoutFromAbove || CGSizeEqualToSize(calculatedLayout->layout.size, boundsSizeForLayout))) { + return _pendingDisplayNodeLayout->constrainedSize; + } else if (_calculatedDisplayNodeLayout->layout != nil + && (_calculatedDisplayNodeLayout->requestedLayoutFromAbove + || CGSizeEqualToSize(_calculatedDisplayNodeLayout->layout.size, boundsSizeForLayout))) { // We assume the _calculatedDisplayNodeLayout is still valid and the frame is not different - return calculatedLayout->constrainedSize; + return _calculatedDisplayNodeLayout->constrainedSize; } else { // In this case neither the _pendingDisplayNodeLayout or the _calculatedDisplayNodeLayout constrained size can // be reused, so the current bounds is used. This is usual the case if a frame was set manually that differs to @@ -663,9 +663,9 @@ - (void)transitionLayoutWithSizeRange:(ASSizeRange)constrainedSize // Update calculated layout let previousLayout = _calculatedDisplayNodeLayout; let pendingLayout = std::make_shared(newLayout, - constrainedSize, - constrainedSize.max, - newLayoutVersion); + constrainedSize, + constrainedSize.max, + newLayoutVersion); [self _locked_setCalculatedDisplayNodeLayout:pendingLayout]; // Setup pending layout transition for animation From b4543880df116d4895fa959eac346b07f792f483 Mon Sep 17 00:00:00 2001 From: Huy Nguyen Date: Thu, 12 Jul 2018 14:25:59 -0700 Subject: [PATCH 4/7] Rename -_isNodeLoaded to -_unsafe_unlocked_isNodeLoaded --- Source/ASDisplayNode.mm | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Source/ASDisplayNode.mm b/Source/ASDisplayNode.mm index 36a1f4441..c4bf38aa6 100644 --- a/Source/ASDisplayNode.mm +++ b/Source/ASDisplayNode.mm @@ -590,7 +590,7 @@ - (BOOL)isNodeLoaded if (ASDisplayNodeThreadIsMain()) { // Because the view and layer can only be created and destroyed on Main, that is also the only thread // where the state of this property can change. As an optimization, we can avoid locking. - return [self _isNodeLoaded]; + return [self _unsafe_unlocked_isNodeLoaded]; } else { ASDN::MutexLocker l(__instanceLock__); return [self _locked_isNodeLoaded]; @@ -603,7 +603,8 @@ - (BOOL)_locked_isNodeLoaded return (_view != nil || (_layer != nil && _flags.layerBacked)); } -- (BOOL)_isNodeLoaded +/// Called without the lock. Client is responsible for locking safety. +- (BOOL)_unsafe_unlocked_isNodeLoaded { return (_view != nil || (_layer != nil && _flags.layerBacked)); } From 49c21ae767ba40ce74704fa4cbdca475afd29f4c Mon Sep 17 00:00:00 2001 From: Huy Nguyen Date: Thu, 12 Jul 2018 17:53:57 -0700 Subject: [PATCH 5/7] Use inline method --- Source/ASDisplayNode.mm | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/Source/ASDisplayNode.mm b/Source/ASDisplayNode.mm index c4bf38aa6..e315c6057 100644 --- a/Source/ASDisplayNode.mm +++ b/Source/ASDisplayNode.mm @@ -590,7 +590,7 @@ - (BOOL)isNodeLoaded if (ASDisplayNodeThreadIsMain()) { // Because the view and layer can only be created and destroyed on Main, that is also the only thread // where the state of this property can change. As an optimization, we can avoid locking. - return [self _unsafe_unlocked_isNodeLoaded]; + return _ASIsNodeLoaded(self); } else { ASDN::MutexLocker l(__instanceLock__); return [self _locked_isNodeLoaded]; @@ -600,13 +600,13 @@ - (BOOL)isNodeLoaded - (BOOL)_locked_isNodeLoaded { ASAssertLocked(__instanceLock__); - return (_view != nil || (_layer != nil && _flags.layerBacked)); + return _ASIsNodeLoaded(self); } -/// Called without the lock. Client is responsible for locking safety. -- (BOOL)_unsafe_unlocked_isNodeLoaded +/// Can be called without the node's lock. Client is responsible for thread safety. +ASDISPLAYNODE_INLINE AS_WARN_UNUSED_RESULT BOOL _ASIsNodeLoaded(ASDisplayNode *node) { - return (_view != nil || (_layer != nil && _flags.layerBacked)); + return (node->_view != nil || (node->_layer != nil && node->_flags.layerBacked)); } #pragma mark - Misc Setter / Getter From 0325062ea2bfe898d2ad2efdbb4319e8cbdb85e7 Mon Sep 17 00:00:00 2001 From: Huy Nguyen Date: Fri, 13 Jul 2018 13:00:26 -0700 Subject: [PATCH 6/7] Move __loaded(node) macro to ASDisplayNodeInternal and use it throughout --- Source/ASDisplayNode.mm | 10 ++------ Source/Private/ASDisplayNode+UIViewBridge.mm | 24 +++++++++----------- Source/Private/ASDisplayNodeInternal.h | 3 +++ 3 files changed, 16 insertions(+), 21 deletions(-) diff --git a/Source/ASDisplayNode.mm b/Source/ASDisplayNode.mm index e315c6057..057e49af8 100644 --- a/Source/ASDisplayNode.mm +++ b/Source/ASDisplayNode.mm @@ -590,7 +590,7 @@ - (BOOL)isNodeLoaded if (ASDisplayNodeThreadIsMain()) { // Because the view and layer can only be created and destroyed on Main, that is also the only thread // where the state of this property can change. As an optimization, we can avoid locking. - return _ASIsNodeLoaded(self); + return _loaded(self); } else { ASDN::MutexLocker l(__instanceLock__); return [self _locked_isNodeLoaded]; @@ -600,13 +600,7 @@ - (BOOL)isNodeLoaded - (BOOL)_locked_isNodeLoaded { ASAssertLocked(__instanceLock__); - return _ASIsNodeLoaded(self); -} - -/// Can be called without the node's lock. Client is responsible for thread safety. -ASDISPLAYNODE_INLINE AS_WARN_UNUSED_RESULT BOOL _ASIsNodeLoaded(ASDisplayNode *node) -{ - return (node->_view != nil || (node->_layer != nil && node->_flags.layerBacked)); + return _loaded(self); } #pragma mark - Misc Setter / Getter diff --git a/Source/Private/ASDisplayNode+UIViewBridge.mm b/Source/Private/ASDisplayNode+UIViewBridge.mm index 3d99f425e..55d970467 100644 --- a/Source/Private/ASDisplayNode+UIViewBridge.mm +++ b/Source/Private/ASDisplayNode+UIViewBridge.mm @@ -41,8 +41,6 @@ #define DISPLAYNODE_USE_LOCKS 1 -#define __loaded(node) (node->_view != nil || (node->_layer != nil && node->_flags.layerBacked)) - #if DISPLAYNODE_USE_LOCKS #define _bridge_prologue_read ASDN::MutexLocker l(__instanceLock__); ASDisplayNodeAssertThreadAffinity(self) #define _bridge_prologue_write ASDN::MutexLocker l(__instanceLock__) @@ -59,7 +57,7 @@ /// returns NO. Otherwise, the pending state can be scheduled and flushed *before* you get a chance /// to apply it. ASDISPLAYNODE_INLINE BOOL ASDisplayNodeShouldApplyBridgedWriteToView(ASDisplayNode *node) { - BOOL loaded = __loaded(node); + BOOL loaded = _loaded(node); if (ASDisplayNodeThreadIsMain()) { return loaded; } else { @@ -70,7 +68,7 @@ ASDISPLAYNODE_INLINE BOOL ASDisplayNodeShouldApplyBridgedWriteToView(ASDisplayNo } }; -#define _getFromViewOrLayer(layerProperty, viewAndPendingViewStateProperty) __loaded(self) ? \ +#define _getFromViewOrLayer(layerProperty, viewAndPendingViewStateProperty) _loaded(self) ? \ (_view ? _view.viewAndPendingViewStateProperty : _layer.layerProperty )\ : ASDisplayNodeGetPendingState(self).viewAndPendingViewStateProperty @@ -80,9 +78,9 @@ ASDISPLAYNODE_INLINE BOOL ASDisplayNodeShouldApplyBridgedWriteToView(ASDisplayNo #define _setToViewOnly(viewAndPendingViewStateProperty, viewAndPendingViewStateExpr) BOOL shouldApply = ASDisplayNodeShouldApplyBridgedWriteToView(self); \ if (shouldApply) { _view.viewAndPendingViewStateProperty = (viewAndPendingViewStateExpr); } else { ASDisplayNodeGetPendingState(self).viewAndPendingViewStateProperty = (viewAndPendingViewStateExpr); } -#define _getFromViewOnly(viewAndPendingViewStateProperty) __loaded(self) ? _view.viewAndPendingViewStateProperty : ASDisplayNodeGetPendingState(self).viewAndPendingViewStateProperty +#define _getFromViewOnly(viewAndPendingViewStateProperty) _loaded(self) ? _view.viewAndPendingViewStateProperty : ASDisplayNodeGetPendingState(self).viewAndPendingViewStateProperty -#define _getFromLayer(layerProperty) __loaded(self) ? _layer.layerProperty : ASDisplayNodeGetPendingState(self).layerProperty +#define _getFromLayer(layerProperty) _loaded(self) ? _layer.layerProperty : ASDisplayNodeGetPendingState(self).layerProperty #define _setToLayer(layerProperty, layerValueExpr) BOOL shouldApply = ASDisplayNodeShouldApplyBridgedWriteToView(self); \ if (shouldApply) { _layer.layerProperty = (layerValueExpr); } else { ASDisplayNodeGetPendingState(self).layerProperty = (layerValueExpr); } @@ -304,7 +302,7 @@ - (void)setFrame:(CGRect)rect struct ASDisplayNodeFlags flags = _flags; BOOL specialPropertiesHandling = ASDisplayNodeNeedsSpecialPropertiesHandling(checkFlag(Synchronous), flags.layerBacked); - BOOL nodeLoaded = __loaded(self); + BOOL nodeLoaded = _loaded(self); BOOL isMainThread = ASDisplayNodeThreadIsMain(); if (!specialPropertiesHandling) { BOOL canReadProperties = isMainThread || !nodeLoaded; @@ -416,7 +414,7 @@ - (void)setNeedsLayout { _bridge_prologue_write; shouldApply = ASDisplayNodeShouldApplyBridgedWriteToView(self); - loaded = __loaded(self); + loaded = _loaded(self); viewOrLayer = _view ?: _layer; if (shouldApply == NO && loaded) { // The node is loaded but we're not on main. @@ -447,7 +445,7 @@ - (void)layoutIfNeeded { _bridge_prologue_write; shouldApply = ASDisplayNodeShouldApplyBridgedWriteToView(self); - loaded = __loaded(self); + loaded = _loaded(self); viewOrLayer = _view ?: _layer; if (shouldApply == NO && loaded) { // The node is loaded but we're not on main. @@ -658,7 +656,7 @@ - (void)setAutoresizingMask:(UIViewAutoresizing)mask - (UIViewContentMode)contentMode { _bridge_prologue_read; - if (__loaded(self)) { + if (_loaded(self)) { if (_flags.layerBacked) { return ASDisplayNodeUIContentModeFromCAContentsGravity(_layer.contentsGravity); } else { @@ -909,7 +907,7 @@ - (UIEdgeInsets)safeAreaInsets _bridge_prologue_read; if (AS_AVAILABLE_IOS(11.0)) { - if (!_flags.layerBacked && __loaded(self)) { + if (!_flags.layerBacked && _loaded(self)) { return self.view.safeAreaInsets; } } @@ -938,7 +936,7 @@ - (void)setInsetsLayoutMarginsFromSafeArea:(BOOL)insetsLayoutMarginsFromSafeArea } } - shouldNotifyAboutUpdate = __loaded(self) && (!AS_AT_LEAST_IOS11 || _flags.layerBacked); + shouldNotifyAboutUpdate = _loaded(self) && (!AS_AT_LEAST_IOS11 || _flags.layerBacked); } if (shouldNotifyAboutUpdate) { @@ -997,7 +995,7 @@ - (BOOL)_locked_insetsLayoutMarginsFromSafeArea // - In case the node is loaded // - Check if the node has a view and get the value from the view if loaded or from the pending state // - If view is not available, e.g. the node is layer backed return the property value -#define _getAccessibilityFromViewOrProperty(nodeProperty, viewAndPendingViewStateProperty) __loaded(self) ? \ +#define _getAccessibilityFromViewOrProperty(nodeProperty, viewAndPendingViewStateProperty) _loaded(self) ? \ (_view ? _view.viewAndPendingViewStateProperty : nodeProperty )\ : ASDisplayNodeGetPendingState(self).viewAndPendingViewStateProperty diff --git a/Source/Private/ASDisplayNodeInternal.h b/Source/Private/ASDisplayNodeInternal.h index 252202d51..ed644b964 100644 --- a/Source/Private/ASDisplayNodeInternal.h +++ b/Source/Private/ASDisplayNodeInternal.h @@ -65,6 +65,9 @@ typedef NS_OPTIONS(uint_least32_t, ASDisplayNodeAtomicFlags) YogaLayoutInProgress = 1 << 1, }; +// Can be called without the node's lock. Client is responsible for thread safety. +#define _loaded(node) (node->_view != nil || (node->_layer != nil && node->_flags.layerBacked)) + #define checkFlag(flag) ((_atomicFlags.load() & flag) != 0) // Returns the old value of the flag as a BOOL. #define setFlag(flag, x) (((x ? _atomicFlags.fetch_or(flag) \ From f4234df038ae4844f2a35c642917a92b7a57cb79 Mon Sep 17 00:00:00 2001 From: Huy Nguyen Date: Fri, 13 Jul 2018 13:07:04 -0700 Subject: [PATCH 7/7] Simpler node loaded check: regardless of whether the node is view or layer backed, the layer should always be set if loaded --- Source/Private/ASDisplayNodeInternal.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Source/Private/ASDisplayNodeInternal.h b/Source/Private/ASDisplayNodeInternal.h index ed644b964..4c668a3f3 100644 --- a/Source/Private/ASDisplayNodeInternal.h +++ b/Source/Private/ASDisplayNodeInternal.h @@ -66,7 +66,7 @@ typedef NS_OPTIONS(uint_least32_t, ASDisplayNodeAtomicFlags) }; // Can be called without the node's lock. Client is responsible for thread safety. -#define _loaded(node) (node->_view != nil || (node->_layer != nil && node->_flags.layerBacked)) +#define _loaded(node) (node->_layer != nil) #define checkFlag(flag) ((_atomicFlags.load() & flag) != 0) // Returns the old value of the flag as a BOOL.