Skip to content

Commit e7879c5

Browse files
cherylEnkiducherylEnkidu
committed
Address feedback again
1 parent b599bfa commit e7879c5

File tree

4 files changed

+215
-72
lines changed

4 files changed

+215
-72
lines changed

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

Lines changed: 200 additions & 65 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,8 @@
1616

1717
#include "firestore/src/jni/arena_ref.h"
1818
#include "firestore/src/android/firestore_android.h"
19+
#include "firestore/src/common/make_unique.h"
20+
#include "firestore/src/jni/loader.h"
1921
#include "firestore/src/jni/long.h"
2022

2123
#include "firestore_integration_test.h"
@@ -27,114 +29,247 @@ namespace firestore {
2729
namespace jni {
2830
namespace {
2931

30-
using ArenaRefTestAndroid = FirestoreIntegrationTest;
31-
32-
TEST_F(ArenaRefTestAndroid, DefaultConstructorCreatesReferenceToNull) {
33-
Env env;
32+
constexpr char kException[] = "java/lang/Exception";
33+
Method<String> kConstructor("<init>", "(Ljava/lang/String;)V");
34+
35+
class ArenaRefTestAndroid : public FirestoreIntegrationTest {
36+
public:
37+
ArenaRefTestAndroid() : loader_(app()), env_(make_unique<Env>(GetEnv())) {
38+
loader_.LoadClass(kException);
39+
loader_.Load(kConstructor);
40+
}
41+
42+
~ArenaRefTestAndroid() override {
43+
// Ensure that after the test is done that any pending exception is cleared
44+
// to prevent spurious errors in the teardown of FirestoreIntegrationTest.
45+
env_->ExceptionClear();
46+
}
47+
48+
Env& env() const { return *env_; }
49+
50+
void throwException() {
51+
Local<Class> clazz = env().FindClass(kException);
52+
jmethodID ctor =
53+
env().GetMethodId(clazz, "<init>", "(Ljava/lang/String;)V");
54+
55+
Local<String> message = env().NewStringUtf("Testing throw");
56+
Local<Throwable> exception = env().New<Throwable>(clazz, ctor, message);
57+
// After throwing, use EXPECT rather than ASSERT to ensure that the
58+
// exception is cleared.
59+
env().Throw(exception);
60+
EXPECT_FALSE(env().ok());
61+
}
62+
63+
void clearExceptionOccurred() {
64+
Local<Throwable> thrown = env().ClearExceptionOccurred();
65+
EXPECT_EQ(thrown.GetMessage(env()), "Testing throw");
66+
}
67+
68+
private:
69+
// Env is declared as having a `noexcept(false)` destructor, which causes the
70+
// compiler to propagate this into `EnvTest`'s destructor, but this conflicts
71+
// with the declaration of the parent class. Holding `Env` with a unique
72+
// pointer sidesteps this restriction.
73+
std::unique_ptr<Env> env_;
74+
Loader loader_;
75+
};
76+
77+
TEST_F(ArenaRefTestAndroid, DefaultConstructor) {
3478
ArenaRef arena_ref;
35-
EXPECT_EQ(arena_ref.get(env).get(), nullptr);
79+
EXPECT_EQ(arena_ref.get(env()).get(), nullptr);
3680
}
3781

38-
TEST_F(ArenaRefTestAndroid, ConstructFromEnvAndObject) {
39-
Env env;
40-
Local<String> string = env.NewStringUtf("hello world");
41-
42-
ArenaRef arena_ref(env, string);
43-
EXPECT_TRUE(arena_ref.get(env).Equals(env, string));
82+
TEST_F(ArenaRefTestAndroid, ConstructsFromNull) {
83+
Local<String> string;
84+
ArenaRef arena_ref(env(), string);
85+
EXPECT_EQ(arena_ref.get(env()).get(), nullptr);
4486
}
4587

46-
TEST_F(ArenaRefTestAndroid, CopysReferenceToNull) {
47-
Env env;
88+
TEST_F(ArenaRefTestAndroid, ConstructsFromValid) {
89+
Local<String> string = env().NewStringUtf("hello world");
90+
ArenaRef arena_ref(env(), string);
91+
EXPECT_TRUE(env().IsSameObject(arena_ref.get(env()), string));
92+
}
4893

94+
TEST_F(ArenaRefTestAndroid, CopyConstructsFromNull) {
4995
ArenaRef arena_ref1;
5096
ArenaRef arena_ref2(arena_ref1);
51-
EXPECT_EQ(arena_ref2.get(env).get(), nullptr);
97+
EXPECT_EQ(arena_ref2.get(env()).get(), nullptr);
5298
}
5399

54-
TEST_F(ArenaRefTestAndroid, CopysReferenceToValidObject) {
55-
Env env;
100+
TEST_F(ArenaRefTestAndroid, CopyConstructsFromValid) {
101+
Local<String> string = env().NewStringUtf("hello world");
56102

57-
Local<String> string = env.NewStringUtf("hello world");
58-
59-
ArenaRef arena_ref1(env, string);
103+
ArenaRef arena_ref1(env(), string);
60104
ArenaRef arena_ref2(arena_ref1);
61-
62-
EXPECT_TRUE(arena_ref1.get(env).Equals(env, string));
63-
EXPECT_TRUE(arena_ref2.get(env).Equals(env, string));
105+
EXPECT_TRUE(env().IsSameObject(arena_ref1.get(env()), string));
106+
EXPECT_TRUE(env().IsSameObject(arena_ref2.get(env()), string));
64107
}
65108

66-
TEST_F(ArenaRefTestAndroid, CopyAssignsReferenceToNull) {
67-
Env env;
68-
109+
TEST_F(ArenaRefTestAndroid, CopyAssignsFromNullToNull) {
69110
ArenaRef arena_ref1, arena_ref2;
70111
arena_ref2 = arena_ref1;
71-
EXPECT_EQ(arena_ref1.get(env).get(), nullptr);
72-
EXPECT_EQ(arena_ref2.get(env).get(), nullptr);
112+
EXPECT_EQ(arena_ref1.get(env()).get(), nullptr);
113+
EXPECT_EQ(arena_ref2.get(env()).get(), nullptr);
73114
}
74115

75-
TEST_F(ArenaRefTestAndroid, CopyAssignsReferenceToValidObject) {
76-
Env env;
116+
TEST_F(ArenaRefTestAndroid, CopyAssignsFromNullToValid) {
117+
Local<String> string = env().NewStringUtf("hello world");
77118

78-
Local<String> string1 = env.NewStringUtf("hello world");
79-
Local<String> string2 = env.NewStringUtf("hello earth");
119+
ArenaRef arena_ref1;
120+
ArenaRef arena_ref2(env(), string);
121+
arena_ref2 = arena_ref1;
122+
EXPECT_EQ(arena_ref1.get(env()).get(), nullptr);
123+
EXPECT_EQ(arena_ref2.get(env()).get(), nullptr);
124+
}
125+
126+
TEST_F(ArenaRefTestAndroid, CopyAssignsFromValidToNull) {
127+
Local<String> string = env().NewStringUtf("hello world");
80128

81129
ArenaRef arena_ref1;
82-
ArenaRef arena_ref2(env, string1);
83-
ArenaRef arena_ref3(env, string2);
84-
arena_ref3 = arena_ref2;
85-
arena_ref2 = arena_ref2;
130+
ArenaRef arena_ref2(env(), string);
131+
arena_ref1 = arena_ref2;
132+
EXPECT_TRUE(env().IsSameObject(arena_ref1.get(env()), string));
133+
EXPECT_TRUE(env().IsSameObject(arena_ref2.get(env()), string));
134+
}
86135

87-
EXPECT_TRUE(arena_ref3.get(env).Equals(env, string1));
88-
EXPECT_TRUE(arena_ref2.get(env).Equals(env, string1));
136+
TEST_F(ArenaRefTestAndroid, CopyAssignsFromValidToValid) {
137+
Local<String> string1 = env().NewStringUtf("hello world");
138+
Local<String> string2 = env().NewStringUtf("hello earth");
89139

90-
arena_ref2 = arena_ref1;
91-
EXPECT_EQ(arena_ref2.get(env).get(), nullptr);
92-
EXPECT_TRUE(arena_ref3.get(env).Equals(env, string1));
140+
ArenaRef arena_ref1(env(), string1);
141+
ArenaRef arena_ref2(env(), string2);
142+
arena_ref1 = arena_ref2;
143+
144+
EXPECT_TRUE(env().IsSameObject(arena_ref1.get(env()), string2));
145+
EXPECT_TRUE(env().IsSameObject(arena_ref2.get(env()), string2));
146+
}
147+
148+
TEST_F(ArenaRefTestAndroid, CopyAssignsFromNullObjectItself) {
149+
ArenaRef arena_ref1;
150+
arena_ref1 = arena_ref1;
151+
EXPECT_EQ(arena_ref1.get(env()).get(), nullptr);
93152
}
94153

95-
TEST_F(ArenaRefTestAndroid, MovesReferenceToNull) {
96-
Env env;
154+
TEST_F(ArenaRefTestAndroid, CopyAssignsFromValidObjectItself) {
155+
Local<String> string1 = env().NewStringUtf("hello world");
156+
157+
ArenaRef arena_ref1(env(), string1);
158+
arena_ref1 = arena_ref1;
159+
EXPECT_TRUE(env().IsSameObject(arena_ref1.get(env()), string1));
160+
}
97161

162+
TEST_F(ArenaRefTestAndroid, MoveConstructsFromNull) {
98163
ArenaRef arena_ref1;
99164
ArenaRef arena_ref2(std::move(arena_ref1));
100-
EXPECT_EQ(arena_ref2.get(env).get(), nullptr);
165+
EXPECT_EQ(arena_ref2.get(env()).get(), nullptr);
101166
}
102167

103-
TEST_F(ArenaRefTestAndroid, MovesReferenceToValidObject) {
104-
Env env;
168+
TEST_F(ArenaRefTestAndroid, MoveConstructsFromValid) {
169+
Local<String> string = env().NewStringUtf("hello world");
105170

106-
Local<String> string = env.NewStringUtf("hello world");
107-
108-
ArenaRef arena_ref2(env, string);
171+
ArenaRef arena_ref2(env(), string);
109172
ArenaRef arena_ref3(std::move(arena_ref2));
110-
EXPECT_TRUE(arena_ref3.get(env).Equals(env, string));
173+
EXPECT_TRUE(env().IsSameObject(arena_ref3.get(env()), string));
111174
}
112175

113-
TEST_F(ArenaRefTestAndroid, MoveAssignsReferenceToNull) {
114-
Env env;
115-
176+
TEST_F(ArenaRefTestAndroid, MoveAssignsFromNullToNull) {
116177
ArenaRef arena_ref1, arena_ref2;
117178
arena_ref2 = std::move(arena_ref1);
118-
EXPECT_EQ(arena_ref2.get(env).get(), nullptr);
179+
EXPECT_EQ(arena_ref2.get(env()).get(), nullptr);
119180
}
120181

121-
TEST_F(ArenaRefTestAndroid, MoveAssignsReferenceToValidObject) {
122-
Env env;
182+
TEST_F(ArenaRefTestAndroid, MoveAssignsFromNullToValid) {
183+
Local<String> string = env().NewStringUtf("hello world");
123184

124-
Local<String> string1 = env.NewStringUtf("hello world");
125-
Local<String> string2 = env.NewStringUtf("hello earth");
185+
ArenaRef arena_ref1;
186+
ArenaRef arena_ref2(env(), string);
187+
arena_ref2 = std::move(arena_ref1);
188+
EXPECT_EQ(arena_ref2.get(env()).get(), nullptr);
189+
}
190+
191+
TEST_F(ArenaRefTestAndroid, MoveAssignsFromValidToNull) {
192+
Local<String> string = env().NewStringUtf("hello world");
126193

127194
ArenaRef arena_ref1;
128-
ArenaRef arena_ref2(env, string1);
129-
arena_ref2 = std::move(arena_ref2);
130-
EXPECT_TRUE(arena_ref2.get(env).Equals(env, string1));
195+
ArenaRef arena_ref2(env(), string);
196+
arena_ref1 = std::move(arena_ref2);
197+
EXPECT_TRUE(env().IsSameObject(arena_ref1.get(env()), string));
198+
}
199+
200+
TEST_F(ArenaRefTestAndroid, MoveAssignsFromValidToValid) {
201+
Local<String> string1 = env().NewStringUtf("hello world");
202+
Local<String> string2 = env().NewStringUtf("hello earth");
203+
204+
ArenaRef arena_ref1(env(), string1);
205+
ArenaRef arena_ref2(env(), string2);
206+
arena_ref1 = std::move(arena_ref2);
207+
EXPECT_TRUE(env().IsSameObject(arena_ref1.get(env()), string2));
208+
}
209+
210+
TEST_F(ArenaRefTestAndroid, MoveAssignsFromNullObjectItself) {
211+
ArenaRef arena_ref1;
212+
arena_ref1 = std::move(arena_ref1);
213+
EXPECT_EQ(arena_ref1.get(env()).get(), nullptr);
214+
}
215+
216+
TEST_F(ArenaRefTestAndroid, MoveAssignsFromValidObjectItself) {
217+
Local<String> string1 = env().NewStringUtf("hello world");
218+
219+
ArenaRef arena_ref1(env(), string1);
220+
arena_ref1 = std::move(arena_ref1);
221+
EXPECT_TRUE(env().IsSameObject(arena_ref1.get(env()), string1));
222+
}
131223

132-
ArenaRef arena_ref3(env, string2);
133-
arena_ref3 = std::move(arena_ref2);
134-
EXPECT_TRUE(arena_ref3.get(env).Equals(env, string1));
224+
TEST_F(ArenaRefTestAndroid, ThrowBeforeConstruct) {
225+
Local<String> string = env().NewStringUtf("hello world");
226+
EXPECT_EQ(string.ToString(env()).size(), 11U);
227+
throwException();
228+
ArenaRef arena_ref(env(), string);
229+
clearExceptionOccurred();
230+
EXPECT_EQ(arena_ref.get(env()).get(), nullptr);
231+
}
232+
233+
TEST_F(ArenaRefTestAndroid, ThrowBeforeCopyConstruct) {
234+
Local<String> string = env().NewStringUtf("hello world");
235+
ArenaRef arena_ref1(env(), string);
236+
EXPECT_EQ(arena_ref1.get(env()).ToString(env()).size(), 11U);
237+
throwException();
238+
ArenaRef arena_ref2(arena_ref1);
239+
clearExceptionOccurred();
240+
EXPECT_EQ(arena_ref2.get(env()).get(), nullptr);
241+
}
135242

136-
arena_ref3 = std::move(arena_ref1);
137-
EXPECT_EQ(arena_ref3.get(env).get(), nullptr);
243+
TEST_F(ArenaRefTestAndroid, ThrowBeforeCopyAssignment) {
244+
Local<String> string = env().NewStringUtf("hello world");
245+
ArenaRef arena_ref1(env(), string);
246+
ArenaRef arena_ref2;
247+
EXPECT_EQ(arena_ref1.get(env()).ToString(env()).size(), 11U);
248+
throwException();
249+
arena_ref2 = arena_ref1;
250+
clearExceptionOccurred();
251+
EXPECT_EQ(arena_ref2.get(env()).get(), nullptr);
252+
}
253+
254+
TEST_F(ArenaRefTestAndroid, ThrowBeforeMoveConstruct) {
255+
Local<String> string = env().NewStringUtf("hello world");
256+
ArenaRef arena_ref1(env(), string);
257+
EXPECT_EQ(arena_ref1.get(env()).ToString(env()).size(), 11U);
258+
throwException();
259+
ArenaRef arena_ref2(std::move(arena_ref1));
260+
clearExceptionOccurred();
261+
EXPECT_TRUE(env().IsSameObject(arena_ref2.get(env()), string));
262+
}
263+
264+
TEST_F(ArenaRefTestAndroid, ThrowBeforeMoveAssignment) {
265+
Local<String> string = env().NewStringUtf("hello world");
266+
ArenaRef arena_ref1(env(), string);
267+
ArenaRef arena_ref2;
268+
EXPECT_EQ(arena_ref1.get(env()).ToString(env()).size(), 11U);
269+
throwException();
270+
arena_ref2 = std::move(arena_ref1);
271+
clearExceptionOccurred();
272+
EXPECT_TRUE(env().IsSameObject(arena_ref2.get(env()), string));
138273
}
139274

140275
} // namespace

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

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -179,6 +179,14 @@ TEST_F(EnvTest, IsInstanceOfChecksValidArenaRef) {
179179
EXPECT_TRUE(env().IsInstanceOf(arena_ref, clazz));
180180
}
181181

182+
TEST_F(EnvTest, IsInstanceOfChecksNullArenaRef) {
183+
GTEST_SKIP()
184+
<< "Skip until edge case of IsInstanceOf (b/268420201) is covered.";
185+
Local<Class> clazz = env().FindClass("java/lang/String");
186+
ArenaRef arena_ref;
187+
EXPECT_FALSE(env().IsInstanceOf(arena_ref, clazz));
188+
}
189+
182190
TEST_F(EnvTest, Throw) {
183191
Local<Class> clazz = env().FindClass("java/lang/Exception");
184192
jmethodID ctor = env().GetMethodId(clazz, "<init>", "(Ljava/lang/String;)V");

‎firestore/src/jni/arena_ref.cc

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -53,22 +53,25 @@ ArenaRef::ArenaRef(Env& env, const Object& object)
5353
ArenaRef::ArenaRef(const ArenaRef& other)
5454
: key_(other.key_ == kInvalidKey ? kInvalidKey : GetNextArenaRefKey()) {
5555
if (other.key_ != kInvalidKey) {
56-
Env env = FirestoreInternal::GetEnv();
56+
Env env;
5757
Local<Object> object = other.get(env);
5858
Local<Long> key_ref = key_object(env);
5959
std::lock_guard<std::mutex> lock(mutex_);
6060
gArenaRefHashMap->Put(env, key_ref, object);
6161
}
6262
}
6363

64-
ArenaRef::ArenaRef(ArenaRef&& other) { std::swap(key_, other.key_); }
64+
ArenaRef::ArenaRef(ArenaRef&& other) {
65+
using std::swap; // go/using-std-swap
66+
swap(key_, other.key_);
67+
}
6568

6669
ArenaRef& ArenaRef::operator=(const ArenaRef& other) {
6770
if (this == &other || (key_ == kInvalidKey && other.key_ == kInvalidKey)) {
6871
return *this;
6972
}
7073

71-
Env env = FirestoreInternal::GetEnv();
74+
Env env;
7275
if (other.key_ != kInvalidKey) {
7376
if (key_ == kInvalidKey) {
7477
key_ = GetNextArenaRefKey();

‎firestore/src/jni/arena_ref.h

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -29,10 +29,6 @@ namespace firebase {
2929
namespace firestore {
3030
namespace jni {
3131

32-
namespace {
33-
constexpr int64_t kInvalidKey = -1;
34-
}
35-
3632
/**
3733
* ArenaRef is an RAII wrapper which serves the same purpose as Global<T>, both
3834
* of them manage the lifetime of JNI reference. Compared to Global<T>, the
@@ -59,6 +55,7 @@ class ArenaRef {
5955
private:
6056
Local<Long> key_object(Env&) const;
6157

58+
static constexpr int64_t kInvalidKey = -1;
6259
int64_t key_ = kInvalidKey;
6360
};
6461

0 commit comments

Comments
 (0)