From 515e8b9c812786055c2343ae30a9c1e123482f42 Mon Sep 17 00:00:00 2001 From: Michael Schneider Date: Mon, 24 Sep 2018 14:06:38 -0700 Subject: [PATCH] Fix crash if setting attributed text on multiple threads --- CHANGELOG.md | 1 + Source/ASDisplayNode+Layout.mm | 5 +++ Source/ASTextNode.mm | 56 +++++++++++++++----------- Source/ASTextNode2.mm | 16 +++++--- Source/Private/ASDisplayNodeInternal.h | 9 +++++ 5 files changed, 59 insertions(+), 28 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 85b2f0843..6503a5085 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -53,6 +53,7 @@ - Remove display node's reliance on shared_ptr. [Adlai Holler](https://github.com/Adlai-Holler) - [ASCollectionView] Fix a crash that is caused by clearing a collection view's data while it's still being used. [Huy Nguyen](https://github.com/nguyenhuy) [#1154](https://github.com/TextureGroup/Texture/pull/1154) - Clean up timing of layout tree flattening/ copying of unflattened tree for Weaver. [Michael Zuccarino](https://github.com/mikezucc) [#1157](https://github.com/TextureGroup/Texture/pull/1157) +- Fix crash setting attributed text on multiple threads [Michael Schneider](https://github.com/maicki) ## 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 866f66187..bf85800ab 100644 --- a/Source/ASDisplayNode+Layout.mm +++ b/Source/ASDisplayNode+Layout.mm @@ -34,6 +34,11 @@ - (BOOL)implementsLayoutMethod - (ASLayoutElementStyle *)style { ASDN::MutexLocker l(__instanceLock__); + return [self _locked_style]; +} + +- (ASLayoutElementStyle *)_locked_style +{ if (_style == nil) { _style = [[ASLayoutElementStyle alloc] init]; } diff --git a/Source/ASTextNode.mm b/Source/ASTextNode.mm index 01819de41..cc1ff4e01 100644 --- a/Source/ASTextNode.mm +++ b/Source/ASTextNode.mm @@ -440,39 +440,46 @@ - (void)setAttributedText:(NSAttributedString *)attributedText if (attributedText == nil) { attributedText = [[NSAttributedString alloc] initWithString:@"" attributes:nil]; } - - // Don't hold textLock for too long. + { ASLockScopeSelf(); if (ASObjectIsEqual(attributedText, _attributedText)) { return; } - _attributedText = ASCleanseAttributedStringOfCoreTextAttributes(attributedText); -#if AS_TEXTNODE_RECORD_ATTRIBUTED_STRINGS - [ASTextNode _registerAttributedText:_attributedText]; -#endif + NSAttributedString *cleanedAttributedString = ASCleanseAttributedStringOfCoreTextAttributes(attributedText); + + // Invalidating the truncation text must be done while we still hold the lock. Because after we release it, + // another thread may set a new truncation text that will then be cleared by this thread, other may draw + // this soon-to-be-invalidated text. + [self _locked_invalidateTruncationText]; + + NSUInteger length = cleanedAttributedString.length; + if (length > 0) { + // Updating ascender and descender in one transaction while holding the lock. + ASLayoutElementStyle *style = [self _locked_style]; + style.ascender = [[self class] ascenderWithAttributedString:cleanedAttributedString]; + style.descender = [[attributedText attribute:NSFontAttributeName atIndex:cleanedAttributedString.length - 1 effectiveRange:NULL] descender]; + } + + // Update attributed text with cleaned attributed string + _attributedText = cleanedAttributedString; } - - // Since truncation text matches style of attributedText, invalidate it now. - [self _invalidateTruncationText]; - NSUInteger length = _attributedText.length; - if (length > 0) { - self.style.ascender = [[self class] ascenderWithAttributedString:_attributedText]; - self.style.descender = [[_attributedText attribute:NSFontAttributeName atIndex:length - 1 effectiveRange:NULL] descender]; - } - // Tell the display node superclasses that the cached layout is incorrect now [self setNeedsLayout]; // Force display to create renderer with new size and redisplay with new string [self setNeedsDisplay]; - // Accessiblity - self.accessibilityLabel = _attributedText.string; - self.isAccessibilityElement = (length != 0); // We're an accessibility element by default if there is a string. + let currentAttributedText = self.attributedText; // Grab attributed string again in case it changed in the meantime + self.accessibilityLabel = currentAttributedText.string; + self.isAccessibilityElement = (currentAttributedText.length != 0); // We're an accessibility element by default if there is a string. + +#if AS_TEXTNODE_RECORD_ATTRIBUTED_STRINGS + [ASTextNode _registerAttributedText:_attributedText]; +#endif } #pragma mark - Text Layout @@ -1166,6 +1173,7 @@ - (void)setTruncationAttributedText:(NSAttributedString *)truncationAttributedTe { if (ASLockedSelfCompareAssignCopy(_truncationAttributedText, truncationAttributedText)) { [self _invalidateTruncationText]; + [self setNeedsDisplay]; } } @@ -1173,6 +1181,7 @@ - (void)setAdditionalTruncationMessage:(NSAttributedString *)additionalTruncatio { if (ASLockedSelfCompareAssignCopy(_additionalTruncationMessage, additionalTruncationMessage)) { [self _invalidateTruncationText]; + [self setNeedsDisplay]; } } @@ -1231,12 +1240,13 @@ - (NSUInteger)lineCount - (void)_invalidateTruncationText { - { - ASLockScopeSelf(); - _composedTruncationText = nil; - } + ASLockScopeSelf(); + [self _locked_invalidateTruncationText]; +} - [self setNeedsDisplay]; +- (void)_locked_invalidateTruncationText +{ + _composedTruncationText = nil; } /** diff --git a/Source/ASTextNode2.mm b/Source/ASTextNode2.mm index 813326bdd..65c193fcc 100644 --- a/Source/ASTextNode2.mm +++ b/Source/ASTextNode2.mm @@ -280,12 +280,13 @@ - (void)setAttributedText:(NSAttributedString *)attributedText } // Since truncation text matches style of attributedText, invalidate it now. - [self _invalidateTruncationText]; - + [self _locked_invalidateTruncationText]; + NSUInteger length = attributedText.length; if (length > 0) { - self.style.ascender = [[self class] ascenderWithAttributedString:attributedText]; - self.style.descender = [[attributedText attribute:NSFontAttributeName atIndex:attributedText.length - 1 effectiveRange:NULL] descender]; + ASLayoutElementStyle *style = [self _locked_style]; + style.ascender = [[self class] ascenderWithAttributedString:attributedText]; + style.descender = [[attributedText attribute:NSFontAttributeName atIndex:attributedText.length - 1 effectiveRange:NULL] descender]; } // Tell the display node superclasses that the cached layout is incorrect now @@ -1061,10 +1062,15 @@ - (NSUInteger)lineCount - (void)_invalidateTruncationText { ASLockScopeSelf(); - _textContainer.truncationToken = nil; + [self _locked_invalidateTruncationText]; [self setNeedsDisplay]; } +- (void)_locked_invalidateTruncationText +{ + _textContainer.truncationToken = nil; +} + /** * @return the additional truncation message range within the as-rendered text. * Must be called from main thread diff --git a/Source/Private/ASDisplayNodeInternal.h b/Source/Private/ASDisplayNodeInternal.h index 5f9092ffa..1e907d3e7 100644 --- a/Source/Private/ASDisplayNodeInternal.h +++ b/Source/Private/ASDisplayNodeInternal.h @@ -386,4 +386,13 @@ AS_EXTERN NSString * const ASRenderingEngineDidDisplayNodesScheduledBeforeTimest @end +@interface ASDisplayNode (ASLayoutElementPrivate) + +/** + * Returns the internal style object or creates a new if no exists. Need to be called with lock held. + */ +- (ASLayoutElementStyle *)_locked_style; + +@end + NS_ASSUME_NONNULL_END