Fix crash if setting attributed text on multiple threads

TextureGroup · Oct 4, 2018 · 515e8b9 · 515e8b9
1 parent e703255
commit 515e8b9
Show file tree

Hide file tree

Showing 5 changed files with 59 additions and 28 deletions.
diff --git 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
@@ -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
@@ -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,13 +1173,15 @@ - (void)setTruncationAttributedText:(NSAttributedString *)truncationAttributedTe
 {
   if (ASLockedSelfCompareAssignCopy(_truncationAttributedText, truncationAttributedText)) {
     [self _invalidateTruncationText];
+    [self setNeedsDisplay];
   }
 }
 
 - (void)setAdditionalTruncationMessage:(NSAttributedString *)additionalTruncationMessage
 {
   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
@@ -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
@@ -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