From 902541e87473596bb0a930e397d34140488521bc Mon Sep 17 00:00:00 2001
From: Valentin Shergin <shergin@fb.com>
Date: Fri, 28 Jun 2019 22:50:23 -0700
Subject: [PATCH] Fabric: Fixing a concurrency issue in RawProp parsing infra

Summary:
Previously, we stored `keyIndex_` in a Parser object which is incorrect because it makes the parsing process thread-unsafe (the Parser is shared between parsing processes).

That worked previously most of the time because we never parsed props concurrently but we actually do this in the native animation library.

Reviewed By: yungsters

Differential Revision: D16064557

fbshipit-source-id: 211f4301d0746e0f5126a1dcdd59f1ae9a420aa9
---
 ReactCommon/fabric/core/primitives/RawProps.h   |  6 ++++++
 .../fabric/core/primitives/RawPropsParser.cpp   | 17 ++++++++++-------
 .../fabric/core/primitives/RawPropsParser.h     |  2 --
 3 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/ReactCommon/fabric/core/primitives/RawProps.h b/ReactCommon/fabric/core/primitives/RawProps.h
index 6a87f688b1d..29ac85bcbda 100644
--- a/ReactCommon/fabric/core/primitives/RawProps.h
+++ b/ReactCommon/fabric/core/primitives/RawProps.h
@@ -116,6 +116,12 @@ class RawProps final {
   // Case 2: Source data is represented as `folly::dynamic`.
   folly::dynamic dynamic_;
 
+  /*
+   * The index of a prop value that was evaluated on the previous iterations of
+   * calling `at()`.
+   */
+  mutable int keyIndexCursor_{0};
+
   /*
    * Parsed artefacts:
    * To be used by `RawPropParser`.
diff --git a/ReactCommon/fabric/core/primitives/RawPropsParser.cpp b/ReactCommon/fabric/core/primitives/RawPropsParser.cpp
index c829ad0f44e..0da2edf6202 100644
--- a/ReactCommon/fabric/core/primitives/RawPropsParser.cpp
+++ b/ReactCommon/fabric/core/primitives/RawPropsParser.cpp
@@ -17,6 +17,8 @@ RawValue const *RawPropsParser::at(
     RawProps const &rawProps,
     RawPropsKey const &key) const {
   if (UNLIKELY(!ready_)) {
+    // This is not thread-safe part; this happens only during initialization of
+    // a `ComponentDescriptor` where it is actually safe.
     keys_.push_back(key);
     nameToIndex_.insert(key, size_);
     size_++;
@@ -24,14 +26,14 @@ RawValue const *RawPropsParser::at(
   }
 
   do {
-    keyIndex_++;
+    rawProps.keyIndexCursor_++;
 
-    if (UNLIKELY(keyIndex_ >= size_)) {
-      keyIndex_ = 0;
+    if (UNLIKELY(rawProps.keyIndexCursor_ >= size_)) {
+      rawProps.keyIndexCursor_ = 0;
     }
-  } while (UNLIKELY(key != keys_[keyIndex_]));
+  } while (UNLIKELY(key != keys_[rawProps.keyIndexCursor_]));
 
-  auto valueIndex = rawProps.keyIndexToValueIndex_[keyIndex_];
+  auto valueIndex = rawProps.keyIndexToValueIndex_[rawProps.keyIndexCursor_];
   return valueIndex == kRawPropsValueIndexEmpty ? nullptr
                                                 : &rawProps.values_[valueIndex];
 }
@@ -39,13 +41,14 @@ RawValue const *RawPropsParser::at(
 void RawPropsParser::postPrepare() {
   ready_ = true;
   nameToIndex_.reindex();
-  // Next increment will give `0`.
-  keyIndex_ = size_ - 1;
 }
 
 void RawPropsParser::preparse(RawProps const &rawProps) const {
   rawProps.keyIndexToValueIndex_.resize(size_, kRawPropsValueIndexEmpty);
 
+  // Resetting the cursor, the next increment will give `0`.
+  rawProps.keyIndexCursor_ = size_ - 1;
+
   switch (rawProps.mode_) {
     case RawProps::Mode::Empty:
       return;
diff --git a/ReactCommon/fabric/core/primitives/RawPropsParser.h b/ReactCommon/fabric/core/primitives/RawPropsParser.h
index 8a0f8b6e138..e9b142fdd51 100644
--- a/ReactCommon/fabric/core/primitives/RawPropsParser.h
+++ b/ReactCommon/fabric/core/primitives/RawPropsParser.h
@@ -69,8 +69,6 @@ class RawPropsParser final {
   mutable better::small_vector<RawPropsKey, kNumberOfPropsPerComponentSoftCap>
       keys_{};
   mutable RawPropsKeyMap nameToIndex_{};
-
-  mutable int keyIndex_{0};
   mutable int size_{0};
   mutable bool ready_{false};
 };