From 105beed56989904a2ff112af305563b6cc1c769c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20Norte?= Date: Wed, 20 Nov 2024 10:00:48 -0800 Subject: [PATCH] Fix bugs in MutationObserver (#47760) Summary: Pull Request resolved: https://github.com/facebook/react-native/pull/47760 Changelog: [internal] (this is internal because this API isn't enabled in OSS yet) This fixes 2 bugs in `MutationObserver`: 1. Incorrectly reporting updates for the same node multiple times, if changes are observable by different targets in the same observer. 2. Incorrectly ignoring observations of subsequent targets in a given observer. These were caught when migrating the unit tests for `MutationObserver` that were mocking Fabric to a Fantom integration test that uses the whole C++ infra. Reviewed By: sammy-SC Differential Revision: D66232571 fbshipit-source-id: b6e967ca4deaa1a69d35f14d4f921103fec2bbaf --- .../observers/mutation/MutationObserver.cpp | 30 +++++++++---------- .../observers/mutation/MutationObserver.h | 19 +++++++++--- .../mutation/MutationObserverManager.cpp | 4 +-- 3 files changed, 31 insertions(+), 22 deletions(-) diff --git a/packages/react-native/ReactCommon/react/renderer/observers/mutation/MutationObserver.cpp b/packages/react-native/ReactCommon/react/renderer/observers/mutation/MutationObserver.cpp index 4946c1c3e8a..032e5e4c74a 100644 --- a/packages/react-native/ReactCommon/react/renderer/observers/mutation/MutationObserver.cpp +++ b/packages/react-native/ReactCommon/react/renderer/observers/mutation/MutationObserver.cpp @@ -133,31 +133,30 @@ void MutationObserver::recordMutationsInTarget( } recordMutationsInSubtrees( - std::move(targetShadowNode), - *oldTargetShadowNode, - *newTargetShadowNode, + oldTargetShadowNode, + newTargetShadowNode, observeSubtree, recordedMutations, processedNodes); } void MutationObserver::recordMutationsInSubtrees( - ShadowNode::Shared targetShadowNode, - const ShadowNode& oldNode, - const ShadowNode& newNode, + const ShadowNode::Shared& oldNode, + const ShadowNode::Shared& newNode, bool observeSubtree, std::vector& recordedMutations, - SetOfShadowNodePointers processedNodes) const { - bool isSameNode = &oldNode == &newNode; + SetOfShadowNodePointers& processedNodes) const { + bool isSameNode = oldNode.get() == newNode.get(); // If the nodes are referentially equal, their children are also the same. - if (isSameNode || processedNodes.find(&newNode) != processedNodes.end()) { + if (isSameNode || + processedNodes.find(oldNode.get()) != processedNodes.end()) { return; } - processedNodes.insert(&newNode); + processedNodes.insert(oldNode.get()); - auto oldChildren = oldNode.getChildren(); - auto newChildren = newNode.getChildren(); + auto oldChildren = oldNode->getChildren(); + auto newChildren = newNode->getChildren(); std::vector addedNodes; std::vector removedNodes; @@ -171,9 +170,8 @@ void MutationObserver::recordMutationsInSubtrees( // Nodes are present in both tress. If `subtree` is set to true, // we continue checking their children. recordMutationsInSubtrees( - targetShadowNode, - *oldChild, - *newChild, + oldChild, + newChild, observeSubtree, recordedMutations, processedNodes); @@ -191,7 +189,7 @@ void MutationObserver::recordMutationsInSubtrees( if (!addedNodes.empty() || !removedNodes.empty()) { recordedMutations.emplace_back(MutationRecord{ mutationObserverId_, - targetShadowNode, + oldNode, std::move(addedNodes), std::move(removedNodes)}); } diff --git a/packages/react-native/ReactCommon/react/renderer/observers/mutation/MutationObserver.h b/packages/react-native/ReactCommon/react/renderer/observers/mutation/MutationObserver.h index 2e05cfc0e57..62a6c4d3461 100644 --- a/packages/react-native/ReactCommon/react/renderer/observers/mutation/MutationObserver.h +++ b/packages/react-native/ReactCommon/react/renderer/observers/mutation/MutationObserver.h @@ -27,6 +27,18 @@ class MutationObserver { public: MutationObserver(MutationObserverId intersectionObserverId); + // delete copy constructor + MutationObserver(const MutationObserver&) = delete; + + // delete copy assignment + MutationObserver& operator=(const MutationObserver&) = delete; + + // allow move constructor + MutationObserver(MutationObserver&&) = default; + + // allow move assignment + MutationObserver& operator=(MutationObserver&&) = default; + void observe(ShadowNode::Shared targetShadowNode, bool observeSubtree); void unobserve(const ShadowNode& targetShadowNode); @@ -53,12 +65,11 @@ class MutationObserver { SetOfShadowNodePointers& processedNodes) const; void recordMutationsInSubtrees( - ShadowNode::Shared targetShadowNode, - const ShadowNode& oldNode, - const ShadowNode& newNode, + const ShadowNode::Shared& oldNode, + const ShadowNode::Shared& newNode, bool observeSubtree, std::vector& recordedMutations, - SetOfShadowNodePointers processedNodes) const; + SetOfShadowNodePointers& processedNodes) const; }; } // namespace facebook::react diff --git a/packages/react-native/ReactCommon/react/renderer/observers/mutation/MutationObserverManager.cpp b/packages/react-native/ReactCommon/react/renderer/observers/mutation/MutationObserverManager.cpp index fec927b1874..9c06684f539 100644 --- a/packages/react-native/ReactCommon/react/renderer/observers/mutation/MutationObserverManager.cpp +++ b/packages/react-native/ReactCommon/react/renderer/observers/mutation/MutationObserverManager.cpp @@ -29,9 +29,9 @@ void MutationObserverManager::observe( if (observerIt == observers.end()) { auto observer = MutationObserver{mutationObserverId}; observer.observe(shadowNode, observeSubtree); - observers.insert({mutationObserverId, std::move(observer)}); + observers.emplace(mutationObserverId, std::move(observer)); } else { - auto observer = observerIt->second; + auto& observer = observerIt->second; observer.observe(shadowNode, observeSubtree); } }