Implement ObjectArena and ArenaRef to solve JNI Global Reference Issue

firestore/CMakeLists.txt

@@ -138,6 +138,8 @@ set(android_SRCS
     src/android/wrapper.h
     src/android/write_batch_android.cc
     src/android/write_batch_android.h
+    src/jni/arena_ref.cc
+    src/jni/arena_ref.h
     src/jni/array.h
     src/jni/array_list.cc
     src/jni/array_list.h

firestore/integration_test_internal/CMakeLists.txt

@@ -131,6 +131,7 @@ set(FIREBASE_INTEGRATION_TEST_ANDROID_TEST_SRCS
     src/android/snapshot_metadata_android_test.cc
     src/android/timestamp_android_test.cc
     src/android/transaction_options_android_test.cc
+    src/jni/arena_ref_test.cc
     src/jni/declaration_test.cc
     src/jni/env_test.cc
     src/jni/object_test.cc

firestore/integration_test_internal/src/document_reference_test.cc

@@ -59,7 +59,7 @@ TEST_F(DocumentReferenceTest, RecoverFirestore) {
   DocumentReference doc = Document();
   ASSERT_EQ(db, doc.firestore());  // Sanity check
 
-  jni::Object doc_java = GetInternal(doc)->ToJava();
+  jni::Local<jni::Object> doc_java = GetInternal(doc)->ToJava();
   result = DocumentReferenceInternal::Create(env, doc_java);
   ASSERT_EQ(db, result.firestore());
 }

firestore/integration_test_internal/src/field_value_test.cc

@@ -367,5 +367,12 @@ TEST_F(FieldValueTest, TestIncrementChoosesTheCorrectType) {
   // clang-format on
 }
 
+TEST_F(FieldValueTest, TestArenaRefMinimunLimit) {
+  std::vector<FieldValue> numbers;
+  for (size_t i = 0U; i < 60000; i++) {
+    numbers.push_back(FieldValue::Integer(i));
+  }
+}
+
 }  // namespace firestore
 }  // namespace firebase

firestore/integration_test_internal/src/jni/arena_ref_test.cc

