Breaking: YGConfigRef related const-correctness fixes (#39374)

Summary: Pull Request resolved: #39374 X-link: facebook/yoga#1371 Right now `YGConfigGetDefault` and `YGNodeGetConfig` both return mutable, freeable, configs, which is bad, since the former points to a global singleton config, and the latter usually does too. Mutating this is not thread safe, and it should never be freed. This change makes these functions return `YGConfigConstRef` to prevent mutation, and also lets us allow `YGConfigNewWithConfig` to accept a const config. If a caller does want to mutate a config (such as to free it), it must be tracked manually. Changelog: [Internal] Reviewed By: javache Differential Revision: D49132476 fbshipit-source-id: ac9ce61149e69c6c25cadb99711435b0a5b9f38a
facebook · Sep 12, 2023 · a4a8ab9 · a4a8ab9
1 parent 5cde80c
commit a4a8ab9
Show file tree

Hide file tree

Showing 10 changed files with 46 additions and 37 deletions.
diff --git a/...ages/react-native/ReactCommon/react/renderer/components/view/YogaLayoutableShadowNode.cpp b/...ages/react-native/ReactCommon/react/renderer/components/view/YogaLayoutableShadowNode.cpp
@@ -123,9 +123,9 @@ YogaLayoutableShadowNode::YogaLayoutableShadowNode(
     }
   }
 
-  YGConfigRef previousConfig = YGNodeGetConfig(
+  YGConfigConstRef previousConfig =
       &static_cast<const YogaLayoutableShadowNode&>(sourceShadowNode)
-           .yogaNode_);
+           .yogaConfig_;
 
   yogaNode_.setContext(this);
   yogaNode_.setOwner(nullptr);
