Fix -[RCTDynamicColor effectiveColor] being called off UI thread #438
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I'm not sure what the usage would be to call effectiveColor when not on the UI thread. The call site that's trying to do this seems like the issue to be fixed, not RCTDynamicColor. Dynamic colors by definition don't resolve to a specific color until resolved within a given UI context. So in this case, we're just caching the last context in which this color was resolved and hoping that it stays accurate. That's by no means guaranteed to be true. Different view hierarchies within the same window/application can have different appearances or the user could change the system setting at runtime, so depending on a cached version of the color seems likely to provide invalid colors. |
|
+1 to what @markavitale said. This seems like this would just silence the flaw and make such bugs more difficult to detect |
anandrajeswaran
requested changes
Jun 7, 2020
anandrajeswaran
left a comment
anandrajeswaran
left a comment
There was a problem hiding this comment.
There shouldn't be a use case for calling this off the UI thread and adding a cache in here just increases the odds of a subtle bug.
ghost
added
the
Member
Author
|
It gets called from |
|
@tido64 so then the real bug would be the shadow view should not be dealing with color attributes at all since that doesn't affect sizing? |
tido64
force-pushed
the
tido64/fix-appearance-ui-thread
branch
from
df1dd35 to
35d57dd
Compare
Member
Author
You're right. It seems that we only need |
tido64
changed the title
|
@tido64 can we go one more level up? RCTDynamicColor should be able to be completely unaware of other threads. RCTBaseTextShadowView is the one that is asking for it on a background thread and that's what we should fix |
Member
Author
Libraries/Text/BaseText/RCTBaseTextShadowView.m:100 if (text) {
NSAttributedString *rawTextAttributedString =
[[NSAttributedString alloc] initWithString:[textAttributes applyTextAttributesToText:text]
attributes:textAttributes.effectiveTextAttributes];
[attributedText appendAttributedString:rawTextAttributedString];
}This piece of code works fine with any |
|
@tido64 or just not include color attributes? We still should not be referring to NSColors on background threads even if that currently happens to not throw an error of some sort. |
Collaborator
|
The [RCTDynamicColor effectiveColor] method is being called because [NSColor colorSpaceName] is used in NSColor's NSObject hash function. And the hash is being called because NSAttributedString's implementation uses hashes for its attribute dictionary. A simple fix is to override - (NSUInteger)hash {
NSUInteger prime = 31, result = 1;
result = prime * result + [_aquaColor hash];
result = prime * result + [_darkAquaColor hash];
return result;
}When overriding - (BOOL)isEqual:(id)other {
if (other == self)
return YES;
if (!other || ![other isKindOfClass:[self class]])
return NO;
return [self isEqualToDynamicColor:other];
}
- (BOOL)isEqualToDynamicColor:(RCTDynamicColor *)other {
if (self == other)
return YES;
if ([_aquaColor isNotEqualTo:other->_aquaColor])
return NO;
if ([_darkAquaColor isNotEqualTo:other->_darkAquaColor])
return NO;
return YES;
}Long term react-native layout should be manipulating the NSAttributedStrings and NSColor/UIColors only on the main thread, but such a change is way out of scope of this fix. But such a think can and probably is being done as part of the Fabric rearchitecture. |
tido64
force-pushed
the
tido64/fix-appearance-ui-thread
branch
from
35d57dd to
85dca79
Compare
Member
Author
Thanks, @tom-un. Your solution does work. |
tom-un
approved these changes
Jun 9, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Xcode logs warnings to console about the use of the appearance API on a non-UI thread. This fix should allow the current behaviour.
Resolves #335
Changelog
[macOS] [Fixed] - Allow
-[RCTDynamicColor effectiveColor]to be called off UI threadTest Plan
Xcode should warn about the use of the appearance API on a non-UI thread.
Microsoft Reviewers: Open in CodeFlow