Fix crash setting attributed text on multiple threads

[maicki]

Currently some race condition can happen within -[ASTextNode setAttributedText:]:

• Thread A calls setAttributedText:
• We change the text with the lock held and we grab the length of the _attributedString
• After getting the length a thread switch happens
• A thread B calls setAttributedText:. The attributed text instance var get's updated and it finishes the method
• Thread switch happens back to A and it's going again. It goes into the if block and try to access the last character based on the old length that is not up to date anymore and it crashes

You can repro it if you simulate a thread switch and the crash on the descender line via a semaphore like:

- (void)setAttributedText:(NSAttributedString *)attributedText semaphore:(dispatch_semaphore_t)semaphore
{
  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
  }
    
  // Since truncation text matches style of attributedText, invalidate it now.
  [self _invalidateTruncationText];
  
  NSUInteger length = _attributedText.length;
  if (length > 0) {
    if (semaphore) {
      // Wait until the sempahore is released
      dispatch_semaphore_wait(semaphore, DISPATCH_TIME_FOREVER);
    }
    
    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.
}

And in the frontend with something like:

_textNode = [[ASTextNode alloc] init];

dispatch_semaphore_t sem = dispatch_semaphore_create(0);
dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(1.0 * NSEC_PER_SEC)), dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{
    // Set some text
        [self.textNode setAttributedText:[[NSAttributedString alloc] initWithString:@"Short" attributes:@{}] semaphore:nil];
    dispatch_semaphore_signal(sem);
});

// Set some textand let it wait on the semaphore
[_textNode setAttributedText:[[NSAttributedString alloc] initWithString:@"Longer longer text" attributes:@{}] semaphore:sem];

[maicki]

@Adlai-Holler Any reason we are not using ASCleanseAttributedStringOfCoreTextAttributes in ASTextNode2?

[nguyenhuy]

ok, so 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, or may draw this soon-to-be-invalidated text.

Let's leave a comment/explaination here.

[nguyenhuy]

Same deal with ascender and descender here. We basically use the lock as a transactional one.

[nguyenhuy]

_attributedText is accessed without the lock here. Can we do these operations while having the locks as well?

At least, we need to access the text via its setter (self.attributedText) so that if it's changed by another thread right before this, we'll end up grabbing the updated text (and its length) here. Also, it's no longer safe to use the attributedText param of this method.

[nguyenhuy]

I feel like now is the time to let callers of this (unlocked) method to call -setNeedsDisplay themselves. It's a bit confusing to only automatically do it with the unlock version.

[nguyenhuy]

Is it actually ok to call -setNeedsDisplay here?

[maicki]

I asked that myself too, but I didn't want to change it within this diff. In theory the code in ASTextNode2 should be adjusted to the same as in ASTextNode after this lands.

[nguyenhuy]

👍

[maicki]

@nguyenhuy Would appreciate if you could give that another look today. Thanks!

[nguyenhuy]

👍