From 3acc2af24671ef264eed1183aff2e76b1f00ea00 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Wed, 21 Dec 2022 15:32:37 -0500 Subject: [PATCH 01/16] Add a skeleton for ArenaRef --- firestore/CMakeLists.txt | 2 + .../integration_test_internal/CMakeLists.txt | 1 + .../src/android/arena_ref_android_test.cc | 51 ++++++++++++++ firestore/src/android/firestore_android.cc | 5 ++ firestore/src/jni/arena_ref.cc | 69 +++++++++++++++++++ firestore/src/jni/arena_ref.h | 51 ++++++++++++++ 6 files changed, 179 insertions(+) create mode 100644 firestore/integration_test_internal/src/android/arena_ref_android_test.cc create mode 100644 firestore/src/jni/arena_ref.cc create mode 100644 firestore/src/jni/arena_ref.h diff --git a/firestore/CMakeLists.txt b/firestore/CMakeLists.txt index 1d9c9c5f07..feaf22d323 100644 --- a/firestore/CMakeLists.txt +++ b/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 diff --git a/firestore/integration_test_internal/CMakeLists.txt b/firestore/integration_test_internal/CMakeLists.txt index 0205e5b164..0afaadeae5 100644 --- a/firestore/integration_test_internal/CMakeLists.txt +++ b/firestore/integration_test_internal/CMakeLists.txt @@ -121,6 +121,7 @@ set(FIREBASE_INTEGRATION_TEST_PORTABLE_TEST_SRCS # These sources contain the actual tests that run on Android only. set(FIREBASE_INTEGRATION_TEST_ANDROID_TEST_SRCS + src/android/arena_ref_android_test.cc src/android/field_path_portable_test.cc src/android/firestore_integration_test_android_test.cc src/android/geo_point_android_test.cc diff --git a/firestore/integration_test_internal/src/android/arena_ref_android_test.cc b/firestore/integration_test_internal/src/android/arena_ref_android_test.cc new file mode 100644 index 0000000000..a1738a81db --- /dev/null +++ b/firestore/integration_test_internal/src/android/arena_ref_android_test.cc @@ -0,0 +1,51 @@ +/* + * 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_integration_test_android.h" + +#include "gmock/gmock.h" +#include "gtest/gtest.h" + +namespace firebase { +namespace firestore { +namespace jni { +namespace { + +using ArenaRefTestAndroid = FirestoreAndroidIntegrationTest; + +TEST_F(ArenaRefTestAndroid, DefaultConstructorCreatesReferenceToNull) { + Env env; + ArenaRef arena_ref; + EXPECT_EQ(arena_ref.get(env).get(), nullptr); +} + +TEST_F(ArenaRefTestAndroid, ConstructFromEnvAndObject) { + Env env; + Local string = env.NewStringUtf("hello world"); + + ArenaRef arena_ref(env, string); + + Local stringRef2 = string; + string.clear(); + EXPECT_TRUE(arena_ref.get(env).Equals(env, stringRef2)); +} + +} // namespace +} // namespace jni +} // namespace firestore +} // namespace firebase diff --git a/firestore/src/android/firestore_android.cc b/firestore/src/android/firestore_android.cc index 62917accd8..3d41b7cb87 100644 --- a/firestore/src/android/firestore_android.cc +++ b/firestore/src/android/firestore_android.cc @@ -59,6 +59,7 @@ #include "firestore/src/common/hard_assert_common.h" #include "firestore/src/common/make_unique.h" #include "firestore/src/include/firebase/firestore.h" +#include "firestore/src/jni/arena_ref.h" #include "firestore/src/jni/array.h" #include "firestore/src/jni/array_list.h" #include "firestore/src/jni/boolean.h" @@ -314,6 +315,10 @@ bool FirestoreInternal::Initialize(App* app) { jni::Long::Initialize(loader); jni::Map::Initialize(loader); + // Initialize ArenaRef _after_ the other JNI classes because it relies on + // HashMap having already been initialized. + jni::ArenaRef::Initialize(env); + InitializeFirestore(loader); InitializeFirestoreTasks(loader); InitializeUserCallbackExecutor(loader); diff --git a/firestore/src/jni/arena_ref.cc b/firestore/src/jni/arena_ref.cc new file mode 100644 index 0000000000..b5a4e7a44f --- /dev/null +++ b/firestore/src/jni/arena_ref.cc @@ -0,0 +1,69 @@ +/* + * 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 + +#include "firestore/src/jni/arena_ref.h" + +#include "firestore/src/jni/hash_map.h" +#include "firestore/src/jni/long.h" +#include "firestore/src/jni/map.h" + +namespace firebase { +namespace firestore { +namespace jni { + +namespace { + +HashMap* gArenaRefHashMap = nullptr; + +int64_t GetNextArenaRefKey() { + static std::atomic next_key(1); + return next_key.fetch_add(1); +} + +} + +ArenaRef::ArenaRef(Env& env, const Object& object) : key_(GetNextArenaRefKey()) { + gArenaRefHashMap->Put(env, key_object(env), object); +} + +ArenaRef::~ArenaRef() { + if (key_ != 0) { + Env env; + gArenaRefHashMap->Remove(env, key_object(env)); + } +} + +Local ArenaRef::key_object(Env& env) const { + return Long::Create(env, key_); +} + +void ArenaRef::Initialize(Env& env) { + if (gArenaRefHashMap) { + return; + } + Global hash_map(HashMap::Create(env)); + jobject hash_map_jobject = hash_map.release(); + gArenaRefHashMap = new HashMap(hash_map_jobject); +} + +Local ArenaRef::get(Env& env) const { + return gArenaRefHashMap->Get(env, key_object(env)); +} + +} // namespace jni +} // namespace firestore +} // namespace firebase diff --git a/firestore/src/jni/arena_ref.h b/firestore/src/jni/arena_ref.h new file mode 100644 index 0000000000..8aa1538da8 --- /dev/null +++ b/firestore/src/jni/arena_ref.h @@ -0,0 +1,51 @@ +/* + * 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 + +#include + +#include "firestore/src/jni/env.h" +#include "firestore/src/jni/object.h" +#include "firestore/src/jni/ownership.h" + +#ifndef FIREBASE_FIRESTORE_SRC_JNI_ARENA_REF_H_ +#define FIREBASE_FIRESTORE_SRC_JNI_ARENA_REF_H_ + +namespace firebase { +namespace firestore { +namespace jni { + +class ArenaRef { + public: + ArenaRef() = default; + ArenaRef(Env&, const Object&); + ~ArenaRef(); + + static void Initialize(Env&); + + Local get(Env&) const; + + private: + Local key_object(Env&) const; + + int64_t key_ = 0; +}; + +} // namespace jni +} // namespace firestore +} // namespace firebase + +#endif // FIREBASE_FIRESTORE_SRC_JNI_ARENA_REF_H_ From 89755802342767bcfd3c088536e83e664bae07e8 Mon Sep 17 00:00:00 2001 From: cherylEnkidu Date: Fri, 23 Dec 2022 16:49:10 -0500 Subject: [PATCH 02/16] Create big five of ArenaRef. --- .../src/android/arena_ref_android_test.cc | 88 ++++++++++++++++++- firestore/src/jni/arena_ref.cc | 49 ++++++++++- firestore/src/jni/arena_ref.h | 13 ++- 3 files changed, 141 insertions(+), 9 deletions(-) diff --git a/firestore/integration_test_internal/src/android/arena_ref_android_test.cc b/firestore/integration_test_internal/src/android/arena_ref_android_test.cc index a1738a81db..90a1a2df9e 100644 --- a/firestore/integration_test_internal/src/android/arena_ref_android_test.cc +++ b/firestore/integration_test_internal/src/android/arena_ref_android_test.cc @@ -39,10 +39,92 @@ TEST_F(ArenaRefTestAndroid, ConstructFromEnvAndObject) { Local string = env.NewStringUtf("hello world"); ArenaRef arena_ref(env, string); + EXPECT_TRUE(arena_ref.get(env).Equals(env, string)); +} + +TEST_F(ArenaRefTestAndroid, CopyConstructor) { + Env env; + + ArenaRef arena_ref1; + ArenaRef arena_ref2(arena_ref1); + EXPECT_EQ(arena_ref2.get(env).get(), nullptr); + + Local string = env.NewStringUtf("hello world"); + + ArenaRef arena_ref3(env, string); + ArenaRef arena_ref4(arena_ref3); + ArenaRef arena_ref5 = arena_ref3; + + EXPECT_TRUE(arena_ref3.get(env).Equals(env, string)); + EXPECT_TRUE(arena_ref4.get(env).Equals(env, string)); + EXPECT_TRUE(arena_ref5.get(env).Equals(env, string)); +} + +TEST_F(ArenaRefTestAndroid, CopyAssignmentOp) { + Env env; + + 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); + + Local string1 = env.NewStringUtf("hello world"); + Local string2 = env.NewStringUtf("hello earth"); + + ArenaRef arena_ref3(env, string1); + ArenaRef arena_ref4(env, string2); + arena_ref4 = arena_ref3; + arena_ref3 = arena_ref3; + + EXPECT_TRUE(arena_ref3.get(env).Equals(env, string1)); + EXPECT_TRUE(arena_ref4.get(env).Equals(env, string1)); + + arena_ref3 = arena_ref1; + EXPECT_EQ(arena_ref3.get(env).get(), nullptr); + EXPECT_TRUE(arena_ref4.get(env).Equals(env, string1)); +} + +TEST_F(ArenaRefTestAndroid, MoveConstructor) { + Env env; + + ArenaRef arena_ref1; + ArenaRef arena_ref2(std::move(arena_ref1)); + EXPECT_EQ(arena_ref1.get(env).get(), nullptr); + EXPECT_EQ(arena_ref2.get(env).get(), nullptr); + + Local string = env.NewStringUtf("hello world"); + + ArenaRef arena_ref3(env, string); + ArenaRef arena_ref4(std::move(arena_ref3)); + EXPECT_EQ(arena_ref3.get(env).get(), nullptr); + EXPECT_TRUE(arena_ref4.get(env).Equals(env, string)); + + ArenaRef arena_ref5 = std::move(arena_ref4); + EXPECT_TRUE(arena_ref5.get(env).Equals(env, string)); +} + +TEST_F(ArenaRefTestAndroid, MoveAssignmentOp) { + Env env; + + ArenaRef arena_ref1, arena_ref2; + arena_ref2 = std::move(arena_ref1); + EXPECT_EQ(arena_ref1.get(env).get(), nullptr); + EXPECT_EQ(arena_ref2.get(env).get(), nullptr); + + Local string1 = env.NewStringUtf("hello world"); + Local string2 = env.NewStringUtf("hello earth"); + + ArenaRef arena_ref3(env, string1); + arena_ref3 = std::move(arena_ref3); + EXPECT_TRUE(arena_ref3.get(env).Equals(env, string1)); + + ArenaRef arena_ref4(env, string2); + arena_ref4 = std::move(arena_ref3); + EXPECT_EQ(arena_ref3.get(env).get(), nullptr); + EXPECT_TRUE(arena_ref4.get(env).Equals(env, string1)); - Local stringRef2 = string; - string.clear(); - EXPECT_TRUE(arena_ref.get(env).Equals(env, stringRef2)); + arena_ref4 = std::move(arena_ref1); + EXPECT_EQ(arena_ref4.get(env).get(), nullptr); } } // namespace diff --git a/firestore/src/jni/arena_ref.cc b/firestore/src/jni/arena_ref.cc index b5a4e7a44f..260681e67b 100644 --- a/firestore/src/jni/arena_ref.cc +++ b/firestore/src/jni/arena_ref.cc @@ -34,12 +34,57 @@ int64_t GetNextArenaRefKey() { return next_key.fetch_add(1); } -} +} // namespace -ArenaRef::ArenaRef(Env& env, const Object& object) : key_(GetNextArenaRefKey()) { +ArenaRef::ArenaRef(Env& env, const Object& object) + : key_(GetNextArenaRefKey()) { gArenaRefHashMap->Put(env, key_object(env), object); } +ArenaRef::ArenaRef(const ArenaRef& other) : key_(GetNextArenaRefKey()) { + if (other.key_ != 0) { + Env env; + Local object = other.get(env); + gArenaRefHashMap->Put(env, key_object(env), object); + } +} + +ArenaRef::ArenaRef(ArenaRef&& other) { + key_ = other.key_; + other.key_ = 0; +} + +ArenaRef& ArenaRef::operator=(const ArenaRef& other) { + Env env; + + if (key_ != other.key_) { + if (key_ != 0) { + gArenaRefHashMap->Remove(env, key_object(env)); + } + + if (other.key_ != 0) { + key_ = GetNextArenaRefKey(); + Local object = other.get(env); + gArenaRefHashMap->Put(env, key_object(env), object); + } + } + return *this; +} + +ArenaRef& ArenaRef::operator=(ArenaRef&& other) { + Env env; + + if (key_ != other.key_) { + if (key_ != 0) { + gArenaRefHashMap->Remove(env, key_object(env)); + } + + key_ = other.key_; + other.key_ = 0; + } + return *this; +} + ArenaRef::~ArenaRef() { if (key_ != 0) { Env env; diff --git a/firestore/src/jni/arena_ref.h b/firestore/src/jni/arena_ref.h index 8aa1538da8..f97f015b66 100644 --- a/firestore/src/jni/arena_ref.h +++ b/firestore/src/jni/arena_ref.h @@ -13,17 +13,18 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -#include + +#ifndef FIREBASE_FIRESTORE_SRC_JNI_ARENA_REF_H_ +#define FIREBASE_FIRESTORE_SRC_JNI_ARENA_REF_H_ #include +#include + #include "firestore/src/jni/env.h" #include "firestore/src/jni/object.h" #include "firestore/src/jni/ownership.h" -#ifndef FIREBASE_FIRESTORE_SRC_JNI_ARENA_REF_H_ -#define FIREBASE_FIRESTORE_SRC_JNI_ARENA_REF_H_ - namespace firebase { namespace firestore { namespace jni { @@ -32,6 +33,10 @@ class ArenaRef { public: ArenaRef() = default; ArenaRef(Env&, const Object&); + ArenaRef(const ArenaRef& other); + ArenaRef(ArenaRef&& other); + ArenaRef& operator=(const ArenaRef& other); + ArenaRef& operator=(ArenaRef&& other); ~ArenaRef(); static void Initialize(Env&); From d30beccaf5d47e241d1cab57e2ba31fb8aad0f1f Mon Sep 17 00:00:00 2001 From: cherylEnkidu Date: Thu, 29 Dec 2022 17:03:09 -0500 Subject: [PATCH 03/16] Use as example to show the efficiency drops down --- firestore/src/android/field_value_android.cc | 95 +++++++++++++------- firestore/src/android/field_value_android.h | 17 +++- 2 files changed, 74 insertions(+), 38 deletions(-) diff --git a/firestore/src/android/field_value_android.cc b/firestore/src/android/field_value_android.cc index 071188a726..a4641a5af8 100644 --- a/firestore/src/android/field_value_android.cc +++ b/firestore/src/android/field_value_android.cc @@ -38,6 +38,7 @@ namespace firebase { namespace firestore { namespace { +using jni::ArenaRef; using jni::Array; using jni::ArrayList; using jni::Boolean; @@ -98,39 +99,50 @@ FieldValue FieldValueInternal::Create(Env& env, FieldValueInternal::FieldValueInternal() : cached_type_(Type::kNull) {} FieldValueInternal::FieldValueInternal(const Object& object) - : object_(object), cached_type_(Type::kNull) {} + : cached_type_(Type::kNull) { + Env env; + object_ = ArenaRef(env, object); +} FieldValueInternal::FieldValueInternal(Type type, const Object& object) - : object_(object), cached_type_(type) {} + : cached_type_(type) { + Env env; + object_ = ArenaRef(env, object); +} FieldValueInternal::FieldValueInternal(bool value) : cached_type_(Type::kBoolean) { - Env env = GetEnv(); - object_ = Boolean::Create(env, value); + Env env; + auto boxed_ptr = Boolean::Create(env, value); + object_ = ArenaRef(env, 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_ = ArenaRef(env, 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_ = ArenaRef(env, 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_ = ArenaRef(env, 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_ = ArenaRef(env, boxed_ptr); } // We do not initialize cached_blob_ with value here as the instance constructed @@ -139,21 +151,24 @@ FieldValueInternal::FieldValueInternal(std::string value) // blob_value(). FieldValueInternal::FieldValueInternal(const uint8_t* value, size_t size) : cached_type_(Type::kBlob) { - Env env = GetEnv(); - object_ = BlobInternal::Create(env, value, size); + Env env; + auto boxed_ptr = BlobInternal::Create(env, value, size); + object_ = ArenaRef(env, boxed_ptr); } FieldValueInternal::FieldValueInternal(DocumentReference value) : cached_type_{Type::kReference} { if (value.internal_ != nullptr) { - object_ = value.internal_->ToJava(); + Env env; + object_ = ArenaRef(env, 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_ = ArenaRef(env, boxed_ptr); } FieldValueInternal::FieldValueInternal(const std::vector& value) @@ -164,7 +179,7 @@ FieldValueInternal::FieldValueInternal(const std::vector& value) // TODO(b/150016438): don't conflate invalid `FieldValue`s and null. list.Add(env, ToJava(element)); } - object_ = list; + object_ = ArenaRef(env, list); } FieldValueInternal::FieldValueInternal(const MapFieldValue& value) @@ -176,63 +191,64 @@ FieldValueInternal::FieldValueInternal(const MapFieldValue& value) Local key = env.NewStringUtf(kv.first); map.Put(env, key, ToJava(kv.second)); } - object_ = map; + object_ = ArenaRef(env, map); } Type FieldValueInternal::type() const { if (cached_type_ != Type::kNull) { return cached_type_; } - if (!object_) { + + Env env; + if (!object_.get(env)) { 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; } @@ -258,7 +274,7 @@ Timestamp FieldValueInternal::timestamp_value() const { std::string FieldValueInternal::string_value() const { Env env = GetEnv(); - return Cast(env, Type::kString).ToString(env); + return CastString(env, Type::kString).ToString(env); } const uint8_t* FieldValueInternal::blob_value() const { @@ -389,15 +405,24 @@ bool operator==(const FieldValueInternal& lhs, const FieldValueInternal& rhs) { } template -T FieldValueInternal::Cast(jni::Env& env, Type type) const { +Local FieldValueInternal::Cast(jni::Env& env, Type type) const { + if (cached_type_ == Type::kNull) { + FIREBASE_ASSERT(env.IsInstanceOf(object_.get(env), T::GetClass())); + cached_type_ = type; + } else { + FIREBASE_ASSERT(cached_type_ == type); + } + return object_.get(env).CastTo(); +} + +Local FieldValueInternal::CastString(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), String::GetClass())); cached_type_ = type; } else { FIREBASE_ASSERT(cached_type_ == type); } - auto typed_value = static_cast>(object_.get()); - return T(typed_value); + return env.NewStringUtf(object_.get(env).ToString(env)); } Local> FieldValueInternal::MakeArray( @@ -411,8 +436,10 @@ Local> FieldValueInternal::MakeArray( Env FieldValueInternal::GetEnv() { return FirestoreInternal::GetEnv(); } -Object FieldValueInternal::ToJava(const FieldValue& value) { - return value.internal_ ? value.internal_->object_ : Object(); +Local FieldValueInternal::ToJava(const FieldValue& value) { + Env env; + return value.internal_ ? value.internal_->object_.get(env) + : Local(Object()); } } // namespace firestore diff --git a/firestore/src/android/field_value_android.h b/firestore/src/android/field_value_android.h index ceb49d23da..f304e728ed 100644 --- a/firestore/src/android/field_value_android.h +++ b/firestore/src/android/field_value_android.h @@ -28,6 +28,8 @@ #include "firestore/src/jni/jni_fwd.h" #include "firestore/src/jni/object.h" #include "firestore/src/jni/ownership.h" +#include "firestore/src/jni/env.h" +#include "firestore/src/jni/arena_ref.h" namespace firebase { namespace firestore { @@ -87,7 +89,10 @@ class FieldValueInternal { std::vector array_value() const; MapFieldValue map_value() const; - const jni::Global& ToJava() const { return object_; } + jni::Local ToJava() const { + jni::Env env; + return object_.get(env); + } static FieldValue Delete(); static FieldValue ServerTimestamp(); @@ -96,7 +101,7 @@ class FieldValueInternal { static FieldValue IntegerIncrement(int64_t by_value); static FieldValue DoubleIncrement(double by_value); - static jni::Object ToJava(const FieldValue& value); + static jni::Local ToJava(const FieldValue& value); private: friend class FirestoreInternal; @@ -107,7 +112,9 @@ class FieldValueInternal { // This performs a run-time `instanceof` check to verify that the object // has the type `T::GetClass()`. template - T Cast(jni::Env& env, Type type) const; + jni::Local Cast(jni::Env& env, Type type) const; + + jni::Local CastString(jni::Env& env, Type type) const; static jni::Local> MakeArray( jni::Env& env, const std::vector& elements); @@ -116,7 +123,7 @@ class FieldValueInternal { static jni::Env GetEnv(); - jni::Global 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. @@ -130,10 +137,12 @@ inline jni::Object ToJava(const FieldValue& value) { } inline jobject ToJni(const FieldValueInternal* value) { + jni::Env env; return value->ToJava().get(); } inline jobject ToJni(const FieldValueInternal& value) { + jni::Env env; return value.ToJava().get(); } From 8ef27ff558582b8e78f2238fca76717f90df2f22 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Tue, 3 Jan 2023 12:33:54 -0500 Subject: [PATCH 04/16] fix crash in DocumentReferenceInternal::Set() due to using a stale local ref --- .../src/android/document_reference_android.cc | 4 ++-- firestore/src/android/field_value_android.h | 19 ++----------------- firestore/src/android/util_android.cc | 9 +++++---- firestore/src/android/util_android.h | 2 +- 4 files changed, 10 insertions(+), 24 deletions(-) diff --git a/firestore/src/android/document_reference_android.cc b/firestore/src/android/document_reference_android.cc index f62e7f7407..c5d5176e44 100644 --- a/firestore/src/android/document_reference_android.cc +++ b/firestore/src/android/document_reference_android.cc @@ -147,14 +147,14 @@ Future DocumentReferenceInternal::Set(const MapFieldValue& data, Env env = GetEnv(); FieldValueInternal map_value(data); Local java_options = SetOptionsInternal::Create(env, options); - Local task = env.Call(obj_, kSet, map_value, java_options); + Local task = env.Call(obj_, kSet, map_value.ToJava(), java_options); return promises_.NewFuture(env, AsyncFn::kSet, task); } Future DocumentReferenceInternal::Update(const MapFieldValue& data) { Env env = GetEnv(); FieldValueInternal map_value(data); - Local task = env.Call(obj_, kUpdate, map_value); + Local task = env.Call(obj_, kUpdate, map_value.ToJava()); return promises_.NewFuture(env, AsyncFn::kUpdate, task); } diff --git a/firestore/src/android/field_value_android.h b/firestore/src/android/field_value_android.h index f304e728ed..24dac90fb1 100644 --- a/firestore/src/android/field_value_android.h +++ b/firestore/src/android/field_value_android.h @@ -25,11 +25,11 @@ #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/env.h" #include "firestore/src/jni/jni_fwd.h" #include "firestore/src/jni/object.h" #include "firestore/src/jni/ownership.h" -#include "firestore/src/jni/env.h" -#include "firestore/src/jni/arena_ref.h" namespace firebase { namespace firestore { @@ -131,21 +131,6 @@ class FieldValueInternal { mutable std::shared_ptr> cached_blob_; }; -inline jni::Object ToJava(const FieldValue& value) { - // This indirection is required to make use of the `friend` in FieldValue. - return FieldValueInternal::ToJava(value); -} - -inline jobject ToJni(const FieldValueInternal* value) { - jni::Env env; - return value->ToJava().get(); -} - -inline jobject ToJni(const FieldValueInternal& value) { - jni::Env env; - return value.ToJava().get(); -} - } // namespace firestore } // namespace firebase diff --git a/firestore/src/android/util_android.cc b/firestore/src/android/util_android.cc index 5e9a534499..f7b6126caf 100644 --- a/firestore/src/android/util_android.cc +++ b/firestore/src/android/util_android.cc @@ -36,7 +36,7 @@ Local MakeJavaMap(Env& env, const MapFieldValue& data) { Local result = HashMap::Create(env); for (const auto& kv : data) { Local key = env.NewStringUtf(kv.first); - const Object& value = ToJava(kv.second); + Local value = FieldValueInternal::ToJava(kv.second); result.Put(env, key, value); } return result; @@ -49,7 +49,7 @@ UpdateFieldPathArgs MakeUpdateFieldPathArgs(Env& env, FIREBASE_DEV_ASSERT_MESSAGE(iter != end, "data must be non-empty"); Local first_field = FieldPathConverter::Create(env, iter->first); - const Object& first_value = ToJava(iter->second); + Local first_value = FieldValueInternal::ToJava(iter->second); ++iter; const auto size = std::distance(iter, end) * 2; @@ -58,13 +58,14 @@ UpdateFieldPathArgs MakeUpdateFieldPathArgs(Env& env, int index = 0; for (; iter != end; ++iter) { Local field = FieldPathConverter::Create(env, iter->first); - const Object& value = ToJava(iter->second); + Local value = FieldValueInternal::ToJava(iter->second); varargs.Set(env, index++, field); varargs.Set(env, index++, value); } - return UpdateFieldPathArgs{Move(first_field), first_value, Move(varargs)}; + return UpdateFieldPathArgs{Move(first_field), Move(first_value), + Move(varargs)}; } } // namespace firestore diff --git a/firestore/src/android/util_android.h b/firestore/src/android/util_android.h index c0e43c0ae0..2dcd03d32b 100644 --- a/firestore/src/android/util_android.h +++ b/firestore/src/android/util_android.h @@ -70,7 +70,7 @@ jni::Local MakeJavaMap(jni::Env& env, const MapFieldValue& data); */ struct UpdateFieldPathArgs { jni::Local first_field; - jni::Object first_value; + jni::Local first_value; jni::Local> varargs; }; From ebc739b04049fd8f86b8456ff27c1e3131cfa077 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Tue, 3 Jan 2023 12:50:13 -0500 Subject: [PATCH 05/16] Remove invalid Wrapper::ToJava() overload for FieldValue --- firestore/src/android/query_android.cc | 7 ++++--- firestore/src/android/wrapper.cc | 4 ---- firestore/src/android/wrapper.h | 2 -- 3 files changed, 4 insertions(+), 9 deletions(-) diff --git a/firestore/src/android/query_android.cc b/firestore/src/android/query_android.cc index b28de7eb81..201571432a 100644 --- a/firestore/src/android/query_android.cc +++ b/firestore/src/android/query_android.cc @@ -272,7 +272,8 @@ Query QueryInternal::Where(const FieldPath& field, const FieldValue& value) const { Env env = GetEnv(); Local java_field = FieldPathConverter::Create(env, field); - Local query = env.Call(obj_, method, java_field, ToJava(value)); + Local java_value = FieldValueInternal::ToJava(value); + Local query = env.Call(obj_, method, java_field, java_value); return firestore_->NewQuery(env, query); } @@ -284,7 +285,7 @@ Query QueryInternal::Where(const FieldPath& field, size_t size = values.size(); Local java_values = ArrayList::Create(env, size); for (size_t i = 0; i < size; ++i) { - java_values.Add(env, ToJava(values[i])); + java_values.Add(env, FieldValueInternal::ToJava(values[i])); } Local java_field = FieldPathConverter::Create(env, field); @@ -342,7 +343,7 @@ Local> QueryInternal::ConvertFieldValues( size_t size = field_values.size(); Local> result = env.NewArray(size, Object::GetClass()); for (size_t i = 0; i < size; ++i) { - result.Set(env, i, ToJava(field_values[i])); + result.Set(env, i, FieldValueInternal::ToJava(field_values[i])); } return result; } diff --git a/firestore/src/android/wrapper.cc b/firestore/src/android/wrapper.cc index c4304abe29..115967eed7 100644 --- a/firestore/src/android/wrapper.cc +++ b/firestore/src/android/wrapper.cc @@ -58,9 +58,5 @@ Wrapper::~Wrapper() = default; jni::Env Wrapper::GetEnv() const { return firestore_->GetEnv(); } -Object Wrapper::ToJava(const FieldValue& value) { - return FieldValueInternal::ToJava(value); -} - } // namespace firestore } // namespace firebase diff --git a/firestore/src/android/wrapper.h b/firestore/src/android/wrapper.h index b1da5fce65..eb7ca26504 100644 --- a/firestore/src/android/wrapper.h +++ b/firestore/src/android/wrapper.h @@ -46,8 +46,6 @@ class Wrapper { FirestoreInternal* firestore_internal() { return firestore_; } const jni::Object& ToJava() const { return obj_; } - static jni::Object ToJava(const FieldValue& value); - protected: // Default constructor. Subclass is expected to set the obj_ a meaningful // value. From ce47da604ad2c8a11a2c95138c199a07c4ada74a Mon Sep 17 00:00:00 2001 From: cherylEnkidu Date: Tue, 3 Jan 2023 23:44:38 -0500 Subject: [PATCH 06/16] Replace Global reference in Wrapper --- .../src/document_reference_test.cc | 2 +- .../android/collection_reference_android.cc | 12 ++++----- .../src/android/document_change_android.cc | 10 +++---- .../src/android/document_reference_android.cc | 25 +++++++++--------- .../src/android/document_snapshot_android.cc | 17 ++++++------ firestore/src/android/field_value_android.cc | 5 ++-- firestore/src/android/field_value_android.h | 1 + .../load_bundle_task_progress_android.cc | 10 +++---- firestore/src/android/query_android.cc | 26 ++++++++++--------- .../src/android/query_snapshot_android.cc | 13 +++++----- firestore/src/android/transaction_android.cc | 14 +++++----- firestore/src/android/wrapper.cc | 5 +++- firestore/src/android/wrapper.h | 8 ++++-- firestore/src/android/write_batch_android.cc | 14 +++++----- firestore/src/android/write_batch_android.h | 2 +- 15 files changed, 90 insertions(+), 74 deletions(-) diff --git a/firestore/integration_test_internal/src/document_reference_test.cc b/firestore/integration_test_internal/src/document_reference_test.cc index cffda408ab..6ecb257a05 100644 --- a/firestore/integration_test_internal/src/document_reference_test.cc +++ b/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 doc_java = GetInternal(doc)->ToJava(); result = DocumentReferenceInternal::Create(env, doc_java); ASSERT_EQ(db, result.firestore()); } diff --git a/firestore/src/android/collection_reference_android.cc b/firestore/src/android/collection_reference_android.cc index 9bbbeceabc..8c41f003ad 100644 --- a/firestore/src/android/collection_reference_android.cc +++ b/firestore/src/android/collection_reference_android.cc @@ -63,7 +63,7 @@ void CollectionReferenceInternal::Initialize(jni::Loader& loader) { const std::string& CollectionReferenceInternal::id() const { if (cached_id_.empty()) { Env env = GetEnv(); - cached_id_ = env.Call(obj_, kGetId).ToString(env); + cached_id_ = env.Call(obj_.get(env), kGetId).ToString(env); } return cached_id_; } @@ -71,20 +71,20 @@ const std::string& CollectionReferenceInternal::id() const { const std::string& CollectionReferenceInternal::path() const { if (cached_path_.empty()) { Env env = GetEnv(); - cached_path_ = env.Call(obj_, kGetPath).ToString(env); + cached_path_ = env.Call(obj_.get(env), kGetPath).ToString(env); } return cached_path_; } DocumentReference CollectionReferenceInternal::Parent() const { Env env = GetEnv(); - Local parent = env.Call(obj_, kGetParent); + Local parent = env.Call(obj_.get(env), kGetParent); return firestore_->NewDocumentReference(env, parent); } DocumentReference CollectionReferenceInternal::Document() const { Env env = GetEnv(); - Local document = env.Call(obj_, kDocumentAutoId); + Local document = env.Call(obj_.get(env), kDocumentAutoId); return firestore_->NewDocumentReference(env, document); } @@ -92,7 +92,7 @@ DocumentReference CollectionReferenceInternal::Document( const std::string& document_path) const { Env env = GetEnv(); Local java_path = env.NewStringUtf(document_path); - Local document = env.Call(obj_, kDocument, java_path); + Local document = env.Call(obj_.get(env), kDocument, java_path); return firestore_->NewDocumentReference(env, document); } @@ -101,7 +101,7 @@ Future CollectionReferenceInternal::Add( FieldValueInternal map_value(data); Env env = GetEnv(); - Local task = env.Call(obj_, kAdd, map_value.ToJava()); + Local task = env.Call(obj_.get(env), kAdd, map_value.ToJava()); return promises_.NewFuture(env, AsyncFn::kAdd, task); } diff --git a/firestore/src/android/document_change_android.cc b/firestore/src/android/document_change_android.cc index 9c176f6e18..da9fb5d231 100644 --- a/firestore/src/android/document_change_android.cc +++ b/firestore/src/android/document_change_android.cc @@ -51,29 +51,29 @@ void DocumentChangeInternal::Initialize(jni::Loader& loader) { Type DocumentChangeInternal::type() const { Env env = GetEnv(); - Local type = env.Call(obj_, kType); + Local type = env.Call(obj_.get(env), kType); return type.GetType(env); } DocumentSnapshot DocumentChangeInternal::document() const { Env env = GetEnv(); - Local snapshot = env.Call(obj_, kDocument); + Local snapshot = env.Call(obj_.get(env), kDocument); return firestore_->NewDocumentSnapshot(env, snapshot); } std::size_t DocumentChangeInternal::old_index() const { Env env = GetEnv(); - return env.Call(obj_, kOldIndex); + return env.Call(obj_.get(env), kOldIndex); } std::size_t DocumentChangeInternal::new_index() const { Env env = GetEnv(); - return env.Call(obj_, kNewIndex); + return env.Call(obj_.get(env), kNewIndex); } std::size_t DocumentChangeInternal::Hash() const { Env env = GetEnv(); - return env.Call(obj_, kHashCode); + return env.Call(obj_.get(env), kHashCode); } bool operator==(const DocumentChangeInternal& lhs, diff --git a/firestore/src/android/document_reference_android.cc b/firestore/src/android/document_reference_android.cc index c5d5176e44..a9a6e3a18f 100644 --- a/firestore/src/android/document_reference_android.cc +++ b/firestore/src/android/document_reference_android.cc @@ -108,7 +108,7 @@ Firestore* DocumentReferenceInternal::firestore() { const std::string& DocumentReferenceInternal::id() const { if (cached_id_.empty()) { Env env = GetEnv(); - cached_id_ = env.Call(obj_, kGetId).ToString(env); + cached_id_ = env.Call(obj_.get(env), kGetId).ToString(env); } return cached_id_; } @@ -116,14 +116,14 @@ const std::string& DocumentReferenceInternal::id() const { const std::string& DocumentReferenceInternal::path() const { if (cached_path_.empty()) { Env env = GetEnv(); - cached_path_ = env.Call(obj_, kGetPath).ToString(env); + cached_path_ = env.Call(obj_.get(env), kGetPath).ToString(env); } return cached_path_; } CollectionReference DocumentReferenceInternal::Parent() const { Env env = GetEnv(); - Local parent = env.Call(obj_, kGetParent); + Local parent = env.Call(obj_.get(env), kGetParent); return firestore_->NewCollectionReference(env, parent); } @@ -131,14 +131,14 @@ CollectionReference DocumentReferenceInternal::Collection( const std::string& collection_path) { Env env = GetEnv(); Local java_path = env.NewStringUtf(collection_path); - Local collection = env.Call(obj_, kCollection, java_path); + Local collection = env.Call(obj_.get(env), kCollection, java_path); return firestore_->NewCollectionReference(env, collection); } Future DocumentReferenceInternal::Get(Source source) { Env env = GetEnv(); Local java_source = SourceInternal::Create(env, source); - Local task = env.Call(obj_, kGet, java_source); + Local task = env.Call(obj_.get(env), kGet, java_source); return promises_.NewFuture(env, AsyncFn::kGet, task); } @@ -147,14 +147,15 @@ Future DocumentReferenceInternal::Set(const MapFieldValue& data, Env env = GetEnv(); FieldValueInternal map_value(data); Local java_options = SetOptionsInternal::Create(env, options); - Local task = env.Call(obj_, kSet, map_value.ToJava(), java_options); + Local task = + env.Call(obj_.get(env), kSet, map_value.ToJava(), java_options); return promises_.NewFuture(env, AsyncFn::kSet, task); } Future DocumentReferenceInternal::Update(const MapFieldValue& data) { Env env = GetEnv(); FieldValueInternal map_value(data); - Local task = env.Call(obj_, kUpdate, map_value.ToJava()); + Local task = env.Call(obj_.get(env), kUpdate, map_value.ToJava()); return promises_.NewFuture(env, AsyncFn::kUpdate, task); } @@ -165,7 +166,7 @@ Future DocumentReferenceInternal::Update(const MapFieldPathValue& data) { Env env = GetEnv(); UpdateFieldPathArgs args = MakeUpdateFieldPathArgs(env, data); - Local task = env.Call(obj_, kUpdateVarargs, args.first_field, + Local task = env.Call(obj_.get(env), kUpdateVarargs, args.first_field, args.first_value, args.varargs); return promises_.NewFuture(env, AsyncFn::kUpdate, task); @@ -173,7 +174,7 @@ Future DocumentReferenceInternal::Update(const MapFieldPathValue& data) { Future DocumentReferenceInternal::Delete() { Env env = GetEnv(); - Local task = env.Call(obj_, kDelete); + Local task = env.Call(obj_.get(env), kDelete); return promises_.NewFuture(env, AsyncFn::kDelete, task); } @@ -198,9 +199,9 @@ ListenerRegistration DocumentReferenceInternal::AddSnapshotListener( Local java_listener = EventListenerInternal::Create(env, firestore_, listener); - Local java_registration = - env.Call(obj_, kAddSnapshotListener, firestore_->user_callback_executor(), - java_metadata, java_listener); + Local java_registration = env.Call( + obj_.get(env), kAddSnapshotListener, firestore_->user_callback_executor(), + java_metadata, java_listener); if (!env.ok() || !java_registration) return {}; return ListenerRegistration(new ListenerRegistrationInternal( diff --git a/firestore/src/android/document_snapshot_android.cc b/firestore/src/android/document_snapshot_android.cc index ec269904f8..2ce55889f5 100644 --- a/firestore/src/android/document_snapshot_android.cc +++ b/firestore/src/android/document_snapshot_android.cc @@ -76,33 +76,33 @@ Firestore* DocumentSnapshotInternal::firestore() const { const std::string& DocumentSnapshotInternal::id() const { if (cached_id_.empty()) { Env env = GetEnv(); - cached_id_ = env.Call(obj_, kGetId).ToString(env); + cached_id_ = env.Call(obj_.get(env), kGetId).ToString(env); } return cached_id_; } DocumentReference DocumentSnapshotInternal::reference() const { Env env = GetEnv(); - Local reference = env.Call(obj_, kGetReference); + Local reference = env.Call(obj_.get(env), kGetReference); return firestore_->NewDocumentReference(env, reference); } SnapshotMetadata DocumentSnapshotInternal::metadata() const { Env env = GetEnv(); - auto java_metadata = env.Call(obj_, kGetMetadata); + auto java_metadata = env.Call(obj_.get(env), kGetMetadata); return java_metadata.ToPublic(env); } bool DocumentSnapshotInternal::exists() const { Env env = GetEnv(); - return env.Call(obj_, kExists); + return env.Call(obj_.get(env), kExists); } MapFieldValue DocumentSnapshotInternal::GetData( ServerTimestampBehavior stb) const { Env env = GetEnv(); Local java_stb = ServerTimestampBehaviorInternal::Create(env, stb); - Local java_data = env.Call(obj_, kGetData, java_stb); + Local java_data = env.Call(obj_.get(env), kGetData, java_stb); if (!java_data) { // If the document doesn't exist, Android returns a null Map. In C++, the @@ -120,19 +120,20 @@ FieldValue DocumentSnapshotInternal::Get(const FieldPath& field, // Android returns null for both null fields and nonexistent fields, so first // use contains() to check if the field exists. - bool contains_field = env.Call(obj_, kContains, java_field); + bool contains_field = env.Call(obj_.get(env), kContains, java_field); if (!contains_field) { return FieldValue(); } Local java_stb = ServerTimestampBehaviorInternal::Create(env, stb); - Local field_value = env.Call(obj_, kGet, java_field, java_stb); + Local field_value = + env.Call(obj_.get(env), kGet, java_field, java_stb); return FieldValueInternal::Create(env, field_value); } std::size_t DocumentSnapshotInternal::Hash() const { Env env = GetEnv(); - return env.Call(obj_, kHashCode); + return env.Call(obj_.get(env), kHashCode); } bool operator==(const DocumentSnapshotInternal& lhs, diff --git a/firestore/src/android/field_value_android.cc b/firestore/src/android/field_value_android.cc index a4641a5af8..439807cebe 100644 --- a/firestore/src/android/field_value_android.cc +++ b/firestore/src/android/field_value_android.cc @@ -230,7 +230,8 @@ Type FieldValueInternal::type() const { cached_type_ = Type::kBlob; return Type::kBlob; } - if (env.IsInstanceOf(object_.get(env), DocumentReferenceInternal::GetClass())) { + if (env.IsInstanceOf(object_.get(env), + DocumentReferenceInternal::GetClass())) { cached_type_ = Type::kReference; return Type::kReference; } @@ -439,7 +440,7 @@ Env FieldValueInternal::GetEnv() { return FirestoreInternal::GetEnv(); } Local FieldValueInternal::ToJava(const FieldValue& value) { Env env; return value.internal_ ? value.internal_->object_.get(env) - : Local(Object()); + : Local(Object()); } } // namespace firestore diff --git a/firestore/src/android/field_value_android.h b/firestore/src/android/field_value_android.h index 24dac90fb1..e380e90d95 100644 --- a/firestore/src/android/field_value_android.h +++ b/firestore/src/android/field_value_android.h @@ -20,6 +20,7 @@ #include #include #include +#include #include "firebase/firestore/geo_point.h" #include "firebase/firestore/timestamp.h" diff --git a/firestore/src/android/load_bundle_task_progress_android.cc b/firestore/src/android/load_bundle_task_progress_android.cc index 1593a4a022..40994fbe6f 100644 --- a/firestore/src/android/load_bundle_task_progress_android.cc +++ b/firestore/src/android/load_bundle_task_progress_android.cc @@ -67,27 +67,27 @@ Class LoadBundleTaskProgressInternal::GetClass() { return Class(g_clazz); } int32_t LoadBundleTaskProgressInternal::documents_loaded() const { Env env = GetEnv(); - return env.Call(obj_, kGetDocumentsLoaded); + return env.Call(obj_.get(env), kGetDocumentsLoaded); } int32_t LoadBundleTaskProgressInternal::total_documents() const { Env env = GetEnv(); - return env.Call(obj_, kGetTotalDocuments); + return env.Call(obj_.get(env), kGetTotalDocuments); } int64_t LoadBundleTaskProgressInternal::bytes_loaded() const { Env env = GetEnv(); - return env.Call(obj_, kGetBytesLoaded); + return env.Call(obj_.get(env), kGetBytesLoaded); } int64_t LoadBundleTaskProgressInternal::total_bytes() const { Env env = GetEnv(); - return env.Call(obj_, kGetTotalBytes); + return env.Call(obj_.get(env), kGetTotalBytes); } LoadBundleTaskProgress::State LoadBundleTaskProgressInternal::state() const { Env env = GetEnv(); - Local state = env.Call(obj_, kGetTaskState); + Local state = env.Call(obj_.get(env), kGetTaskState); Local running_state = env.Get(kTaskStateRunning); Local success_state = env.Get(kTaskStateSuccess); diff --git a/firestore/src/android/query_android.cc b/firestore/src/android/query_android.cc index 201571432a..ddfa805b8c 100644 --- a/firestore/src/android/query_android.cc +++ b/firestore/src/android/query_android.cc @@ -204,7 +204,7 @@ Query QueryInternal::OrderBy(const FieldPath& field, Local java_field = FieldPathConverter::Create(env, field); Local java_direction = DirectionInternal::Create(env, direction); Local query = - env.Call(obj_, kOrderBy, java_field.get(), java_direction.get()); + env.Call(obj_.get(env), kOrderBy, java_field.get(), java_direction.get()); return firestore_->NewQuery(env, query); } @@ -214,7 +214,7 @@ Query QueryInternal::Limit(int32_t limit) const { // Although the backend only supports int32, the Android client SDK uses long // as parameter type. auto java_limit = static_cast(limit); - Local query = env.Call(obj_, kLimit, java_limit); + Local query = env.Call(obj_.get(env), kLimit, java_limit); return firestore_->NewQuery(env, query); } @@ -224,7 +224,7 @@ Query QueryInternal::LimitToLast(int32_t limit) const { // Although the backend only supports int32, the Android client SDK uses long // as parameter type. auto java_limit = static_cast(limit); - Local query = env.Call(obj_, kLimitToLast, java_limit); + Local query = env.Call(obj_.get(env), kLimitToLast, java_limit); return firestore_->NewQuery(env, query); } @@ -263,7 +263,7 @@ Query QueryInternal::EndAt(const std::vector& values) const { Future QueryInternal::Get(Source source) { Env env = GetEnv(); Local java_source = SourceInternal::Create(env, source); - Local task = env.Call(obj_, kGet, java_source); + Local task = env.Call(obj_.get(env), kGet, java_source); return promises_.NewFuture(env, AsyncFn::kGet, task); } @@ -273,7 +273,7 @@ Query QueryInternal::Where(const FieldPath& field, Env env = GetEnv(); Local java_field = FieldPathConverter::Create(env, field); Local java_value = FieldValueInternal::ToJava(value); - Local query = env.Call(obj_, method, java_field, java_value); + Local query = env.Call(obj_.get(env), method, java_field, java_value); return firestore_->NewQuery(env, query); } @@ -289,14 +289,16 @@ Query QueryInternal::Where(const FieldPath& field, } Local java_field = FieldPathConverter::Create(env, field); - Local query = env.Call(obj_, method, java_field, java_values); + Local query = + env.Call(obj_.get(env), method, java_field, java_values); return firestore_->NewQuery(env, query); } Query QueryInternal::WithBound(const Method& method, const DocumentSnapshot& snapshot) const { Env env = GetEnv(); - Local query = env.Call(obj_, method, snapshot.internal_->ToJava()); + Local query = + env.Call(obj_.get(env), method, snapshot.internal_->ToJava()); return firestore_->NewQuery(env, query); } @@ -304,7 +306,7 @@ Query QueryInternal::WithBound(const Method& method, const std::vector& values) const { Env env = GetEnv(); Local> java_values = ConvertFieldValues(env, values); - Local query = env.Call(obj_, method, java_values); + Local query = env.Call(obj_.get(env), method, java_values); return firestore_->NewQuery(env, query); } @@ -329,9 +331,9 @@ ListenerRegistration QueryInternal::AddSnapshotListener( Local java_metadata = MetadataChangesInternal::Create(env, metadata_changes); - Local java_registration = - env.Call(obj_, kAddSnapshotListener, firestore_->user_callback_executor(), - java_metadata, java_listener); + Local java_registration = env.Call( + obj_.get(env), kAddSnapshotListener, firestore_->user_callback_executor(), + java_metadata, java_listener); if (!env.ok()) return {}; return ListenerRegistration(new ListenerRegistrationInternal( @@ -350,7 +352,7 @@ Local> QueryInternal::ConvertFieldValues( size_t QueryInternal::Hash() const { Env env = GetEnv(); - return env.Call(obj_, kHashCode); + return env.Call(obj_.get(env), kHashCode); } bool operator==(const QueryInternal& lhs, const QueryInternal& rhs) { diff --git a/firestore/src/android/query_snapshot_android.cc b/firestore/src/android/query_snapshot_android.cc index 97e1befcf2..599df28221 100644 --- a/firestore/src/android/query_snapshot_android.cc +++ b/firestore/src/android/query_snapshot_android.cc @@ -59,13 +59,13 @@ void QuerySnapshotInternal::Initialize(jni::Loader& loader) { Query QuerySnapshotInternal::query() const { Env env = GetEnv(); - Local query = env.Call(obj_, kGetQuery); + Local query = env.Call(obj_.get(env), kGetQuery); return firestore_->NewQuery(env, query); } SnapshotMetadata QuerySnapshotInternal::metadata() const { Env env = GetEnv(); - return env.Call(obj_, kGetMetadata).ToPublic(env); + return env.Call(obj_.get(env), kGetMetadata).ToPublic(env); } std::vector QuerySnapshotInternal::DocumentChanges( @@ -73,24 +73,25 @@ std::vector QuerySnapshotInternal::DocumentChanges( Env env = GetEnv(); auto java_metadata = MetadataChangesInternal::Create(env, metadata_changes); - Local change_list = env.Call(obj_, kGetDocumentChanges, java_metadata); + Local change_list = + env.Call(obj_.get(env), kGetDocumentChanges, java_metadata); return MakePublicVector(env, firestore_, change_list); } std::vector QuerySnapshotInternal::documents() const { Env env = GetEnv(); - Local document_list = env.Call(obj_, kGetDocuments); + Local document_list = env.Call(obj_.get(env), kGetDocuments); return MakePublicVector(env, firestore_, document_list); } std::size_t QuerySnapshotInternal::size() const { Env env = GetEnv(); - return env.Call(obj_, kSize); + return env.Call(obj_.get(env), kSize); } std::size_t QuerySnapshotInternal::Hash() const { Env env = GetEnv(); - return env.Call(obj_, kHashCode); + return env.Call(obj_.get(env), kHashCode); } bool operator==(const QuerySnapshotInternal& lhs, diff --git a/firestore/src/android/transaction_android.cc b/firestore/src/android/transaction_android.cc index cf6497fc70..6d5027a317 100644 --- a/firestore/src/android/transaction_android.cc +++ b/firestore/src/android/transaction_android.cc @@ -95,14 +95,15 @@ void TransactionInternal::Set(const DocumentReference& document, Env env = GetEnv(); Local java_data = MakeJavaMap(env, data); Local java_options = SetOptionsInternal::Create(env, options); - env.Call(obj_, kSet, document.internal_->ToJava(), java_data, java_options); + env.Call(obj_.get(env), kSet, document.internal_->ToJava(), java_data, + java_options); } void TransactionInternal::Update(const DocumentReference& document, const MapFieldValue& data) { Env env = GetEnv(); Local java_data = MakeJavaMap(env, data); - env.Call(obj_, kUpdate, document.internal_->ToJava(), java_data); + env.Call(obj_.get(env), kUpdate, document.internal_->ToJava(), java_data); } void TransactionInternal::Update(const DocumentReference& document, @@ -114,13 +115,13 @@ void TransactionInternal::Update(const DocumentReference& document, Env env = GetEnv(); UpdateFieldPathArgs args = MakeUpdateFieldPathArgs(env, data); - env.Call(obj_, kUpdateVarargs, document.internal_->ToJava(), args.first_field, - args.first_value, args.varargs); + env.Call(obj_.get(env), kUpdateVarargs, document.internal_->ToJava(), + args.first_field, args.first_value, args.varargs); } void TransactionInternal::Delete(const DocumentReference& document) { Env env = GetEnv(); - env.Call(obj_, kDelete, document.internal_->ToJava()); + env.Call(obj_.get(env), kDelete, document.internal_->ToJava()); } DocumentSnapshot TransactionInternal::Get(const DocumentReference& document, @@ -128,7 +129,8 @@ DocumentSnapshot TransactionInternal::Get(const DocumentReference& document, std::string* error_message) { Env env = GetEnv(); - Local snapshot = env.Call(obj_, kGet, document.internal_->ToJava()); + Local snapshot = + env.Call(obj_.get(env), kGet, document.internal_->ToJava()); Local exception = env.ClearExceptionOccurred(); if (exception) { diff --git a/firestore/src/android/wrapper.cc b/firestore/src/android/wrapper.cc index 115967eed7..1685f3cf70 100644 --- a/firestore/src/android/wrapper.cc +++ b/firestore/src/android/wrapper.cc @@ -29,13 +29,16 @@ namespace firebase { namespace firestore { namespace { +using jni::ArenaRef; using jni::Env; using jni::Object; } // namespace Wrapper::Wrapper(FirestoreInternal* firestore, const Object& obj) - : firestore_(firestore), obj_(obj) { + : firestore_(firestore) { + Env env; + obj_ = ArenaRef(env, obj); FIREBASE_ASSERT(obj); } diff --git a/firestore/src/android/wrapper.h b/firestore/src/android/wrapper.h index eb7ca26504..a5c19278f0 100644 --- a/firestore/src/android/wrapper.h +++ b/firestore/src/android/wrapper.h @@ -17,6 +17,7 @@ #ifndef FIREBASE_FIRESTORE_SRC_ANDROID_WRAPPER_H_ #define FIREBASE_FIRESTORE_SRC_ANDROID_WRAPPER_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" @@ -44,7 +45,10 @@ class Wrapper { Wrapper& operator=(Wrapper&& wrapper) = delete; FirestoreInternal* firestore_internal() { return firestore_; } - const jni::Object& ToJava() const { return obj_; } + jni::Local ToJava() const { + jni::Env env; + return obj_.get(env); + } protected: // Default constructor. Subclass is expected to set the obj_ a meaningful @@ -57,7 +61,7 @@ class Wrapper { jni::Env GetEnv() const; FirestoreInternal* firestore_ = nullptr; // not owning - jni::Global obj_; + jni::ArenaRef obj_; private: friend class FirestoreInternal; diff --git a/firestore/src/android/write_batch_android.cc b/firestore/src/android/write_batch_android.cc index 31b659308f..d48bf8e74c 100644 --- a/firestore/src/android/write_batch_android.cc +++ b/firestore/src/android/write_batch_android.cc @@ -71,7 +71,7 @@ void WriteBatchInternal::Set(const DocumentReference& document, Local java_data = MakeJavaMap(env, data); Local java_options = SetOptionsInternal::Create(env, options); - env.Call(obj_, kSet, ToJava(document), java_data, java_options); + env.Call(obj_.get(env), kSet, ToJava(document), java_data, java_options); } void WriteBatchInternal::Update(const DocumentReference& document, @@ -79,7 +79,7 @@ void WriteBatchInternal::Update(const DocumentReference& document, Env env = GetEnv(); Local java_data = MakeJavaMap(env, data); - env.Call(obj_, kUpdate, ToJava(document), java_data); + env.Call(obj_.get(env), kUpdate, ToJava(document), java_data); } void WriteBatchInternal::Update(const DocumentReference& document, @@ -92,23 +92,23 @@ void WriteBatchInternal::Update(const DocumentReference& document, Env env = GetEnv(); UpdateFieldPathArgs args = MakeUpdateFieldPathArgs(env, data); - env.Call(obj_, kUpdateVarargs, ToJava(document), args.first_field, + env.Call(obj_.get(env), kUpdateVarargs, ToJava(document), args.first_field, args.first_value, args.varargs); } void WriteBatchInternal::Delete(const DocumentReference& document) { Env env = GetEnv(); - env.Call(obj_, kDelete, ToJava(document)); + env.Call(obj_.get(env), kDelete, ToJava(document)); } Future WriteBatchInternal::Commit() { Env env = GetEnv(); - Local task = env.Call(obj_, kCommit); + Local task = env.Call(obj_.get(env), kCommit); return promises_.NewFuture(env, AsyncFn::kCommit, task); } -jni::Object WriteBatchInternal::ToJava(const DocumentReference& reference) { - return reference.internal_ ? reference.internal_->ToJava() : jni::Object(); +Local WriteBatchInternal::ToJava(const DocumentReference& reference) { + return reference.internal_ ? reference.internal_->ToJava() : Local(); } } // namespace firestore diff --git a/firestore/src/android/write_batch_android.h b/firestore/src/android/write_batch_android.h index 86c7247bbc..e309ae3d78 100644 --- a/firestore/src/android/write_batch_android.h +++ b/firestore/src/android/write_batch_android.h @@ -64,7 +64,7 @@ class WriteBatchInternal : public Wrapper { */ // TODO(mcg): Move this out of WriteBatchInternal // This needs to be here now because of existing friend relationships. - static jni::Object ToJava(const DocumentReference& reference); + static jni::Local ToJava(const DocumentReference& reference); PromiseFactory promises_; }; From 476fd441e73a19e5af8b5ca1269d5fd9c6386d21 Mon Sep 17 00:00:00 2001 From: cherylEnkidu Date: Tue, 10 Jan 2023 15:23:59 -0500 Subject: [PATCH 07/16] Address Feedback --- .../src/android/arena_ref_android_test.cc | 108 ++++++++++++------ .../android/collection_reference_android.cc | 12 +- .../src/android/document_change_android.cc | 10 +- .../src/android/document_reference_android.cc | 19 ++- .../src/android/document_snapshot_android.cc | 17 ++- firestore/src/android/field_value_android.cc | 51 ++++----- firestore/src/android/field_value_android.h | 5 +- .../load_bundle_task_progress_android.cc | 10 +- firestore/src/android/query_android.cc | 20 ++-- .../src/android/query_snapshot_android.cc | 13 +-- firestore/src/android/transaction_android.cc | 14 +-- firestore/src/android/wrapper.cc | 5 + firestore/src/android/wrapper.h | 6 +- firestore/src/android/write_batch_android.cc | 10 +- firestore/src/jni/arena_ref.cc | 47 ++++---- firestore/src/jni/arena_ref.h | 13 ++- firestore/src/jni/env.h | 11 ++ firestore/src/jni/jni_fwd.h | 1 + 18 files changed, 215 insertions(+), 157 deletions(-) diff --git a/firestore/integration_test_internal/src/android/arena_ref_android_test.cc b/firestore/integration_test_internal/src/android/arena_ref_android_test.cc index 90a1a2df9e..8cdcd5885c 100644 --- a/firestore/integration_test_internal/src/android/arena_ref_android_test.cc +++ b/firestore/integration_test_internal/src/android/arena_ref_android_test.cc @@ -14,7 +14,10 @@ * limitations under the License. */ +#include "app/src/util_android.h" #include "firestore/src/jni/arena_ref.h" +#include "firestore/src/jni/hash_map.h" +#include "firestore/src/jni/long.h" #include "firestore_integration_test_android.h" @@ -26,7 +29,21 @@ namespace firestore { namespace jni { namespace { -using ArenaRefTestAndroid = FirestoreAndroidIntegrationTest; +jclass clazz = nullptr; + +Method kGet("get", "(Ljava/lang/Object;)Ljava/lang/Object;"); +Method kPut("put", + "(Ljava/lang/Object;Ljava/lang/Object;)Ljava/lang/Object;"); + +class ArenaRefTestAndroid : public FirestoreAndroidIntegrationTest { + public: + void SetUp() override { + FirestoreAndroidIntegrationTest::SetUp(); + jclass g_clazz = util::map::GetClass(); + loader().LoadFromExistingClass("java/util/HashMap", g_clazz, kGet, kPut); + ASSERT_TRUE(loader().ok()); + } +}; TEST_F(ArenaRefTestAndroid, DefaultConstructorCreatesReferenceToNull) { Env env; @@ -42,89 +59,114 @@ TEST_F(ArenaRefTestAndroid, ConstructFromEnvAndObject) { EXPECT_TRUE(arena_ref.get(env).Equals(env, string)); } -TEST_F(ArenaRefTestAndroid, CopyConstructor) { +TEST_F(ArenaRefTestAndroid, CopysReferenceToNull) { Env env; ArenaRef arena_ref1; ArenaRef arena_ref2(arena_ref1); EXPECT_EQ(arena_ref2.get(env).get(), nullptr); +} + +TEST_F(ArenaRefTestAndroid, CopysReferenceToValidObject) { + Env env; Local string = env.NewStringUtf("hello world"); - ArenaRef arena_ref3(env, string); - ArenaRef arena_ref4(arena_ref3); - ArenaRef arena_ref5 = arena_ref3; + ArenaRef arena_ref1(env, string); + ArenaRef arena_ref2(arena_ref1); - EXPECT_TRUE(arena_ref3.get(env).Equals(env, string)); - EXPECT_TRUE(arena_ref4.get(env).Equals(env, string)); - EXPECT_TRUE(arena_ref5.get(env).Equals(env, string)); + EXPECT_TRUE(arena_ref1.get(env).Equals(env, string)); + EXPECT_TRUE(arena_ref2.get(env).Equals(env, string)); } -TEST_F(ArenaRefTestAndroid, CopyAssignmentOp) { +TEST_F(ArenaRefTestAndroid, CopyAssignsReferenceToNull) { Env env; 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, CopyAssignsReferenceToValidObject) { + Env env; Local string1 = env.NewStringUtf("hello world"); Local string2 = env.NewStringUtf("hello earth"); - ArenaRef arena_ref3(env, string1); - ArenaRef arena_ref4(env, string2); - arena_ref4 = arena_ref3; - arena_ref3 = arena_ref3; + ArenaRef arena_ref1; + ArenaRef arena_ref2(env, string1); + ArenaRef arena_ref3(env, string2); + arena_ref3 = arena_ref2; + arena_ref2 = arena_ref2; EXPECT_TRUE(arena_ref3.get(env).Equals(env, string1)); - EXPECT_TRUE(arena_ref4.get(env).Equals(env, string1)); + EXPECT_TRUE(arena_ref2.get(env).Equals(env, string1)); - arena_ref3 = arena_ref1; - EXPECT_EQ(arena_ref3.get(env).get(), nullptr); - EXPECT_TRUE(arena_ref4.get(env).Equals(env, string1)); + arena_ref2 = arena_ref1; + EXPECT_EQ(arena_ref2.get(env).get(), nullptr); + EXPECT_TRUE(arena_ref3.get(env).Equals(env, string1)); } -TEST_F(ArenaRefTestAndroid, MoveConstructor) { +TEST_F(ArenaRefTestAndroid, MovesReferenceToNull) { Env env; ArenaRef arena_ref1; ArenaRef arena_ref2(std::move(arena_ref1)); EXPECT_EQ(arena_ref1.get(env).get(), nullptr); EXPECT_EQ(arena_ref2.get(env).get(), nullptr); +} - Local string = env.NewStringUtf("hello world"); +TEST_F(ArenaRefTestAndroid, MovesReferenceToValidObject) { + Env env; - ArenaRef arena_ref3(env, string); - ArenaRef arena_ref4(std::move(arena_ref3)); - EXPECT_EQ(arena_ref3.get(env).get(), nullptr); - EXPECT_TRUE(arena_ref4.get(env).Equals(env, string)); + Local string = env.NewStringUtf("hello world"); - ArenaRef arena_ref5 = std::move(arena_ref4); - EXPECT_TRUE(arena_ref5.get(env).Equals(env, string)); + ArenaRef arena_ref2(env, string); + ArenaRef arena_ref3(std::move(arena_ref2)); + EXPECT_EQ(arena_ref2.get(env).get(), nullptr); + EXPECT_TRUE(arena_ref3.get(env).Equals(env, string)); } -TEST_F(ArenaRefTestAndroid, MoveAssignmentOp) { +TEST_F(ArenaRefTestAndroid, MoveAssignsReferenceToNull) { Env env; ArenaRef arena_ref1, arena_ref2; arena_ref2 = std::move(arena_ref1); EXPECT_EQ(arena_ref1.get(env).get(), nullptr); EXPECT_EQ(arena_ref2.get(env).get(), nullptr); +} + +TEST_F(ArenaRefTestAndroid, MoveAssignsReferenceToValidObject) { + Env env; Local string1 = env.NewStringUtf("hello world"); Local string2 = env.NewStringUtf("hello earth"); - ArenaRef arena_ref3(env, string1); - arena_ref3 = std::move(arena_ref3); + ArenaRef arena_ref1; + ArenaRef arena_ref2(env, string1); + arena_ref2 = std::move(arena_ref2); + EXPECT_TRUE(arena_ref2.get(env).Equals(env, string1)); + + ArenaRef arena_ref3(env, string2); + arena_ref3 = std::move(arena_ref2); + EXPECT_EQ(arena_ref2.get(env).get(), nullptr); EXPECT_TRUE(arena_ref3.get(env).Equals(env, string1)); - ArenaRef arena_ref4(env, string2); - arena_ref4 = std::move(arena_ref3); + arena_ref3 = std::move(arena_ref1); EXPECT_EQ(arena_ref3.get(env).get(), nullptr); - EXPECT_TRUE(arena_ref4.get(env).Equals(env, string1)); +} + +TEST_F(ArenaRefTestAndroid, EnvCallTakeArenaRefTest) { + Env env; - arena_ref4 = std::move(arena_ref1); - EXPECT_EQ(arena_ref4.get(env).get(), nullptr); + ArenaRef hash_map(env, HashMap::Create(env)); + Local key = Long::Create(env, 1); + Local val = Long::Create(env, 2); + env.Call(hash_map, kPut, key, val); + Local result = env.Call(hash_map, kGet, key); + EXPECT_TRUE(result.Equals(env, val)); + EXPECT_TRUE(env.ok()); } } // namespace diff --git a/firestore/src/android/collection_reference_android.cc b/firestore/src/android/collection_reference_android.cc index 8c41f003ad..9bbbeceabc 100644 --- a/firestore/src/android/collection_reference_android.cc +++ b/firestore/src/android/collection_reference_android.cc @@ -63,7 +63,7 @@ void CollectionReferenceInternal::Initialize(jni::Loader& loader) { const std::string& CollectionReferenceInternal::id() const { if (cached_id_.empty()) { Env env = GetEnv(); - cached_id_ = env.Call(obj_.get(env), kGetId).ToString(env); + cached_id_ = env.Call(obj_, kGetId).ToString(env); } return cached_id_; } @@ -71,20 +71,20 @@ const std::string& CollectionReferenceInternal::id() const { const std::string& CollectionReferenceInternal::path() const { if (cached_path_.empty()) { Env env = GetEnv(); - cached_path_ = env.Call(obj_.get(env), kGetPath).ToString(env); + cached_path_ = env.Call(obj_, kGetPath).ToString(env); } return cached_path_; } DocumentReference CollectionReferenceInternal::Parent() const { Env env = GetEnv(); - Local parent = env.Call(obj_.get(env), kGetParent); + Local parent = env.Call(obj_, kGetParent); return firestore_->NewDocumentReference(env, parent); } DocumentReference CollectionReferenceInternal::Document() const { Env env = GetEnv(); - Local document = env.Call(obj_.get(env), kDocumentAutoId); + Local document = env.Call(obj_, kDocumentAutoId); return firestore_->NewDocumentReference(env, document); } @@ -92,7 +92,7 @@ DocumentReference CollectionReferenceInternal::Document( const std::string& document_path) const { Env env = GetEnv(); Local java_path = env.NewStringUtf(document_path); - Local document = env.Call(obj_.get(env), kDocument, java_path); + Local document = env.Call(obj_, kDocument, java_path); return firestore_->NewDocumentReference(env, document); } @@ -101,7 +101,7 @@ Future CollectionReferenceInternal::Add( FieldValueInternal map_value(data); Env env = GetEnv(); - Local task = env.Call(obj_.get(env), kAdd, map_value.ToJava()); + Local task = env.Call(obj_, kAdd, map_value.ToJava()); return promises_.NewFuture(env, AsyncFn::kAdd, task); } diff --git a/firestore/src/android/document_change_android.cc b/firestore/src/android/document_change_android.cc index da9fb5d231..9c176f6e18 100644 --- a/firestore/src/android/document_change_android.cc +++ b/firestore/src/android/document_change_android.cc @@ -51,29 +51,29 @@ void DocumentChangeInternal::Initialize(jni::Loader& loader) { Type DocumentChangeInternal::type() const { Env env = GetEnv(); - Local type = env.Call(obj_.get(env), kType); + Local type = env.Call(obj_, kType); return type.GetType(env); } DocumentSnapshot DocumentChangeInternal::document() const { Env env = GetEnv(); - Local snapshot = env.Call(obj_.get(env), kDocument); + Local snapshot = env.Call(obj_, kDocument); return firestore_->NewDocumentSnapshot(env, snapshot); } std::size_t DocumentChangeInternal::old_index() const { Env env = GetEnv(); - return env.Call(obj_.get(env), kOldIndex); + return env.Call(obj_, kOldIndex); } std::size_t DocumentChangeInternal::new_index() const { Env env = GetEnv(); - return env.Call(obj_.get(env), kNewIndex); + return env.Call(obj_, kNewIndex); } std::size_t DocumentChangeInternal::Hash() const { Env env = GetEnv(); - return env.Call(obj_.get(env), kHashCode); + return env.Call(obj_, kHashCode); } bool operator==(const DocumentChangeInternal& lhs, diff --git a/firestore/src/android/document_reference_android.cc b/firestore/src/android/document_reference_android.cc index a9a6e3a18f..417d186ca1 100644 --- a/firestore/src/android/document_reference_android.cc +++ b/firestore/src/android/document_reference_android.cc @@ -108,7 +108,7 @@ Firestore* DocumentReferenceInternal::firestore() { const std::string& DocumentReferenceInternal::id() const { if (cached_id_.empty()) { Env env = GetEnv(); - cached_id_ = env.Call(obj_.get(env), kGetId).ToString(env); + cached_id_ = env.Call(obj_, kGetId).ToString(env); } return cached_id_; } @@ -116,14 +116,14 @@ const std::string& DocumentReferenceInternal::id() const { const std::string& DocumentReferenceInternal::path() const { if (cached_path_.empty()) { Env env = GetEnv(); - cached_path_ = env.Call(obj_.get(env), kGetPath).ToString(env); + cached_path_ = env.Call(obj_, kGetPath).ToString(env); } return cached_path_; } CollectionReference DocumentReferenceInternal::Parent() const { Env env = GetEnv(); - Local parent = env.Call(obj_.get(env), kGetParent); + Local parent = env.Call(obj_, kGetParent); return firestore_->NewCollectionReference(env, parent); } @@ -131,14 +131,14 @@ CollectionReference DocumentReferenceInternal::Collection( const std::string& collection_path) { Env env = GetEnv(); Local java_path = env.NewStringUtf(collection_path); - Local collection = env.Call(obj_.get(env), kCollection, java_path); + Local collection = env.Call(obj_, kCollection, java_path); return firestore_->NewCollectionReference(env, collection); } Future DocumentReferenceInternal::Get(Source source) { Env env = GetEnv(); Local java_source = SourceInternal::Create(env, source); - Local task = env.Call(obj_.get(env), kGet, java_source); + Local task = env.Call(obj_, kGet, java_source); return promises_.NewFuture(env, AsyncFn::kGet, task); } @@ -147,15 +147,14 @@ Future DocumentReferenceInternal::Set(const MapFieldValue& data, Env env = GetEnv(); FieldValueInternal map_value(data); Local java_options = SetOptionsInternal::Create(env, options); - Local task = - env.Call(obj_.get(env), kSet, map_value.ToJava(), java_options); + Local task = env.Call(obj_, kSet, map_value.ToJava(), java_options); return promises_.NewFuture(env, AsyncFn::kSet, task); } Future DocumentReferenceInternal::Update(const MapFieldValue& data) { Env env = GetEnv(); FieldValueInternal map_value(data); - Local task = env.Call(obj_.get(env), kUpdate, map_value.ToJava()); + Local task = env.Call(obj_, kUpdate, map_value.ToJava()); return promises_.NewFuture(env, AsyncFn::kUpdate, task); } @@ -166,7 +165,7 @@ Future DocumentReferenceInternal::Update(const MapFieldPathValue& data) { Env env = GetEnv(); UpdateFieldPathArgs args = MakeUpdateFieldPathArgs(env, data); - Local task = env.Call(obj_.get(env), kUpdateVarargs, args.first_field, + Local task = env.Call(obj_, kUpdateVarargs, args.first_field, args.first_value, args.varargs); return promises_.NewFuture(env, AsyncFn::kUpdate, task); @@ -174,7 +173,7 @@ Future DocumentReferenceInternal::Update(const MapFieldPathValue& data) { Future DocumentReferenceInternal::Delete() { Env env = GetEnv(); - Local task = env.Call(obj_.get(env), kDelete); + Local task = env.Call(obj_, kDelete); return promises_.NewFuture(env, AsyncFn::kDelete, task); } diff --git a/firestore/src/android/document_snapshot_android.cc b/firestore/src/android/document_snapshot_android.cc index 2ce55889f5..ec269904f8 100644 --- a/firestore/src/android/document_snapshot_android.cc +++ b/firestore/src/android/document_snapshot_android.cc @@ -76,33 +76,33 @@ Firestore* DocumentSnapshotInternal::firestore() const { const std::string& DocumentSnapshotInternal::id() const { if (cached_id_.empty()) { Env env = GetEnv(); - cached_id_ = env.Call(obj_.get(env), kGetId).ToString(env); + cached_id_ = env.Call(obj_, kGetId).ToString(env); } return cached_id_; } DocumentReference DocumentSnapshotInternal::reference() const { Env env = GetEnv(); - Local reference = env.Call(obj_.get(env), kGetReference); + Local reference = env.Call(obj_, kGetReference); return firestore_->NewDocumentReference(env, reference); } SnapshotMetadata DocumentSnapshotInternal::metadata() const { Env env = GetEnv(); - auto java_metadata = env.Call(obj_.get(env), kGetMetadata); + auto java_metadata = env.Call(obj_, kGetMetadata); return java_metadata.ToPublic(env); } bool DocumentSnapshotInternal::exists() const { Env env = GetEnv(); - return env.Call(obj_.get(env), kExists); + return env.Call(obj_, kExists); } MapFieldValue DocumentSnapshotInternal::GetData( ServerTimestampBehavior stb) const { Env env = GetEnv(); Local java_stb = ServerTimestampBehaviorInternal::Create(env, stb); - Local java_data = env.Call(obj_.get(env), kGetData, java_stb); + Local java_data = env.Call(obj_, kGetData, java_stb); if (!java_data) { // If the document doesn't exist, Android returns a null Map. In C++, the @@ -120,20 +120,19 @@ FieldValue DocumentSnapshotInternal::Get(const FieldPath& field, // Android returns null for both null fields and nonexistent fields, so first // use contains() to check if the field exists. - bool contains_field = env.Call(obj_.get(env), kContains, java_field); + bool contains_field = env.Call(obj_, kContains, java_field); if (!contains_field) { return FieldValue(); } Local java_stb = ServerTimestampBehaviorInternal::Create(env, stb); - Local field_value = - env.Call(obj_.get(env), kGet, java_field, java_stb); + Local field_value = env.Call(obj_, kGet, java_field, java_stb); return FieldValueInternal::Create(env, field_value); } std::size_t DocumentSnapshotInternal::Hash() const { Env env = GetEnv(); - return env.Call(obj_.get(env), kHashCode); + return env.Call(obj_, kHashCode); } bool operator==(const DocumentSnapshotInternal& lhs, diff --git a/firestore/src/android/field_value_android.cc b/firestore/src/android/field_value_android.cc index 439807cebe..7b904a1a6a 100644 --- a/firestore/src/android/field_value_android.cc +++ b/firestore/src/android/field_value_android.cc @@ -113,36 +113,31 @@ FieldValueInternal::FieldValueInternal(Type type, const Object& object) FieldValueInternal::FieldValueInternal(bool value) : cached_type_(Type::kBoolean) { Env env; - auto boxed_ptr = Boolean::Create(env, value); - object_ = ArenaRef(env, boxed_ptr); + object_ = ArenaRef(env, Boolean::Create(env, value)); } FieldValueInternal::FieldValueInternal(int64_t value) : cached_type_(Type::kInteger) { Env env = GetEnv(); - auto boxed_ptr = Long::Create(env, value); - object_ = ArenaRef(env, boxed_ptr); + object_ = ArenaRef(env, Long::Create(env, value)); } FieldValueInternal::FieldValueInternal(double value) : cached_type_(Type::kDouble) { Env env = GetEnv(); - auto boxed_ptr = Double::Create(env, value); - object_ = ArenaRef(env, boxed_ptr); + object_ = ArenaRef(env, Double::Create(env, value)); } FieldValueInternal::FieldValueInternal(Timestamp value) : cached_type_(Type::kTimestamp) { Env env = GetEnv(); - auto boxed_ptr = TimestampInternal::Create(env, value); - object_ = ArenaRef(env, boxed_ptr); + object_ = ArenaRef(env, TimestampInternal::Create(env, value)); } FieldValueInternal::FieldValueInternal(std::string value) : cached_type_(Type::kString) { Env env = GetEnv(); - auto boxed_ptr = env.NewStringUtf(value); - object_ = ArenaRef(env, boxed_ptr); + object_ = ArenaRef(env, env.NewStringUtf(value)); } // We do not initialize cached_blob_ with value here as the instance constructed @@ -152,8 +147,7 @@ FieldValueInternal::FieldValueInternal(std::string value) FieldValueInternal::FieldValueInternal(const uint8_t* value, size_t size) : cached_type_(Type::kBlob) { Env env; - auto boxed_ptr = BlobInternal::Create(env, value, size); - object_ = ArenaRef(env, boxed_ptr); + object_ = ArenaRef(env, BlobInternal::Create(env, value, size)); } FieldValueInternal::FieldValueInternal(DocumentReference value) @@ -167,8 +161,7 @@ FieldValueInternal::FieldValueInternal(DocumentReference value) FieldValueInternal::FieldValueInternal(GeoPoint value) : cached_type_(Type::kGeoPoint) { Env env = GetEnv(); - auto boxed_ptr = GeoPointInternal::Create(env, value); - object_ = ArenaRef(env, boxed_ptr); + object_ = ArenaRef(env, GeoPointInternal::Create(env, value)); } FieldValueInternal::FieldValueInternal(const std::vector& value) @@ -206,44 +199,43 @@ Type FieldValueInternal::type() const { // We do not have any knowledge on the type yet. Check the runtime type with // each known type. - if (env.IsInstanceOf(object_.get(env), Boolean::GetClass())) { + if (env.IsInstanceOf(object_, Boolean::GetClass())) { cached_type_ = Type::kBoolean; return Type::kBoolean; } - if (env.IsInstanceOf(object_.get(env), Long::GetClass())) { + if (env.IsInstanceOf(object_, Long::GetClass())) { cached_type_ = Type::kInteger; return Type::kInteger; } - if (env.IsInstanceOf(object_.get(env), Double::GetClass())) { + if (env.IsInstanceOf(object_, Double::GetClass())) { cached_type_ = Type::kDouble; return Type::kDouble; } - if (env.IsInstanceOf(object_.get(env), TimestampInternal::GetClass())) { + if (env.IsInstanceOf(object_, TimestampInternal::GetClass())) { cached_type_ = Type::kTimestamp; return Type::kTimestamp; } - if (env.IsInstanceOf(object_.get(env), String::GetClass())) { + if (env.IsInstanceOf(object_, String::GetClass())) { cached_type_ = Type::kString; return Type::kString; } - if (env.IsInstanceOf(object_.get(env), BlobInternal::GetClass())) { + if (env.IsInstanceOf(object_, BlobInternal::GetClass())) { cached_type_ = Type::kBlob; return Type::kBlob; } - if (env.IsInstanceOf(object_.get(env), - DocumentReferenceInternal::GetClass())) { + if (env.IsInstanceOf(object_, DocumentReferenceInternal::GetClass())) { cached_type_ = Type::kReference; return Type::kReference; } - if (env.IsInstanceOf(object_.get(env), GeoPointInternal::GetClass())) { + if (env.IsInstanceOf(object_, GeoPointInternal::GetClass())) { cached_type_ = Type::kGeoPoint; return Type::kGeoPoint; } - if (env.IsInstanceOf(object_.get(env), List::GetClass())) { + if (env.IsInstanceOf(object_, List::GetClass())) { cached_type_ = Type::kArray; return Type::kArray; } - if (env.IsInstanceOf(object_.get(env), Map::GetClass())) { + if (env.IsInstanceOf(object_, Map::GetClass())) { cached_type_ = Type::kMap; return Type::kMap; } @@ -408,7 +400,7 @@ bool operator==(const FieldValueInternal& lhs, const FieldValueInternal& rhs) { template Local FieldValueInternal::Cast(jni::Env& env, Type type) const { if (cached_type_ == Type::kNull) { - FIREBASE_ASSERT(env.IsInstanceOf(object_.get(env), T::GetClass())); + FIREBASE_ASSERT(env.IsInstanceOf(object_, T::GetClass())); cached_type_ = type; } else { FIREBASE_ASSERT(cached_type_ == type); @@ -418,7 +410,7 @@ Local FieldValueInternal::Cast(jni::Env& env, Type type) const { Local FieldValueInternal::CastString(jni::Env& env, Type type) const { if (cached_type_ == Type::kNull) { - FIREBASE_ASSERT(env.IsInstanceOf(object_.get(env), String::GetClass())); + FIREBASE_ASSERT(env.IsInstanceOf(object_, String::GetClass())); cached_type_ = type; } else { FIREBASE_ASSERT(cached_type_ == type); @@ -437,6 +429,11 @@ Local> FieldValueInternal::MakeArray( Env FieldValueInternal::GetEnv() { return FirestoreInternal::GetEnv(); } +Local FieldValueInternal::ToJava() const { + Env env; + return object_.get(env); +} + Local FieldValueInternal::ToJava(const FieldValue& value) { Env env; return value.internal_ ? value.internal_->object_.get(env) diff --git a/firestore/src/android/field_value_android.h b/firestore/src/android/field_value_android.h index e380e90d95..832b6b30d3 100644 --- a/firestore/src/android/field_value_android.h +++ b/firestore/src/android/field_value_android.h @@ -90,10 +90,7 @@ class FieldValueInternal { std::vector array_value() const; MapFieldValue map_value() const; - jni::Local ToJava() const { - jni::Env env; - return object_.get(env); - } + jni::Local ToJava() const; static FieldValue Delete(); static FieldValue ServerTimestamp(); diff --git a/firestore/src/android/load_bundle_task_progress_android.cc b/firestore/src/android/load_bundle_task_progress_android.cc index 40994fbe6f..1593a4a022 100644 --- a/firestore/src/android/load_bundle_task_progress_android.cc +++ b/firestore/src/android/load_bundle_task_progress_android.cc @@ -67,27 +67,27 @@ Class LoadBundleTaskProgressInternal::GetClass() { return Class(g_clazz); } int32_t LoadBundleTaskProgressInternal::documents_loaded() const { Env env = GetEnv(); - return env.Call(obj_.get(env), kGetDocumentsLoaded); + return env.Call(obj_, kGetDocumentsLoaded); } int32_t LoadBundleTaskProgressInternal::total_documents() const { Env env = GetEnv(); - return env.Call(obj_.get(env), kGetTotalDocuments); + return env.Call(obj_, kGetTotalDocuments); } int64_t LoadBundleTaskProgressInternal::bytes_loaded() const { Env env = GetEnv(); - return env.Call(obj_.get(env), kGetBytesLoaded); + return env.Call(obj_, kGetBytesLoaded); } int64_t LoadBundleTaskProgressInternal::total_bytes() const { Env env = GetEnv(); - return env.Call(obj_.get(env), kGetTotalBytes); + return env.Call(obj_, kGetTotalBytes); } LoadBundleTaskProgress::State LoadBundleTaskProgressInternal::state() const { Env env = GetEnv(); - Local state = env.Call(obj_.get(env), kGetTaskState); + Local state = env.Call(obj_, kGetTaskState); Local running_state = env.Get(kTaskStateRunning); Local success_state = env.Get(kTaskStateSuccess); diff --git a/firestore/src/android/query_android.cc b/firestore/src/android/query_android.cc index ddfa805b8c..95953c0383 100644 --- a/firestore/src/android/query_android.cc +++ b/firestore/src/android/query_android.cc @@ -204,7 +204,7 @@ Query QueryInternal::OrderBy(const FieldPath& field, Local java_field = FieldPathConverter::Create(env, field); Local java_direction = DirectionInternal::Create(env, direction); Local query = - env.Call(obj_.get(env), kOrderBy, java_field.get(), java_direction.get()); + env.Call(obj_, kOrderBy, java_field.get(), java_direction.get()); return firestore_->NewQuery(env, query); } @@ -214,7 +214,7 @@ Query QueryInternal::Limit(int32_t limit) const { // Although the backend only supports int32, the Android client SDK uses long // as parameter type. auto java_limit = static_cast(limit); - Local query = env.Call(obj_.get(env), kLimit, java_limit); + Local query = env.Call(obj_, kLimit, java_limit); return firestore_->NewQuery(env, query); } @@ -224,7 +224,7 @@ Query QueryInternal::LimitToLast(int32_t limit) const { // Although the backend only supports int32, the Android client SDK uses long // as parameter type. auto java_limit = static_cast(limit); - Local query = env.Call(obj_.get(env), kLimitToLast, java_limit); + Local query = env.Call(obj_, kLimitToLast, java_limit); return firestore_->NewQuery(env, query); } @@ -263,7 +263,7 @@ Query QueryInternal::EndAt(const std::vector& values) const { Future QueryInternal::Get(Source source) { Env env = GetEnv(); Local java_source = SourceInternal::Create(env, source); - Local task = env.Call(obj_.get(env), kGet, java_source); + Local task = env.Call(obj_, kGet, java_source); return promises_.NewFuture(env, AsyncFn::kGet, task); } @@ -273,7 +273,7 @@ Query QueryInternal::Where(const FieldPath& field, Env env = GetEnv(); Local java_field = FieldPathConverter::Create(env, field); Local java_value = FieldValueInternal::ToJava(value); - Local query = env.Call(obj_.get(env), method, java_field, java_value); + Local query = env.Call(obj_, method, java_field, java_value); return firestore_->NewQuery(env, query); } @@ -289,16 +289,14 @@ Query QueryInternal::Where(const FieldPath& field, } Local java_field = FieldPathConverter::Create(env, field); - Local query = - env.Call(obj_.get(env), method, java_field, java_values); + Local query = env.Call(obj_, method, java_field, java_values); return firestore_->NewQuery(env, query); } Query QueryInternal::WithBound(const Method& method, const DocumentSnapshot& snapshot) const { Env env = GetEnv(); - Local query = - env.Call(obj_.get(env), method, snapshot.internal_->ToJava()); + Local query = env.Call(obj_, method, snapshot.internal_->ToJava()); return firestore_->NewQuery(env, query); } @@ -306,7 +304,7 @@ Query QueryInternal::WithBound(const Method& method, const std::vector& values) const { Env env = GetEnv(); Local> java_values = ConvertFieldValues(env, values); - Local query = env.Call(obj_.get(env), method, java_values); + Local query = env.Call(obj_, method, java_values); return firestore_->NewQuery(env, query); } @@ -352,7 +350,7 @@ Local> QueryInternal::ConvertFieldValues( size_t QueryInternal::Hash() const { Env env = GetEnv(); - return env.Call(obj_.get(env), kHashCode); + return env.Call(obj_, kHashCode); } bool operator==(const QueryInternal& lhs, const QueryInternal& rhs) { diff --git a/firestore/src/android/query_snapshot_android.cc b/firestore/src/android/query_snapshot_android.cc index 599df28221..97e1befcf2 100644 --- a/firestore/src/android/query_snapshot_android.cc +++ b/firestore/src/android/query_snapshot_android.cc @@ -59,13 +59,13 @@ void QuerySnapshotInternal::Initialize(jni::Loader& loader) { Query QuerySnapshotInternal::query() const { Env env = GetEnv(); - Local query = env.Call(obj_.get(env), kGetQuery); + Local query = env.Call(obj_, kGetQuery); return firestore_->NewQuery(env, query); } SnapshotMetadata QuerySnapshotInternal::metadata() const { Env env = GetEnv(); - return env.Call(obj_.get(env), kGetMetadata).ToPublic(env); + return env.Call(obj_, kGetMetadata).ToPublic(env); } std::vector QuerySnapshotInternal::DocumentChanges( @@ -73,25 +73,24 @@ std::vector QuerySnapshotInternal::DocumentChanges( Env env = GetEnv(); auto java_metadata = MetadataChangesInternal::Create(env, metadata_changes); - Local change_list = - env.Call(obj_.get(env), kGetDocumentChanges, java_metadata); + Local change_list = env.Call(obj_, kGetDocumentChanges, java_metadata); return MakePublicVector(env, firestore_, change_list); } std::vector QuerySnapshotInternal::documents() const { Env env = GetEnv(); - Local document_list = env.Call(obj_.get(env), kGetDocuments); + Local document_list = env.Call(obj_, kGetDocuments); return MakePublicVector(env, firestore_, document_list); } std::size_t QuerySnapshotInternal::size() const { Env env = GetEnv(); - return env.Call(obj_.get(env), kSize); + return env.Call(obj_, kSize); } std::size_t QuerySnapshotInternal::Hash() const { Env env = GetEnv(); - return env.Call(obj_.get(env), kHashCode); + return env.Call(obj_, kHashCode); } bool operator==(const QuerySnapshotInternal& lhs, diff --git a/firestore/src/android/transaction_android.cc b/firestore/src/android/transaction_android.cc index 6d5027a317..cf6497fc70 100644 --- a/firestore/src/android/transaction_android.cc +++ b/firestore/src/android/transaction_android.cc @@ -95,15 +95,14 @@ void TransactionInternal::Set(const DocumentReference& document, Env env = GetEnv(); Local java_data = MakeJavaMap(env, data); Local java_options = SetOptionsInternal::Create(env, options); - env.Call(obj_.get(env), kSet, document.internal_->ToJava(), java_data, - java_options); + env.Call(obj_, kSet, document.internal_->ToJava(), java_data, java_options); } void TransactionInternal::Update(const DocumentReference& document, const MapFieldValue& data) { Env env = GetEnv(); Local java_data = MakeJavaMap(env, data); - env.Call(obj_.get(env), kUpdate, document.internal_->ToJava(), java_data); + env.Call(obj_, kUpdate, document.internal_->ToJava(), java_data); } void TransactionInternal::Update(const DocumentReference& document, @@ -115,13 +114,13 @@ void TransactionInternal::Update(const DocumentReference& document, Env env = GetEnv(); UpdateFieldPathArgs args = MakeUpdateFieldPathArgs(env, data); - env.Call(obj_.get(env), kUpdateVarargs, document.internal_->ToJava(), - args.first_field, args.first_value, args.varargs); + env.Call(obj_, kUpdateVarargs, document.internal_->ToJava(), args.first_field, + args.first_value, args.varargs); } void TransactionInternal::Delete(const DocumentReference& document) { Env env = GetEnv(); - env.Call(obj_.get(env), kDelete, document.internal_->ToJava()); + env.Call(obj_, kDelete, document.internal_->ToJava()); } DocumentSnapshot TransactionInternal::Get(const DocumentReference& document, @@ -129,8 +128,7 @@ DocumentSnapshot TransactionInternal::Get(const DocumentReference& document, std::string* error_message) { Env env = GetEnv(); - Local snapshot = - env.Call(obj_.get(env), kGet, document.internal_->ToJava()); + Local snapshot = env.Call(obj_, kGet, document.internal_->ToJava()); Local exception = env.ClearExceptionOccurred(); if (exception) { diff --git a/firestore/src/android/wrapper.cc b/firestore/src/android/wrapper.cc index 1685f3cf70..8416c62fbc 100644 --- a/firestore/src/android/wrapper.cc +++ b/firestore/src/android/wrapper.cc @@ -59,6 +59,11 @@ Wrapper::Wrapper(Wrapper* rhs) : Wrapper() { Wrapper::~Wrapper() = default; +jni::Local Wrapper::ToJava() const { + jni::Env env; + return obj_.get(env); +} + jni::Env Wrapper::GetEnv() const { return firestore_->GetEnv(); } } // namespace firestore diff --git a/firestore/src/android/wrapper.h b/firestore/src/android/wrapper.h index a5c19278f0..d88a1baed1 100644 --- a/firestore/src/android/wrapper.h +++ b/firestore/src/android/wrapper.h @@ -18,6 +18,7 @@ #define FIREBASE_FIRESTORE_SRC_ANDROID_WRAPPER_H_ #include "firestore/src/jni/arena_ref.h" +#include "firestore/src/jni/env.h" #include "firestore/src/jni/jni_fwd.h" #include "firestore/src/jni/object.h" #include "firestore/src/jni/ownership.h" @@ -45,10 +46,7 @@ class Wrapper { Wrapper& operator=(Wrapper&& wrapper) = delete; FirestoreInternal* firestore_internal() { return firestore_; } - jni::Local ToJava() const { - jni::Env env; - return obj_.get(env); - } + jni::Local ToJava() const; protected: // Default constructor. Subclass is expected to set the obj_ a meaningful diff --git a/firestore/src/android/write_batch_android.cc b/firestore/src/android/write_batch_android.cc index d48bf8e74c..35b9367df8 100644 --- a/firestore/src/android/write_batch_android.cc +++ b/firestore/src/android/write_batch_android.cc @@ -71,7 +71,7 @@ void WriteBatchInternal::Set(const DocumentReference& document, Local java_data = MakeJavaMap(env, data); Local java_options = SetOptionsInternal::Create(env, options); - env.Call(obj_.get(env), kSet, ToJava(document), java_data, java_options); + env.Call(obj_, kSet, ToJava(document), java_data, java_options); } void WriteBatchInternal::Update(const DocumentReference& document, @@ -79,7 +79,7 @@ void WriteBatchInternal::Update(const DocumentReference& document, Env env = GetEnv(); Local java_data = MakeJavaMap(env, data); - env.Call(obj_.get(env), kUpdate, ToJava(document), java_data); + env.Call(obj_, kUpdate, ToJava(document), java_data); } void WriteBatchInternal::Update(const DocumentReference& document, @@ -92,18 +92,18 @@ void WriteBatchInternal::Update(const DocumentReference& document, Env env = GetEnv(); UpdateFieldPathArgs args = MakeUpdateFieldPathArgs(env, data); - env.Call(obj_.get(env), kUpdateVarargs, ToJava(document), args.first_field, + env.Call(obj_, kUpdateVarargs, ToJava(document), args.first_field, args.first_value, args.varargs); } void WriteBatchInternal::Delete(const DocumentReference& document) { Env env = GetEnv(); - env.Call(obj_.get(env), kDelete, ToJava(document)); + env.Call(obj_, kDelete, ToJava(document)); } Future WriteBatchInternal::Commit() { Env env = GetEnv(); - Local task = env.Call(obj_.get(env), kCommit); + Local task = env.Call(obj_, kCommit); return promises_.NewFuture(env, AsyncFn::kCommit, task); } diff --git a/firestore/src/jni/arena_ref.cc b/firestore/src/jni/arena_ref.cc index 260681e67b..1bc09870cc 100644 --- a/firestore/src/jni/arena_ref.cc +++ b/firestore/src/jni/arena_ref.cc @@ -17,6 +17,7 @@ #include "firestore/src/jni/arena_ref.h" +#include "firestore/src/jni/env.h" #include "firestore/src/jni/hash_map.h" #include "firestore/src/jni/long.h" #include "firestore/src/jni/map.h" @@ -30,7 +31,7 @@ namespace { HashMap* gArenaRefHashMap = nullptr; int64_t GetNextArenaRefKey() { - static std::atomic next_key(1); + static std::atomic next_key(-1); return next_key.fetch_add(1); } @@ -42,7 +43,7 @@ ArenaRef::ArenaRef(Env& env, const Object& object) } ArenaRef::ArenaRef(const ArenaRef& other) : key_(GetNextArenaRefKey()) { - if (other.key_ != 0) { + if (other.key_ != -1) { Env env; Local object = other.get(env); gArenaRefHashMap->Put(env, key_object(env), object); @@ -51,22 +52,26 @@ ArenaRef::ArenaRef(const ArenaRef& other) : key_(GetNextArenaRefKey()) { ArenaRef::ArenaRef(ArenaRef&& other) { key_ = other.key_; - other.key_ = 0; + other.key_ = -1; } ArenaRef& ArenaRef::operator=(const ArenaRef& other) { Env env; - if (key_ != other.key_) { - if (key_ != 0) { - gArenaRefHashMap->Remove(env, key_object(env)); - } + if (this == &other) { + return *this; + } - if (other.key_ != 0) { - key_ = GetNextArenaRefKey(); - Local object = other.get(env); - gArenaRefHashMap->Put(env, key_object(env), object); - } + if (key_ != -1) { + gArenaRefHashMap->Remove(env, key_object(env)); + } + + if (other.key_ != -1) { + key_ = GetNextArenaRefKey(); + Local object = other.get(env); + gArenaRefHashMap->Put(env, key_object(env), object); + } else { + key_ = -1; } return *this; } @@ -74,19 +79,20 @@ ArenaRef& ArenaRef::operator=(const ArenaRef& other) { ArenaRef& ArenaRef::operator=(ArenaRef&& other) { Env env; - if (key_ != other.key_) { - if (key_ != 0) { - gArenaRefHashMap->Remove(env, key_object(env)); - } + if (this == &other) { + return *this; + } - key_ = other.key_; - other.key_ = 0; + if (key_ != -1) { + gArenaRefHashMap->Remove(env, key_object(env)); } + key_ = other.key_; + other.key_ = -1; return *this; } ArenaRef::~ArenaRef() { - if (key_ != 0) { + if (key_ != -1) { Env env; gArenaRefHashMap->Remove(env, key_object(env)); } @@ -101,8 +107,7 @@ void ArenaRef::Initialize(Env& env) { return; } Global hash_map(HashMap::Create(env)); - jobject hash_map_jobject = hash_map.release(); - gArenaRefHashMap = new HashMap(hash_map_jobject); + gArenaRefHashMap = new HashMap(hash_map.release()); } Local ArenaRef::get(Env& env) const { diff --git a/firestore/src/jni/arena_ref.h b/firestore/src/jni/arena_ref.h index f97f015b66..48eb7d5f53 100644 --- a/firestore/src/jni/arena_ref.h +++ b/firestore/src/jni/arena_ref.h @@ -21,7 +21,7 @@ #include -#include "firestore/src/jni/env.h" +#include "firestore/src/jni/jni_fwd.h" #include "firestore/src/jni/object.h" #include "firestore/src/jni/ownership.h" @@ -29,6 +29,15 @@ namespace firebase { namespace firestore { namespace jni { +/** + * ArenaRef is an RAII wrapper which serves the same purpose as Global, both + * of them manage the lifetime of JNI reference. Compared to Global, the + * advantage of ArenaRef is it gets rid of the restriction of total number of + * JNI global reference (the pointer inside Global) need to be under 51,200, + * since after that number the app will crash. The drawback is ArenaRef holds + * JNI Reference indirectly using HashMap so there is a efficiency lost against + * Global. + */ class ArenaRef { public: ArenaRef() = default; @@ -46,7 +55,7 @@ class ArenaRef { private: Local key_object(Env&) const; - int64_t key_ = 0; + int64_t key_ = -1; }; } // namespace jni diff --git a/firestore/src/jni/env.h b/firestore/src/jni/env.h index a7c3354d68..33179be679 100644 --- a/firestore/src/jni/env.h +++ b/firestore/src/jni/env.h @@ -24,6 +24,7 @@ #include #include "app/meta/move.h" +#include "firestore/src/jni/arena_ref.h" #include "firestore/src/jni/call_traits.h" #include "firestore/src/jni/class.h" #include "firestore/src/jni/declaration.h" @@ -183,6 +184,9 @@ class Env { bool IsInstanceOf(jobject object, jclass clazz) { return IsInstanceOf(Object(object), Class(clazz)); } + bool IsInstanceOf(const ArenaRef& object, const Class& clazz) { + return IsInstanceOf(object.get(*this), Class(clazz)); + } bool IsSameObject(const Object& object1, const Object& object2); @@ -240,6 +244,13 @@ class Env { ToJni(Forward(args))...); } + template + ResultType Call(const ArenaRef& object, + const Method& method, + Args&&... args) { + return Call(object.get(*this), method, Forward(args)...); + } + // MARK: Accessing Static Fields jfieldID GetStaticFieldId(const Class& clazz, diff --git a/firestore/src/jni/jni_fwd.h b/firestore/src/jni/jni_fwd.h index 5969ddc2c1..a2f7e0aff0 100644 --- a/firestore/src/jni/jni_fwd.h +++ b/firestore/src/jni/jni_fwd.h @@ -38,6 +38,7 @@ template class Global; template class NonOwning; +class ArenaRef; class Class; class Object; From 3ec8673d2f5acbc5fda87211de34e330b087c723 Mon Sep 17 00:00:00 2001 From: cherylEnkidu Date: Wed, 11 Jan 2023 16:30:26 -0500 Subject: [PATCH 08/16] Possible Fix --- firestore/src/android/field_value_android.cc | 3 +++ firestore/src/common/field_value.cc | 2 ++ firestore/src/jni/arena_ref.cc | 2 +- firestore/src/jni/arena_ref.h | 1 - 4 files changed, 6 insertions(+), 2 deletions(-) diff --git a/firestore/src/android/field_value_android.cc b/firestore/src/android/field_value_android.cc index 7b904a1a6a..2ba1a6e8c8 100644 --- a/firestore/src/android/field_value_android.cc +++ b/firestore/src/android/field_value_android.cc @@ -16,6 +16,8 @@ #include "firestore/src/android/field_value_android.h" +#include + #include "app/meta/move.h" #include "firestore/src/android/blob_android.h" #include "firestore/src/android/document_reference_android.h" @@ -193,6 +195,7 @@ Type FieldValueInternal::type() const { } Env env; + std::cout << object_.key_ << std::endl; if (!object_.get(env)) { return Type::kNull; } diff --git a/firestore/src/common/field_value.cc b/firestore/src/common/field_value.cc index b64b38cdfb..dd5bd38076 100644 --- a/firestore/src/common/field_value.cc +++ b/firestore/src/common/field_value.cc @@ -19,6 +19,7 @@ #include #include #include +#include #include "app/meta/move.h" #include "firebase/firestore/geo_point.h" @@ -98,6 +99,7 @@ FieldValue::FieldValue(const FieldValue& value) { } FieldValue::FieldValue(FieldValue&& value) noexcept { + std::cout << (value.type() == Type::kNull) << std::endl; std::swap(internal_, value.internal_); } diff --git a/firestore/src/jni/arena_ref.cc b/firestore/src/jni/arena_ref.cc index 1bc09870cc..35e5d19de4 100644 --- a/firestore/src/jni/arena_ref.cc +++ b/firestore/src/jni/arena_ref.cc @@ -31,7 +31,7 @@ namespace { HashMap* gArenaRefHashMap = nullptr; int64_t GetNextArenaRefKey() { - static std::atomic next_key(-1); + static std::atomic next_key(0); return next_key.fetch_add(1); } diff --git a/firestore/src/jni/arena_ref.h b/firestore/src/jni/arena_ref.h index 48eb7d5f53..488fb952ca 100644 --- a/firestore/src/jni/arena_ref.h +++ b/firestore/src/jni/arena_ref.h @@ -52,7 +52,6 @@ class ArenaRef { Local get(Env&) const; - private: Local key_object(Env&) const; int64_t key_ = -1; From 5db27da6942ff359db27919a07c3f7d5ab8d357a Mon Sep 17 00:00:00 2001 From: cherylEnkidu Date: Wed, 11 Jan 2023 22:18:24 -0500 Subject: [PATCH 09/16] Remove DebugLog --- firestore/src/android/field_value_android.cc | 3 --- firestore/src/common/field_value.cc | 3 +-- firestore/src/jni/arena_ref.h | 1 + 3 files changed, 2 insertions(+), 5 deletions(-) diff --git a/firestore/src/android/field_value_android.cc b/firestore/src/android/field_value_android.cc index 2ba1a6e8c8..7b904a1a6a 100644 --- a/firestore/src/android/field_value_android.cc +++ b/firestore/src/android/field_value_android.cc @@ -16,8 +16,6 @@ #include "firestore/src/android/field_value_android.h" -#include - #include "app/meta/move.h" #include "firestore/src/android/blob_android.h" #include "firestore/src/android/document_reference_android.h" @@ -195,7 +193,6 @@ Type FieldValueInternal::type() const { } Env env; - std::cout << object_.key_ << std::endl; if (!object_.get(env)) { return Type::kNull; } diff --git a/firestore/src/common/field_value.cc b/firestore/src/common/field_value.cc index dd5bd38076..847dd79239 100644 --- a/firestore/src/common/field_value.cc +++ b/firestore/src/common/field_value.cc @@ -17,9 +17,9 @@ #include "firestore/src/include/firebase/firestore/field_value.h" #include +#include #include #include -#include #include "app/meta/move.h" #include "firebase/firestore/geo_point.h" @@ -99,7 +99,6 @@ FieldValue::FieldValue(const FieldValue& value) { } FieldValue::FieldValue(FieldValue&& value) noexcept { - std::cout << (value.type() == Type::kNull) << std::endl; std::swap(internal_, value.internal_); } diff --git a/firestore/src/jni/arena_ref.h b/firestore/src/jni/arena_ref.h index 488fb952ca..48eb7d5f53 100644 --- a/firestore/src/jni/arena_ref.h +++ b/firestore/src/jni/arena_ref.h @@ -52,6 +52,7 @@ class ArenaRef { Local get(Env&) const; + private: Local key_object(Env&) const; int64_t key_ = -1; From 6c3fc507a9d347a89b4f00d1d796e5004cb4d403 Mon Sep 17 00:00:00 2001 From: cherylEnkidu Date: Wed, 18 Jan 2023 00:55:17 -0500 Subject: [PATCH 10/16] Address Feedback 2 --- .../integration_test_internal/CMakeLists.txt | 1 + .../src/android/arena_ref_android_test.cc | 53 ++++----------- .../src/android/env_android_test.cc | 68 +++++++++++++++++++ .../src/android/document_reference_android.cc | 6 +- firestore/src/android/field_value_android.cc | 24 +++---- firestore/src/android/field_value_android.h | 3 +- firestore/src/android/wrapper.cc | 11 ++- firestore/src/common/field_value.cc | 1 - firestore/src/jni/arena_ref.cc | 23 +++---- firestore/src/jni/arena_ref.h | 2 +- 10 files changed, 116 insertions(+), 76 deletions(-) create mode 100644 firestore/integration_test_internal/src/android/env_android_test.cc diff --git a/firestore/integration_test_internal/CMakeLists.txt b/firestore/integration_test_internal/CMakeLists.txt index 0afaadeae5..c9276a373b 100644 --- a/firestore/integration_test_internal/CMakeLists.txt +++ b/firestore/integration_test_internal/CMakeLists.txt @@ -122,6 +122,7 @@ set(FIREBASE_INTEGRATION_TEST_PORTABLE_TEST_SRCS # These sources contain the actual tests that run on Android only. set(FIREBASE_INTEGRATION_TEST_ANDROID_TEST_SRCS src/android/arena_ref_android_test.cc + src/android/env_android_test.cc src/android/field_path_portable_test.cc src/android/firestore_integration_test_android_test.cc src/android/geo_point_android_test.cc diff --git a/firestore/integration_test_internal/src/android/arena_ref_android_test.cc b/firestore/integration_test_internal/src/android/arena_ref_android_test.cc index 8cdcd5885c..617ded14a5 100644 --- a/firestore/integration_test_internal/src/android/arena_ref_android_test.cc +++ b/firestore/integration_test_internal/src/android/arena_ref_android_test.cc @@ -1,5 +1,5 @@ /* - * Copyright 2022 Google LLC + * 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. @@ -15,13 +15,12 @@ */ #include "app/src/util_android.h" +#include "firestore/src/android/firestore_android.h" #include "firestore/src/jni/arena_ref.h" -#include "firestore/src/jni/hash_map.h" #include "firestore/src/jni/long.h" #include "firestore_integration_test_android.h" -#include "gmock/gmock.h" #include "gtest/gtest.h" namespace firebase { @@ -29,30 +28,16 @@ namespace firestore { namespace jni { namespace { -jclass clazz = nullptr; - -Method kGet("get", "(Ljava/lang/Object;)Ljava/lang/Object;"); -Method kPut("put", - "(Ljava/lang/Object;Ljava/lang/Object;)Ljava/lang/Object;"); - -class ArenaRefTestAndroid : public FirestoreAndroidIntegrationTest { - public: - void SetUp() override { - FirestoreAndroidIntegrationTest::SetUp(); - jclass g_clazz = util::map::GetClass(); - loader().LoadFromExistingClass("java/util/HashMap", g_clazz, kGet, kPut); - ASSERT_TRUE(loader().ok()); - } -}; +using ArenaRefTestAndroid = FirestoreIntegrationTest; TEST_F(ArenaRefTestAndroid, DefaultConstructorCreatesReferenceToNull) { - Env env; + Env env = FirestoreInternal::GetEnv(); ArenaRef arena_ref; EXPECT_EQ(arena_ref.get(env).get(), nullptr); } TEST_F(ArenaRefTestAndroid, ConstructFromEnvAndObject) { - Env env; + Env env = FirestoreInternal::GetEnv(); Local string = env.NewStringUtf("hello world"); ArenaRef arena_ref(env, string); @@ -60,7 +45,7 @@ TEST_F(ArenaRefTestAndroid, ConstructFromEnvAndObject) { } TEST_F(ArenaRefTestAndroid, CopysReferenceToNull) { - Env env; + Env env = FirestoreInternal::GetEnv(); ArenaRef arena_ref1; ArenaRef arena_ref2(arena_ref1); @@ -68,7 +53,7 @@ TEST_F(ArenaRefTestAndroid, CopysReferenceToNull) { } TEST_F(ArenaRefTestAndroid, CopysReferenceToValidObject) { - Env env; + Env env = FirestoreInternal::GetEnv(); Local string = env.NewStringUtf("hello world"); @@ -80,7 +65,7 @@ TEST_F(ArenaRefTestAndroid, CopysReferenceToValidObject) { } TEST_F(ArenaRefTestAndroid, CopyAssignsReferenceToNull) { - Env env; + Env env = FirestoreInternal::GetEnv(); ArenaRef arena_ref1, arena_ref2; arena_ref2 = arena_ref1; @@ -89,7 +74,7 @@ TEST_F(ArenaRefTestAndroid, CopyAssignsReferenceToNull) { } TEST_F(ArenaRefTestAndroid, CopyAssignsReferenceToValidObject) { - Env env; + Env env = FirestoreInternal::GetEnv(); Local string1 = env.NewStringUtf("hello world"); Local string2 = env.NewStringUtf("hello earth"); @@ -109,7 +94,7 @@ TEST_F(ArenaRefTestAndroid, CopyAssignsReferenceToValidObject) { } TEST_F(ArenaRefTestAndroid, MovesReferenceToNull) { - Env env; + Env env = FirestoreInternal::GetEnv(); ArenaRef arena_ref1; ArenaRef arena_ref2(std::move(arena_ref1)); @@ -118,7 +103,7 @@ TEST_F(ArenaRefTestAndroid, MovesReferenceToNull) { } TEST_F(ArenaRefTestAndroid, MovesReferenceToValidObject) { - Env env; + Env env = FirestoreInternal::GetEnv(); Local string = env.NewStringUtf("hello world"); @@ -129,7 +114,7 @@ TEST_F(ArenaRefTestAndroid, MovesReferenceToValidObject) { } TEST_F(ArenaRefTestAndroid, MoveAssignsReferenceToNull) { - Env env; + Env env = FirestoreInternal::GetEnv(); ArenaRef arena_ref1, arena_ref2; arena_ref2 = std::move(arena_ref1); @@ -138,7 +123,7 @@ TEST_F(ArenaRefTestAndroid, MoveAssignsReferenceToNull) { } TEST_F(ArenaRefTestAndroid, MoveAssignsReferenceToValidObject) { - Env env; + Env env = FirestoreInternal::GetEnv(); Local string1 = env.NewStringUtf("hello world"); Local string2 = env.NewStringUtf("hello earth"); @@ -157,18 +142,6 @@ TEST_F(ArenaRefTestAndroid, MoveAssignsReferenceToValidObject) { EXPECT_EQ(arena_ref3.get(env).get(), nullptr); } -TEST_F(ArenaRefTestAndroid, EnvCallTakeArenaRefTest) { - Env env; - - ArenaRef hash_map(env, HashMap::Create(env)); - Local key = Long::Create(env, 1); - Local val = Long::Create(env, 2); - env.Call(hash_map, kPut, key, val); - Local result = env.Call(hash_map, kGet, key); - EXPECT_TRUE(result.Equals(env, val)); - EXPECT_TRUE(env.ok()); -} - } // namespace } // namespace jni } // namespace firestore diff --git a/firestore/integration_test_internal/src/android/env_android_test.cc b/firestore/integration_test_internal/src/android/env_android_test.cc new file mode 100644 index 0000000000..cfbf27f615 --- /dev/null +++ b/firestore/integration_test_internal/src/android/env_android_test.cc @@ -0,0 +1,68 @@ +/* +* 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 "app/src/util_android.h" +#include "firestore/src/jni/env.h" +#include "firestore/src/jni/arena_ref.h" +#include "firestore/src/jni/hash_map.h" +#include "firestore/src/android/firestore_android.h" +#include "firestore/src/jni/long.h" + +#include "firestore_integration_test_android.h" + +#include "gtest/gtest.h" + +namespace firebase { +namespace firestore { +namespace jni { +namespace { + +Method kGet("get", "(Ljava/lang/Object;)Ljava/lang/Object;"); +Method kPut("put", + "(Ljava/lang/Object;Ljava/lang/Object;)Ljava/lang/Object;"); + +class EnvTestAndroid : public FirestoreAndroidIntegrationTest { + public: + void SetUp() override { + FirestoreAndroidIntegrationTest::SetUp(); + jclass g_clazz = util::map::GetClass(); + loader().LoadFromExistingClass("java/util/HashMap", g_clazz, kGet, kPut); + ASSERT_TRUE(loader().ok()); + } +}; + +TEST_F(EnvTestAndroid, EnvCallTakeArenaRefTest) { + Env env = FirestoreInternal::GetEnv(); + + ArenaRef hash_map(env, HashMap::Create(env)); + Local key = Long::Create(env, 1); + Local val = Long::Create(env, 2); + env.Call(hash_map, kPut, key, val); + Local result = env.Call(hash_map, kGet, key); + EXPECT_TRUE(result.Equals(env, val)); +} + +TEST_F(EnvTestAndroid, EnvIsInstanceOfTakeArenaRefTest) { + Env env = FirestoreInternal::GetEnv(); + + ArenaRef hash_map(env, HashMap::Create(env)); + EXPECT_TRUE(env.IsInstanceOf(hash_map, HashMap::GetClass())); +} + +} // namespace +} // namespace jni +} // namespace firestore +} // namespace firebase diff --git a/firestore/src/android/document_reference_android.cc b/firestore/src/android/document_reference_android.cc index 417d186ca1..c5d5176e44 100644 --- a/firestore/src/android/document_reference_android.cc +++ b/firestore/src/android/document_reference_android.cc @@ -198,9 +198,9 @@ ListenerRegistration DocumentReferenceInternal::AddSnapshotListener( Local java_listener = EventListenerInternal::Create(env, firestore_, listener); - Local java_registration = env.Call( - obj_.get(env), kAddSnapshotListener, firestore_->user_callback_executor(), - java_metadata, java_listener); + Local java_registration = + env.Call(obj_, kAddSnapshotListener, firestore_->user_callback_executor(), + java_metadata, java_listener); if (!env.ok() || !java_registration) return {}; return ListenerRegistration(new ListenerRegistrationInternal( diff --git a/firestore/src/android/field_value_android.cc b/firestore/src/android/field_value_android.cc index 7b904a1a6a..7e22a406a1 100644 --- a/firestore/src/android/field_value_android.cc +++ b/firestore/src/android/field_value_android.cc @@ -100,19 +100,19 @@ FieldValueInternal::FieldValueInternal() : cached_type_(Type::kNull) {} FieldValueInternal::FieldValueInternal(const Object& object) : cached_type_(Type::kNull) { - Env env; + Env env = GetEnv(); object_ = ArenaRef(env, object); } FieldValueInternal::FieldValueInternal(Type type, const Object& object) : cached_type_(type) { - Env env; + Env env = GetEnv(); object_ = ArenaRef(env, object); } FieldValueInternal::FieldValueInternal(bool value) : cached_type_(Type::kBoolean) { - Env env; + Env env = GetEnv(); object_ = ArenaRef(env, Boolean::Create(env, value)); } @@ -146,14 +146,14 @@ FieldValueInternal::FieldValueInternal(std::string value) // blob_value(). FieldValueInternal::FieldValueInternal(const uint8_t* value, size_t size) : cached_type_(Type::kBlob) { - Env env; + Env env = GetEnv(); object_ = ArenaRef(env, BlobInternal::Create(env, value, size)); } FieldValueInternal::FieldValueInternal(DocumentReference value) : cached_type_{Type::kReference} { if (value.internal_ != nullptr) { - Env env; + Env env = GetEnv(); object_ = ArenaRef(env, value.internal_->ToJava()); } } @@ -192,7 +192,7 @@ Type FieldValueInternal::type() const { return cached_type_; } - Env env; + Env env = GetEnv(); if (!object_.get(env)) { return Type::kNull; } @@ -267,7 +267,7 @@ Timestamp FieldValueInternal::timestamp_value() const { std::string FieldValueInternal::string_value() const { Env env = GetEnv(); - return CastString(env, Type::kString).ToString(env); + return Cast(env, Type::kString).ToString(env); } const uint8_t* FieldValueInternal::blob_value() const { @@ -408,7 +408,8 @@ Local FieldValueInternal::Cast(jni::Env& env, Type type) const { return object_.get(env).CastTo(); } -Local FieldValueInternal::CastString(jni::Env& env, Type type) const { +template <> +Local FieldValueInternal::Cast(jni::Env& env, Type type) const { if (cached_type_ == Type::kNull) { FIREBASE_ASSERT(env.IsInstanceOf(object_, String::GetClass())); cached_type_ = type; @@ -430,14 +431,13 @@ Local> FieldValueInternal::MakeArray( Env FieldValueInternal::GetEnv() { return FirestoreInternal::GetEnv(); } Local FieldValueInternal::ToJava() const { - Env env; + Env env = GetEnv(); return object_.get(env); } Local FieldValueInternal::ToJava(const FieldValue& value) { - Env env; - return value.internal_ ? value.internal_->object_.get(env) - : Local(Object()); + Env env = GetEnv(); + return value.internal_ ? value.internal_->object_.get(env) : Local(); } } // namespace firestore diff --git a/firestore/src/android/field_value_android.h b/firestore/src/android/field_value_android.h index 832b6b30d3..6639258413 100644 --- a/firestore/src/android/field_value_android.h +++ b/firestore/src/android/field_value_android.h @@ -112,7 +112,8 @@ class FieldValueInternal { template jni::Local Cast(jni::Env& env, Type type) const; - jni::Local CastString(jni::Env& env, Type type) const; + template <> + jni::Local Cast(jni::Env& env, Type type) const; static jni::Local> MakeArray( jni::Env& env, const std::vector& elements); diff --git a/firestore/src/android/wrapper.cc b/firestore/src/android/wrapper.cc index 8416c62fbc..29a294fcbd 100644 --- a/firestore/src/android/wrapper.cc +++ b/firestore/src/android/wrapper.cc @@ -16,8 +16,6 @@ #include "firestore/src/android/wrapper.h" -#include - #include "app/src/assert.h" #include "firestore/src/android/field_path_android.h" #include "firestore/src/android/field_value_android.h" @@ -31,15 +29,16 @@ namespace { using jni::ArenaRef; using jni::Env; +using jni::Local; using jni::Object; } // namespace Wrapper::Wrapper(FirestoreInternal* firestore, const Object& obj) : firestore_(firestore) { - Env env; - obj_ = ArenaRef(env, obj); FIREBASE_ASSERT(obj); + Env env = GetEnv(); + obj_ = ArenaRef(env, obj); } Wrapper::Wrapper() { @@ -59,8 +58,8 @@ Wrapper::Wrapper(Wrapper* rhs) : Wrapper() { Wrapper::~Wrapper() = default; -jni::Local Wrapper::ToJava() const { - jni::Env env; +Local Wrapper::ToJava() const { + Env env = GetEnv(); return obj_.get(env); } diff --git a/firestore/src/common/field_value.cc b/firestore/src/common/field_value.cc index 847dd79239..b64b38cdfb 100644 --- a/firestore/src/common/field_value.cc +++ b/firestore/src/common/field_value.cc @@ -17,7 +17,6 @@ #include "firestore/src/include/firebase/firestore/field_value.h" #include -#include #include #include diff --git a/firestore/src/jni/arena_ref.cc b/firestore/src/jni/arena_ref.cc index 35e5d19de4..616456f2ec 100644 --- a/firestore/src/jni/arena_ref.cc +++ b/firestore/src/jni/arena_ref.cc @@ -1,5 +1,5 @@ /* - * Copyright 2022 Google LLC + * 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. @@ -17,6 +17,7 @@ #include "firestore/src/jni/arena_ref.h" +#include "firestore/src/android/firestore_android.h" #include "firestore/src/jni/env.h" #include "firestore/src/jni/hash_map.h" #include "firestore/src/jni/long.h" @@ -42,21 +43,19 @@ ArenaRef::ArenaRef(Env& env, const Object& object) gArenaRefHashMap->Put(env, key_object(env), object); } -ArenaRef::ArenaRef(const ArenaRef& other) : key_(GetNextArenaRefKey()) { +ArenaRef::ArenaRef(const ArenaRef& other) + : key_(other.key_ == -1 ? -1 : GetNextArenaRefKey()) { if (other.key_ != -1) { - Env env; + Env env = FirestoreInternal::GetEnv(); Local object = other.get(env); gArenaRefHashMap->Put(env, key_object(env), object); } } -ArenaRef::ArenaRef(ArenaRef&& other) { - key_ = other.key_; - other.key_ = -1; -} +ArenaRef::ArenaRef(ArenaRef&& other) { std::swap(key_, other.key_); } ArenaRef& ArenaRef::operator=(const ArenaRef& other) { - Env env; + Env env = FirestoreInternal::GetEnv(); if (this == &other) { return *this; @@ -64,20 +63,19 @@ ArenaRef& ArenaRef::operator=(const ArenaRef& other) { if (key_ != -1) { gArenaRefHashMap->Remove(env, key_object(env)); + key_ = -1; } if (other.key_ != -1) { key_ = GetNextArenaRefKey(); Local object = other.get(env); gArenaRefHashMap->Put(env, key_object(env), object); - } else { - key_ = -1; } return *this; } ArenaRef& ArenaRef::operator=(ArenaRef&& other) { - Env env; + Env env = FirestoreInternal::GetEnv(); if (this == &other) { return *this; @@ -93,7 +91,8 @@ ArenaRef& ArenaRef::operator=(ArenaRef&& other) { ArenaRef::~ArenaRef() { if (key_ != -1) { - Env env; + Env env = FirestoreInternal::GetEnv(); + ExceptionClearGuard block(env); gArenaRefHashMap->Remove(env, key_object(env)); } } diff --git a/firestore/src/jni/arena_ref.h b/firestore/src/jni/arena_ref.h index 48eb7d5f53..e60aa2fabc 100644 --- a/firestore/src/jni/arena_ref.h +++ b/firestore/src/jni/arena_ref.h @@ -1,5 +1,5 @@ /* - * Copyright 2022 Google LLC + * 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. From 54c0826749a786cbbb52eec3ca1a43f853885b25 Mon Sep 17 00:00:00 2001 From: cherylEnkidu Date: Thu, 19 Jan 2023 03:52:36 -0500 Subject: [PATCH 11/16] Fix "Address Feedback 2" --- .../src/android/env_android_test.cc | 32 +++++++++---------- firestore/src/jni/arena_ref.cc | 6 ++-- 2 files changed, 20 insertions(+), 18 deletions(-) diff --git a/firestore/integration_test_internal/src/android/env_android_test.cc b/firestore/integration_test_internal/src/android/env_android_test.cc index cfbf27f615..8062e44422 100644 --- a/firestore/integration_test_internal/src/android/env_android_test.cc +++ b/firestore/integration_test_internal/src/android/env_android_test.cc @@ -1,24 +1,24 @@ /* -* 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. -*/ + * 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 "app/src/util_android.h" -#include "firestore/src/jni/env.h" +#include "firestore/src/android/firestore_android.h" #include "firestore/src/jni/arena_ref.h" +#include "firestore/src/jni/env.h" #include "firestore/src/jni/hash_map.h" -#include "firestore/src/android/firestore_android.h" #include "firestore/src/jni/long.h" #include "firestore_integration_test_android.h" diff --git a/firestore/src/jni/arena_ref.cc b/firestore/src/jni/arena_ref.cc index 616456f2ec..f742f8e196 100644 --- a/firestore/src/jni/arena_ref.cc +++ b/firestore/src/jni/arena_ref.cc @@ -13,10 +13,12 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -#include #include "firestore/src/jni/arena_ref.h" +#include +#include + #include "firestore/src/android/firestore_android.h" #include "firestore/src/jni/env.h" #include "firestore/src/jni/hash_map.h" @@ -91,7 +93,7 @@ ArenaRef& ArenaRef::operator=(ArenaRef&& other) { ArenaRef::~ArenaRef() { if (key_ != -1) { - Env env = FirestoreInternal::GetEnv(); + Env env; ExceptionClearGuard block(env); gArenaRefHashMap->Remove(env, key_object(env)); } From 7e14163e8433258257c622b61359fda29eeeacf3 Mon Sep 17 00:00:00 2001 From: cherylEnkidu Date: Fri, 20 Jan 2023 16:27:44 -0500 Subject: [PATCH 12/16] Avoid throw exception in cpp destructor in the previous commit, instead ignoring exception and only printing description --- firestore/src/jni/arena_ref.cc | 4 ++++ firestore/src/jni/env.h | 3 +++ 2 files changed, 7 insertions(+) diff --git a/firestore/src/jni/arena_ref.cc b/firestore/src/jni/arena_ref.cc index f742f8e196..dcfb8505fe 100644 --- a/firestore/src/jni/arena_ref.cc +++ b/firestore/src/jni/arena_ref.cc @@ -96,6 +96,10 @@ ArenaRef::~ArenaRef() { Env env; ExceptionClearGuard block(env); gArenaRefHashMap->Remove(env, key_object(env)); + if (!env.ok()) { + env.ExceptionDescribe(); + env.ExceptionClear(); + } } } diff --git a/firestore/src/jni/env.h b/firestore/src/jni/env.h index 33179be679..2267599ede 100644 --- a/firestore/src/jni/env.h +++ b/firestore/src/jni/env.h @@ -123,6 +123,9 @@ class Env { /** Clears the last exception. */ void ExceptionClear(); + /** Prints out exception. */ + void ExceptionDescribe() const { return env_->ExceptionDescribe(); } + /** * Returns the last Java exception to occur and clears the pending exception. */ From 84cbd190b42a97dae0be6df15279fa5c35df1399 Mon Sep 17 00:00:00 2001 From: cherylEnkidu Date: Thu, 26 Jan 2023 12:25:58 -0500 Subject: [PATCH 13/16] Add mutex for HashMap inside ArenaRef --- firestore/src/jni/arena_ref.cc | 39 ++++++++++++++++++++++------------ 1 file changed, 25 insertions(+), 14 deletions(-) diff --git a/firestore/src/jni/arena_ref.cc b/firestore/src/jni/arena_ref.cc index dcfb8505fe..39a8a140df 100644 --- a/firestore/src/jni/arena_ref.cc +++ b/firestore/src/jni/arena_ref.cc @@ -17,7 +17,6 @@ #include "firestore/src/jni/arena_ref.h" #include -#include #include "firestore/src/android/firestore_android.h" #include "firestore/src/jni/env.h" @@ -32,6 +31,7 @@ namespace jni { namespace { HashMap* gArenaRefHashMap = nullptr; +std::mutex mutex_; int64_t GetNextArenaRefKey() { static std::atomic next_key(0); @@ -42,6 +42,7 @@ int64_t GetNextArenaRefKey() { ArenaRef::ArenaRef(Env& env, const Object& object) : key_(GetNextArenaRefKey()) { + std::unique_lock lock(mutex_); gArenaRefHashMap->Put(env, key_object(env), object); } @@ -50,6 +51,7 @@ ArenaRef::ArenaRef(const ArenaRef& other) if (other.key_ != -1) { Env env = FirestoreInternal::GetEnv(); Local object = other.get(env); + std::unique_lock lock(mutex_); gArenaRefHashMap->Put(env, key_object(env), object); } } @@ -63,15 +65,18 @@ ArenaRef& ArenaRef::operator=(const ArenaRef& other) { return *this; } - if (key_ != -1) { - gArenaRefHashMap->Remove(env, key_object(env)); - key_ = -1; - } + { + std::unique_lock lock(mutex_); + if (key_ != -1) { + gArenaRefHashMap->Remove(env, key_object(env)); + key_ = -1; + } - if (other.key_ != -1) { - key_ = GetNextArenaRefKey(); - Local object = other.get(env); - gArenaRefHashMap->Put(env, key_object(env), object); + if (other.key_ != -1) { + key_ = GetNextArenaRefKey(); + Local object = other.get(env); + gArenaRefHashMap->Put(env, key_object(env), object); + } } return *this; } @@ -83,11 +88,14 @@ ArenaRef& ArenaRef::operator=(ArenaRef&& other) { return *this; } - if (key_ != -1) { - gArenaRefHashMap->Remove(env, key_object(env)); + { + std::unique_lock lock(mutex_); + if (key_ != -1) { + gArenaRefHashMap->Remove(env, key_object(env)); + } + key_ = other.key_; + other.key_ = -1; } - key_ = other.key_; - other.key_ = -1; return *this; } @@ -95,7 +103,10 @@ ArenaRef::~ArenaRef() { if (key_ != -1) { Env env; ExceptionClearGuard block(env); - gArenaRefHashMap->Remove(env, key_object(env)); + { + std::unique_lock lock(mutex_); + gArenaRefHashMap->Remove(env, key_object(env)); + } if (!env.ok()) { env.ExceptionDescribe(); env.ExceptionClear(); From b599bfab12a4b4d2f3cd2fff88168222420fdbd8 Mon Sep 17 00:00:00 2001 From: cherylEnkidu Date: Wed, 8 Feb 2023 11:26:42 -0500 Subject: [PATCH 14/16] Address feedback --- .../integration_test_internal/CMakeLists.txt | 3 +- .../src/android/env_android_test.cc | 68 ---------------- .../arena_ref_test.cc} | 29 +++---- .../src/jni/env_test.cc | 19 +++++ firestore/src/jni/arena_ref.cc | 80 ++++++++++--------- firestore/src/jni/arena_ref.h | 6 +- firestore/src/jni/env.h | 5 +- 7 files changed, 83 insertions(+), 127 deletions(-) delete mode 100644 firestore/integration_test_internal/src/android/env_android_test.cc rename firestore/integration_test_internal/src/{android/arena_ref_android_test.cc => jni/arena_ref_test.cc} (84%) diff --git a/firestore/integration_test_internal/CMakeLists.txt b/firestore/integration_test_internal/CMakeLists.txt index c9276a373b..1e9469e3fc 100644 --- a/firestore/integration_test_internal/CMakeLists.txt +++ b/firestore/integration_test_internal/CMakeLists.txt @@ -121,8 +121,6 @@ set(FIREBASE_INTEGRATION_TEST_PORTABLE_TEST_SRCS # These sources contain the actual tests that run on Android only. set(FIREBASE_INTEGRATION_TEST_ANDROID_TEST_SRCS - src/android/arena_ref_android_test.cc - src/android/env_android_test.cc src/android/field_path_portable_test.cc src/android/firestore_integration_test_android_test.cc src/android/geo_point_android_test.cc @@ -133,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 diff --git a/firestore/integration_test_internal/src/android/env_android_test.cc b/firestore/integration_test_internal/src/android/env_android_test.cc deleted file mode 100644 index 8062e44422..0000000000 --- a/firestore/integration_test_internal/src/android/env_android_test.cc +++ /dev/null @@ -1,68 +0,0 @@ -/* - * 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 "app/src/util_android.h" -#include "firestore/src/android/firestore_android.h" -#include "firestore/src/jni/arena_ref.h" -#include "firestore/src/jni/env.h" -#include "firestore/src/jni/hash_map.h" -#include "firestore/src/jni/long.h" - -#include "firestore_integration_test_android.h" - -#include "gtest/gtest.h" - -namespace firebase { -namespace firestore { -namespace jni { -namespace { - -Method kGet("get", "(Ljava/lang/Object;)Ljava/lang/Object;"); -Method kPut("put", - "(Ljava/lang/Object;Ljava/lang/Object;)Ljava/lang/Object;"); - -class EnvTestAndroid : public FirestoreAndroidIntegrationTest { - public: - void SetUp() override { - FirestoreAndroidIntegrationTest::SetUp(); - jclass g_clazz = util::map::GetClass(); - loader().LoadFromExistingClass("java/util/HashMap", g_clazz, kGet, kPut); - ASSERT_TRUE(loader().ok()); - } -}; - -TEST_F(EnvTestAndroid, EnvCallTakeArenaRefTest) { - Env env = FirestoreInternal::GetEnv(); - - ArenaRef hash_map(env, HashMap::Create(env)); - Local key = Long::Create(env, 1); - Local val = Long::Create(env, 2); - env.Call(hash_map, kPut, key, val); - Local result = env.Call(hash_map, kGet, key); - EXPECT_TRUE(result.Equals(env, val)); -} - -TEST_F(EnvTestAndroid, EnvIsInstanceOfTakeArenaRefTest) { - Env env = FirestoreInternal::GetEnv(); - - ArenaRef hash_map(env, HashMap::Create(env)); - EXPECT_TRUE(env.IsInstanceOf(hash_map, HashMap::GetClass())); -} - -} // namespace -} // namespace jni -} // namespace firestore -} // namespace firebase diff --git a/firestore/integration_test_internal/src/android/arena_ref_android_test.cc b/firestore/integration_test_internal/src/jni/arena_ref_test.cc similarity index 84% rename from firestore/integration_test_internal/src/android/arena_ref_android_test.cc rename to firestore/integration_test_internal/src/jni/arena_ref_test.cc index 617ded14a5..f0de34e321 100644 --- a/firestore/integration_test_internal/src/android/arena_ref_android_test.cc +++ b/firestore/integration_test_internal/src/jni/arena_ref_test.cc @@ -14,12 +14,11 @@ * limitations under the License. */ -#include "app/src/util_android.h" -#include "firestore/src/android/firestore_android.h" #include "firestore/src/jni/arena_ref.h" +#include "firestore/src/android/firestore_android.h" #include "firestore/src/jni/long.h" -#include "firestore_integration_test_android.h" +#include "firestore_integration_test.h" #include "gtest/gtest.h" @@ -31,13 +30,13 @@ namespace { using ArenaRefTestAndroid = FirestoreIntegrationTest; TEST_F(ArenaRefTestAndroid, DefaultConstructorCreatesReferenceToNull) { - Env env = FirestoreInternal::GetEnv(); + Env env; ArenaRef arena_ref; EXPECT_EQ(arena_ref.get(env).get(), nullptr); } TEST_F(ArenaRefTestAndroid, ConstructFromEnvAndObject) { - Env env = FirestoreInternal::GetEnv(); + Env env; Local string = env.NewStringUtf("hello world"); ArenaRef arena_ref(env, string); @@ -45,7 +44,7 @@ TEST_F(ArenaRefTestAndroid, ConstructFromEnvAndObject) { } TEST_F(ArenaRefTestAndroid, CopysReferenceToNull) { - Env env = FirestoreInternal::GetEnv(); + Env env; ArenaRef arena_ref1; ArenaRef arena_ref2(arena_ref1); @@ -53,7 +52,7 @@ TEST_F(ArenaRefTestAndroid, CopysReferenceToNull) { } TEST_F(ArenaRefTestAndroid, CopysReferenceToValidObject) { - Env env = FirestoreInternal::GetEnv(); + Env env; Local string = env.NewStringUtf("hello world"); @@ -65,7 +64,7 @@ TEST_F(ArenaRefTestAndroid, CopysReferenceToValidObject) { } TEST_F(ArenaRefTestAndroid, CopyAssignsReferenceToNull) { - Env env = FirestoreInternal::GetEnv(); + Env env; ArenaRef arena_ref1, arena_ref2; arena_ref2 = arena_ref1; @@ -74,7 +73,7 @@ TEST_F(ArenaRefTestAndroid, CopyAssignsReferenceToNull) { } TEST_F(ArenaRefTestAndroid, CopyAssignsReferenceToValidObject) { - Env env = FirestoreInternal::GetEnv(); + Env env; Local string1 = env.NewStringUtf("hello world"); Local string2 = env.NewStringUtf("hello earth"); @@ -94,36 +93,33 @@ TEST_F(ArenaRefTestAndroid, CopyAssignsReferenceToValidObject) { } TEST_F(ArenaRefTestAndroid, MovesReferenceToNull) { - Env env = FirestoreInternal::GetEnv(); + Env env; ArenaRef arena_ref1; ArenaRef arena_ref2(std::move(arena_ref1)); - EXPECT_EQ(arena_ref1.get(env).get(), nullptr); EXPECT_EQ(arena_ref2.get(env).get(), nullptr); } TEST_F(ArenaRefTestAndroid, MovesReferenceToValidObject) { - Env env = FirestoreInternal::GetEnv(); + Env env; Local string = env.NewStringUtf("hello world"); ArenaRef arena_ref2(env, string); ArenaRef arena_ref3(std::move(arena_ref2)); - EXPECT_EQ(arena_ref2.get(env).get(), nullptr); EXPECT_TRUE(arena_ref3.get(env).Equals(env, string)); } TEST_F(ArenaRefTestAndroid, MoveAssignsReferenceToNull) { - Env env = FirestoreInternal::GetEnv(); + Env env; ArenaRef arena_ref1, arena_ref2; arena_ref2 = std::move(arena_ref1); - EXPECT_EQ(arena_ref1.get(env).get(), nullptr); EXPECT_EQ(arena_ref2.get(env).get(), nullptr); } TEST_F(ArenaRefTestAndroid, MoveAssignsReferenceToValidObject) { - Env env = FirestoreInternal::GetEnv(); + Env env; Local string1 = env.NewStringUtf("hello world"); Local string2 = env.NewStringUtf("hello earth"); @@ -135,7 +131,6 @@ TEST_F(ArenaRefTestAndroid, MoveAssignsReferenceToValidObject) { ArenaRef arena_ref3(env, string2); arena_ref3 = std::move(arena_ref2); - EXPECT_EQ(arena_ref2.get(env).get(), nullptr); EXPECT_TRUE(arena_ref3.get(env).Equals(env, string1)); arena_ref3 = std::move(arena_ref1); diff --git a/firestore/integration_test_internal/src/jni/env_test.cc b/firestore/integration_test_internal/src/jni/env_test.cc index d31992e48e..51506dadcb 100644 --- a/firestore/integration_test_internal/src/jni/env_test.cc +++ b/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 str = env().NewStringUtf("Foo"); + ArenaRef arena_ref(env(), str); + + Local clazz = env().FindClass("java/lang/String"); + jmethodID to_lower_case = + env().GetMethodId(clazz, "toLowerCase", "()Ljava/lang/String;"); + + Local result = env().Call(arena_ref, Method(to_lower_case)); + EXPECT_EQ("foo", result.ToString(env())); +} + TEST_F(EnvTest, GetsStaticFields) { Local clazz = env().FindClass("java/lang/String"); jfieldID comparator = env().GetStaticFieldId(clazz, "CASE_INSENSITIVE_ORDER", @@ -160,6 +172,13 @@ TEST_F(EnvTest, ToString) { EXPECT_EQ("Foo", result); } +TEST_F(EnvTest, IsInstanceOfChecksValidArenaRef) { + Local clazz = env().FindClass("java/lang/String"); + Local str = env().NewStringUtf("Foo"); + ArenaRef arena_ref(env(), str); + EXPECT_TRUE(env().IsInstanceOf(arena_ref, clazz)); +} + TEST_F(EnvTest, Throw) { Local clazz = env().FindClass("java/lang/Exception"); jmethodID ctor = env().GetMethodId(clazz, "", "(Ljava/lang/String;)V"); diff --git a/firestore/src/jni/arena_ref.cc b/firestore/src/jni/arena_ref.cc index 39a8a140df..456b7fecc7 100644 --- a/firestore/src/jni/arena_ref.cc +++ b/firestore/src/jni/arena_ref.cc @@ -17,6 +17,7 @@ #include "firestore/src/jni/arena_ref.h" #include +#include #include "firestore/src/android/firestore_android.h" #include "firestore/src/jni/env.h" @@ -31,6 +32,8 @@ namespace jni { namespace { HashMap* gArenaRefHashMap = nullptr; +jclass gLongClass = nullptr; +jmethodID gLongConstructor = nullptr; std::mutex mutex_; int64_t GetNextArenaRefKey() { @@ -42,80 +45,78 @@ int64_t GetNextArenaRefKey() { ArenaRef::ArenaRef(Env& env, const Object& object) : key_(GetNextArenaRefKey()) { - std::unique_lock lock(mutex_); - gArenaRefHashMap->Put(env, key_object(env), object); + Local key_ref = key_object(env); + std::lock_guard lock(mutex_); + gArenaRefHashMap->Put(env, key_ref, object); } ArenaRef::ArenaRef(const ArenaRef& other) - : key_(other.key_ == -1 ? -1 : GetNextArenaRefKey()) { - if (other.key_ != -1) { + : key_(other.key_ == kInvalidKey ? kInvalidKey : GetNextArenaRefKey()) { + if (other.key_ != kInvalidKey) { Env env = FirestoreInternal::GetEnv(); Local object = other.get(env); - std::unique_lock lock(mutex_); - gArenaRefHashMap->Put(env, key_object(env), object); + Local key_ref = key_object(env); + std::lock_guard lock(mutex_); + gArenaRefHashMap->Put(env, key_ref, object); } } ArenaRef::ArenaRef(ArenaRef&& other) { std::swap(key_, other.key_); } ArenaRef& ArenaRef::operator=(const ArenaRef& other) { - Env env = FirestoreInternal::GetEnv(); - - if (this == &other) { + if (this == &other || (key_ == kInvalidKey && other.key_ == kInvalidKey)) { return *this; } - { - std::unique_lock lock(mutex_); - if (key_ != -1) { - gArenaRefHashMap->Remove(env, key_object(env)); - key_ = -1; - } - - if (other.key_ != -1) { + Env env = FirestoreInternal::GetEnv(); + if (other.key_ != kInvalidKey) { + if (key_ == kInvalidKey) { key_ = GetNextArenaRefKey(); - Local object = other.get(env); - gArenaRefHashMap->Put(env, key_object(env), object); } + Local object = other.get(env); + std::lock_guard lock(mutex_); + gArenaRefHashMap->Put(env, key_object(env), object); + } else { + { + std::lock_guard lock(mutex_); + gArenaRefHashMap->Remove(env, key_object(env)); + } + key_ = kInvalidKey; } return *this; } ArenaRef& ArenaRef::operator=(ArenaRef&& other) { - Env env = FirestoreInternal::GetEnv(); - - if (this == &other) { - return *this; - } - - { - std::unique_lock lock(mutex_); - if (key_ != -1) { - gArenaRefHashMap->Remove(env, key_object(env)); - } - key_ = other.key_; - other.key_ = -1; + if (this != &other) { + std::swap(key_, other.key_); } return *this; } ArenaRef::~ArenaRef() { - if (key_ != -1) { + if (key_ != kInvalidKey) { Env env; ExceptionClearGuard block(env); { - std::unique_lock lock(mutex_); + std::lock_guard lock(mutex_); gArenaRefHashMap->Remove(env, key_object(env)); } if (!env.ok()) { - env.ExceptionDescribe(); + env.RecordException(); env.ExceptionClear(); } } } Local ArenaRef::key_object(Env& env) const { - return Long::Create(env, key_); + if (!env.ok()) return {}; + // The reason why using raw JNI calls here rather than the JNI convenience + // layer is because we need to get rid of JNI convenience layer dependencies + // in the ArenaRef destructor to avoid racing condition when firestore object + // gets destroyed. + jobject key = env.get()->NewObject(gLongClass, gLongConstructor, key_); + env.RecordException(); + return {env.get(), key}; } void ArenaRef::Initialize(Env& env) { @@ -124,6 +125,13 @@ void ArenaRef::Initialize(Env& env) { } Global hash_map(HashMap::Create(env)); gArenaRefHashMap = new HashMap(hash_map.release()); + + auto long_class_local = env.get()->FindClass("java/lang/Long"); + gLongClass = static_cast(env.get()->NewGlobalRef(long_class_local)); + env.get()->DeleteLocalRef(long_class_local); + gLongConstructor = env.get()->GetMethodID(gLongClass, "", "(J)V"); + + if (!env.ok()) return; } Local ArenaRef::get(Env& env) const { diff --git a/firestore/src/jni/arena_ref.h b/firestore/src/jni/arena_ref.h index e60aa2fabc..81ca7d8116 100644 --- a/firestore/src/jni/arena_ref.h +++ b/firestore/src/jni/arena_ref.h @@ -29,6 +29,10 @@ namespace firebase { namespace firestore { namespace jni { +namespace { +constexpr int64_t kInvalidKey = -1; +} + /** * ArenaRef is an RAII wrapper which serves the same purpose as Global, both * of them manage the lifetime of JNI reference. Compared to Global, the @@ -55,7 +59,7 @@ class ArenaRef { private: Local key_object(Env&) const; - int64_t key_ = -1; + int64_t key_ = kInvalidKey; }; } // namespace jni diff --git a/firestore/src/jni/env.h b/firestore/src/jni/env.h index 2267599ede..3906c35d83 100644 --- a/firestore/src/jni/env.h +++ b/firestore/src/jni/env.h @@ -123,8 +123,8 @@ class Env { /** Clears the last exception. */ void ExceptionClear(); - /** Prints out exception. */ - void ExceptionDescribe() const { return env_->ExceptionDescribe(); } + /** Prints out exception to logcat if `Env` has an exception. */ + void RecordException(); /** * Returns the last Java exception to occur and clears the pending exception. @@ -527,7 +527,6 @@ class Env { RecordException(); } - void RecordException(); std::string ErrorDescription(const Object& object); const char* ErrorName(jint error); From e7879c50f95a159db8f4bbb92131c42afe225a0d Mon Sep 17 00:00:00 2001 From: cherylEnkidu Date: Sun, 12 Feb 2023 17:18:23 -0500 Subject: [PATCH 15/16] Address feedback again --- .../src/jni/arena_ref_test.cc | 265 +++++++++++++----- .../src/jni/env_test.cc | 8 + firestore/src/jni/arena_ref.cc | 9 +- firestore/src/jni/arena_ref.h | 5 +- 4 files changed, 215 insertions(+), 72 deletions(-) diff --git a/firestore/integration_test_internal/src/jni/arena_ref_test.cc b/firestore/integration_test_internal/src/jni/arena_ref_test.cc index f0de34e321..d621e765a7 100644 --- a/firestore/integration_test_internal/src/jni/arena_ref_test.cc +++ b/firestore/integration_test_internal/src/jni/arena_ref_test.cc @@ -16,6 +16,8 @@ #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" @@ -27,114 +29,247 @@ namespace firestore { namespace jni { namespace { -using ArenaRefTestAndroid = FirestoreIntegrationTest; - -TEST_F(ArenaRefTestAndroid, DefaultConstructorCreatesReferenceToNull) { - Env env; +constexpr char kException[] = "java/lang/Exception"; +Method kConstructor("", "(Ljava/lang/String;)V"); + +class ArenaRefTestAndroid : public FirestoreIntegrationTest { + public: + ArenaRefTestAndroid() : loader_(app()), env_(make_unique(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 clazz = env().FindClass(kException); + jmethodID ctor = + env().GetMethodId(clazz, "", "(Ljava/lang/String;)V"); + + Local message = env().NewStringUtf("Testing throw"); + Local exception = env().New(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 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_; + Loader loader_; +}; + +TEST_F(ArenaRefTestAndroid, DefaultConstructor) { ArenaRef arena_ref; - EXPECT_EQ(arena_ref.get(env).get(), nullptr); + EXPECT_EQ(arena_ref.get(env()).get(), nullptr); } -TEST_F(ArenaRefTestAndroid, ConstructFromEnvAndObject) { - Env env; - Local string = env.NewStringUtf("hello world"); - - ArenaRef arena_ref(env, string); - EXPECT_TRUE(arena_ref.get(env).Equals(env, string)); +TEST_F(ArenaRefTestAndroid, ConstructsFromNull) { + Local string; + ArenaRef arena_ref(env(), string); + EXPECT_EQ(arena_ref.get(env()).get(), nullptr); } -TEST_F(ArenaRefTestAndroid, CopysReferenceToNull) { - Env env; +TEST_F(ArenaRefTestAndroid, ConstructsFromValid) { + Local 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); + EXPECT_EQ(arena_ref2.get(env()).get(), nullptr); } -TEST_F(ArenaRefTestAndroid, CopysReferenceToValidObject) { - Env env; +TEST_F(ArenaRefTestAndroid, CopyConstructsFromValid) { + Local string = env().NewStringUtf("hello world"); - Local string = env.NewStringUtf("hello world"); - - ArenaRef arena_ref1(env, string); + ArenaRef arena_ref1(env(), string); ArenaRef arena_ref2(arena_ref1); - - EXPECT_TRUE(arena_ref1.get(env).Equals(env, string)); - EXPECT_TRUE(arena_ref2.get(env).Equals(env, string)); + EXPECT_TRUE(env().IsSameObject(arena_ref1.get(env()), string)); + EXPECT_TRUE(env().IsSameObject(arena_ref2.get(env()), string)); } -TEST_F(ArenaRefTestAndroid, CopyAssignsReferenceToNull) { - Env env; - +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); + EXPECT_EQ(arena_ref1.get(env()).get(), nullptr); + EXPECT_EQ(arena_ref2.get(env()).get(), nullptr); } -TEST_F(ArenaRefTestAndroid, CopyAssignsReferenceToValidObject) { - Env env; +TEST_F(ArenaRefTestAndroid, CopyAssignsFromNullToValid) { + Local string = env().NewStringUtf("hello world"); - Local string1 = env.NewStringUtf("hello world"); - Local string2 = env.NewStringUtf("hello earth"); + 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 = env().NewStringUtf("hello world"); ArenaRef arena_ref1; - ArenaRef arena_ref2(env, string1); - ArenaRef arena_ref3(env, string2); - arena_ref3 = arena_ref2; - arena_ref2 = arena_ref2; + 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)); +} - EXPECT_TRUE(arena_ref3.get(env).Equals(env, string1)); - EXPECT_TRUE(arena_ref2.get(env).Equals(env, string1)); +TEST_F(ArenaRefTestAndroid, CopyAssignsFromValidToValid) { + Local string1 = env().NewStringUtf("hello world"); + Local string2 = env().NewStringUtf("hello earth"); - arena_ref2 = arena_ref1; - EXPECT_EQ(arena_ref2.get(env).get(), nullptr); - EXPECT_TRUE(arena_ref3.get(env).Equals(env, string1)); + 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, MovesReferenceToNull) { - Env env; +TEST_F(ArenaRefTestAndroid, CopyAssignsFromValidObjectItself) { + Local 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); + EXPECT_EQ(arena_ref2.get(env()).get(), nullptr); } -TEST_F(ArenaRefTestAndroid, MovesReferenceToValidObject) { - Env env; +TEST_F(ArenaRefTestAndroid, MoveConstructsFromValid) { + Local string = env().NewStringUtf("hello world"); - Local string = env.NewStringUtf("hello world"); - - ArenaRef arena_ref2(env, string); + ArenaRef arena_ref2(env(), string); ArenaRef arena_ref3(std::move(arena_ref2)); - EXPECT_TRUE(arena_ref3.get(env).Equals(env, string)); + EXPECT_TRUE(env().IsSameObject(arena_ref3.get(env()), string)); } -TEST_F(ArenaRefTestAndroid, MoveAssignsReferenceToNull) { - Env env; - +TEST_F(ArenaRefTestAndroid, MoveAssignsFromNullToNull) { ArenaRef arena_ref1, arena_ref2; arena_ref2 = std::move(arena_ref1); - EXPECT_EQ(arena_ref2.get(env).get(), nullptr); + EXPECT_EQ(arena_ref2.get(env()).get(), nullptr); } -TEST_F(ArenaRefTestAndroid, MoveAssignsReferenceToValidObject) { - Env env; +TEST_F(ArenaRefTestAndroid, MoveAssignsFromNullToValid) { + Local string = env().NewStringUtf("hello world"); - Local string1 = env.NewStringUtf("hello world"); - Local string2 = env.NewStringUtf("hello earth"); + 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 = env().NewStringUtf("hello world"); ArenaRef arena_ref1; - ArenaRef arena_ref2(env, string1); - arena_ref2 = std::move(arena_ref2); - EXPECT_TRUE(arena_ref2.get(env).Equals(env, string1)); + 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 string1 = env().NewStringUtf("hello world"); + Local 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 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)); +} - ArenaRef arena_ref3(env, string2); - arena_ref3 = std::move(arena_ref2); - EXPECT_TRUE(arena_ref3.get(env).Equals(env, string1)); +TEST_F(ArenaRefTestAndroid, ThrowBeforeConstruct) { + Local 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 = 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); +} - arena_ref3 = std::move(arena_ref1); - EXPECT_EQ(arena_ref3.get(env).get(), nullptr); +TEST_F(ArenaRefTestAndroid, ThrowBeforeCopyAssignment) { + Local 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 = 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 = 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 diff --git a/firestore/integration_test_internal/src/jni/env_test.cc b/firestore/integration_test_internal/src/jni/env_test.cc index 51506dadcb..e33b09b552 100644 --- a/firestore/integration_test_internal/src/jni/env_test.cc +++ b/firestore/integration_test_internal/src/jni/env_test.cc @@ -179,6 +179,14 @@ TEST_F(EnvTest, IsInstanceOfChecksValidArenaRef) { EXPECT_TRUE(env().IsInstanceOf(arena_ref, clazz)); } +TEST_F(EnvTest, IsInstanceOfChecksNullArenaRef) { + GTEST_SKIP() + << "Skip until edge case of IsInstanceOf (b/268420201) is covered."; + Local clazz = env().FindClass("java/lang/String"); + ArenaRef arena_ref; + EXPECT_FALSE(env().IsInstanceOf(arena_ref, clazz)); +} + TEST_F(EnvTest, Throw) { Local clazz = env().FindClass("java/lang/Exception"); jmethodID ctor = env().GetMethodId(clazz, "", "(Ljava/lang/String;)V"); diff --git a/firestore/src/jni/arena_ref.cc b/firestore/src/jni/arena_ref.cc index 456b7fecc7..38b66b67a5 100644 --- a/firestore/src/jni/arena_ref.cc +++ b/firestore/src/jni/arena_ref.cc @@ -53,7 +53,7 @@ ArenaRef::ArenaRef(Env& env, const Object& object) ArenaRef::ArenaRef(const ArenaRef& other) : key_(other.key_ == kInvalidKey ? kInvalidKey : GetNextArenaRefKey()) { if (other.key_ != kInvalidKey) { - Env env = FirestoreInternal::GetEnv(); + Env env; Local object = other.get(env); Local key_ref = key_object(env); std::lock_guard lock(mutex_); @@ -61,14 +61,17 @@ ArenaRef::ArenaRef(const ArenaRef& other) } } -ArenaRef::ArenaRef(ArenaRef&& other) { std::swap(key_, other.key_); } +ArenaRef::ArenaRef(ArenaRef&& other) { + using std::swap; // go/using-std-swap + swap(key_, other.key_); +} ArenaRef& ArenaRef::operator=(const ArenaRef& other) { if (this == &other || (key_ == kInvalidKey && other.key_ == kInvalidKey)) { return *this; } - Env env = FirestoreInternal::GetEnv(); + Env env; if (other.key_ != kInvalidKey) { if (key_ == kInvalidKey) { key_ = GetNextArenaRefKey(); diff --git a/firestore/src/jni/arena_ref.h b/firestore/src/jni/arena_ref.h index 81ca7d8116..8ccbb43fab 100644 --- a/firestore/src/jni/arena_ref.h +++ b/firestore/src/jni/arena_ref.h @@ -29,10 +29,6 @@ namespace firebase { namespace firestore { namespace jni { -namespace { -constexpr int64_t kInvalidKey = -1; -} - /** * ArenaRef is an RAII wrapper which serves the same purpose as Global, both * of them manage the lifetime of JNI reference. Compared to Global, the @@ -59,6 +55,7 @@ class ArenaRef { private: Local key_object(Env&) const; + static constexpr int64_t kInvalidKey = -1; int64_t key_ = kInvalidKey; }; From 84400cfdfe0be9a97de74b58e31d56e58dbcbedc Mon Sep 17 00:00:00 2001 From: cherylEnkidu Date: Sun, 12 Feb 2023 20:31:53 -0500 Subject: [PATCH 16/16] Add tests to prove the fix --- .../integration_test_internal/src/field_value_test.cc | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/firestore/integration_test_internal/src/field_value_test.cc b/firestore/integration_test_internal/src/field_value_test.cc index bd82c1aa90..7f75a15eb1 100644 --- a/firestore/integration_test_internal/src/field_value_test.cc +++ b/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 numbers; + for (size_t i = 0U; i < 60000; i++) { + numbers.push_back(FieldValue::Integer(i)); + } +} + } // namespace firestore } // namespace firebase