Fix unsafe rawPointer access in cloneMultiple (#55613)

Summary:

The cloneMultiple method was written in a way to accept a list of families that are presumed to be owned by the api caller. This designe was mostly aimed at reanimated, that holds refernces to ShadowNodes (that own these families). In the case of AnimationBackend this didn't work properly, as when the view is unmounted we would lose the ShadowNodeFamily shared_ptr that we hold and it could get deallocated.

Since now we can get an owning reference to ShadowNodeFamily from ShadowNode::getFamilyShared, we don't have to keep this old unsafe api. Instead we require the caller to have an owning reference with the api itself.

This `cloneMultiple` method isn't really adopted in the community, so the breaking change shouldn't be a big problem.

Reviewed By: zeyap, javache

Differential Revision: D93596770

Author: coado

packages/react-native/ReactCommon/react/renderer/animationbackend/AnimatedPropsRegistry.cpp

@@ -55,7 +55,7 @@ void AnimatedPropsRegistry::update(
   }
 }
 
-std::pair<std::unordered_set<const ShadowNodeFamily*>&, SnapshotMap&>
+std::pair<std::unordered_set<ShadowNodeFamily::Shared>&, SnapshotMap&>
 AnimatedPropsRegistry::getMap(SurfaceId surfaceId) {
   auto lock = std::lock_guard(mutex_);
   auto& [pendingMap, map, pendingFamilies, families] =

packages/react-native/ReactCommon/react/renderer/animationbackend/AnimatedPropsRegistry.h

@@ -24,11 +24,11 @@ struct PropsSnapshot {
 
 struct SurfaceContext {
   std::unordered_map<Tag, std::unique_ptr<PropsSnapshot>> pendingMap, map;
-  std::unordered_set<const ShadowNodeFamily *> pendingFamilies, families;
+  std::unordered_set<std::shared_ptr<const ShadowNodeFamily>> pendingFamilies, families;
 };
 
 struct SurfaceUpdates {
-  std::unordered_set<const ShadowNodeFamily *> families;
+  std::unordered_set<ShadowNodeFamily::Shared> families;
   std::unordered_map<Tag, AnimatedProps> propsMap;
   bool hasLayoutUpdates{false};
 };
@@ -39,7 +39,7 @@ class AnimatedPropsRegistry {
  public:
   void update(const std::unordered_map<SurfaceId, SurfaceUpdates> &surfaceUpdates);
   void clear(SurfaceId surfaceId);
-  std::pair<std::unordered_set<const ShadowNodeFamily *> &, SnapshotMap &> getMap(SurfaceId surfaceId);
+  std::pair<std::unordered_set<ShadowNodeFamily::Shared> &, SnapshotMap &> getMap(SurfaceId surfaceId);
 
  private:
   std::unordered_map<SurfaceId, SurfaceContext> surfaceContexts_;

packages/react-native/ReactCommon/react/renderer/animationbackend/AnimationBackend.cpp

@@ -71,7 +71,7 @@ void AnimationBackend::onAnimationFrame(AnimationTimestamp timestamp) {
       auto& [families, updates, hasLayoutUpdates] =
           surfaceUpdates[family->getSurfaceId()];
       hasLayoutUpdates |= mutation.hasLayoutUpdates;
-      families.insert(family.get());
+      families.insert(family);
       updates[mutation.tag] = std::move(mutation.props);
     }
   }
@@ -146,7 +146,8 @@ void AnimationBackend::commitUpdates(
                           const ShadowNode& shadowNode,
                           const ShadowNodeFragment& fragment) {
                         auto newProps = ShadowNodeFragment::propsPlaceholder();
-                        if (surfaceFamilies.contains(&shadowNode.getFamily())) {
+                        if (surfaceFamilies.contains(
+                                shadowNode.getFamilyShared())) {
                           auto& animatedProps = updates.at(shadowNode.getTag());
                           newProps = cloneProps(animatedProps, shadowNode);
                         }

packages/react-native/ReactCommon/react/renderer/animationbackend/AnimationBackendCommitHook.cpp

@@ -43,9 +43,10 @@ RootShadowNode::Unshared AnimationBackendCommitHook::shadowTreeWillCommit(
               const ShadowNodeFragment& fragment) {
             auto newProps = ShadowNodeFragment::propsPlaceholder();
             std::shared_ptr<BaseViewProps> viewProps = nullptr;
-            if (surfaceFamilies.contains(&shadowNode.getFamily()) &&
-                updates.contains(shadowNode.getTag())) {
-              auto& snapshot = updates.at(shadowNode.getTag());
+            if (auto updatesIter = updates.find(shadowNode.getTag());
+                updatesIter != updates.end() &&
+                surfaceFamilies.contains(shadowNode.getFamilyShared())) {
+              auto& snapshot = updatesIter->second;
               if (!snapshot->propNames.empty() || snapshot->rawProps) {
                 PropsParserContext propsParserContext{
                     shadowNode.getSurfaceId(),

packages/react-native/ReactCommon/react/renderer/core/ShadowNode.cpp

@@ -413,17 +413,16 @@ namespace {
 
 std::shared_ptr<ShadowNode> cloneMultipleRecursive(
     const ShadowNode& shadowNode,
-    const std::unordered_map<const ShadowNodeFamily*, int>& childrenCount,
+    const std::unordered_map<Tag, int>& childrenCount,
     const std::function<std::shared_ptr<
         ShadowNode>(const ShadowNode&, const ShadowNodeFragment&)>& callback) {
-  const auto* family = &shadowNode.getFamily();
   auto& children = shadowNode.getChildren();
   std::shared_ptr<std::vector<std::shared_ptr<const ShadowNode>>> newChildren;
-  auto count = childrenCount.at(family);
+  auto count = childrenCount.at(shadowNode.getTag());
 
   for (size_t i = 0; count > 0 && i < children.size(); i++) {
-    const auto childFamily = &children[i]->getFamily();
-    if (childrenCount.contains(childFamily)) {
+    const auto childTag = children[i]->getTag();
+    if (childrenCount.contains(childTag)) {
       count--;
       if (!newChildren) {
         newChildren =
@@ -441,37 +440,38 @@ std::shared_ptr<ShadowNode> cloneMultipleRecursive(
 } // namespace
 
 std::shared_ptr<ShadowNode> ShadowNode::cloneMultiple(
-    const std::unordered_set<const ShadowNodeFamily*>& familiesToUpdate,
+    const std::unordered_set<ShadowNodeFamily::Shared>& familiesToUpdate,
     const std::function<std::shared_ptr<ShadowNode>(
         const ShadowNode& oldShadowNode,
         const ShadowNodeFragment& fragment)>& callback) const {
-  std::unordered_map<const ShadowNodeFamily*, int> childrenCount;
+  std::unordered_map<Tag, int> childrenCount;
 
   for (const auto& family : familiesToUpdate) {
-    if (childrenCount.contains(family)) {
+    if (childrenCount.contains(family->getTag())) {
       continue;
     }
 
-    childrenCount[family] = 0;
+    childrenCount[family->getTag()] = 0;
 
     auto ancestor = family->parent_.lock();
     while ((ancestor != nullptr) && ancestor != family_) {
-      auto ancestorIt = childrenCount.find(ancestor.get());
+      auto ancestorTag = ancestor->getTag();
+      auto ancestorIt = childrenCount.find(ancestorTag);
       if (ancestorIt != childrenCount.end()) {
         ancestorIt->second++;
         break;
       }
-      childrenCount[ancestor.get()] = 1;
+      childrenCount[ancestorTag] = 1;
 
       ancestor = ancestor->parent_.lock();
     }
 
     if (ancestor == family_) {
-      childrenCount[ancestor.get()]++;
+      childrenCount[ancestor->getTag()]++;
     }
   }
 
-  if (!childrenCount.contains(&this->getFamily())) {
+  if (!childrenCount.contains(getTag())) {
     return nullptr;
   }
 

packages/react-native/ReactCommon/react/renderer/core/ShadowNode.h

@@ -109,7 +109,7 @@ class ShadowNode : public Sealable, public DebugStringConvertible, public jsi::N
    * Returns `nullptr` if the operation cannot be performed successfully.
    */
   std::shared_ptr<ShadowNode> cloneMultiple(
-      const std::unordered_set<const ShadowNodeFamily *> &familiesToUpdate,
+      const std::unordered_set<ShadowNodeFamily::Shared> &familiesToUpdate,
       const std::function<
           std::shared_ptr<ShadowNode>(const ShadowNode &oldShadowNode, const ShadowNodeFragment &fragment)> &callback)
       const;

packages/react-native/ReactCommon/react/renderer/core/tests/ShadowNodeTest.cpp

@@ -316,7 +316,7 @@ TEST_F(ShadowNodeTest, handleRuntimeReferenceTransferOnClone) {
 TEST_F(ShadowNodeTest, cloneMultiple) {
   auto newProps = std::make_shared<const TestProps>();
   auto newRoot = nodeA_->cloneMultiple(
-      {&nodeA_->getFamily(), &nodeAB_->getFamily()},
+      {nodeA_->getFamilyShared(), nodeAB_->getFamilyShared()},
       [&](const ShadowNode& oldShadowNode, const ShadowNodeFragment& fragment) {
         return oldShadowNode.clone({
             .props = newProps,
@@ -346,7 +346,7 @@ TEST_F(ShadowNodeTest, cloneMultiple) {
 TEST_F(ShadowNodeTest, cloneMultipleWithSingleFamily) {
   auto newProps = std::make_shared<const TestProps>();
   auto newRoot = nodeA_->cloneMultiple(
-      {&nodeAB_->getFamily()},
+      {nodeAB_->getFamilyShared()},
       [&](const ShadowNode& oldShadowNode, const ShadowNodeFragment& fragment) {
         return oldShadowNode.clone({
             .props = newProps,
@@ -379,7 +379,7 @@ TEST_F(ShadowNodeTest, cloneMultipleReturnsNullptrWhenFamilyHasNoPathToRoot) {
   auto newProps = std::make_shared<const TestProps>();
   // nodeZ_ is not part of nodeA_'s tree
   auto result = nodeA_->cloneMultiple(
-      {&nodeZ_->getFamily()},
+      {nodeZ_->getFamilyShared()},
       [&](const ShadowNode& oldShadowNode, const ShadowNodeFragment& fragment) {
         return oldShadowNode.clone({
             .props = newProps,
@@ -396,7 +396,7 @@ TEST_F(ShadowNodeTest, cloneMultipleWithMixOfValidAndInvalidFamilies) {
   auto newProps = std::make_shared<const TestProps>();
   // nodeAB_ is in the tree, nodeZ_ is not
   auto result = nodeA_->cloneMultiple(
-      {&nodeAB_->getFamily(), &nodeZ_->getFamily()},
+      {nodeAB_->getFamilyShared(), nodeZ_->getFamilyShared()},
       [&](const ShadowNode& oldShadowNode, const ShadowNodeFragment& fragment) {
         return oldShadowNode.clone({
             .props = newProps,