@@ -851,7 +851,7 @@ YogaLayoutableShadowNode& YogaLayoutableShadowNode::shadowNodeFromContext(
 
 yoga::Config& YogaLayoutableShadowNode::initializeYogaConfig(
     yoga::Config& config,
-    const YGConfigRef previousConfig) {
+    YGConfigConstRef previousConfig) {
   YGConfigSetCloneNodeFunc(
       &config, YogaLayoutableShadowNode::yogaNodeCloneCallbackConnector);
   if (previousConfig != nullptr) {

diff --git a/packages/react-native/ReactCommon/react/renderer/components/view/YogaLayoutableShadowNode.h b/packages/react-native/ReactCommon/react/renderer/components/view/YogaLayoutableShadowNode.h
@@ -157,7 +157,7 @@ class YogaLayoutableShadowNode : public LayoutableShadowNode {
 
   static yoga::Config& initializeYogaConfig(
       yoga::Config& config,
-      YGConfigRef previousConfig = nullptr);
+      YGConfigConstRef previousConfig = nullptr);
   static YGNodeRef yogaNodeCloneCallbackConnector(
       YGNodeConstRef oldYogaNode,
       YGNodeConstRef parentYogaNode,

diff --git a/packages/react-native/ReactCommon/yoga/yoga/Yoga.cpp b/packages/react-native/ReactCommon/yoga/yoga/Yoga.cpp
@@ -32,7 +32,7 @@ YOGA_EXPORT void YGNodeSetContext(YGNodeRef node, void* context) {
   return resolveRef(node)->setContext(context);
 }
 
-YOGA_EXPORT YGConfigRef YGNodeGetConfig(YGNodeRef node) {
+YOGA_EXPORT YGConfigConstRef YGNodeGetConfig(YGNodeRef node) {
   return resolveRef(node)->getConfig();
 }
 
@@ -103,22 +103,18 @@ YOGA_EXPORT void YGNodeMarkDirtyAndPropagateToDescendants(
   return resolveRef(node)->markDirtyAndPropagateDownwards();
 }
 
-YOGA_EXPORT WIN_EXPORT YGNodeRef YGNodeNewWithConfig(const YGConfigRef config) {
+YOGA_EXPORT WIN_EXPORT YGNodeRef
+YGNodeNewWithConfig(const YGConfigConstRef config) {
   auto* node = new yoga::Node{resolveRef(config)};
   yoga::assertFatal(
       config != nullptr, "Tried to construct YGNode with null config");
-  yoga::assertFatalWithConfig(
-      resolveRef(config),
-      node != nullptr,
-      "Could not allocate memory for node");
   Event::publish<Event::NodeAllocation>(node, {config});
 
   return node;
 }
 
-YOGA_EXPORT YGConfigRef YGConfigGetDefault() {
-  static YGConfigRef defaultConfig = YGConfigNew();
-  return defaultConfig;
+YOGA_EXPORT YGConfigConstRef YGConfigGetDefault() {
+  return &yoga::Config::getDefault();
 }
 
 YOGA_EXPORT YGNodeRef YGNodeNew(void) {

diff --git a/packages/react-native/ReactCommon/yoga/yoga/Yoga.h b/packages/react-native/ReactCommon/yoga/yoga/Yoga.h
@@ -52,7 +52,7 @@ typedef YGNodeRef (*YGCloneNodeFunc)(
 
 // YGNode
 WIN_EXPORT YGNodeRef YGNodeNew(void);
-WIN_EXPORT YGNodeRef YGNodeNewWithConfig(YGConfigRef config);
+WIN_EXPORT YGNodeRef YGNodeNewWithConfig(YGConfigConstRef config);
 WIN_EXPORT YGNodeRef YGNodeClone(YGNodeConstRef node);
 WIN_EXPORT void YGNodeFree(YGNodeRef node);
 WIN_EXPORT void YGNodeFreeRecursiveWithCleanupFunc(
@@ -131,7 +131,7 @@ WIN_EXPORT void YGNodeCopyStyle(YGNodeRef dstNode, YGNodeConstRef srcNode);
 WIN_EXPORT void* YGNodeGetContext(YGNodeConstRef node);
 WIN_EXPORT void YGNodeSetContext(YGNodeRef node, void* context);
 
-WIN_EXPORT YGConfigRef YGNodeGetConfig(YGNodeRef node);
+WIN_EXPORT YGConfigConstRef YGNodeGetConfig(YGNodeRef node);
 WIN_EXPORT void YGNodeSetConfig(YGNodeRef node, YGConfigRef config);
 
 void YGConfigSetPrintTreeFlag(YGConfigRef config, bool enabled);
@@ -327,7 +327,7 @@ WIN_EXPORT void YGConfigSetCloneNodeFunc(
     YGConfigRef config,
     YGCloneNodeFunc callback);
 
-WIN_EXPORT YGConfigRef YGConfigGetDefault(void);
+WIN_EXPORT YGConfigConstRef YGConfigGetDefault(void);
 
 WIN_EXPORT void YGConfigSetContext(YGConfigRef config, void* context);
 WIN_EXPORT void* YGConfigGetContext(YGConfigConstRef config);

diff --git a/packages/react-native/ReactCommon/yoga/yoga/config/Config.cpp b/packages/react-native/ReactCommon/yoga/yoga/config/Config.cpp
@@ -6,15 +6,18 @@
  */
 
 #include <yoga/config/Config.h>
+#include <yoga/debug/Log.h>
 #include <yoga/node/Node.h>
 
 namespace facebook::yoga {
 
-bool configUpdateInvalidatesLayout(Config* a, Config* b) {
-  return a->getErrata() != b->getErrata() ||
-      a->getEnabledExperiments() != b->getEnabledExperiments() ||
-      a->getPointScaleFactor() != b->getPointScaleFactor() ||
-      a->useWebDefaults() != b->useWebDefaults();
+bool configUpdateInvalidatesLayout(
+    const Config& oldConfig,
+    const Config& newConfig) {
+  return oldConfig.getErrata() != newConfig.getErrata() ||
+      oldConfig.getEnabledExperiments() != newConfig.getEnabledExperiments() ||
+      oldConfig.getPointScaleFactor() != newConfig.getPointScaleFactor() ||
+      oldConfig.useWebDefaults() != newConfig.useWebDefaults();
 }
 
 Config::Config(YGLogger logger) : cloneNodeCallback_{nullptr} {
@@ -145,4 +148,9 @@ YGNodeRef Config::cloneNode(
   return clone;
 }
 
+/*static*/ const Config& Config::getDefault() {
+  static Config config{getDefaultLogger()};
+  return config;
+}
+
 } // namespace facebook::yoga
diff --git a/packages/react-native/ReactCommon/yoga/yoga/config/Config.h b/packages/react-native/ReactCommon/yoga/yoga/config/Config.h
@@ -18,9 +18,11 @@ namespace facebook::yoga {
 class Config;
 class Node;
 
-// Whether moving a node from config "a" to config "b" should dirty previously
+// Whether moving a node from an old to new config should dirty previously
 // calculated layout results.
-bool configUpdateInvalidatesLayout(Config* a, Config* b);
+bool configUpdateInvalidatesLayout(
+    const Config& oldConfig,
+    const Config& newConfig);
 
 // Internal variants of log functions, currently used only by JNI bindings.
 // TODO: Reconcile this with the public API
@@ -95,6 +97,8 @@ class YOGA_EXPORT Config : public ::YGConfig {
       size_t childIndex,
       void* cloneContext) const;
 
+  static const Config& getDefault();
+
 private:
   union {
     CloneWithContextFn withContext;

diff --git a/packages/react-native/ReactCommon/yoga/yoga/debug/Log.cpp b/packages/react-native/ReactCommon/yoga/yoga/debug/Log.cpp
@@ -22,10 +22,11 @@ void vlog(
     void* context,
     const char* format,
     va_list args) {
-  const yoga::Config* logConfig =
-      config != nullptr ? config : resolveRef(YGConfigGetDefault());
-
-  logConfig->log(const_cast<yoga::Node*>(node), level, context, format, args);
+  if (config == nullptr) {
+    getDefaultLogger()(nullptr, node, level, format, args);
+  } else {
+    config->log(node, level, context, format, args);
+  }
 }
 } // namespace
 

diff --git a/packages/react-native/ReactCommon/yoga/yoga/event/event.h b/packages/react-native/ReactCommon/yoga/yoga/event/event.h
@@ -95,12 +95,12 @@ struct YOGA_EXPORT Event {
 
 template <>
 struct Event::TypedData<Event::NodeAllocation> {
-  YGConfigRef config;
+  YGConfigConstRef config;
 };
 
 template <>
 struct Event::TypedData<Event::NodeDeallocation> {
-  YGConfigRef config;
+  YGConfigConstRef config;
 };
 
 template <>

diff --git a/packages/react-native/ReactCommon/yoga/yoga/node/Node.cpp b/packages/react-native/ReactCommon/yoga/yoga/node/Node.cpp
@@ -17,7 +17,9 @@
 
 namespace facebook::yoga {
 
-Node::Node(yoga::Config* config) : config_{config} {
+Node::Node() : Node{&Config::getDefault()} {}
+
+Node::Node(const yoga::Config* config) : config_{config} {
   yoga::assertFatal(
       config != nullptr, "Attempting to construct Node with null config");
 
@@ -285,7 +287,7 @@ void Node::setConfig(yoga::Config* config) {
       config->useWebDefaults() == config_->useWebDefaults(),
       "UseWebDefaults may not be changed after constructing a Node");
 
-  if (yoga::configUpdateInvalidatesLayout(config_, config)) {
+  if (yoga::configUpdateInvalidatesLayout(*config_, *config)) {
     markDirtyAndPropagate();
   }
 

diff --git a/packages/react-native/ReactCommon/yoga/yoga/node/Node.h b/packages/react-native/ReactCommon/yoga/yoga/node/Node.h
@@ -71,7 +71,7 @@ class YOGA_EXPORT Node : public ::YGNode {
   size_t lineIndex_ = 0;
   Node* owner_ = nullptr;
   std::vector<Node*> children_ = {};
-  Config* config_;
+  const Config* config_;
   std::array<YGValue, 2> resolvedDimensions_ = {
       {YGValueUndefined, YGValueUndefined}};
 
@@ -95,10 +95,8 @@ class YOGA_EXPORT Node : public ::YGNode {
   Node& operator=(Node&&) = default;
 
 public:
-  Node() : Node{static_cast<Config*>(YGConfigGetDefault())} {
-    flags_.hasNewLayout = true;
-  }
-  explicit Node(Config* config);
+  Node();
+  explicit Node(const Config* config);
   ~Node() = default; // cleanup of owner/children relationships in YGNodeFree
 
   Node(Node&&);
@@ -165,7 +163,7 @@ class YOGA_EXPORT Node : public ::YGNode {
 
   size_t getChildCount() const { return children_.size(); }
 
-  Config* getConfig() const { return config_; }
+  const Config* getConfig() const { return config_; }
 
   bool isDirty() const { return flags_.isDirty; }