Skip to content

Commit 16c9cf9

Browse files
var-consta-maurice
authored andcommitted
Make Promises participate in cleanup.
When a Firestore instance is cleaned up, some asynchronous operations might still be in progress. In case those operations involve resolving promises, those promises have to be cleaned up as well, lest they access already-destroyed objects. This fixes a class of bugs where the Unity testapp on desktop would sometimes crash upon rerun. PiperOrigin-RevId: 295010293
1 parent c8edacc commit 16c9cf9

File tree

3 files changed

+115
-10
lines changed

3 files changed

+115
-10
lines changed

firestore/src/ios/firestore_ios.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -132,7 +132,7 @@ class FirestoreInternal {
132132
CleanupNotifier cleanup_;
133133

134134
FutureManager future_manager_;
135-
PromiseFactory<AsyncApi> promise_factory_{&future_manager_};
135+
PromiseFactory<AsyncApi> promise_factory_{&cleanup_, &future_manager_};
136136

137137
// TODO(b/136119216): revamp this mechanism on both iOS and Android.
138138
std::mutex listeners_mutex_;

firestore/src/ios/promise_factory_ios.h

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,8 @@
1010
#include "firestore/src/ios/promise_ios.h"
1111

1212
namespace firebase {
13+
class CleanupNotifier;
14+
1315
namespace firestore {
1416

1517
// Wraps a `FutureManager` and allows creating `Promise`s and getting
@@ -23,22 +25,24 @@ class PromiseFactory {
2325
// convention that the object has `firestore_internal` member function.
2426
template <typename T>
2527
static PromiseFactory Create(T* object) {
26-
return PromiseFactory(&object->firestore_internal()->future_manager());
28+
return PromiseFactory(&object->firestore_internal()->cleanup(),
29+
&object->firestore_internal()->future_manager());
2730
}
2831

29-
explicit PromiseFactory(FutureManager* future_manager)
30-
: future_manager_{NOT_NULL(future_manager)} {
32+
PromiseFactory(CleanupNotifier* cleanup, FutureManager* future_manager)
33+
: cleanup_{NOT_NULL(cleanup)}, future_manager_{NOT_NULL(future_manager)} {
3134
future_manager_->AllocFutureApi(this, ApisCount());
3235
}
3336

3437
~PromiseFactory() { future_manager_->ReleaseFutureApi(this); }
3538

3639
PromiseFactory(const PromiseFactory& rhs)
37-
: future_manager_{rhs.future_manager_} {
40+
: cleanup_{rhs.cleanup_}, future_manager_{rhs.future_manager_} {
3841
future_manager_->AllocFutureApi(this, ApisCount());
3942
}
4043

41-
PromiseFactory(PromiseFactory&& rhs) : future_manager_{rhs.future_manager_} {
44+
PromiseFactory(PromiseFactory&& rhs)
45+
: cleanup_{rhs.cleanup_}, future_manager_{rhs.future_manager_} {
4246
future_manager_->MoveFutureApi(&rhs, this);
4347
}
4448

@@ -47,7 +51,7 @@ class PromiseFactory {
4751

4852
template <typename T>
4953
Promise<T> CreatePromise(ApiEnum index) {
50-
return Promise<T>{future_api(), static_cast<int>(index)};
54+
return Promise<T>{cleanup_, future_api(), static_cast<int>(index)};
5155
}
5256

5357
template <typename T>
@@ -63,6 +67,7 @@ class PromiseFactory {
6367

6468
int ApisCount() const { return static_cast<int>(ApiEnum::kCount); }
6569

70+
CleanupNotifier* cleanup_ = nullptr;
6671
FutureManager* future_manager_ = nullptr;
6772
};
6873

firestore/src/ios/promise_ios.h

Lines changed: 103 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33

44
#include <utility>
55

6+
#include "app/src/cleanup_notifier.h"
67
#include "app/src/include/firebase/future.h"
78
#include "app/src/reference_counted_future_impl.h"
89
#include "firestore/src/ios/hard_assert_ios.h"
@@ -27,10 +28,70 @@ template <typename ResultT>
2728
class Promise {
2829
public:
2930
// Creates a future backed by `LastResults` cache.
30-
Promise(ReferenceCountedFutureImpl* future_api, int identifier)
31-
: future_api_{NOT_NULL(future_api)},
31+
Promise(CleanupNotifier* cleanup, ReferenceCountedFutureImpl* future_api,
32+
int identifier)
33+
: cleanup_{NOT_NULL(cleanup)},
34+
future_api_{NOT_NULL(future_api)},
3235
identifier_{identifier},
33-
handle_{future_api->SafeAlloc<ResultT>(identifier)} {}
36+
handle_{future_api->SafeAlloc<ResultT>(identifier)} {
37+
RegisterForCleanup();
38+
}
39+
40+
~Promise() { UnregisterForCleanup(); }
41+
42+
Promise(const Promise& rhs)
43+
: cleanup_{rhs.cleanup_},
44+
future_api_{rhs.future_api_},
45+
identifier_{rhs.identifier_},
46+
handle_{rhs.handle_} {
47+
RegisterForCleanup();
48+
}
49+
50+
Promise(Promise&& rhs) noexcept
51+
: cleanup_{rhs.cleanup_},
52+
future_api_{rhs.future_api_},
53+
identifier_{rhs.identifier_},
54+
handle_{rhs.handle_} {
55+
rhs.UnregisterForCleanup();
56+
rhs.cleanup_ = nullptr;
57+
rhs.future_api_ = nullptr;
58+
rhs.identifier_ = 0;
59+
rhs.handle_ = {};
60+
61+
RegisterForCleanup();
62+
}
63+
64+
Promise& operator=(const Promise& rhs) {
65+
UnregisterForCleanup();
66+
67+
cleanup_ = rhs.cleanup_;
68+
future_api_ = rhs.future_api_;
69+
identifier_ = rhs.identifier_;
70+
handle_ = rhs.handle_;
71+
72+
RegisterForCleanup();
73+
74+
return *this;
75+
}
76+
77+
Promise& operator=(Promise&& rhs) noexcept {
78+
rhs.UnregisterForCleanup();
79+
UnregisterForCleanup();
80+
81+
cleanup_ = rhs.cleanup_;
82+
future_api_ = rhs.future_api_;
83+
identifier_ = rhs.identifier_;
84+
handle_ = rhs.handle_;
85+
86+
RegisterForCleanup();
87+
88+
rhs.cleanup_ = nullptr;
89+
rhs.future_api_ = nullptr;
90+
rhs.identifier_ = 0;
91+
rhs.handle_ = {};
92+
93+
return *this;
94+
}
3495

3596
// This is only a template function to enable SFINAE. The `Promise` will have
3697
// either `SetValue(ResultT)` or `SetValue()` defined, based on whether
@@ -43,6 +104,9 @@ class Promise {
43104
template <typename DummyT = ResultT,
44105
typename = absl::enable_if_t<!std::is_void<DummyT>::value>>
45106
void SetValue(DummyT result) {
107+
if (IsCleanedUp()) {
108+
return;
109+
}
46110
future_api_->Complete(handle_, NoError(), /*error_message=*/"",
47111
[&](ResultT* value) {
48112
// Future API doesn't support moving the value, use
@@ -54,24 +118,60 @@ class Promise {
54118
template <typename DummyT = ResultT,
55119
typename = absl::enable_if_t<std::is_void<DummyT>::value>>
56120
void SetValue() {
121+
if (IsCleanedUp()) {
122+
return;
123+
}
57124
future_api_->Complete(handle_, NoError());
58125
}
59126

60127
void SetError(const util::Status& status) {
61128
HARD_ASSERT_IOS(
62129
!status.ok(),
63130
"To fulfill a promise with 'ok' status, use Promise::SetValue.");
131+
if (IsCleanedUp()) {
132+
return;
133+
}
134+
64135
future_api_->Complete(handle_, status.code(),
65136
status.error_message().c_str());
66137
}
67138

68139
Future<ResultT> future() {
140+
if (IsCleanedUp()) {
141+
return Future<ResultT>{};
142+
}
143+
69144
return Future<ResultT>{future_api_, handle_.get()};
70145
}
71146

72147
private:
148+
Promise() = default;
149+
73150
int NoError() const { return static_cast<int>(Error::Ok); }
74151

152+
// Note: `CleanupFn` is not used because `Promise` is a header-only class, to
153+
// avoid a circular dependency between headers.
154+
void RegisterForCleanup() {
155+
if (IsCleanedUp()) {
156+
return;
157+
}
158+
159+
cleanup_->RegisterObject(this, [](void* raw_this) {
160+
auto* this_ptr = static_cast<Promise*>(raw_this);
161+
*this_ptr = {};
162+
});
163+
}
164+
165+
void UnregisterForCleanup() {
166+
if (IsCleanedUp()) {
167+
return;
168+
}
169+
cleanup_->UnregisterObject(this);
170+
}
171+
172+
bool IsCleanedUp() const { return cleanup_ == nullptr; }
173+
174+
CleanupNotifier* cleanup_ = nullptr;
75175
ReferenceCountedFutureImpl* future_api_ = nullptr;
76176
int identifier_ = 0;
77177
SafeFutureHandle<ResultT> handle_;

0 commit comments

Comments
 (0)