Merge pull request #1551 from Shopify/fix/1517-reanimated-crash

Added locking to mapped properties to fix threading issue with Rea
Shopify · May 4, 2023 · d538096 · d538096
2 parents 8a0e399 + 9fb9220
commit d538096
Show file tree

Hide file tree

Showing 3 changed files with 80 additions and 57 deletions.
diff --git a/package/cpp/rnskia/dom/base/JsiDependencyManager.h b/package/cpp/rnskia/dom/base/JsiDependencyManager.h
@@ -64,53 +64,53 @@ class JsiDependencyManager
 
     // Enumerate registered keys for the given node to only handle known
     // properties
-    for (const auto &propMapping :
-         node->getPropsContainer()->getMappedProperties()) {
-      auto key = propMapping.first;
-      auto jsValue = nextProps.getProperty(runtime, key);
-      JsiValue nativeValue(runtime, jsValue);
-
-      if (isAnimatedValue(nativeValue)) {
-        // Handle Skia Animation Values
-        auto animatedValue = getAnimatedValue(nativeValue);
-        auto unsubscribe = animatedValue->addListener(
-            [animatedValue, propMapping](jsi::Runtime &runtime) {
-              // Get value from animation value
-              auto nextJsValue = animatedValue->getCurrent(runtime);
-              // Update all props that listens to this animation value
-              for (auto &prop : propMapping.second) {
-                prop->updateValue(runtime, nextJsValue);
-              }
-            });
-
-        // Save unsubscribe methods
-        unsubscribers.push_back(std::make_pair(animatedValue, unsubscribe));
-
-      } else if (isSelector(nativeValue)) {
-        // Handle Skia Animation Value Selectors
-        auto animatedValue = std::dynamic_pointer_cast<RNSkReadonlyValue>(
-            nativeValue.getValue(PropNameValue).getAsHostObject());
-
-        auto selector = nativeValue.getValue(PropNameSelector).getAsFunction();
-        // Add subscription to animated value in selector
-        auto unsubscribe = animatedValue->addListener(
-            [nativeValue, propMapping, selector = std::move(selector),
-             animatedValue](jsi::Runtime &runtime) {
-              // Get value from animation value
-              jsi::Value jsValue = animatedValue->getCurrent(runtime);
-              // Call selector to transform new value
-              auto selectedJsValue =
-                  selector(runtime, jsi::Value::null(), &jsValue, 1);
-              // Update all props that listens to this animation value
-              for (auto &prop : propMapping.second) {
-                prop->updateValue(runtime, selectedJsValue);
-              }
-            });
-
-        // Save unsubscribe methods
-        unsubscribers.push_back(std::make_pair(animatedValue, unsubscribe));
-      }
-    }
+    node->getPropsContainer()->enumerateMappedProps(
+        [&](const PropId key, const std::vector<NodeProp *> &propMapping) {
+          auto jsValue = nextProps.getProperty(runtime, key);
+          JsiValue nativeValue(runtime, jsValue);
+
+          if (isAnimatedValue(nativeValue)) {
+            // Handle Skia Animation Values
+            auto animatedValue = getAnimatedValue(nativeValue);
+            auto unsubscribe = animatedValue->addListener(
+                [animatedValue, propMapping](jsi::Runtime &runtime) {
+                  // Get value from animation value
+                  auto nextJsValue = animatedValue->getCurrent(runtime);
+                  // Update all props that listens to this animation value
+                  for (auto &prop : propMapping) {
+                    prop->updateValue(runtime, nextJsValue);
+                  }
+                });
+
+            // Save unsubscribe methods
+            unsubscribers.push_back(std::make_pair(animatedValue, unsubscribe));
+
+          } else if (isSelector(nativeValue)) {
+            // Handle Skia Animation Value Selectors
+            auto animatedValue = std::dynamic_pointer_cast<RNSkReadonlyValue>(
+                nativeValue.getValue(PropNameValue).getAsHostObject());
+
+            auto selector =
+                nativeValue.getValue(PropNameSelector).getAsFunction();
+            // Add subscription to animated value in selector
+            auto unsubscribe = animatedValue->addListener(
+                [nativeValue, propMapping, selector = std::move(selector),
+                 animatedValue](jsi::Runtime &runtime) {
+                  // Get value from animation value
+                  jsi::Value jsValue = animatedValue->getCurrent(runtime);
+                  // Call selector to transform new value
+                  auto selectedJsValue =
+                      selector(runtime, jsi::Value::null(), &jsValue, 1);
+                  // Update all props that listens to this animation value
+                  for (auto &prop : propMapping) {
+                    prop->updateValue(runtime, selectedJsValue);
+                  }
+                });
+
+            // Save unsubscribe methods
+            unsubscribers.push_back(std::make_pair(animatedValue, unsubscribe));
+          }
+        });
 
     // Now let's store the subscription info
     _subscriptions.emplace(node.get(), unsubscribers);

