Skip to content

Commit 6d40d04

Browse files
var-consta-maurice
authored andcommitted
Fix Android crashing when a FieldValue is initialized with an invalid DocumentReference.
Also make sure that Android doesn't crash when an array or a map used to initialize a `FieldValue` contains invalid `FieldValue`s. Instead, store them as nulls; it's somewhat incorrect (other platforms distinguish between null `FieldValue`s and invalid `FieldValue`s) but preferable to crashing. PiperOrigin-RevId: 297200951
1 parent 23b19ee commit 6d40d04

File tree

4 files changed

+30
-8
lines changed

4 files changed

+30
-8
lines changed

firestore/src/android/field_value_android.cc

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -112,8 +112,7 @@ FieldValueInternal::FieldValueInternal(const uint8_t* value, size_t size)
112112
}
113113

114114
FieldValueInternal::FieldValueInternal(DocumentReference value)
115-
: Wrapper(firebase::Move(*value.internal_)),
116-
cached_type_(Type::kReference) {}
115+
: Wrapper(value.internal_), cached_type_{Type::kReference} {}
117116

118117
FieldValueInternal::FieldValueInternal(GeoPoint value)
119118
: cached_type_(Type::kGeoPoint) {
@@ -133,7 +132,8 @@ FieldValueInternal::FieldValueInternal(std::vector<FieldValue> value)
133132
jmethodID add_method = util::array_list::GetMethodId(util::array_list::kAdd);
134133
for (const FieldValue& element : value) {
135134
// ArrayList.Add() always returns true, which we have no use for.
136-
env->CallBooleanMethod(obj_, add_method, element.internal_->obj_);
135+
// TODO(b/150016438): don't conflate invalid `FieldValue`s and null.
136+
env->CallBooleanMethod(obj_, add_method, TryGetJobject(element));
137137
}
138138
CheckAndClearJniExceptions(env);
139139
}
@@ -148,7 +148,8 @@ FieldValueInternal::FieldValueInternal(MapFieldValue value)
148148
jobject key = env->NewStringUTF(kv.first.c_str());
149149
// Map::Put() returns previously associated value or null, which we have no
150150
// use for.
151-
env->CallObjectMethod(obj_, put_method, key, kv.second.internal_->obj_);
151+
// TODO(b/150016438): don't conflate invalid `FieldValue`s and null.
152+
env->CallObjectMethod(obj_, put_method, key, TryGetJobject(kv.second));
152153
env->DeleteLocalRef(key);
153154
}
154155
CheckAndClearJniExceptions(env);
@@ -345,6 +346,10 @@ DocumentReference FieldValueInternal::reference_value() const {
345346
FIREBASE_ASSERT(cached_type_ == Type::kReference);
346347
}
347348

349+
if (!obj_) {
350+
// Reference may be invalid.
351+
return DocumentReference{};
352+
}
348353
return DocumentReference{new DocumentReferenceInternal(firestore_, obj_)};
349354
}
350355

@@ -585,5 +590,9 @@ bool operator==(const FieldValueInternal& lhs, const FieldValueInternal& rhs) {
585590
return lhs.EqualsJavaObject(rhs);
586591
}
587592

593+
jobject FieldValueInternal::TryGetJobject(const FieldValue& value) {
594+
return value.internal_ ? value.internal_->obj_ : nullptr;
595+
}
596+
588597
} // namespace firestore
589598
} // namespace firebase

firestore/src/android/field_value_android.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,8 @@ class FieldValueInternal : public Wrapper {
7070
static bool Initialize(App* app);
7171
static void Terminate(App* app);
7272

73+
static jobject TryGetJobject(const FieldValue& value);
74+
7375
static jobject delete_;
7476
static jobject server_timestamp_;
7577

firestore/src/android/wrapper.cc

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ Wrapper::Wrapper(jclass clazz, jmethodID method_id, ...) {
4747
Wrapper::Wrapper(const Wrapper& wrapper)
4848
: Wrapper(wrapper.firestore_, wrapper.obj_, AllowNullObject::Yes) {}
4949

50-
Wrapper::Wrapper(Wrapper&& wrapper)
50+
Wrapper::Wrapper(Wrapper&& wrapper) noexcept
5151
: firestore_(wrapper.firestore_), obj_(wrapper.obj_) {
5252
FIREBASE_ASSERT(firestore_ != nullptr);
5353
wrapper.obj_ = nullptr;
@@ -60,6 +60,14 @@ Wrapper::Wrapper() : obj_(nullptr) {
6060
FIREBASE_ASSERT(firestore_ != nullptr);
6161
}
6262

63+
Wrapper::Wrapper(Wrapper* rhs) : Wrapper() {
64+
if (rhs) {
65+
firestore_ = rhs->firestore_;
66+
FIREBASE_ASSERT(firestore_ != nullptr);
67+
obj_ = firestore_->app()->GetJNIEnv()->NewGlobalRef(rhs->obj_);
68+
}
69+
}
70+
6371
Wrapper::Wrapper(FirestoreInternal* firestore, jobject obj, AllowNullObject)
6472
: firestore_(firestore),
6573
// NewGlobalRef() is supposed to accept Null Java object and return Null,

firestore/src/android/wrapper.h

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ class Wrapper {
3737

3838
Wrapper(const Wrapper& wrapper);
3939

40-
Wrapper(Wrapper&& wrapper);
40+
Wrapper(Wrapper&& wrapper) noexcept;
4141

4242
virtual ~Wrapper();
4343

@@ -62,6 +62,9 @@ class Wrapper {
6262
// Java object a.k.a. nullptr.
6363
Wrapper(FirestoreInternal* firestore, jobject obj, AllowNullObject);
6464

65+
// Similar to a copy constructor, but can handle the case where `rhs` is null.
66+
explicit Wrapper(Wrapper* rhs);
67+
6568
// Converts a java list of java type e.g. java.util.List<FirestoreJavaType> to
6669
// a C++ vector of equivalent type e.g. std::vector<FirestoreType>.
6770
template <typename FirestoreType, typename FirestoreTypeInternal>
@@ -100,8 +103,8 @@ class Wrapper {
100103
FirestoreInternal* firestore, MapFieldPathValue::const_iterator begin,
101104
MapFieldPathValue::const_iterator end);
102105

103-
FirestoreInternal* firestore_; // not owning
104-
jobject obj_;
106+
FirestoreInternal* firestore_ = nullptr; // not owning
107+
jobject obj_ = nullptr;
105108

106109
private:
107110
friend class FirestoreInternal;

0 commit comments

Comments
 (0)