Skip to content

Commit b050c27

Browse files
dconeybedconeybe
authored
Fix ReferenceCountedFutureImpl to avoid using FirestoreInternal after its deletion. (#585)
1 parent e61d0b8 commit b050c27

File tree

8 files changed

+364
-95
lines changed

8 files changed

+364
-95
lines changed

‎firestore/integration_test_internal/CMakeLists.txt

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -110,6 +110,7 @@ set(FIREBASE_INTEGRATION_TEST_ANDROID_TEST_SRCS
110110
src/android/geo_point_android_test.cc
111111
src/android/jni_runnable_android_test.cc
112112
src/android/promise_android_test.cc
113+
src/android/promise_factory_android_test.cc
113114
src/android/settings_android_test.cc
114115
src/android/snapshot_metadata_android_test.cc
115116
src/android/timestamp_android_test.cc

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

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -403,6 +403,24 @@ TEST_F(PromiseTest, FutureNonVoidShouldCallCompletionWhenTaskCancels) {
403403
EXPECT_EQ(completion.result(), nullptr);
404404
}
405405

406+
TEST_F(PromiseTest, RegisterForTaskShouldNotCrashIfFirestoreWasDeleted) {
407+
jni::Env env = GetEnv();
408+
auto promise = promises().MakePromise<void>();
409+
DeleteFirestore(TestFirestore());
410+
411+
promise.RegisterForTask(env, AsyncFn::kFn, GetTask());
412+
}
413+
414+
TEST_F(PromiseTest, GetFutureShouldNotCrashIfFirestoreWasDeleted) {
415+
jni::Env env = GetEnv();
416+
auto promise = promises().MakePromise<void>();
417+
promise.RegisterForTask(env, AsyncFn::kFn, GetTask());
418+
DeleteFirestore(TestFirestore());
419+
420+
auto future = promise.GetFuture();
421+
EXPECT_EQ(future.status(), FutureStatus::kFutureStatusInvalid);
422+
}
423+
406424
} // namespace
407425
} // namespace firestore
408426
} // namespace firebase

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

Lines changed: 132 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,132 @@
1+
// Copyright 2021 Google LLC
2+
3+
#include "firestore/src/android/promise_factory_android.h"
4+
5+
#include <utility>
6+
7+
#include "android/firestore_integration_test_android.h"
8+
#include "android/task_completion_source.h"
9+
#include "firebase/future.h"
10+
#include "firestore/src/android/converter_android.h"
11+
#include "firestore/src/android/firestore_android.h"
12+
#include "firestore/src/jni/env.h"
13+
#include "firestore/src/jni/integer.h"
14+
#include "firestore/src/jni/ownership.h"
15+
#include "firestore/src/jni/task.h"
16+
#include "gmock/gmock.h"
17+
#include "gtest/gtest.h"
18+
19+
namespace firebase {
20+
namespace firestore {
21+
namespace {
22+
23+
using jni::Env;
24+
using jni::Integer;
25+
using jni::Local;
26+
using jni::Task;
27+
28+
// Since `PromiseFactory` acts as a "constructor" of `Promise` objects, its
29+
// ability to create `Promise` objects is thoroughly tested in the unit tests
30+
// for `Promise` and therefore the tests here only cover the other aspects of
31+
// `PromiseFactory`, such as move semantics.
32+
33+
class PromiseFactoryTest : public FirestoreAndroidIntegrationTest {
34+
public:
35+
// An enum of asynchronous functions to use in tests, as required by
36+
// `FutureManager`.
37+
enum class AsyncFn {
38+
kFn,
39+
kCount, // Must be the last enum value.
40+
};
41+
42+
protected:
43+
void AssertCreatesValidFutures(Env& env,
44+
PromiseFactory<AsyncFn>& promise_factory) {
45+
Local<TaskCompletionSource> tcs = TaskCompletionSource::Create(env);
46+
Local<Task> task = tcs.GetTask(env);
47+
Future<void> future =
48+
promise_factory.NewFuture<void>(env, AsyncFn::kFn, task);
49+
EXPECT_EQ(future.status(), FutureStatus::kFutureStatusPending);
50+
tcs.SetResult(env, jni::Integer::Create(env, 42));
51+
Await(future);
52+
EXPECT_EQ(future.status(), FutureStatus::kFutureStatusComplete);
53+
}
54+
55+
void AssertCreatesInvalidFutures(Env& env,
56+
PromiseFactory<AsyncFn>& promise_factory) {
57+
Local<TaskCompletionSource> tcs = TaskCompletionSource::Create(env);
58+
Local<Task> task = tcs.GetTask(env);
59+
Future<void> future =
60+
promise_factory.NewFuture<void>(env, AsyncFn::kFn, task);
61+
EXPECT_EQ(future.status(), FutureStatus::kFutureStatusInvalid);
62+
}
63+
};
64+
65+
TEST_F(PromiseFactoryTest, CopyConstructor) {
66+
Firestore* firestore = TestFirestore();
67+
PromiseFactory<AsyncFn> promise_factory1(GetFirestoreInternal(firestore));
68+
69+
PromiseFactory<AsyncFn> promise_factory2(promise_factory1);
70+
71+
Env env;
72+
{
73+
SCOPED_TRACE("promise_factory1");
74+
AssertCreatesValidFutures(env, promise_factory1);
75+
}
76+
{
77+
SCOPED_TRACE("promise_factory2");
78+
AssertCreatesValidFutures(env, promise_factory2);
79+
}
80+
}
81+
82+
TEST_F(PromiseFactoryTest, CopyConstructorWithDeletedFirestore) {
83+
Firestore* firestore = TestFirestore();
84+
PromiseFactory<AsyncFn> promise_factory1(GetFirestoreInternal(firestore));
85+
DeleteFirestore(firestore);
86+
87+
PromiseFactory<AsyncFn> promise_factory2(promise_factory1);
88+
89+
Env env;
90+
{
91+
SCOPED_TRACE("promise_factory1");
92+
AssertCreatesInvalidFutures(env, promise_factory1);
93+
}
94+
{
95+
SCOPED_TRACE("promise_factory2");
96+
AssertCreatesInvalidFutures(env, promise_factory2);
97+
}
98+
}
99+
100+
TEST_F(PromiseFactoryTest, MoveConstructor) {
101+
Firestore* firestore = TestFirestore();
102+
PromiseFactory<AsyncFn> promise_factory1(GetFirestoreInternal(firestore));
103+
104+
PromiseFactory<AsyncFn> promise_factory2(std::move(promise_factory1));
105+
106+
Env env;
107+
AssertCreatesValidFutures(env, promise_factory2);
108+
}
109+
110+
TEST_F(PromiseFactoryTest, MoveConstructorWithDeletedFirestore) {
111+
Firestore* firestore = TestFirestore();
112+
PromiseFactory<AsyncFn> promise_factory1(GetFirestoreInternal(firestore));
113+
DeleteFirestore(firestore);
114+
115+
PromiseFactory<AsyncFn> promise_factory2(std::move(promise_factory1));
116+
117+
Env env;
118+
AssertCreatesInvalidFutures(env, promise_factory2);
119+
}
120+
121+
TEST_F(PromiseFactoryTest, ShouldCreateInvalidPromisesIfFirestoreIsDeleted) {
122+
Firestore* firestore = TestFirestore();
123+
PromiseFactory<AsyncFn> promise_factory(GetFirestoreInternal(firestore));
124+
DeleteFirestore(firestore);
125+
Env env;
126+
127+
AssertCreatesInvalidFutures(env, promise_factory);
128+
}
129+
130+
} // namespace
131+
} // namespace firestore
132+
} // namespace firebase

