Make consumption of NativeArray explicit

Reviewed By: mhorowitz

Differential Revision: D4415232

fbshipit-source-id: a27dd5cc3994c01fb1ca6e4c1d4f87cb8a95896a

ReactAndroid/src/main/jni/xreact/jni/CatalystInstanceImpl.cpp

@@ -237,11 +237,11 @@ void CatalystInstanceImpl::callJSFunction(
   instance_->callJSFunction(token->getExecutorToken(nullptr),
                             std::move(module),
                             std::move(method),
-                            std::move(arguments->array));
+                            arguments->consume());
 }
 
 void CatalystInstanceImpl::callJSCallback(JExecutorToken* token, jint callbackId, NativeArray* arguments) {
-  instance_->callJSCallback(token->getExecutorToken(nullptr), callbackId, std::move(arguments->array));
+  instance_->callJSCallback(token->getExecutorToken(nullptr), callbackId, arguments->consume());
 }
 
 local_ref<JExecutorToken::JavaPart> CatalystInstanceImpl::getMainExecutorToken() {

ReactAndroid/src/main/jni/xreact/jni/JCallback.h

@@ -4,8 +4,8 @@
 
 #include <memory>
 
-#include <folly/dynamic.h>
 #include <fb/fbjni.h>
+#include <folly/dynamic.h>
 
 #include "NativeArray.h"
 
@@ -34,7 +34,7 @@ private:
   JCallbackImpl(Callback callback) : callback_(std::move(callback)) {}
 
   void invoke(NativeArray* arguments) {
-    callback_(std::move(arguments->array));
+    callback_(arguments->consume());
   }
 
   Callback callback_;

ReactAndroid/src/main/jni/xreact/jni/JavaModuleWrapper.cpp

@@ -76,7 +76,7 @@ folly::dynamic JavaNativeModule::getConstants() {
     return nullptr;
   } else {
     // See JavaModuleWrapper#getConstants for the other side of this hack.
-    return cthis(constants)->array[0];
+    return cthis(constants)->consume()[0];
   }
 }
 
@@ -145,7 +145,7 @@ folly::dynamic NewJavaNativeModule::getConstants() {
     return nullptr;
   } else {
     // See JavaModuleWrapper#getConstants for the other side of this hack.
-    return cthis(constants)->array[0];
+    return cthis(constants)->consume()[0];
   }
 }
 

ReactAndroid/src/main/jni/xreact/jni/MethodInvoker.cpp

