Skip to content

Commit 8d98424

Browse files
cherylEnkiducherylEnkidu
committed
Address feedback
1 parent a834eca commit 8d98424

File tree

8 files changed

+78
-133
lines changed

8 files changed

+78
-133
lines changed

‎firestore/integration_test_internal/CMakeLists.txt

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -121,8 +121,6 @@ set(FIREBASE_INTEGRATION_TEST_PORTABLE_TEST_SRCS
121121

122122
# These sources contain the actual tests that run on Android only.
123123
set(FIREBASE_INTEGRATION_TEST_ANDROID_TEST_SRCS
124-
src/android/arena_ref_android_test.cc
125-
src/android/env_android_test.cc
126124
src/android/field_path_portable_test.cc
127125
src/android/firestore_integration_test_android_test.cc
128126
src/android/geo_point_android_test.cc
@@ -133,6 +131,7 @@ set(FIREBASE_INTEGRATION_TEST_ANDROID_TEST_SRCS
133131
src/android/snapshot_metadata_android_test.cc
134132
src/android/timestamp_android_test.cc
135133
src/android/transaction_options_android_test.cc
134+
src/jni/arena_ref_test.cc
136135
src/jni/declaration_test.cc
137136
src/jni/env_test.cc
138137
src/jni/object_test.cc

‎firestore/integration_test_internal/build.gradle

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,8 @@ android {
5959
versionCode 1
6060
versionName '1.0'
6161
externalNativeBuild.cmake {
62-
arguments "-DFIREBASE_CPP_SDK_DIR=$gradle.firebase_cpp_sdk_dir"
62+
arguments "-DFIREBASE_CPP_SDK_DIR=$gradle.firebase_cpp_sdk_dir","-DFIREBASE_PYTHON_EXECUTABLE=/opt/homebrew/bin/python3","-DFIREBASE_PYTHON_HOST_EXECUTABLE=/opt/homebrew/bin/python3"
63+
abiFilters 'arm64-v8a'
6364
}
6465
multiDexEnabled true
6566
}

‎firestore/integration_test_internal/src/android/env_android_test.cc

Lines changed: 0 additions & 68 deletions
This file was deleted.

‎firestore/integration_test_internal/src/android/arena_ref_android_test.cc renamed to ‎firestore/integration_test_internal/src/jni/arena_ref_test.cc

Lines changed: 12 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -14,12 +14,11 @@
1414
* limitations under the License.
1515
*/
1616

17-
#include "app/src/util_android.h"
18-
#include "firestore/src/android/firestore_android.h"
1917
#include "firestore/src/jni/arena_ref.h"
18+
#include "firestore/src/android/firestore_android.h"
2019
#include "firestore/src/jni/long.h"
2120

22-
#include "firestore_integration_test_android.h"
21+
#include "firestore_integration_test.h"
2322

2423
#include "gtest/gtest.h"
2524

@@ -31,29 +30,29 @@ namespace {
3130
using ArenaRefTestAndroid = FirestoreIntegrationTest;
3231

3332
TEST_F(ArenaRefTestAndroid, DefaultConstructorCreatesReferenceToNull) {
34-
Env env = FirestoreInternal::GetEnv();
33+
Env env;
3534
ArenaRef arena_ref;
3635
EXPECT_EQ(arena_ref.get(env).get(), nullptr);
3736
}
3837

3938
TEST_F(ArenaRefTestAndroid, ConstructFromEnvAndObject) {
40-
Env env = FirestoreInternal::GetEnv();
39+
Env env;
4140
Local<String> string = env.NewStringUtf("hello world");
4241

4342
ArenaRef arena_ref(env, string);
4443
EXPECT_TRUE(arena_ref.get(env).Equals(env, string));
4544
}
4645

4746
TEST_F(ArenaRefTestAndroid, CopysReferenceToNull) {
48-
Env env = FirestoreInternal::GetEnv();
47+
Env env;
4948

5049
ArenaRef arena_ref1;
5150
ArenaRef arena_ref2(arena_ref1);
5251
EXPECT_EQ(arena_ref2.get(env).get(), nullptr);
5352
}
5453

5554
TEST_F(ArenaRefTestAndroid, CopysReferenceToValidObject) {
56-
Env env = FirestoreInternal::GetEnv();
55+
Env env;
5756

5857
Local<String> string = env.NewStringUtf("hello world");
5958

@@ -65,7 +64,7 @@ TEST_F(ArenaRefTestAndroid, CopysReferenceToValidObject) {
6564
}
6665

6766
TEST_F(ArenaRefTestAndroid, CopyAssignsReferenceToNull) {
68-
Env env = FirestoreInternal::GetEnv();
67+
Env env;
6968

7069
ArenaRef arena_ref1, arena_ref2;
7170
arena_ref2 = arena_ref1;
@@ -74,7 +73,7 @@ TEST_F(ArenaRefTestAndroid, CopyAssignsReferenceToNull) {
7473
}
7574

7675
TEST_F(ArenaRefTestAndroid, CopyAssignsReferenceToValidObject) {
77-
Env env = FirestoreInternal::GetEnv();
76+
Env env;
7877

7978
Local<String> string1 = env.NewStringUtf("hello world");
8079
Local<String> string2 = env.NewStringUtf("hello earth");
@@ -94,36 +93,33 @@ TEST_F(ArenaRefTestAndroid, CopyAssignsReferenceToValidObject) {
9493
}
9594

9695
TEST_F(ArenaRefTestAndroid, MovesReferenceToNull) {
97-
Env env = FirestoreInternal::GetEnv();
96+
Env env;
9897

9998
ArenaRef arena_ref1;
10099
ArenaRef arena_ref2(std::move(arena_ref1));
101-
EXPECT_EQ(arena_ref1.get(env).get(), nullptr);
102100
EXPECT_EQ(arena_ref2.get(env).get(), nullptr);
103101
}
104102

105103
TEST_F(ArenaRefTestAndroid, MovesReferenceToValidObject) {
106-
Env env = FirestoreInternal::GetEnv();
104+
Env env;
107105

108106
Local<String> string = env.NewStringUtf("hello world");
109107

110108
ArenaRef arena_ref2(env, string);
111109
ArenaRef arena_ref3(std::move(arena_ref2));
112-
EXPECT_EQ(arena_ref2.get(env).get(), nullptr);
113110
EXPECT_TRUE(arena_ref3.get(env).Equals(env, string));
114111
}
115112

116113
TEST_F(ArenaRefTestAndroid, MoveAssignsReferenceToNull) {
117-
Env env = FirestoreInternal::GetEnv();
114+
Env env;
118115

119116
ArenaRef arena_ref1, arena_ref2;
120117
arena_ref2 = std::move(arena_ref1);
121-
EXPECT_EQ(arena_ref1.get(env).get(), nullptr);
122118
EXPECT_EQ(arena_ref2.get(env).get(), nullptr);
123119
}
124120

125121
TEST_F(ArenaRefTestAndroid, MoveAssignsReferenceToValidObject) {
126-
Env env = FirestoreInternal::GetEnv();
122+
Env env;
127123

128124
Local<String> string1 = env.NewStringUtf("hello world");
129125
Local<String> string2 = env.NewStringUtf("hello earth");
@@ -135,7 +131,6 @@ TEST_F(ArenaRefTestAndroid, MoveAssignsReferenceToValidObject) {
135131

136132
ArenaRef arena_ref3(env, string2);
137133
arena_ref3 = std::move(arena_ref2);
138-
EXPECT_EQ(arena_ref2.get(env).get(), nullptr);
139134
EXPECT_TRUE(arena_ref3.get(env).Equals(env, string1));
140135

141136
arena_ref3 = std::move(arena_ref1);

‎firestore/integration_test_internal/src/jni/env_test.cc

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,18 @@ TEST_F(EnvTest, CallsVoidMethods) {
128128
EXPECT_EQ(length, 42);
129129
}
130130

131+
TEST_F(EnvTest, CallsValidArenaRef) {
132+
Local<String> str = env().NewStringUtf("Foo");
133+
ArenaRef arena_ref(env(), str);
134+
135+
Local<Class> clazz = env().FindClass("java/lang/String");
136+
jmethodID to_lower_case =
137+
env().GetMethodId(clazz, "toLowerCase", "()Ljava/lang/String;");
138+
139+
Local<Object> result = env().Call(arena_ref, Method<Object>(to_lower_case));
140+
EXPECT_EQ("foo", result.ToString(env()));
141+
}
142+
131143
TEST_F(EnvTest, GetsStaticFields) {
132144
Local<Class> clazz = env().FindClass("java/lang/String");
133145
jfieldID comparator = env().GetStaticFieldId(clazz, "CASE_INSENSITIVE_ORDER",
@@ -160,6 +172,13 @@ TEST_F(EnvTest, ToString) {
160172
EXPECT_EQ("Foo", result);
161173
}
162174

175+
TEST_F(EnvTest, IsInstanceOfChecksValidArenaRef) {
176+
Local<Class> clazz = env().FindClass("java/lang/String");
177+
Local<String> str = env().NewStringUtf("Foo");
178+
ArenaRef arena_ref(env(), str);
179+
EXPECT_TRUE(env().IsInstanceOf(arena_ref, clazz));
180+
}
181+
163182
TEST_F(EnvTest, Throw) {
164183
Local<Class> clazz = env().FindClass("java/lang/Exception");
165184
jmethodID ctor = env().GetMethodId(clazz, "<init>", "(Ljava/lang/String;)V");

‎firestore/src/jni/arena_ref.cc

Lines changed: 37 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,6 @@ namespace jni {
3232
namespace {
3333

3434
HashMap* gArenaRefHashMap = nullptr;
35-
jclass gHashMapClass = nullptr;
3635
jclass gLongClass = nullptr;
3736
jmethodID gLongConstructor = nullptr;
3837
std::mutex mutex_;
@@ -46,80 +45,77 @@ int64_t GetNextArenaRefKey() {
4645

4746
ArenaRef::ArenaRef(Env& env, const Object& object)
4847
: key_(GetNextArenaRefKey()) {
49-
std::unique_lock<std::mutex> lock(mutex_);
50-
gArenaRefHashMap->Put(env, key_object(env), object);
48+
Local<Long> key_ref = key_object(env);
49+
std::lock_guard<std::mutex> lock(mutex_);
50+
gArenaRefHashMap->Put(env, key_ref, object);
5151
}
5252

5353
ArenaRef::ArenaRef(const ArenaRef& other)
54-
: key_(other.key_ == -1 ? -1 : GetNextArenaRefKey()) {
55-
if (other.key_ != -1) {
54+
: key_(other.key_ == kInvalidKey ? kInvalidKey : GetNextArenaRefKey()) {
55+
if (other.key_ != kInvalidKey) {
5656
Env env = FirestoreInternal::GetEnv();
5757
Local<Object> object = other.get(env);
58-
std::unique_lock<std::mutex> lock(mutex_);
59-
gArenaRefHashMap->Put(env, key_object(env), object);
58+
Local<Long> key_ref = key_object(env);
59+
std::lock_guard<std::mutex> lock(mutex_);
60+
gArenaRefHashMap->Put(env, key_ref, object);
6061
}
6162
}
6263

6364
ArenaRef::ArenaRef(ArenaRef&& other) { std::swap(key_, other.key_); }
6465

6566
ArenaRef& ArenaRef::operator=(const ArenaRef& other) {
66-
Env env = FirestoreInternal::GetEnv();
67-
68-
if (this == &other) {
67+
if (this == &other || (key_ == kInvalidKey && other.key_ == kInvalidKey)) {
6968
return *this;
7069
}
7170

72-
{
73-
std::unique_lock<std::mutex> lock(mutex_);
74-
if (key_ != -1) {
75-
gArenaRefHashMap->Remove(env, key_object(env));
76-
key_ = -1;
77-
}
78-
79-
if (other.key_ != -1) {
71+
Env env = FirestoreInternal::GetEnv();
72+
if (other.key_ != kInvalidKey) {
73+
if (key_ == kInvalidKey) {
8074
key_ = GetNextArenaRefKey();
81-
Local<Object> object = other.get(env);
82-
gArenaRefHashMap->Put(env, key_object(env), object);
8375
}
76+
Local<Object> object = other.get(env);
77+
std::lock_guard<std::mutex> lock(mutex_);
78+
gArenaRefHashMap->Put(env, key_object(env), object);
79+
} else {
80+
{
81+
std::lock_guard<std::mutex> lock(mutex_);
82+
gArenaRefHashMap->Remove(env, key_object(env));
83+
}
84+
key_ = kInvalidKey;
8485
}
8586
return *this;
8687
}
8788

8889
ArenaRef& ArenaRef::operator=(ArenaRef&& other) {
89-
Env env = FirestoreInternal::GetEnv();
90-
91-
if (this == &other) {
92-
return *this;
93-
}
94-
95-
{
96-
std::unique_lock<std::mutex> lock(mutex_);
97-
if (key_ != -1) {
98-
gArenaRefHashMap->Remove(env, key_object(env));
99-
}
100-
key_ = other.key_;
101-
other.key_ = -1;
90+
if (this != &other) {
91+
std::swap(key_, other.key_);
10292
}
10393
return *this;
10494
}
10595

10696
ArenaRef::~ArenaRef() {
107-
if (key_ != -1) {
97+
if (key_ != kInvalidKey) {
10898
Env env;
10999
ExceptionClearGuard block(env);
110100
{
111-
std::unique_lock<std::mutex> lock(mutex_);
101+
std::lock_guard<std::mutex> lock(mutex_);
112102
gArenaRefHashMap->Remove(env, key_object(env));
113103
}
114104
if (!env.ok()) {
115-
env.ExceptionDescribe();
105+
env.RecordException();
116106
env.ExceptionClear();
117107
}
118108
}
119109
}
120110

121111
Local<Long> ArenaRef::key_object(Env& env) const {
112+
if (!env.ok()) return {};
113+
// The reason why using raw JNI calls here rather than the JNI convenience
114+
// layer is because we need to get rid of JNI convenience layer dependencies
115+
// in the ArenaRef destructor to avoid racing condition when firestore object
116+
// gets destroyed.
122117
jobject key = env.get()->NewObject(gLongClass, gLongConstructor, key_);
118+
env.RecordException();
123119
return {env.get(), key};
124120
}
125121

@@ -130,12 +126,12 @@ void ArenaRef::Initialize(Env& env) {
130126
Global<HashMap> hash_map(HashMap::Create(env));
131127
gArenaRefHashMap = new HashMap(hash_map.release());
132128

133-
gHashMapClass = env.get()->FindClass("java/util/HashMap");
134-
gHashMapClass = static_cast<jclass>(env.get()->NewGlobalRef(gHashMapClass));
135-
136-
gLongClass = env.get()->FindClass("java/lang/Long");
137-
gLongClass = static_cast<jclass>(env.get()->NewGlobalRef(gLongClass));
129+
auto long_class_local = env.get()->FindClass("java/lang/Long");
130+
gLongClass = static_cast<jclass>(env.get()->NewGlobalRef(long_class_local));
131+
env.get()->DeleteLocalRef(long_class_local);
138132
gLongConstructor = env.get()->GetMethodID(gLongClass, "<init>", "(J)V");
133+
134+
if (!env.ok()) return;
139135
}
140136

141137
Local<Object> ArenaRef::get(Env& env) const {

0 commit comments

Comments
 (0)