Core: Fix infinite loop in prop parsing

Summary:
See previous diff where I added RawPropsTest.cpp to unit-test this functionality. Before this, if you looked up a prop name that does not exist, the prop parser would enter an infinite loop.

I also took this opportunity to comment the block a little bit as it's not super intuitive at first glance.

Changelog: [Internal]

Reviewed By: mdvacca

Differential Revision: D18662135

fbshipit-source-id: 319a3b80d1c606db18b2added9f2aa99d4d03407

ReactCommon/fabric/core/primitives/RawPropsParser.cpp

@@ -10,6 +10,10 @@
 #include <folly/Likely.h>
 #include <react/core/RawProps.h>
 
+#ifndef NDEBUG
+#include <glog/logging.h>
+#endif
+
 namespace facebook {
 namespace react {
 
@@ -25,10 +29,38 @@ RawValue const *RawPropsParser::at(
     return nullptr;
   }
 
+  // Normally, keys are looked up in-order. For performance we can simply
+  // increment this key counter, and if the key is equal to the key at the next
+  // index, there's no need to do any lookups. However, it's possible for keys
+  // to be accessed out-of-order or multiple times, in which case we start
+  // searching again from index 0.
+  // To prevent infinite loops (which can occur if
+  // you look up a key that doesn't exist) we keep track of whether or not we've
+  // already looped around, and log and return nullptr if so. However, we ONLY
+  // do this in debug mode, where you're more likely to look up a nonexistent
+  // key as part of debugging. You can (and must) ensure infinite loops are not
+  // possible in production by: (1) constructing all props objects without
+  // conditionals, or (2) if there are conditionals, ensure that in the parsing
+  // setup case, the Props constructor will access _all_ possible props. To
+  // ensure this performance optimization is utilized, always access props in
+  // the same order every time. This is trivial if you have a simple Props
+  // constructor, but difficult or impossible if you have a shared sub-prop
+  // Struct that is used by multiple parent Props.
+#ifndef NDEBUG
+  bool resetLoop = false;
+#endif
   do {
     rawProps.keyIndexCursor_++;
 
     if (UNLIKELY(rawProps.keyIndexCursor_ >= size_)) {
+#ifndef NDEBUG
+      if (resetLoop) {
+        LOG(ERROR) << "Looked up RawProps key that does not exist: "
+                   << (std::string)key;
+        return nullptr;
+      }
+      resetLoop = true;
+#endif
       rawProps.keyIndexCursor_ = 0;
     }
   } while (UNLIKELY(key != keys_[rawProps.keyIndexCursor_]));

ReactCommon/fabric/core/tests/RawPropsTest.cpp

@@ -252,3 +252,19 @@ TEST(ShadowNodeTest, handleRawPropsPrimitiveTypesIncomplete) {
   ASSERT_EQ(raw.at("boolValue", nullptr, nullptr), nullptr);
   ASSERT_EQ((int)*raw.at("intValue", nullptr, nullptr), 42);
 }
+
+#ifndef NDEBUG
+TEST(ShadowNodeTest, handleRawPropsPrimitiveTypesIncorrectLookup) {
+  const auto &raw = RawProps(folly::dynamic::object("intValue", (int)42));
+
+  auto parser = RawPropsParser();
+  parser.prepare<PropsPrimitiveTypes>();
+  raw.parse(parser);
+
+  // Before D18662135, looking up an invalid key would trigger
+  // an infinite loop. This is out of contract, so we should only
+  // test this in debug.
+  ASSERT_EQ(raw.at("flurb", nullptr, nullptr), nullptr);
+  ASSERT_EQ((int)*raw.at("intValue", nullptr, nullptr), 42);
+}
+#endif