Fix tint color dead lock facebookarchive#1731 (facebookarchive#1732)

* fix tintColor dead lock update to use scoped lock * pull other accessor out of the scoped lock * fix ASTextNode
aimalygin · Nov 25, 2019 · cbc5104 · cbc5104
1 parent 37e2830
commit cbc5104
Show file tree

Hide file tree

Showing 3 changed files with 62 additions and 45 deletions.
diff --git a/Source/ASImageNode.mm b/Source/ASImageNode.mm
@@ -302,44 +302,48 @@ - (void)setPlaceholderColor:(UIColor *)placeholderColor
 
 - (NSObject *)drawParametersForAsyncLayer:(_ASDisplayLayer *)layer
 {
-  ASLockScopeSelf();
-
-  UIImage *drawImage = _image;
-  if (AS_AVAILABLE_IOS_TVOS(13, 10)) {
-    if (_imageNodeFlags.regenerateFromImageAsset && drawImage != nil) {
-      _imageNodeFlags.regenerateFromImageAsset = NO;
-      UITraitCollection *tc = [UITraitCollection traitCollectionWithUserInterfaceStyle:_primitiveTraitCollection.userInterfaceStyle];
-      UIImage *generatedImage = [drawImage.imageAsset imageWithTraitCollection:tc];
-      if ( generatedImage != nil ) {
-        drawImage = generatedImage;
+  ASImageNodeDrawParameters *drawParameters = [[ASImageNodeDrawParameters alloc] init];
+
+  {
+    ASLockScopeSelf();
+    UIImage *drawImage = _image;
+    if (AS_AVAILABLE_IOS_TVOS(13, 10)) {
+      if (_imageNodeFlags.regenerateFromImageAsset && drawImage != nil) {
+        _imageNodeFlags.regenerateFromImageAsset = NO;
+        UITraitCollection *tc = [UITraitCollection traitCollectionWithUserInterfaceStyle:_primitiveTraitCollection.userInterfaceStyle];
+        UIImage *generatedImage = [drawImage.imageAsset imageWithTraitCollection:tc];
+        if ( generatedImage != nil ) {
+          drawImage = generatedImage;
+        }
       }
     }
-  }
 
-  ASImageNodeDrawParameters *drawParameters = [[ASImageNodeDrawParameters alloc] init];
-  drawParameters->_image = drawImage;
+    drawParameters->_image = drawImage;
+    drawParameters->_contentsScale = _contentsScaleForDisplay;
+    drawParameters->_cropEnabled = _imageNodeFlags.cropEnabled;
+    drawParameters->_forceUpscaling = _imageNodeFlags.forceUpscaling;
+    drawParameters->_forcedSize = _forcedSize;
+    drawParameters->_cropRect = _cropRect;
+    drawParameters->_cropDisplayBounds = _cropDisplayBounds;
+    drawParameters->_imageModificationBlock = _imageModificationBlock;
+    drawParameters->_willDisplayNodeContentWithRenderingContext = _willDisplayNodeContentWithRenderingContext;
+    drawParameters->_didDisplayNodeContentWithRenderingContext = _didDisplayNodeContentWithRenderingContext;
+    drawParameters->_traitCollection = _primitiveTraitCollection;
+
+    // Hack for now to retain the weak entry that was created while this drawing happened
+    drawParameters->_didDrawBlock = ^(ASWeakMapEntry *entry){
+      ASLockScopeSelf();
+      _weakCacheEntry = entry;
+    };
+  }
+
+  // We need to unlock before we access the other accessor.
+  // Especially tintColor because it needs to walk up the view hierarchy
   drawParameters->_bounds = [self threadSafeBounds];
   drawParameters->_opaque = self.opaque;
-  drawParameters->_contentsScale = _contentsScaleForDisplay;
   drawParameters->_backgroundColor = self.backgroundColor;
-  drawParameters->_tintColor = self.tintColor;
   drawParameters->_contentMode = self.contentMode;
-  drawParameters->_cropEnabled = _imageNodeFlags.cropEnabled;
-  drawParameters->_forceUpscaling = _imageNodeFlags.forceUpscaling;
-  drawParameters->_forcedSize = _forcedSize;
-  drawParameters->_cropRect = _cropRect;
-  drawParameters->_cropDisplayBounds = _cropDisplayBounds;
-  drawParameters->_imageModificationBlock = _imageModificationBlock;
-  drawParameters->_willDisplayNodeContentWithRenderingContext = _willDisplayNodeContentWithRenderingContext;
-  drawParameters->_didDisplayNodeContentWithRenderingContext = _didDisplayNodeContentWithRenderingContext;
-  drawParameters->_traitCollection = _primitiveTraitCollection;
-
-
-  // Hack for now to retain the weak entry that was created while this drawing happened
-  drawParameters->_didDrawBlock = ^(ASWeakMapEntry *entry){
-    ASLockScopeSelf();
-    _weakCacheEntry = entry;
-  };
+  drawParameters->_tintColor = self.tintColor;
 
   return drawParameters;
 }

diff --git a/Source/ASTextNode.mm b/Source/ASTextNode.mm
@@ -546,9 +546,11 @@ - (NSArray *)exclusionPaths
 
 - (NSObject *)drawParametersForAsyncLayer:(_ASDisplayLayer *)layer
 {
+  /// have to access tintColor outside of the lock to prevent dead lock when accessing up the view hierarchy
+  UIColor *tintColor = self.tintColor;
   ASLockScopeSelf();
   if (_textColorFollowsTintColor) {
-    _cachedTintColor = self.tintColor;
+    _cachedTintColor = tintColor;
   } else {
     _cachedTintColor = nil;
   }

diff --git a/Source/ASTextNode2.mm b/Source/ASTextNode2.mm
@@ -517,23 +517,34 @@ - (void)prepareAttributedString:(NSMutableAttributedString *)attributedString is
 
 - (NSObject *)drawParametersForAsyncLayer:(_ASDisplayLayer *)layer
 {
-  ASLockScopeSelf();
-  [self _ensureTruncationText];
-
-  // Unlike layout, here we must copy the container since drawing is asynchronous.
-  ASTextContainer *copiedContainer = [_textContainer copy];
-  copiedContainer.size = self.bounds.size;
-  [copiedContainer makeImmutable];
-  NSMutableAttributedString *mutableText = [_attributedText mutableCopy] ?: [[NSMutableAttributedString alloc] init];
-
-  [self prepareAttributedString:mutableText isForIntrinsicSize:NO];
-
+  ASTextContainer *copiedContainer;
+  NSMutableAttributedString *mutableText;
+  BOOL needsTintColor;
+  id bgColor;
+  {
+    // Wrapping all the other access here, because we can't lock while accessing tintColor.
+    ASLockScopeSelf();
+    [self _ensureTruncationText];
+
+    // Unlike layout, here we must copy the container since drawing is asynchronous.
+    copiedContainer = [_textContainer copy];
+    copiedContainer.size = self.bounds.size;
+    [copiedContainer makeImmutable];
+    mutableText = [_attributedText mutableCopy] ?: [[NSMutableAttributedString alloc] init];
+
+    [self prepareAttributedString:mutableText isForIntrinsicSize:NO];
+    needsTintColor = self.textColorFollowsTintColor && mutableText.length > 0;
+    bgColor = self.backgroundColor ?: [NSNull null];
+  }
+
   // After all other attributes are set, apply tint color if needed and foreground color is not already specified
-  if (self.textColorFollowsTintColor && mutableText.length > 0) {
+  if (needsTintColor) {
     // Apply tint color if specified and if foreground color is undefined for attributedString
     NSRange limit = NSMakeRange(0, mutableText.length);
     // Look for previous attributes that define foreground color
     UIColor *attributeValue = (UIColor *)[mutableText attribute:NSForegroundColorAttributeName atIndex:limit.location effectiveRange:NULL];
+
+    // we need to unlock before accessing tintColor
     UIColor *tintColor = self.tintColor;
     if (attributeValue == nil && tintColor) {
       // None are found, apply tint color if available. Fallback to "black" text color
@@ -544,7 +555,7 @@ - (NSObject *)drawParametersForAsyncLayer:(_ASDisplayLayer *)layer
   return @{
     @"container": copiedContainer,
     @"text": mutableText,
-    @"bgColor": self.backgroundColor ?: [NSNull null]
+    @"bgColor": bgColor
   };
 }