@@ -221,7 +221,7 @@ MethodCallResult MethodInvoker::invoke(std::weak_ptr<Instance>& instance, jni::a
 
     CASE_OBJECT('S', JString, toStdString())
     CASE_OBJECT('M', WritableNativeMap, cthis()->consume())
-    CASE_OBJECT('A', WritableNativeArray, cthis()->array)
+    CASE_OBJECT('A', WritableNativeArray, cthis()->consume())
 
     default:
       LOG(FATAL) << "Unknown return type: " << returnType;

ReactAndroid/src/main/jni/xreact/jni/NativeArray.cpp

@@ -5,24 +5,22 @@
 #include <fb/fbjni.h>
 #include <folly/json.h>
 
-#include "NativeCommon.h"
-
 using namespace facebook::jni;
 
 namespace facebook {
 namespace react {
 
-NativeArray::NativeArray(folly::dynamic a)
-    : array(std::move(a)) {
-  if (!array.isArray()) {
+NativeArray::NativeArray(folly::dynamic array)
+    : isConsumed(false), array_(std::move(array)) {
+  if (!array_.isArray()) {
     throwNewJavaException(exceptions::gUnexpectedNativeTypeExceptionClass,
-                               "expected Array, got a %s", array.typeName());
+                               "expected Array, got a %s", array_.typeName());
   }
 }
 
 local_ref<jstring> NativeArray::toString() {
-  exceptions::throwIfObjectAlreadyConsumed(this, "Array already consumed");
-  return make_jstring(folly::toJson(array).c_str());
+  throwIfConsumed();
+  return make_jstring(folly::toJson(array_).c_str());
 }
 
 void NativeArray::registerNatives() {
@@ -31,5 +29,15 @@ void NativeArray::registerNatives() {
   });
 }
 
+folly::dynamic NativeArray::consume() {
+  throwIfConsumed();
+  isConsumed = true;
+  return std::move(array_);
+}
+
+void NativeArray::throwIfConsumed() {
+  exceptions::throwIfObjectAlreadyConsumed(this, "Array already consumed");
+}
+
 }
 }

ReactAndroid/src/main/jni/xreact/jni/NativeArray.h

@@ -5,6 +5,8 @@
 #include <fb/fbjni.h>
 #include <folly/dynamic.h>
 
+#include "NativeCommon.h"
+
 namespace facebook {
 namespace react {
 
@@ -12,16 +14,20 @@ class NativeArray : public jni::HybridClass<NativeArray> {
  public:
   static constexpr const char* kJavaDescriptor = "Lcom/facebook/react/bridge/NativeArray;";
 
+  jni::local_ref<jstring> toString();
+
+  RN_EXPORT folly::dynamic consume();
+
   // Whether this array has been added to another array or map and no longer
   // has a valid array value.
-  bool isConsumed = false;
-  folly::dynamic array;
-
-  jni::local_ref<jstring> toString();
+  bool isConsumed;
+  void throwIfConsumed();
 
   static void registerNatives();
 
  protected:
+  folly::dynamic array_;
+
   friend HybridBase;
   explicit NativeArray(folly::dynamic array);
 };

ReactAndroid/src/main/jni/xreact/jni/NativeCommon.h

@@ -5,6 +5,10 @@
 #include <fb/fbjni.h>
 #include <folly/dynamic.h>
 
+#ifndef RN_EXPORT
+#define RN_EXPORT __attribute__((visibility("default")))
+#endif
+
 namespace facebook {
 namespace react {
 

ReactAndroid/src/main/jni/xreact/jni/NativeMap.cpp

@@ -2,7 +2,7 @@
 
 #include "NativeMap.h"
 
-#include "NativeCommon.h"
+#include <folly/json.h>
 
 using namespace facebook::jni;
 
@@ -16,8 +16,14 @@ std::string NativeMap::toString() {
 
 void NativeMap::registerNatives() {
   registerHybrid({
-      makeNativeMethod("toString", NativeMap::toString),
-    });
+    makeNativeMethod("toString", NativeMap::toString),
+  });
+}
+
+folly::dynamic NativeMap::consume() {
+  throwIfConsumed();
+  isConsumed = true;
+  return std::move(map_);
 }
 
 void NativeMap::throwIfConsumed() {

ReactAndroid/src/main/jni/xreact/jni/NativeMap.h

@@ -4,7 +4,8 @@
 
 #include <fb/fbjni.h>
 #include <folly/dynamic.h>
-#include <folly/json.h>
+
+#include "NativeCommon.h"
 
 namespace facebook {
 namespace react {
@@ -13,20 +14,23 @@ class NativeMap : public jni::HybridClass<NativeMap> {
  public:
   static auto constexpr kJavaDescriptor = "Lcom/facebook/react/bridge/NativeMap;";
 
-  explicit NativeMap(folly::dynamic s) : isConsumed(false), map_(s) {}
-
   std::string toString();
 
+  RN_EXPORT folly::dynamic consume();
+
+  // Whether this map has been added to another array or map and no longer
+  // has a valid map value.
   bool isConsumed;
   void throwIfConsumed();
 
   static void registerNatives();
- protected:
 
+ protected:
   folly::dynamic map_;
 
   friend HybridBase;
   friend struct ReadableNativeMapKeySetIterator;
+  explicit NativeMap(folly::dynamic s) : isConsumed(false), map_(s) {}
 };
 
 }  // namespace react

ReactAndroid/src/main/jni/xreact/jni/OnLoad.cpp

@@ -86,7 +86,7 @@ class JSCJavaScriptExecutorHolder : public HybridClass<JSCJavaScriptExecutorHold
 
   static local_ref<jhybriddata> initHybrid(alias_ref<jclass>, ReadableNativeArray* jscConfigArray) {
     // See JSCJavaScriptExecutor.Factory() for the other side of this hack.
-    folly::dynamic jscConfigMap = jscConfigArray->array[0];
+    folly::dynamic jscConfigMap = jscConfigArray->consume()[0];
     jscConfigMap["PersistentDirectory"] = getApplicationPersistentDir();
     return makeCxxInstance(
       std::make_shared<JSCExecutorFactory>(getApplicationCacheDir(), std::move(jscConfigMap)));

ReactAndroid/src/main/jni/xreact/jni/ReadableNativeArray.cpp

@@ -23,19 +23,19 @@ void ReadableNativeArray::mapException(const std::exception& ex) {
 }
 
 jint ReadableNativeArray::getSize() {
-  return array.size();
+  return array_.size();
 }
 
 jboolean ReadableNativeArray::isNull(jint index) {
-  return array.at(index).isNull() ? JNI_TRUE : JNI_FALSE;
+  return array_.at(index).isNull() ? JNI_TRUE : JNI_FALSE;
 }
 
 jboolean ReadableNativeArray::getBoolean(jint index) {
-  return array.at(index).getBool() ? JNI_TRUE : JNI_FALSE;
+  return array_.at(index).getBool() ? JNI_TRUE : JNI_FALSE;
 }
 
 jdouble ReadableNativeArray::getDouble(jint index) {
-  const folly::dynamic& val = array.at(index);
+  const folly::dynamic& val = array_.at(index);
   if (val.isInt()) {
     return val.getInt();
   }
@@ -43,7 +43,7 @@ jdouble ReadableNativeArray::getDouble(jint index) {
 }
 
 jint ReadableNativeArray::getInt(jint index) {
-  auto integer = array.at(index).getInt();
+  auto integer = array_.at(index).getInt();
   static_assert(std::is_same<decltype(integer), int64_t>::value,
                 "folly::dynamic int is not int64_t");
   jint javaint = static_cast<jint>(integer);
@@ -56,7 +56,7 @@ jint ReadableNativeArray::getInt(jint index) {
 }
 
 const char* ReadableNativeArray::getString(jint index) {
-  const folly::dynamic& dyn = array.at(index);
+  const folly::dynamic& dyn = array_.at(index);
   if (dyn.isNull()) {
     return nullptr;
   }
@@ -64,7 +64,7 @@ const char* ReadableNativeArray::getString(jint index) {
 }
 
 local_ref<ReadableNativeArray::jhybridobject> ReadableNativeArray::getArray(jint index) {
-  auto& elem = array.at(index);
+  auto& elem = array_.at(index);
   if (elem.isNull()) {
     return local_ref<ReadableNativeArray::jhybridobject>(nullptr);
   } else {
@@ -73,11 +73,11 @@ local_ref<ReadableNativeArray::jhybridobject> ReadableNativeArray::getArray(jint
 }
 
 local_ref<ReadableType> ReadableNativeArray::getType(jint index) {
-  return ReadableType::getType(array.at(index).type());
+  return ReadableType::getType(array_.at(index).type());
 }
 
 local_ref<NativeMap::jhybridobject> ReadableNativeArray::getMap(jint index) {
-  auto& elem = array.at(index);
+  auto& elem = array_.at(index);
   return ReadableNativeMap::createWithContents(folly::dynamic(elem));
 }
 

ReactAndroid/src/main/jni/xreact/jni/WritableNativeArray.cpp

@@ -17,23 +17,23 @@ local_ref<WritableNativeArray::jhybriddata> WritableNativeArray::initHybrid(alia
 }
 
 void WritableNativeArray::pushNull() {
-  exceptions::throwIfObjectAlreadyConsumed(this, "Array already consumed");
-  array.push_back(nullptr);
+  throwIfConsumed();
+  array_.push_back(nullptr);
 }
 
 void WritableNativeArray::pushBoolean(jboolean value) {
-  exceptions::throwIfObjectAlreadyConsumed(this, "Array already consumed");
-  array.push_back(value == JNI_TRUE);
+  throwIfConsumed();
+  array_.push_back(value == JNI_TRUE);
 }
 
 void WritableNativeArray::pushDouble(jdouble value) {
-  exceptions::throwIfObjectAlreadyConsumed(this, "Receiving array already consumed");
-  array.push_back(value);
+  throwIfConsumed();
+  array_.push_back(value);
 }
 
 void WritableNativeArray::pushInt(jint value) {
-  exceptions::throwIfObjectAlreadyConsumed(this, "Receiving array already consumed");
-  array.push_back(value);
+  throwIfConsumed();
+  array_.push_back(value);
 }
 
 void WritableNativeArray::pushString(jstring value) {
@@ -41,8 +41,8 @@ void WritableNativeArray::pushString(jstring value) {
     pushNull();
     return;
   }
-  exceptions::throwIfObjectAlreadyConsumed(this, "Receiving array already consumed");
-  array.push_back(wrap_alias(value)->toStdString());
+  throwIfConsumed();
+  array_.push_back(wrap_alias(value)->toStdString());
 }
 
 void WritableNativeArray::pushNativeArray(WritableNativeArray* otherArray) {
@@ -50,10 +50,8 @@ void WritableNativeArray::pushNativeArray(WritableNativeArray* otherArray) {
     pushNull();
     return;
   }
-  exceptions::throwIfObjectAlreadyConsumed(this, "Receiving array already consumed");
-  exceptions::throwIfObjectAlreadyConsumed(otherArray, "Array to push already consumed");
-  array.push_back(std::move(otherArray->array));
-  otherArray->isConsumed = true;
+  throwIfConsumed();
+  array_.push_back(otherArray->consume());
 }
 
 void WritableNativeArray::pushNativeMap(WritableNativeMap* map) {
@@ -61,9 +59,8 @@ void WritableNativeArray::pushNativeMap(WritableNativeMap* map) {
     pushNull();
     return;
   }
-  exceptions::throwIfObjectAlreadyConsumed(this, "Receiving array already consumed");
-  map->throwIfConsumed();
-  array.push_back(map->consume());
+  throwIfConsumed();
+  array_.push_back(map->consume());
 }
 
 void WritableNativeArray::registerNatives() {

ReactAndroid/src/main/jni/xreact/jni/WritableNativeMap.cpp

@@ -21,12 +21,6 @@ local_ref<WritableNativeMap::jhybriddata> WritableNativeMap::initHybrid(alias_re
   return makeCxxInstance();
 }
 
-folly::dynamic WritableNativeMap::consume() {
-  throwIfConsumed();
-  isConsumed = true;
-  return std::move(map_);
-}
-
 void WritableNativeMap::putNull(std::string key) {
   throwIfConsumed();
   map_.insert(std::move(key), nullptr);
@@ -56,26 +50,22 @@ void WritableNativeMap::putString(std::string key, alias_ref<jstring> val) {
   map_.insert(std::move(key), val->toString());
 }
 
-void WritableNativeMap::putNativeArray(std::string key, alias_ref<WritableNativeArray::jhybridobject> val) {
-  if (!val) {
+void WritableNativeMap::putNativeArray(std::string key, WritableNativeArray* otherArray) {
+  if (!otherArray) {
     putNull(std::move(key));
     return;
   }
   throwIfConsumed();
-  auto array = val->cthis();
-  exceptions::throwIfObjectAlreadyConsumed(array, "Array to put already consumed");
-  map_.insert(key, std::move(array->array));
-  array->isConsumed = true;
+  map_.insert(key, otherArray->consume());
 }
 
-void WritableNativeMap::putNativeMap(std::string key, alias_ref<jhybridobject> val) {
-  if (!val) {
+void WritableNativeMap::putNativeMap(std::string key, WritableNativeMap *otherMap) {
+  if (!otherMap) {
     putNull(std::move(key));
     return;
   }
   throwIfConsumed();
-  auto other = val->cthis();
-  map_.insert(std::move(key), other->consume());
+  map_.insert(std::move(key), otherMap->consume());
 }
 
 void WritableNativeMap::mergeNativeMap(ReadableNativeMap* other) {

ReactAndroid/src/main/jni/xreact/jni/WritableNativeMap.h

@@ -20,16 +20,13 @@ struct WritableNativeMap : jni::HybridClass<WritableNativeMap, ReadableNativeMap
 
   static jni::local_ref<jhybriddata> initHybrid(jni::alias_ref<jclass>);
 
-  __attribute__((visibility("default")))
-  folly::dynamic consume();
-
   void putNull(std::string key);
   void putBoolean(std::string key, bool val);
   void putDouble(std::string key, double val);
   void putInt(std::string key, int val);
   void putString(std::string key, jni::alias_ref<jstring> val);
-  void putNativeArray(std::string key, jni::alias_ref<WritableNativeArray::jhybridobject> val);
-  void putNativeMap(std::string key, jni::alias_ref<jhybridobject> val);
+  void putNativeArray(std::string key, WritableNativeArray* val);
+  void putNativeMap(std::string key, WritableNativeMap* val);
   void mergeNativeMap(ReadableNativeMap* other);
 
   static void registerNatives();