Skip to content

Commit

Permalink
Stricter locking assertions
Browse files Browse the repository at this point in the history 
- 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 TextureGroup#1022 and TextureGroup#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.
  • Loading branch information
@nguyenhuy
nguyenhuy committed Jul 12, 2018
1 parent 814fc38 commit c037799
Show file tree
Hide file tree
Showing 13 changed files with 173 additions and 82 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
106 changes: 60 additions & 46 deletions Source/ASDisplayNode+Layout.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,7 @@ - (ASSizeRange)constrainedSizeForCalculatedLayout

- (ASSizeRange)_locked_constrainedSizeForCalculatedLayout
{
ASDisplayNodeAssertLockHeld(__instanceLock__);
if (_pendingDisplayNodeLayout != nullptr && _pendingDisplayNodeLayout->isValid(_layoutVersion)) {
return _pendingDisplayNodeLayout->constrainedSize;
}
Expand Down Expand Up @@ -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];
Expand All @@ -243,7 +245,7 @@ - (void)_u_setNeedsLayoutFromAbove
- (void)_rootNodeDidInvalidateSize
{
ASDisplayNodeAssertThreadAffinity(self);
ASDisplayNodeAssertLockUnownedByCurrentThread(__instanceLock__);
ASDisplayNodeAssertLockNotHeld(__instanceLock__);

__instanceLock__.lock();

Expand Down Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -328,38 +330,42 @@ - (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) {
context = [[ASLayoutElementContext alloc] init];
ASLayoutElementPushContext(context);
didCreateNewContext = YES;
}

// Figure out previous and pending layouts for layout transition
std::shared_ptr<ASDisplayNodeLayout> 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));
} else {
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).
Expand All @@ -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
Expand All @@ -390,49 +396,60 @@ - (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
pendingLayout:nextLayout
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).
[self _completePendingLayoutTransition];
}
}

- (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<ASDisplayNodeLayout> pendingLayout = _pendingDisplayNodeLayout;
std::shared_ptr<ASDisplayNodeLayout> 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
Expand All @@ -444,7 +461,7 @@ - (ASSizeRange)_locked_constrainedSizeForLayoutPass
- (void)_layoutSublayouts
{
ASDisplayNodeAssertThreadAffinity(self);
ASDisplayNodeAssertLockUnownedByCurrentThread(__instanceLock__);
ASDisplayNodeAssertLockNotHeld(__instanceLock__);

ASLayout *layout;
{
Expand Down Expand Up @@ -503,6 +520,7 @@ - (BOOL)_isLayoutTransitionInvalid

- (BOOL)_locked_isLayoutTransitionInvalid
{
ASDisplayNodeAssertLockHeld(__instanceLock__);
if (ASHierarchyStateIncludesLayoutPending(_hierarchyState)) {
ASLayoutElementContext *context = ASLayoutElementGetCurrentContext();
if (context == nil || _pendingTransitionID != context.transitionID) {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -865,8 +875,8 @@ - (void)didCompleteLayoutTransition:(id<ASContextTransitioning>)context
*/
- (void)_completePendingLayoutTransition
{
ASDisplayNodeAssertLockUnownedByCurrentThread(__instanceLock);
ASDisplayNodeAssertLockNotHeld(__instanceLock__);

ASLayoutTransition *pendingLayoutTransition = nil;
{
ASDN::MutexLocker l(__instanceLock__);
Expand All @@ -875,7 +885,7 @@ - (void)_completePendingLayoutTransition
[self _locked_setCalculatedDisplayNodeLayout:pendingLayoutTransition.pendingLayout];
}
}

if (pendingLayoutTransition != nil) {
[self _completeLayoutTransition:pendingLayoutTransition];
[self _pendingLayoutTransitionDidComplete];
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -998,6 +1011,7 @@ - (void)_setCalculatedDisplayNodeLayout:(std::shared_ptr<ASDisplayNodeLayout>)di

- (void)_locked_setCalculatedDisplayNodeLayout:(std::shared_ptr<ASDisplayNodeLayout>)displayNodeLayout
{
ASDisplayNodeAssertLockHeld(__instanceLock__);
ASDisplayNodeAssertTrue(displayNodeLayout->layout.layoutElement == self);
ASDisplayNodeAssertTrue(displayNodeLayout->layout.size.width >= 0.0);
ASDisplayNodeAssertTrue(displayNodeLayout->layout.size.height >= 0.0);
Expand Down
Loading

0 comments on commit c037799

Please sign in to comment.