diff --git a/package/cpp/rnskia/dom/base/JsiDomNode.h b/package/cpp/rnskia/dom/base/JsiDomNode.h
@@ -98,13 +98,12 @@ class JsiDomNode : public JsiHostObject,
     auto propName = arguments[0].asString(runtime).utf8(runtime);
     const jsi::Value &propValue = arguments[1];
 
-    auto mappedProps = _propsContainer->getMappedProperties();
-    auto propMapIt = mappedProps.find(JsiPropId::get(propName));
-    if (propMapIt != mappedProps.end()) {
-      for (auto &prop : propMapIt->second) {
-        prop->updateValue(runtime, propValue);
-      }
-    }
+    // Enumerate all props with this name and update. The
+    // enumerateMappedPropsByName function is thread safe and locks props so it
+    // can be called from all threads.
+    _propsContainer->enumerateMappedPropsByName(propName, [&](NodeProp *prop) {
+      prop->updateValue(runtime, propValue);
+    });
 
     return jsi::Value::undefined();
   }

diff --git a/package/cpp/rnskia/dom/base/NodePropsContainer.h b/package/cpp/rnskia/dom/base/NodePropsContainer.h
@@ -41,10 +41,30 @@ class NodePropsContainer {
   }
 
   /**
-   Returns a list of mappings betwen property names and property objects
+   Enumerate all mapped properties
    */
-  const std::map<PropId, std::vector<NodeProp *>> &getMappedProperties() {
-    return _mappedProperties;
+  void enumerateMappedProps(
+      const std::function<void(const PropId name,
+                               const std::vector<NodeProp *>)> &callback) {
+    std::lock_guard<std::mutex> lock(_mappedPropsLock);
+    for (auto &props : _mappedProperties) {
+      callback(props.first, props.second);
+    }
+  }
+
+  /**
+   Enumerates a named property instances from the mapped properties list
+   */
+  void
+  enumerateMappedPropsByName(const std::string &name,
+                             const std::function<void(NodeProp *)> &callback) {
+    std::lock_guard<std::mutex> lock(_mappedPropsLock);
+    auto propMapIt = _mappedProperties.find(JsiPropId::get(name));
+    if (propMapIt != _mappedProperties.end()) {
+      for (auto &prop : propMapIt->second) {
+        callback(prop);
+      }
+    }
   }
 
   /**
@@ -75,6 +95,7 @@ class NodePropsContainer {
    Clears all props and data from the container
    */
   void dispose() {
+    std::lock_guard<std::mutex> lock(_mappedPropsLock);
     _properties.clear();
     _mappedProperties.clear();
   }
@@ -83,6 +104,8 @@ class NodePropsContainer {
    Called when the React / JS side sets properties on a node
    */
   void setProps(jsi::Runtime &runtime, const jsi::Value &maybePropsObject) {
+    std::lock_guard<std::mutex> lock(_mappedPropsLock);
+
     // Clear property mapping
     _mappedProperties.clear();
 
@@ -129,6 +152,7 @@ class NodePropsContainer {
   std::vector<std::shared_ptr<BaseNodeProp>> _properties;
   std::map<PropId, std::vector<NodeProp *>> _mappedProperties;
   PropId _type;
+  std::mutex _mappedPropsLock;
 };
 
 } // namespace RNSkia