From b599bfab12a4b4d2f3cd2fff88168222420fdbd8 Mon Sep 17 00:00:00 2001 From: cherylEnkidu Date: Wed, 8 Feb 2023 11:26:42 -0500 Subject: [PATCH] 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);