Implement ObjectArena and ArenaRef to solve JNI Global Reference Issue

firestore/CMakeLists.txt

@@ -138,6 +138,7 @@ set(android_SRCS
     src/android/wrapper.h
     src/android/write_batch_android.cc
     src/android/write_batch_android.h
+    src/jni/arena_ref.h
     src/jni/array.h
     src/jni/array_list.cc
     src/jni/array_list.h
@@ -173,6 +174,8 @@ set(android_SRCS
     src/jni/map.h
     src/jni/object.cc
     src/jni/object.h
+    src/jni/object_arena.cc
+    src/jni/object_arena.h
     src/jni/ownership.h
     src/jni/set.h
     src/jni/string.cc

firestore/integration_test_internal/CMakeLists.txt

@@ -118,6 +118,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/field_value_test.cc

@@ -34,6 +34,11 @@ namespace firestore {
 using Type = FieldValue::Type;
 using FieldValueTest = FirestoreIntegrationTest;
 
+TEST(DenverTest, zzyzx) {
+  auto field_value = FieldValue::String("hello");
+  ASSERT_EQ(field_value.ToString(), "zzyzx");
+}
+
 // Sanity test for stubs
 TEST_F(FieldValueTest, TestFieldValueTypes) {
   FieldValue::Null();

firestore/integration_test_internal/src/jni/arena_ref_test.cc

@@ -0,0 +1,62 @@
+/*
+ * Copyright 2022 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/jni/boolean.h"
+
+#include "firestore_integration_test.h"
+
+namespace firebase {
+namespace firestore {
+namespace jni {
+namespace {
+
+using ArenaRefTest = FirestoreIntegrationTest;
+
+TEST_F(ArenaRefTest, ConstructorTest) {
+  Env env(GetEnv());
+  auto boxed_ptr = Boolean::Create(env, true);
+  ArenaRef ref1(boxed_ptr);
+  EXPECT_TRUE(ref1.is_valid());
+
+  ArenaRef ref2(ref1);
+  EXPECT_TRUE(ref1.is_valid());
+  EXPECT_TRUE(ref2.is_valid());
+
+  ArenaRef ref3(std::move(ref2));
+  EXPECT_TRUE(ref3.is_valid());
+}
+
+TEST_F(ArenaRefTest, AssignmentOperatorTest) {
+  Env env(GetEnv());
+  auto boxed_ptr = Boolean::Create(env, true);
+  ArenaRef ref1(boxed_ptr);
+  EXPECT_TRUE(ref1.is_valid());
+
+  ArenaRef ref2, ref3;
+
+  ref2 = ref1;
+  EXPECT_TRUE(ref1.is_valid());
+  EXPECT_TRUE(ref2.is_valid());
+
+  ref3 = std::move(ref2);
+  EXPECT_TRUE(ref3.is_valid());
+}
+
+}  // namespace
+}  // namespace jni
+}  // namespace firestore
+}  // namespace firebase

firestore/src/android/field_value_android.cc

@@ -106,31 +106,36 @@ FieldValueInternal::FieldValueInternal(Type type, const Object& object)
 FieldValueInternal::FieldValueInternal(bool value)
     : cached_type_(Type::kBoolean) {
   Env env = GetEnv();
-  object_ = Boolean::Create(env, value);
+  auto boxed_ptr = Boolean::Create(env, value);
+  object_ = jni::ArenaRef(boxed_ptr);
 }
 
 FieldValueInternal::FieldValueInternal(int64_t value)
     : cached_type_(Type::kInteger) {
   Env env = GetEnv();
-  object_ = Long::Create(env, value);
+  auto boxed_ptr = Long::Create(env, value);
+  object_ = jni::ArenaRef(boxed_ptr);
 }
 
 FieldValueInternal::FieldValueInternal(double value)
     : cached_type_(Type::kDouble) {
   Env env = GetEnv();
-  object_ = Double::Create(env, value);
+  auto boxed_ptr = Double::Create(env, value);
+  object_ = jni::ArenaRef(boxed_ptr);
 }
 
 FieldValueInternal::FieldValueInternal(Timestamp value)
     : cached_type_(Type::kTimestamp) {
   Env env = GetEnv();
-  object_ = TimestampInternal::Create(env, value);
+  auto boxed_ptr = TimestampInternal::Create(env, value);
+  object_ = jni::ArenaRef(boxed_ptr);
 }
 
 FieldValueInternal::FieldValueInternal(std::string value)
     : cached_type_(Type::kString) {
   Env env = GetEnv();
-  object_ = env.NewStringUtf(value);
+  auto boxed_ptr = env.NewStringUtf(value);
+  object_ = jni::ArenaRef(boxed_ptr);
 }
 
 // We do not initialize cached_blob_ with value here as the instance constructed
@@ -140,20 +145,22 @@ FieldValueInternal::FieldValueInternal(std::string value)
 FieldValueInternal::FieldValueInternal(const uint8_t* value, size_t size)
     : cached_type_(Type::kBlob) {
   Env env = GetEnv();
-  object_ = BlobInternal::Create(env, value, size);
+  auto boxed_ptr = BlobInternal::Create(env, value, size);
+  object_ = jni::ArenaRef(boxed_ptr);
 }
 
 FieldValueInternal::FieldValueInternal(DocumentReference value)
     : cached_type_{Type::kReference} {
   if (value.internal_ != nullptr) {
-    object_ = value.internal_->ToJava();
+    object_ = jni::ArenaRef(value.internal_->ToJava());
   }
 }
 
 FieldValueInternal::FieldValueInternal(GeoPoint value)
     : cached_type_(Type::kGeoPoint) {
   Env env = GetEnv();
-  object_ = GeoPointInternal::Create(env, value);
+  auto boxed_ptr = GeoPointInternal::Create(env, value);
+  object_ = jni::ArenaRef(boxed_ptr);
 }
 
 FieldValueInternal::FieldValueInternal(const std::vector<FieldValue>& value)
@@ -164,7 +171,7 @@ FieldValueInternal::FieldValueInternal(const std::vector<FieldValue>& value)
     // TODO(b/150016438): don't conflate invalid `FieldValue`s and null.
     list.Add(env, ToJava(element));
   }
-  object_ = list;
+  object_ = jni::ArenaRef(list);
 }
 
 FieldValueInternal::FieldValueInternal(const MapFieldValue& value)
@@ -176,63 +183,64 @@ FieldValueInternal::FieldValueInternal(const MapFieldValue& value)
     Local<String> key = env.NewStringUtf(kv.first);
     map.Put(env, key, ToJava(kv.second));
   }
-  object_ = map;
+  object_ = jni::ArenaRef(map);
 }
 
 Type FieldValueInternal::type() const {
   if (cached_type_ != Type::kNull) {
     return cached_type_;
   }
-  if (!object_) {
+  if (!object_.has_value()) {
     return Type::kNull;
   }
 
   // We do not have any knowledge on the type yet. Check the runtime type with
   // each known type.
   Env env = GetEnv();
-  if (env.IsInstanceOf(object_, Boolean::GetClass())) {
+  if (env.IsInstanceOf(object_.get(env), Boolean::GetClass())) {
     cached_type_ = Type::kBoolean;
     return Type::kBoolean;
   }
-  if (env.IsInstanceOf(object_, Long::GetClass())) {
+  if (env.IsInstanceOf(object_.get(env), Long::GetClass())) {
     cached_type_ = Type::kInteger;
     return Type::kInteger;
   }
-  if (env.IsInstanceOf(object_, Double::GetClass())) {
+  if (env.IsInstanceOf(object_.get(env), Double::GetClass())) {
     cached_type_ = Type::kDouble;
     return Type::kDouble;
   }
-  if (env.IsInstanceOf(object_, TimestampInternal::GetClass())) {
+  if (env.IsInstanceOf(object_.get(env), TimestampInternal::GetClass())) {
     cached_type_ = Type::kTimestamp;
     return Type::kTimestamp;
   }
-  if (env.IsInstanceOf(object_, String::GetClass())) {
+  if (env.IsInstanceOf(object_.get(env), String::GetClass())) {
     cached_type_ = Type::kString;
     return Type::kString;
   }
-  if (env.IsInstanceOf(object_, BlobInternal::GetClass())) {
+  if (env.IsInstanceOf(object_.get(env), BlobInternal::GetClass())) {
     cached_type_ = Type::kBlob;
     return Type::kBlob;
   }
-  if (env.IsInstanceOf(object_, DocumentReferenceInternal::GetClass())) {
+  if (env.IsInstanceOf(object_.get(env),
+                       DocumentReferenceInternal::GetClass())) {
     cached_type_ = Type::kReference;
     return Type::kReference;
   }
-  if (env.IsInstanceOf(object_, GeoPointInternal::GetClass())) {
+  if (env.IsInstanceOf(object_.get(env), GeoPointInternal::GetClass())) {
     cached_type_ = Type::kGeoPoint;
     return Type::kGeoPoint;
   }
-  if (env.IsInstanceOf(object_, List::GetClass())) {
+  if (env.IsInstanceOf(object_.get(env), List::GetClass())) {
     cached_type_ = Type::kArray;
     return Type::kArray;
   }
-  if (env.IsInstanceOf(object_, Map::GetClass())) {
+  if (env.IsInstanceOf(object_.get(env), Map::GetClass())) {
     cached_type_ = Type::kMap;
     return Type::kMap;
   }
 
   FIREBASE_ASSERT_MESSAGE(false, "Unsupported FieldValue type: %s",
-                          Class::GetClassName(env, object_).c_str());
+                          Class::GetClassName(env, object_.get(env)).c_str());
   return Type::kNull;
 }
 
@@ -391,12 +399,12 @@ bool operator==(const FieldValueInternal& lhs, const FieldValueInternal& rhs) {
 template <typename T>
 T FieldValueInternal::Cast(jni::Env& env, Type type) const {
   if (cached_type_ == Type::kNull) {
-    FIREBASE_ASSERT(env.IsInstanceOf(object_, T::GetClass()));
+    FIREBASE_ASSERT(env.IsInstanceOf(object_.get(env), T::GetClass()));
     cached_type_ = type;
   } else {
     FIREBASE_ASSERT(cached_type_ == type);
   }
-  auto typed_value = static_cast<jni::JniType<T>>(object_.get());
+  auto typed_value = static_cast<jni::JniType<T>>(object_.get(env).get());
   return T(typed_value);
 }
 
@@ -411,8 +419,14 @@ Local<Array<Object>> FieldValueInternal::MakeArray(
 
 Env FieldValueInternal::GetEnv() { return FirestoreInternal::GetEnv(); }
 
+jni::Local<jni::Object> FieldValueInternal::ToJava() const {
+  Env env = GetEnv();
+  return object_.get(env);
+}
+
 Object FieldValueInternal::ToJava(const FieldValue& value) {
-  return value.internal_ ? value.internal_->object_ : Object();
+  Env env = GetEnv();
+  return value.internal_ ? value.internal_->object_.get(env) : Object();
 }
 
 }  // namespace firestore

firestore/src/android/field_value_android.h

@@ -25,6 +25,7 @@
 #include "firebase/firestore/timestamp.h"
 #include "firestore/src/include/firebase/firestore/document_reference.h"
 #include "firestore/src/include/firebase/firestore/field_value.h"
+#include "firestore/src/jni/arena_ref.h"
 #include "firestore/src/jni/jni_fwd.h"
 #include "firestore/src/jni/object.h"
 #include "firestore/src/jni/ownership.h"
@@ -87,7 +88,7 @@ class FieldValueInternal {
   std::vector<FieldValue> array_value() const;
   MapFieldValue map_value() const;
 
-  const jni::Global<jni::Object>& ToJava() const { return object_; }
+  jni::Local<jni::Object> ToJava() const;
 
   static FieldValue Delete();
   static FieldValue ServerTimestamp();
@@ -116,7 +117,7 @@ class FieldValueInternal {
 
   static jni::Env GetEnv();
 
-  jni::Global<jni::Object> object_;
+  jni::ArenaRef object_;
 
   // Below are cached type information. It is costly to get type info from
   // jobject of Object type. So we cache it if we have already known.

firestore/src/android/firestore_android.cc

@@ -73,6 +73,7 @@
 #include "firestore/src/jni/loader.h"
 #include "firestore/src/jni/long.h"
 #include "firestore/src/jni/map.h"
+#include "firestore/src/jni/object_arena.h"
 #include "firestore/src/jni/set.h"
 #include "firestore/src/jni/task.h"