@@ -0,0 +1,278 @@
+/*
+ * Copyright 2023 Google LLC
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include "firestore/src/jni/arena_ref.h"
+#include "firestore/src/android/firestore_android.h"
+#include "firestore/src/common/make_unique.h"
+#include "firestore/src/jni/loader.h"
+#include "firestore/src/jni/long.h"
+
+#include "firestore_integration_test.h"
+
+#include "gtest/gtest.h"
+
+namespace firebase {
+namespace firestore {
+namespace jni {
+namespace {
+
+constexpr char kException[] = "java/lang/Exception";
+Method<String> kConstructor("<init>", "(Ljava/lang/String;)V");
+
+class ArenaRefTestAndroid : public FirestoreIntegrationTest {
+ public:
+  ArenaRefTestAndroid() : loader_(app()), env_(make_unique<Env>(GetEnv())) {
+    loader_.LoadClass(kException);
+    loader_.Load(kConstructor);
+  }
+
+  ~ArenaRefTestAndroid() override {
+    // Ensure that after the test is done that any pending exception is cleared
+    // to prevent spurious errors in the teardown of FirestoreIntegrationTest.
+    env_->ExceptionClear();
+  }
+
+  Env& env() const { return *env_; }
+
+  void throwException() {
+    Local<Class> clazz = env().FindClass(kException);
+    jmethodID ctor =
+        env().GetMethodId(clazz, "<init>", "(Ljava/lang/String;)V");
+
+    Local<String> message = env().NewStringUtf("Testing throw");
+    Local<Throwable> exception = env().New<Throwable>(clazz, ctor, message);
+    // After throwing, use EXPECT rather than ASSERT to ensure that the
+    // exception is cleared.
+    env().Throw(exception);
+    EXPECT_FALSE(env().ok());
+  }
+
+  void clearExceptionOccurred() {
+    Local<Throwable> thrown = env().ClearExceptionOccurred();
+    EXPECT_EQ(thrown.GetMessage(env()), "Testing throw");
+  }
+
+ private:
+  // Env is declared as having a `noexcept(false)` destructor, which causes the
+  // compiler to propagate this into `EnvTest`'s destructor, but this conflicts
+  // with the declaration of the parent class. Holding `Env` with a unique
+  // pointer sidesteps this restriction.
+  std::unique_ptr<Env> env_;
+  Loader loader_;
+};
+
+TEST_F(ArenaRefTestAndroid, DefaultConstructor) {
+  ArenaRef arena_ref;
+  EXPECT_EQ(arena_ref.get(env()).get(), nullptr);
+}
+
+TEST_F(ArenaRefTestAndroid, ConstructsFromNull) {
+  Local<String> string;
+  ArenaRef arena_ref(env(), string);
+  EXPECT_EQ(arena_ref.get(env()).get(), nullptr);
+}
+
+TEST_F(ArenaRefTestAndroid, ConstructsFromValid) {
+  Local<String> string = env().NewStringUtf("hello world");
+  ArenaRef arena_ref(env(), string);
+  EXPECT_TRUE(env().IsSameObject(arena_ref.get(env()), string));
+}
+
+TEST_F(ArenaRefTestAndroid, CopyConstructsFromNull) {
+  ArenaRef arena_ref1;
+  ArenaRef arena_ref2(arena_ref1);
+  EXPECT_EQ(arena_ref2.get(env()).get(), nullptr);
+}
+
+TEST_F(ArenaRefTestAndroid, CopyConstructsFromValid) {
+  Local<String> string = env().NewStringUtf("hello world");
+
+  ArenaRef arena_ref1(env(), string);
+  ArenaRef arena_ref2(arena_ref1);
+  EXPECT_TRUE(env().IsSameObject(arena_ref1.get(env()), string));
+  EXPECT_TRUE(env().IsSameObject(arena_ref2.get(env()), string));
+}
+
+TEST_F(ArenaRefTestAndroid, CopyAssignsFromNullToNull) {
+  ArenaRef arena_ref1, arena_ref2;
+  arena_ref2 = arena_ref1;
+  EXPECT_EQ(arena_ref1.get(env()).get(), nullptr);
+  EXPECT_EQ(arena_ref2.get(env()).get(), nullptr);
+}
+
+TEST_F(ArenaRefTestAndroid, CopyAssignsFromNullToValid) {
+  Local<String> string = env().NewStringUtf("hello world");
+
+  ArenaRef arena_ref1;
+  ArenaRef arena_ref2(env(), string);
+  arena_ref2 = arena_ref1;
+  EXPECT_EQ(arena_ref1.get(env()).get(), nullptr);
+  EXPECT_EQ(arena_ref2.get(env()).get(), nullptr);
+}
+
+TEST_F(ArenaRefTestAndroid, CopyAssignsFromValidToNull) {
+  Local<String> string = env().NewStringUtf("hello world");
+
+  ArenaRef arena_ref1;
+  ArenaRef arena_ref2(env(), string);
+  arena_ref1 = arena_ref2;
+  EXPECT_TRUE(env().IsSameObject(arena_ref1.get(env()), string));
+  EXPECT_TRUE(env().IsSameObject(arena_ref2.get(env()), string));
+}
+
+TEST_F(ArenaRefTestAndroid, CopyAssignsFromValidToValid) {
+  Local<String> string1 = env().NewStringUtf("hello world");
+  Local<String> string2 = env().NewStringUtf("hello earth");
+
+  ArenaRef arena_ref1(env(), string1);
+  ArenaRef arena_ref2(env(), string2);
+  arena_ref1 = arena_ref2;
+
+  EXPECT_TRUE(env().IsSameObject(arena_ref1.get(env()), string2));
+  EXPECT_TRUE(env().IsSameObject(arena_ref2.get(env()), string2));
+}
+
+TEST_F(ArenaRefTestAndroid, CopyAssignsFromNullObjectItself) {
+  ArenaRef arena_ref1;
+  arena_ref1 = arena_ref1;
+  EXPECT_EQ(arena_ref1.get(env()).get(), nullptr);
+}
+
+TEST_F(ArenaRefTestAndroid, CopyAssignsFromValidObjectItself) {
+  Local<String> string1 = env().NewStringUtf("hello world");
+
+  ArenaRef arena_ref1(env(), string1);
+  arena_ref1 = arena_ref1;
+  EXPECT_TRUE(env().IsSameObject(arena_ref1.get(env()), string1));
+}
+
+TEST_F(ArenaRefTestAndroid, MoveConstructsFromNull) {
+  ArenaRef arena_ref1;
+  ArenaRef arena_ref2(std::move(arena_ref1));
+  EXPECT_EQ(arena_ref2.get(env()).get(), nullptr);
+}
+
+TEST_F(ArenaRefTestAndroid, MoveConstructsFromValid) {
+  Local<String> string = env().NewStringUtf("hello world");
+
+  ArenaRef arena_ref2(env(), string);
+  ArenaRef arena_ref3(std::move(arena_ref2));
+  EXPECT_TRUE(env().IsSameObject(arena_ref3.get(env()), string));
+}
+
+TEST_F(ArenaRefTestAndroid, MoveAssignsFromNullToNull) {
+  ArenaRef arena_ref1, arena_ref2;
+  arena_ref2 = std::move(arena_ref1);
+  EXPECT_EQ(arena_ref2.get(env()).get(), nullptr);
+}
+
+TEST_F(ArenaRefTestAndroid, MoveAssignsFromNullToValid) {
+  Local<String> string = env().NewStringUtf("hello world");
+
+  ArenaRef arena_ref1;
+  ArenaRef arena_ref2(env(), string);
+  arena_ref2 = std::move(arena_ref1);
+  EXPECT_EQ(arena_ref2.get(env()).get(), nullptr);
+}
+
+TEST_F(ArenaRefTestAndroid, MoveAssignsFromValidToNull) {
+  Local<String> string = env().NewStringUtf("hello world");
+
+  ArenaRef arena_ref1;
+  ArenaRef arena_ref2(env(), string);
+  arena_ref1 = std::move(arena_ref2);
+  EXPECT_TRUE(env().IsSameObject(arena_ref1.get(env()), string));
+}
+
+TEST_F(ArenaRefTestAndroid, MoveAssignsFromValidToValid) {
+  Local<String> string1 = env().NewStringUtf("hello world");
+  Local<String> string2 = env().NewStringUtf("hello earth");
+
+  ArenaRef arena_ref1(env(), string1);
+  ArenaRef arena_ref2(env(), string2);
+  arena_ref1 = std::move(arena_ref2);
+  EXPECT_TRUE(env().IsSameObject(arena_ref1.get(env()), string2));
+}
+
+TEST_F(ArenaRefTestAndroid, MoveAssignsFromNullObjectItself) {
+  ArenaRef arena_ref1;
+  arena_ref1 = std::move(arena_ref1);
+  EXPECT_EQ(arena_ref1.get(env()).get(), nullptr);
+}
+
+TEST_F(ArenaRefTestAndroid, MoveAssignsFromValidObjectItself) {
+  Local<String> string1 = env().NewStringUtf("hello world");
+
+  ArenaRef arena_ref1(env(), string1);
+  arena_ref1 = std::move(arena_ref1);
+  EXPECT_TRUE(env().IsSameObject(arena_ref1.get(env()), string1));
+}
+
+TEST_F(ArenaRefTestAndroid, ThrowBeforeConstruct) {
+  Local<String> string = env().NewStringUtf("hello world");
+  EXPECT_EQ(string.ToString(env()).size(), 11U);
+  throwException();
+  ArenaRef arena_ref(env(), string);
+  clearExceptionOccurred();
+  EXPECT_EQ(arena_ref.get(env()).get(), nullptr);
+}
+
+TEST_F(ArenaRefTestAndroid, ThrowBeforeCopyConstruct) {
+  Local<String> string = env().NewStringUtf("hello world");
+  ArenaRef arena_ref1(env(), string);
+  EXPECT_EQ(arena_ref1.get(env()).ToString(env()).size(), 11U);
+  throwException();
+  ArenaRef arena_ref2(arena_ref1);
+  clearExceptionOccurred();
+  EXPECT_EQ(arena_ref2.get(env()).get(), nullptr);
+}
+
+TEST_F(ArenaRefTestAndroid, ThrowBeforeCopyAssignment) {
+  Local<String> string = env().NewStringUtf("hello world");
+  ArenaRef arena_ref1(env(), string);
+  ArenaRef arena_ref2;
+  EXPECT_EQ(arena_ref1.get(env()).ToString(env()).size(), 11U);
+  throwException();
+  arena_ref2 = arena_ref1;
+  clearExceptionOccurred();
+  EXPECT_EQ(arena_ref2.get(env()).get(), nullptr);
+}
+
+TEST_F(ArenaRefTestAndroid, ThrowBeforeMoveConstruct) {
+  Local<String> string = env().NewStringUtf("hello world");
+  ArenaRef arena_ref1(env(), string);
+  EXPECT_EQ(arena_ref1.get(env()).ToString(env()).size(), 11U);
+  throwException();
+  ArenaRef arena_ref2(std::move(arena_ref1));
+  clearExceptionOccurred();
+  EXPECT_TRUE(env().IsSameObject(arena_ref2.get(env()), string));
+}
+
+TEST_F(ArenaRefTestAndroid, ThrowBeforeMoveAssignment) {
+  Local<String> string = env().NewStringUtf("hello world");
+  ArenaRef arena_ref1(env(), string);
+  ArenaRef arena_ref2;
+  EXPECT_EQ(arena_ref1.get(env()).ToString(env()).size(), 11U);
+  throwException();
+  arena_ref2 = std::move(arena_ref1);
+  clearExceptionOccurred();
+  EXPECT_TRUE(env().IsSameObject(arena_ref2.get(env()), string));
+}
+
+}  // namespace
+}  // namespace jni
+}  // namespace firestore
+}  // namespace firebase

firestore/integration_test_internal/src/jni/env_test.cc

@@ -128,6 +128,18 @@ TEST_F(EnvTest, CallsVoidMethods) {
   EXPECT_EQ(length, 42);
 }
 
+TEST_F(EnvTest, CallsValidArenaRef) {
+  Local<String> str = env().NewStringUtf("Foo");
+  ArenaRef arena_ref(env(), str);
+
+  Local<Class> clazz = env().FindClass("java/lang/String");
+  jmethodID to_lower_case =
+      env().GetMethodId(clazz, "toLowerCase", "()Ljava/lang/String;");
+
+  Local<Object> result = env().Call(arena_ref, Method<Object>(to_lower_case));
+  EXPECT_EQ("foo", result.ToString(env()));
+}
+
 TEST_F(EnvTest, GetsStaticFields) {
   Local<Class> clazz = env().FindClass("java/lang/String");
   jfieldID comparator = env().GetStaticFieldId(clazz, "CASE_INSENSITIVE_ORDER",
@@ -160,6 +172,21 @@ TEST_F(EnvTest, ToString) {
   EXPECT_EQ("Foo", result);
 }
 
+TEST_F(EnvTest, IsInstanceOfChecksValidArenaRef) {
+  Local<Class> clazz = env().FindClass("java/lang/String");
+  Local<String> str = env().NewStringUtf("Foo");
+  ArenaRef arena_ref(env(), str);
+  EXPECT_TRUE(env().IsInstanceOf(arena_ref, clazz));
+}
+
+TEST_F(EnvTest, IsInstanceOfChecksNullArenaRef) {
+  GTEST_SKIP()
+      << "Skip until edge case of IsInstanceOf (b/268420201) is covered.";
+  Local<Class> clazz = env().FindClass("java/lang/String");
+  ArenaRef arena_ref;
+  EXPECT_FALSE(env().IsInstanceOf(arena_ref, clazz));
+}
+
 TEST_F(EnvTest, Throw) {
   Local<Class> clazz = env().FindClass("java/lang/Exception");
   jmethodID ctor = env().GetMethodId(clazz, "<init>", "(Ljava/lang/String;)V");

firestore/src/android/document_reference_android.cc

@@ -147,14 +147,14 @@ Future<void> DocumentReferenceInternal::Set(const MapFieldValue& data,
   Env env = GetEnv();
   FieldValueInternal map_value(data);
   Local<Object> java_options = SetOptionsInternal::Create(env, options);
-  Local<Task> task = env.Call(obj_, kSet, map_value, java_options);
+  Local<Task> task = env.Call(obj_, kSet, map_value.ToJava(), java_options);
   return promises_.NewFuture<void>(env, AsyncFn::kSet, task);
 }
 
 Future<void> DocumentReferenceInternal::Update(const MapFieldValue& data) {
   Env env = GetEnv();
   FieldValueInternal map_value(data);
-  Local<Task> task = env.Call(obj_, kUpdate, map_value);
+  Local<Task> task = env.Call(obj_, kUpdate, map_value.ToJava());
   return promises_.NewFuture<void>(env, AsyncFn::kUpdate, task);
 }