Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Stricter locking assertions #1024

Merged
merged 9 commits into from
Jul 13, 2018
Merged

Stricter locking assertions #1024

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
c037799
Stricter locking assertions
nguyenhuy Jul 12, 2018
c2bdd1a
Rename to ASAssertLocked and ASAssertUnlocked
nguyenhuy Jul 12, 2018
48b7fa1
Revert unnecessary change
nguyenhuy Jul 12, 2018
b454388
Rename -_isNodeLoaded to -_unsafe_unlocked_isNodeLoaded
nguyenhuy Jul 12, 2018
49c21ae
Use inline method
nguyenhuy Jul 13, 2018
372ff89
Merge branch 'master' into HN-Stricter-Locking-Safety-Checks
Adlai-Holler Jul 13, 2018
0325062
Move __loaded(node) macro to ASDisplayNodeInternal and use it throughout
nguyenhuy Jul 13, 2018
4dd528f
Merge branch 'HN-Stricter-Locking-Safety-Checks' of github.com:nguyen…
nguyenhuy Jul 13, 2018
f4234df
Simpler node loaded check: regardless of whether the node is view or …
nguyenhuy Jul 13, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
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
100 changes: 57 additions & 43 deletions Source/ASDisplayNode+Layout.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,7 @@ - (ASSizeRange)constrainedSizeForCalculatedLayout

- (ASSizeRange)_locked_constrainedSizeForCalculatedLayout
{
ASAssertLocked(__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);
ASAssertUnlocked(__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__);
ASAssertUnlocked(__instanceLock__);

__instanceLock__.lock();

Expand Down Expand Up @@ -273,7 +275,7 @@ - (void)_rootNodeDidInvalidateSize
- (void)displayNodeDidInvalidateSizeNewSize:(CGSize)size
{
ASDisplayNodeAssertThreadAffinity(self);
ASDisplayNodeAssertLockUnownedByCurrentThread(__instanceLock__);
ASAssertUnlocked(__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);
ASAssertUnlocked(__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,41 +396,52 @@ - (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


ASAssertLocked(__instanceLock__);

CGSize boundsSizeForLayout = ASCeilSizeValues(self.threadSafeBounds.size);

// Checkout if constrained size of pending or calculated display node layout can be used
if (_pendingDisplayNodeLayout != nullptr
&& (_pendingDisplayNodeLayout->requestedLayoutFromAbove
|| CGSizeEqualToSize(_pendingDisplayNodeLayout->layout.size, boundsSizeForLayout))) {
|| 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 _pendingDisplayNodeLayout->constrainedSize;
Expand All @@ -444,7 +461,7 @@ - (ASSizeRange)_locked_constrainedSizeForLayoutPass
- (void)_layoutSublayouts
{
ASDisplayNodeAssertThreadAffinity(self);
ASDisplayNodeAssertLockUnownedByCurrentThread(__instanceLock__);
ASAssertUnlocked(__instanceLock__);

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

- (BOOL)_locked_isLayoutTransitionInvalid
{
ASAssertLocked(__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]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Easier to just call the "unlocked" variant.

animated:animated
shouldMeasureAsync:shouldMeasureAsync
measurementCompletion:completion];

}

- (void)transitionLayoutWithSizeRange:(ASSizeRange)constrainedSize
Expand Down Expand Up @@ -653,9 +663,9 @@ - (void)transitionLayoutWithSizeRange:(ASSizeRange)constrainedSize
// Update calculated layout
let previousLayout = _calculatedDisplayNodeLayout;
let pendingLayout = std::make_shared<ASDisplayNodeLayout>(newLayout,
constrainedSize,
constrainedSize.max,
newLayoutVersion);
constrainedSize,
constrainedSize.max,
newLayoutVersion);
[self _locked_setCalculatedDisplayNodeLayout:pendingLayout];

// Setup pending layout transition for animation
Expand Down Expand Up @@ -865,8 +875,8 @@ - (void)didCompleteLayoutTransition:(id<ASContextTransitioning>)context
*/
- (void)_completePendingLayoutTransition
{
ASDisplayNodeAssertLockUnownedByCurrentThread(__instanceLock);
ASAssertUnlocked(__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
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
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
ASAssertUnlocked(__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
{
ASAssertLocked(__instanceLock__);
ASDisplayNodeAssertTrue(displayNodeLayout->layout.layoutElement == self);
ASDisplayNodeAssertTrue(displayNodeLayout->layout.size.width >= 0.0);
ASDisplayNodeAssertTrue(displayNodeLayout->layout.size.height >= 0.0);
Expand Down
Loading