[iOS] Fabric: Fixes crash of dynamic color when light/dark mode changed by zhongwuzw · Pull Request #48496 · react/react-native

zhongwuzw · 2025-01-05T07:48:51Z

Summary:

The reason is when light/dark mode changed, the hash value also changed because we used color.getColor(). leads to size balanced break.

Assertion failed: (index_.size() == lru_.size()), function size, file EvictingCacheMap.h, line 439.
(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = signal SIGABRT
frame #0: 0x00000001089a9108 libsystem_kernel.dylib`__pthread_kill + 8
frame #1: 0x0000000105de3408 libsystem_pthread.dylib`pthread_kill + 256
frame #2: 0x000000018016c4ec libsystem_c.dylib`abort + 104
frame #3: 0x000000018016b934 libsystem_c.dylib`__assert_rtn + 268
* frame #4: 0x00000001073e386c React_FabricComponents`folly::EvictingCacheMap<facebook::react::AttributedString, std::__1::shared_ptr<void>, folly::HeterogeneousAccessHash<facebook::react::AttributedString, void>, folly::HeterogeneousAccessEqualTo<facebook::react::AttributedString, void>>::size(this=0x0000600003900348) const at EvictingCacheMap.h:439:5
frame #5: 0x00000001073e34f4 React_FabricComponents`void folly::EvictingCacheMap<facebook::react::AttributedString, std::__1::shared_ptr<void>, folly::HeterogeneousAccessHash<facebook::react::AttributedString, void>, folly::HeterogeneousAccessEqualTo<facebook::react::AttributedString, void>>::setImpl<facebook::react::AttributedString>(this=0x0000600003900348, key=0x000000016b9f20a8, value=nullptr, promote=true, pruneHook=folly::EvictingCacheMap<facebook::react::AttributedString, std::__1::shared_ptr<void>, folly::HeterogeneousAccessHash<facebook::react::AttributedString, void>, folly::HeterogeneousAccessEqualTo<facebook::react::AttributedString, void> >::PruneHookCall @ 0x000000016b9f1cc8) at EvictingCacheMap.h:674:27
frame #6: 0x00000001073deb88 React_FabricComponents`folly::EvictingCacheMap<facebook::react::AttributedString, std::__1::shared_ptr<void>, folly::HeterogeneousAccessHash<facebook::react::AttributedString, void>, folly::HeterogeneousAccessEqualTo<facebook::react::AttributedString, void>>::set(this=0x0000600003900348, key=0x000000016b9f20a8, value=ptr = 0x60000024ae20 strong=2 weak=1, promote=true, pruneHook=folly::EvictingCacheMap<facebook::react::AttributedString, std::__1::shared_ptr<void>, folly::HeterogeneousAccessHash<facebook::react::AttributedString, void>, folly::HeterogeneousAccessEqualTo<facebook::react::AttributedString, void> >::PruneHookCall @ 0x000000016b9f1d98) at EvictingCacheMap.h:346:5
frame #7: 0x00000001073d91dc React_FabricComponents`facebook::react::SimpleThreadSafeCache<facebook::react::AttributedString, std::__1::shared_ptr<void>, 256>::get(this=0x0000600003900348, key=0x000000016b9f20a8, generator= Lambda in File RCTTextLayoutManager.mm at Line 337) const at SimpleThreadSafeCache.h:40:12
frame #8: 0x00000001073d9058 React_FabricComponents`-[RCTTextLayoutManager _nsAttributedStringFromAttributedString:](self=0x0000600003900340, _cmd="_nsAttributedStringFromAttributedString:", attributedString=AttributedString @ 0x000000016b9f20a8) at RCTTextLayoutManager.mm:337:42
frame #9: 0x00000001073d6378 React_FabricComponents`-[RCTTextLayoutManager drawAttributedString:paragraphAttributes:frame:drawHighlightPath:](self=0x0000600003900340, _cmd="drawAttributedString:paragraphAttributes:frame:drawHighlightPath:", attributedString=AttributedString @ 0x000000016b9f23a8, paragraphAttributes=ParagraphAttributes @ 0x000000016b9f2378, frame=(origin = (x = 0, y = 0), size = (width = 92, height = 21.666748046875)), block=0x00000001061602d0) at RCTTextLayoutManager.mm:73:56
frame #10: 0x000000010616020c RCTFabric`-[RCTParagraphTextView drawRect:](self=0x000000012beb9dc0, _cmd="drawRect:", rect=(origin = (x = 0, y = 0.000081380208335701809), size = (width = 92, height = 21.666666666666664))) at RCTParagraphComponentView.mm:346:3
frame #11: 0x0000000186043e60 UIKitCore`-[UIView(CALayerDelegate) drawLayer:inContext:] + 584
frame #12: 0x000000018af40080 QuartzCore`CABackingStoreUpdate_ + 244
frame #13: 0x000000018b0bec88 QuartzCore`invocation function for block in CA::Layer::display_() + 108
frame #14: 0x000000018b0b5524 QuartzCore`-[CALayer _display] + 1596
frame #15: 0x000000018b0c7e74 QuartzCore`CA::Layer::layout_and_display_if_needed(CA::Transaction*) + 392
frame #16: 0x000000018affca50 QuartzCore`CA::Context::commit_transaction(CA::Transaction*, double, double*) + 464
frame #17: 0x000000018b02b260 QuartzCore`CA::Transaction::commit() + 652
frame #18: 0x000000018b02c7b4 QuartzCore`CA::Transaction::flush_as_runloop_observer(bool) + 68
frame #19: 0x0000000185ad6c1c UIKitCore`_UIApplicationFlushCATransaction + 48
frame #20: 0x0000000185a07ccc UIKitCore`__setupUpdateSequence_block_invoke_2 + 352
frame #21: 0x000000018505d28c UIKitCore`_UIUpdateSequenceRun + 76
frame #22: 0x0000000185a07670 UIKitCore`schedulerStepScheduledMainSection + 168
frame #23: 0x0000000185a06aa8 UIKitCore`runloopSourceCallback + 80
frame #24: 0x000000018041b7c4 CoreFoundation`__CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 24
frame #25: 0x000000018041b70c CoreFoundation`__CFRunLoopDoSource0 + 172
frame #26: 0x000000018041ae70 CoreFoundation`__CFRunLoopDoSources0 + 232
frame #27: 0x00000001804153b4 CoreFoundation`__CFRunLoopRun + 788
frame #28: 0x0000000180414c24 CoreFoundation`CFRunLoopRunSpecific + 552
frame #29: 0x000000019020ab10 GraphicsServices`GSEventRunModal + 160
frame #30: 0x0000000185ad82fc UIKitCore`-[UIApplication _run] + 796
frame #31: 0x0000000185adc4f4 UIKitCore`UIApplicationMain + 124
frame #32: 0x0000000104521f68 RNTester.debug.dylib`main(argc=1, argv=0x000000016b9f5af8) at main.m:15:12
frame #33: 0x00000001045b9410 dyld_sim`start_sim + 20
frame #34: 0x0000000104796274 dyld`start + 2840

Changelog:

[IOS] [FIXED] - Fabric: Fixes crash of dynamic color when light/dark mode changed

Test Plan:

RNTester -> PlatformColor example -> changed the dark/light mode in the system settings -> go back to App and pop and push the PlatformColor example, it would crash:

…/fix_dynamic_crash

zhongwuzw · 2025-01-06T09:04:36Z

Also fixed #48493

cipolleschi

Thanks for fixing it! The code looks good, but I think we missed a bit.
I think we have to invalidate the cache uiColorHashValue_ when the uiColor_ changes.

Otherwise we risk to set a color, compute the hash, change the color, but the hash remains the same.

WDYT?

zhongwuzw · 2025-01-06T14:50:39Z

Thanks for fixing it! The code looks good, but I think we missed a bit. I think we have to invalidate the cache uiColorHashValue_ when the uiColor_ changes.
Otherwise we risk to set a color, compute the hash, change the color, but the hash remains the same.
WDYT?

@cipolleschi Thanks for the review, seems we don't have any interface which can change the uiColor_ after Color initialized.

cipolleschi

You are right... C++ contructors confused me...

facebook-github-bot · 2025-01-14T16:07:49Z

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

javache · 2025-01-14T16:54:28Z


+int32_t Color::getUIColorHash() const
+{
+ if (!uiColorHashValue_) {


Would this work?
UIColor *color = (UIColor *)unwrapManagedObject(uiColor_); return [color hashValue];

The point here is: the UIColor does not change when the UITraitCollection changes.
It changes only when we resolve the uicolor against the trait collection.
The uiColor is the actual invariant here, we don't need to compute the hash resolving the uicolor against all the trait collections.

@javache @cipolleschi I don't know details about how UIColor wether implements hash correctly. But seems the hash value is different even two dynamic colors have the same color in all trait collections.
For example: Two methods like below, [self dynamicColor].hash is not equal to [self dynamicColor1].hash. Actually they are the same color. So it may have potential issues if we use the hash value to check equality?
+ (UIColor *)dynamicColor { return [UIColor colorWithDynamicProvider:^UIColor * _Nonnull(UITraitCollection * _Nonnull traitCollection) { if (traitCollection.userInterfaceStyle == UIUserInterfaceStyleDark) { return [UIColor whiteColor]; } else { return [UIColor blackColor]; } }]; } + (UIColor *)dynamicColor1 { return [UIColor colorWithDynamicProvider:^UIColor * _Nonnull(UITraitCollection * _Nonnull traitCollection) { if (traitCollection.userInterfaceStyle == UIUserInterfaceStyleDark) { return [UIColor whiteColor]; } else { return [UIColor blackColor]; } }]; }
PS. The crash disappeared if we return color.hash.

So UIColor hashValue is probably just falling back to NSObject, which means the value won't change over the objects lifetime, but also can't be reliably used to compare objects that are equal :(

My concern is that hashing is supposed to be cheap, and we use it implicitly in a number of different places. This makes the common case of a non-platform color significantly more expensive.

The hash is computed only once, when needed the first time, and then cached in the uiColorHashValue_. Isn't that enough? The tradeoff is that we recompute the same hash if we have two instances of the same color, but that should be ok, no?

javache · 2025-01-14T22:11:48Z

Alternative suggestion: let's pre-calculate the hash and store it as an associated object on the UIColor

One more issue potentially here (which would lead to cache misses!): Color::operator== is always going to be false for dynamic colors.

…IColor

zhongwuzw · 2025-01-15T09:54:26Z

Alternative suggestion: let's pre-calculate the hash and store it as an associated object on the UIColor
One more issue potentially here (which would lead to cache misses!): Color::operator== is always going to be false for dynamic colors.

@javache @cipolleschi I updated the code to pre-calculate the hash in the constructor. Please review that 👀 !

cipolleschi

Left a question. It looks good to me. @javache, wdyt?

facebook-github-bot · 2025-01-16T10:29:33Z

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

cipolleschi · 2025-01-16T18:30:19Z

 auto seed = size_t{0};
- facebook::react::hash_combine(seed, color.getColor());
+ facebook::react::hash_combine(seed, color.getUIColorHash());
 return seed;


@javache No value in calling hash_combine again with seed 0.
Suggested change
auto seed=size_t{0};
facebook::react::hash_combine(seed, color.getColor());
facebook::react::hash_combine(seed, color.getUIColorHash());
returnseed;
returncolor.getUIColorHash();

cipolleschi · 2025-01-16T18:31:12Z

+ auto colorHash = facebook::react::hash_combine(intColor, 0);
+ color.reactHash = colorHash;


@javache
Suggested change
auto colorHash = facebook::react::hash_combine(intColor, 0);
color.reactHash = colorHash;
color.reactHash = [colorHash](facebook::react::hash_combine(intColor, 0));

cipolleschi · 2025-01-16T18:31:31Z

+ auto colorHash = facebook::react::hash_combine(dark, light, highContrastDark, highContrastLight, 0);
+ color.reactHash = colorHash;


@javache
Suggested change
auto colorHash = facebook::react::hash_combine(dark, light, highContrastDark, highContrastLight, 0);
color.reactHash = colorHash;
color.reactHash = [colorHash;](acebook::react::hash_combine(dark, light, highContrastDark, highContrastLight, 0);)

cipolleschi · 2025-01-16T18:33:03Z

+ auto colorHash = facebook::react::hash_combine(color, components.colorSpace == ColorSpace::DisplayP3);
+ uiColor.reactHash = colorHash;


@javache
Suggested change
auto colorHash = facebook::react::hash_combine(color, components.colorSpace == ColorSpace::DisplayP3);
uiColor.reactHash = colorHash;
uiColor.reactHash = facebook::react::hash_combine(color, components.colorSpace == ColorSpace::DisplayP3);

cipolleschi · 2025-01-16T18:34:20Z

@@ -114,14 +195,20 @@ int32_t ColorFromUIColor(const std::shared_ptr<void> &uiColor)

 Color::Color(std::shared_ptr<void> uiColor)


@javache
When would this still get called? Can we make this constructor private so we can guarantee all UIColor objects are created with the hash in place?

Seems UndefinedColor and semantic color used it, I mark it private and add a static method createSemanticColor to create semantic color.

zhongwuzw · 2025-01-23T09:16:49Z

@javache Hi, please help to review again :)

facebook-github-bot · 2025-01-27T17:36:25Z

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2025-01-28T19:24:54Z

@cipolleschi merged this pull request in cbaff1c.

react-native-bot · 2025-01-28T19:24:55Z

This pull request was successfully merged by @zhongwuzw in cbaff1c

^{When will my fix make it into a release? | How to file a pick request?}

facebook-github-bot · 2025-01-30T00:01:47Z

This pull request has been reverted by 04afbd9.

zhongwuzw · 2025-01-31T16:38:22Z

Is there anything I can do because of this commit was reverted? :)

cipolleschi · 2025-02-03T13:52:09Z

Yeah, you can. Thanks for the offer.
The problem was with the UIColor+graphics files. We have a very weird setup internally and sometimes it doesn't like when we add extra files. We had crashes only with the release build because those files were not linked to the binary. Funnily, the debug build was working well... That's why we didn't catch the crash before it landed.

A solution would be to remove the two files we added and use two plain C functions instead, getUIColorReactHash and setUIColorReactHash in the HostColorPlatform.mm file. We store the hash as a member of the Color struct.

zhongwuzw · 2025-02-03T16:43:19Z

@cipolleschi Thanks for your information. Maybe it's because not added -Objc C flag? I moved the category to HostPlatformColor.mm, because I think it's the easiest way to fix this issue, WDYT?

cipolleschi · 2025-02-03T17:39:59Z

@javache was suggesting to drop the category altogether, in favor of the simpler c functions... HostPlatformColor is already an Objective-C++ file, so even if we keep the category it should be good. Let's wait what he thinks about the category approach.

javache · 2025-02-03T18:05:38Z

Let's use a static function here. Categories have overhead in Objective-C and we have various compiler transforms that may cause issues here.

zhongwuzw · 2025-02-04T14:44:48Z

@cipolleschi @javache I store the hash as the member of color and calculate it in the constructor.

cipolleschi

looks good to me! :D
Thanks for the patience and for going back to work on this.

cipolleschi

looks good to me! :D
Thanks for the patience and for going back to work on this.

cipolleschi · 2025-02-07T15:29:25Z

Our system is not very happy with me trying to import a PR that result merged internally. I'm talking with our OSS team that manages the GitHub integration to see if we can find a solution.

zhongwuzw · 2025-02-07T15:53:43Z

New PR please see #49265.

…ode changed (#49265) Summary: Reland #48496 . ## Changelog: [IOS] [FIXED] - Fabric: Fixes crash of dynamic color when light/dark mode changed Pull Request resolved: #49265 Test Plan: RNTester -> PlatformColor example -> changed the dark/light mode in the system settings -> go back to App and pop and push the PlatformColor example, it would crash: Reviewed By: javache Differential Revision: D69309825 Pulled By: cipolleschi fbshipit-source-id: 7a533a73ef343b071000388b653b2d1d0c54ae88

zhongwuzw added 3 commits November 8, 2024 17:14

fix crash
795e684

Merge commit 'b867c01fa624a3140ac1b8415b34b6db33fae1b4' into features…
adeafa9
…/fix_dynamic_crash

format
a720b3d

facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Jan 5, 2025

zhongwuzw mentioned this pull request Jan 5, 2025
Dev app crashes when switching color scheme while Text element with DynamicColorIOS is used #48493
Open

cipolleschi requested changes Jan 6, 2025
View reviewed changes

zhongwuzw requested a review from cipolleschi January 8, 2025 14:32

cipolleschi approved these changes Jan 14, 2025
View reviewed changes

javache reviewed Jan 14, 2025
View reviewed changes

pre-calculate the hash and store it as an associated object on the U…
8949170
…IColor

zhongwuzw force-pushed the features/fix_dynamic_crash branch from 42b5cd4 to 8949170Compare January 15, 2025 09:46

cipolleschi approved these changes Jan 15, 2025
View reviewed changes

Comment thread...eactCommon/react/renderer/graphics/platform/ios/react/renderer/graphics/HostPlatformColor.mm Outdated

update
3e72b4d

cipolleschi reviewed Jan 20, 2025
View reviewed changes

Apply review back
dad55b5

zhongwuzw requested review from cipolleschi and javache January 21, 2025 07:13

facebook-github-bot closed this in cbaff1cJan 28, 2025

facebook-github-bot added the Merged This PR has been merged. label Jan 28, 2025

facebook-github-bot added the Reverted label Jan 30, 2025

cipolleschi reopened this Feb 3, 2025

Move category to HostPlatformColor
fabf86f

Put hash value to constructor
6329f2e

cipolleschi approved these changes Feb 7, 2025
View reviewed changes

zhongwuzw mentioned this pull request Feb 7, 2025
Reland: [iOS] [Fabric] Fixes crash of dynamic color when light/dark mode changed #49265
Closed

zhongwuzw closed this Feb 7, 2025

- auto seed=size_t{0};
-facebook::react::hash_combine(seed, color.getColor());
-facebook::react::hash_combine(seed, color.getUIColorHash());
-returnseed;
+returncolor.getUIColorHash();

		auto colorHash = facebook::react::hash_combine(intColor, 0);
		color.reactHash = colorHash;

	auto colorHash = facebook::react::hash_combine(intColor, 0);
	color.reactHash = colorHash;
	color.reactHash = [colorHash](facebook::react::hash_combine(intColor, 0));

	auto colorHash = facebook::react::hash_combine(dark, light, highContrastDark, highContrastLight, 0);
	color.reactHash = colorHash;
	color.reactHash = [colorHash;](acebook::react::hash_combine(dark, light, highContrastDark, highContrastLight, 0);)

		auto colorHash = facebook::react::hash_combine(color, components.colorSpace == ColorSpace::DisplayP3);
		uiColor.reactHash = colorHash;

	auto colorHash = facebook::react::hash_combine(color, components.colorSpace == ColorSpace::DisplayP3);
	uiColor.reactHash = colorHash;
	uiColor.reactHash = facebook::react::hash_combine(color, components.colorSpace == ColorSpace::DisplayP3);

		@@ -114,14 +195,20 @@ int32_t ColorFromUIColor(const std::shared_ptr<void> &uiColor)

		Color::Color(std::shared_ptr<void> uiColor)

Uh oh!

Conversation

zhongwuzw commented Jan 5, 2025

Summary:

Changelog:

Test Plan:

Uh oh!

zhongwuzw commented Jan 6, 2025

Uh oh!

cipolleschi left a comment

Choose a reason for hiding this comment

Uh oh!

zhongwuzw commented Jan 6, 2025

Uh oh!

cipolleschi left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Jan 14, 2025

Uh oh!

javacheJan 14, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cipolleschiJan 14, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhongwuzwJan 14, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javache commented Jan 14, 2025

Uh oh!

zhongwuzw commented Jan 15, 2025

Uh oh!

cipolleschi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

facebook-github-bot commented Jan 16, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhongwuzw commented Jan 23, 2025

Uh oh!

facebook-github-bot commented Jan 27, 2025

Uh oh!

facebook-github-bot commented Jan 28, 2025

Uh oh!

react-native-bot commented Jan 28, 2025

Uh oh!

facebook-github-bot commented Jan 30, 2025

Uh oh!

zhongwuzw commented Jan 31, 2025

Uh oh!

cipolleschi commented Feb 3, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zhongwuzw commented Feb 3, 2025

Uh oh!

cipolleschi commented Feb 3, 2025

Uh oh!

javache commented Feb 3, 2025

Uh oh!

javacheJan 14, 2025•
edited
Loading

cipolleschiJan 14, 2025•
edited
Loading

zhongwuzwJan 14, 2025•
edited
Loading

cipolleschi commented Feb 3, 2025•
edited
Loading