[ASTextNode2] Upgrade lock safety by protecting all ivars (including rarely-changed ones). #918
Conversation
Source/ASTextNode2.mm
Outdated
|
|
||
|
|
||
| _shadowOpacity = shadowOpacity; | ||
|
|
||
| [self setNeedsDisplay]; |
There was a problem hiding this comment.
Great place to use ASLockedSelfCompareAssign!
Source/ASTextNode2.mm
Outdated
| if (ASLockedSelfCompareAssignCopy(_truncationAttributedText, truncationAttributedText)) { | ||
| ASLockScopeSelf(); | ||
| if (!ASObjectIsEqual(_truncationAttributedText, truncationAttributedText)) { | ||
| _truncationAttributedText = [truncationAttributedText copy]; | ||
| [self _invalidateTruncationText]; |
There was a problem hiding this comment.
Why this change? Is it so that we now call _invalidateTruncationText with the lock held?
There was a problem hiding this comment.
Yes, otherwise there is no transactional integrity.
There was a problem hiding this comment.
OK in cases such as this (side effect keeps lock), we can use the non-locked compare-assign macros, like so:
ASLockScopeSelf();
if (ASCompareAssignCopy(_truncationAttributedText, truncationATtributedText)) {
[self _invalidateTruncationText];
}
I set it up to work either way!
There was a problem hiding this comment.
Great! This is definitely an improvement.
I'm having trouble getting the minutes to circle back to this; will try, maybe this weekend.
Source/ASTextNode2.mm
Outdated
| @@ -210,6 +212,7 @@ - (BOOL)supportsLayerBacking | |||
|
|
|||
| - (void)setTextContainerInset:(UIEdgeInsets)textContainerInset | |||
| { | |||
| ASLockScopeSelf(); | |||
| BOOL needsUpdate = !UIEdgeInsetsEqualToEdgeInsets(_textContainer.insets, textContainerInset); | |||
| _textContainer.insets = textContainerInset; | |||
There was a problem hiding this comment.
Good place for ASLockedSelfCompareAssignCustom
| // If the text contains any links, return NO. | ||
| NSAttributedString *attributedText = self.attributedText; | ||
| NSAttributedString *attributedText = _attributedText; |
There was a problem hiding this comment.
Since we just took the lock, can we go back to using the ivar here?
|
Just to note: |
…rarely-changed ones). Although I don't know of any specific crashes caused by this, I think we should lock all properties by default. There are also some indications of premature optimization in keeping lock scope small, where it is actually important to have transactional integrity, and also where the ASDisplayNode base class is otherwise going to repeatedly re-lock the object anyway. I think this will remain pretty efficient, especially with os_unfair_lock enabled.
Adlai-Holler
force-pushed
the
ASTextNode2ThreadSafety
branch
2 times, most recently
from
b94c2ba
to
f7ac965
Compare
Adlai-Holler
force-pushed
the
ASTextNode2ThreadSafety
branch
from
f7ac965
to
767dc5d
Compare
Although I don't know of any specific crashes caused by this, I think we should
lock all properties by default. There are also some indications of premature
optimization in keeping lock scope small, where it is actually important to
have transactional integrity, and also where the ASDisplayNode base class is
otherwise going to repeatedly re-lock the object anyway.
I think this will remain pretty efficient, especially with os_unfair_lock enabled.