From f2bb793699b85232b4d86cef208c5bb33a08c5c3 Mon Sep 17 00:00:00 2001 From: Valentin Shergin Date: Fri, 1 Nov 2019 11:37:00 -0700 Subject: [PATCH] Fabric: Re-registration a ComponentDescriptorProvider is now no-op and an assert in DEBUG mode Summary: We suspect that the re-registration of `ComponentDescriptorProvider`s during Bridge reloading might cause crashes in Fabric core. That happens because the re-registration process replaces already existing and being used ComponentDescriptors in the managed registries with the exact same new ones, which forces old ones to be deallocated and all pointers to them invalid. (On of the fundamental Fabric design decision is that `ShadowNode`s don't own/retain ComponentDescriptors.) It seems was already indirectly addressed in application code on iOS but still fixing that in the core is valuable. Android implementation does not use reactive component registration, so it was already fine. As the follow-up diff, we plan to remove "removing" capabilities from ComponentDescriptorRegistry and ComponentDescriptorProviderRegistry to make it even more future-proof. Changelog: [Internal] Fabric-specific internal change. Reviewed By: sammy-SC Differential Revision: D18273683 fbshipit-source-id: 7615627842855f078a3fdf3049f5511f59700972 --- .../ComponentDescriptorProviderRegistry.cpp | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/ReactCommon/fabric/uimanager/ComponentDescriptorProviderRegistry.cpp b/ReactCommon/fabric/uimanager/ComponentDescriptorProviderRegistry.cpp index 5a9a4c4e2f8..ec1269e7ea7 100644 --- a/ReactCommon/fabric/uimanager/ComponentDescriptorProviderRegistry.cpp +++ b/ReactCommon/fabric/uimanager/ComponentDescriptorProviderRegistry.cpp @@ -13,6 +13,19 @@ namespace react { void ComponentDescriptorProviderRegistry::add( ComponentDescriptorProvider provider) const { std::unique_lock lock(mutex_); + + assert( + componentDescriptorProviders_.find(provider.handle) == + componentDescriptorProviders_.end() && + "Attempt to register an already registered ComponentDescriptorProvider."); + + if (componentDescriptorProviders_.find(provider.handle) != + componentDescriptorProviders_.end()) { + // Re-registering a provider makes no sense because it's copyable: already + // registered one is as good as any new can be. + return; + } + componentDescriptorProviders_.insert({provider.handle, provider}); for (auto const &weakRegistry : componentDescriptorRegistries_) {