‎firestore/integration_test_internal/src/firestore_test.cc

Lines changed: 24 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -3,11 +3,8 @@
33
#include "firebase/firestore.h"
44

55
#include <algorithm>
6+
#include <future>
67
#include <memory>
7-
8-
#if !defined(__ANDROID__)
9-
#include <future> // NOLINT(build/c++11)
10-
#endif
118
#include <stdexcept>
129

1310
#if defined(__ANDROID__)
@@ -1489,12 +1486,11 @@ TEST_F(FirestoreIntegrationTest, AuthWorks) {
14891486
WriteDocument(doc, MapFieldValue{{"foo", FieldValue::Integer(43)}});
14901487
}
14911488

1492-
#if !defined(__ANDROID__)
14931489
// This test is to ensure b/172986326 doesn't regress.
14941490
// Note: this test only exists in C++.
1495-
TEST_F(FirestoreIntegrationTest, FirestoreCanBeDeletedFromTransaction) {
1496-
auto* app = App::Create(this->app()->options(), "foo");
1497-
auto* db = Firestore::GetInstance(app);
1491+
TEST_F(FirestoreIntegrationTest, FirestoreCanBeDeletedFromTransactionAsync) {
1492+
Firestore* db = TestFirestore();
1493+
DisownFirestore(db);
14981494

14991495
auto future = db->RunTransaction(
15001496
[](Transaction&, std::string&) { return Error::kErrorOk; });
@@ -1511,7 +1507,26 @@ TEST_F(FirestoreIntegrationTest, FirestoreCanBeDeletedFromTransaction) {
15111507
callback_done.wait();
15121508
deletion.wait();
15131509
}
1514-
#endif // #if !defined(__ANDROID__)
1510+
1511+
// This test is to ensure b/172986326 doesn't regress.
1512+
// Note: this test only exists in C++.
1513+
TEST_F(FirestoreIntegrationTest, FirestoreCanBeDeletedFromTransaction) {
1514+
Firestore* db = TestFirestore();
1515+
DisownFirestore(db);
1516+
1517+
auto future = db->RunTransaction(
1518+
[](Transaction&, std::string&) { return Error::kErrorOk; });
1519+
1520+
std::promise<void> callback_done_promise;
1521+
auto callback_done = callback_done_promise.get_future();
1522+
future.AddOnCompletion([&](const Future<void>&) mutable {
1523+
delete db;
1524+
callback_done_promise.set_value();
1525+
});
1526+
1527+
Await(future);
1528+
callback_done.wait();
1529+
}
15151530

15161531
#if defined(__ANDROID__)
15171532
TEST_F(FirestoreAndroidIntegrationTest,

0 commit comments

Comments
 (0)