Skip to content

Commit 0f50cd0

Browse files
nguyenhuyAdlai Holler
authored andcommitted
Fix locking situation of layout transition (facebookarchive#3217)
- It's not safe to hold the lock of a supernode and: - Edit states of its subnodes - Call subclass hooks - Run completion block - Run animation, which can trigger layout passes on subnodes, especially if one of them is a collection view.
1 parent ead2086 commit 0f50cd0

1 file changed

Lines changed: 41 additions & 27 deletions

File tree

Source/ASDisplayNode.mm

Lines changed: 41 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -1305,14 +1305,19 @@ - (ASLayout *)calculatedLayout
13051305
- (void)_setCalculatedDisplayNodeLayout:(std::shared_ptr<ASDisplayNodeLayout>)displayNodeLayout
13061306
{
13071307
ASDN::MutexLocker l(__instanceLock__);
1308-
1308+
[self _locked_setCalculatedDisplayNodeLayout:displayNodeLayout];
1309+
}
1310+
1311+
- (void)_locked_setCalculatedDisplayNodeLayout:(std::shared_ptr<ASDisplayNodeLayout>)displayNodeLayout
1312+
{
13091313
ASDisplayNodeAssertTrue(displayNodeLayout->layout.layoutElement == self);
13101314
ASDisplayNodeAssertTrue(displayNodeLayout->layout.size.width >= 0.0);
13111315
ASDisplayNodeAssertTrue(displayNodeLayout->layout.size.height >= 0.0);
13121316

13131317
_calculatedDisplayNodeLayout = displayNodeLayout;
13141318
}
13151319

1320+
13161321
- (CGSize)calculatedSize
13171322
{
13181323
ASDN::MutexLocker l(__instanceLock__);
@@ -1540,22 +1545,35 @@ - (void)transitionLayoutWithSizeRange:(ASSizeRange)constrainedSize
15401545
}
15411546

15421547
ASPerformBlockOnMainThread(^{
1543-
// Grab __instanceLock__ here to make sure this transition isn't invalidated
1544-
// right after it passed the validation test and before it proceeds
1545-
ASDN::MutexLocker l(__instanceLock__);
1546-
1547-
if ([self _shouldAbortTransitionWithID:transitionID]) {
1548-
return;
1548+
ASLayoutTransition *pendingLayoutTransition;
1549+
_ASTransitionContext *pendingLayoutTransitionContext;
1550+
{
1551+
// Grab __instanceLock__ here to make sure this transition isn't invalidated
1552+
// right after it passed the validation test and before it proceeds
1553+
ASDN::MutexLocker l(__instanceLock__);
1554+
1555+
if ([self _locked_shouldAbortTransitionWithID:transitionID]) {
1556+
return;
1557+
}
1558+
1559+
// Update calculated layout
1560+
auto previousLayout = _calculatedDisplayNodeLayout;
1561+
auto pendingLayout = std::make_shared<ASDisplayNodeLayout>(
1562+
newLayout,
1563+
constrainedSize,
1564+
constrainedSize.max
1565+
);
1566+
[self _locked_setCalculatedDisplayNodeLayout:pendingLayout];
1567+
1568+
// Setup pending layout transition for animation
1569+
_pendingLayoutTransition = pendingLayoutTransition = [[ASLayoutTransition alloc] initWithNode:self
1570+
pendingLayout:pendingLayout
1571+
previousLayout:previousLayout];
1572+
// Setup context for pending layout transition. we need to hold a strong reference to the context
1573+
_pendingLayoutTransitionContext = pendingLayoutTransitionContext = [[_ASTransitionContext alloc] initWithAnimation:animated
1574+
layoutDelegate:_pendingLayoutTransition
1575+
completionDelegate:self];
15491576
}
1550-
1551-
// Update calculated layout
1552-
auto previousLayout = _calculatedDisplayNodeLayout;
1553-
auto pendingLayout = std::make_shared<ASDisplayNodeLayout>(
1554-
newLayout,
1555-
constrainedSize,
1556-
constrainedSize.max
1557-
);
1558-
[self _setCalculatedDisplayNodeLayout:pendingLayout];
15591577

15601578
// Apply complete layout transitions for all subnodes
15611579
ASDisplayNodePerformBlockOnEverySubnode(self, NO, ^(ASDisplayNode * _Nonnull node) {
@@ -1570,20 +1588,11 @@ - (void)transitionLayoutWithSizeRange:(ASSizeRange)constrainedSize
15701588
completion();
15711589
}
15721590

1573-
// Setup pending layout transition for animation
1574-
_pendingLayoutTransition = [[ASLayoutTransition alloc] initWithNode:self
1575-
pendingLayout:pendingLayout
1576-
previousLayout:previousLayout];
1577-
// Setup context for pending layout transition. we need to hold a strong reference to the context
1578-
_pendingLayoutTransitionContext = [[_ASTransitionContext alloc] initWithAnimation:animated
1579-
layoutDelegate:_pendingLayoutTransition
1580-
completionDelegate:self];
1581-
15821591
// Apply the subnode insertion immediately to be able to animate the nodes
1583-
[_pendingLayoutTransition applySubnodeInsertions];
1592+
[pendingLayoutTransition applySubnodeInsertions];
15841593

15851594
// Kick off animating the layout transition
1586-
[self animateLayoutTransition:_pendingLayoutTransitionContext];
1595+
[self animateLayoutTransition:pendingLayoutTransitionContext];
15871596

15881597
// Mark transaction as finished
15891598
[self _finishOrCancelTransition];
@@ -1669,6 +1678,11 @@ - (int32_t)pendingTransitionID
16691678
- (BOOL)_shouldAbortTransitionWithID:(int32_t)transitionID
16701679
{
16711680
ASDN::MutexLocker l(__instanceLock__);
1681+
return [self _locked_shouldAbortTransitionWithID:transitionID];
1682+
}
1683+
1684+
- (BOOL)_locked_shouldAbortTransitionWithID:(int32_t)transitionID
1685+
{
16721686
return (!_transitionInProgress || _transitionID != transitionID);
16731687
}
16741688

0 commit comments

Comments
 (0)