Skip to content

Commit 12f9d30

Browse files
var-consta-maurice
authored andcommitted
Make ListenerRegistration::Cleanup delegate to Remove.
Not entirely sure why it wasn't done before. This prevents the problem where `FirestoreInternal` might be holding on to invalid (already deleted) `ListenerRegistration`s after cleanup. SKIP_FIRESTORE_KOKORO_BUILD_TEST PiperOrigin-RevId: 298664884
1 parent c1458f6 commit 12f9d30

File tree

3 files changed

+13
-7
lines changed

3 files changed

+13
-7
lines changed

firestore/src/android/firestore_android.cc

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -233,8 +233,11 @@ FirestoreInternal::~FirestoreInternal() {
233233

234234
{
235235
MutexLock lock(listener_registration_mutex_);
236-
for (ListenerRegistrationInternal* registration : listener_registrations_) {
237-
delete registration;
236+
// TODO(varconst): investigate why not all listener registrations are
237+
// cleared.
238+
// FIREBASE_ASSERT(listener_registrations_.empty());
239+
for (auto* reg : listener_registrations_) {
240+
delete reg;
238241
}
239242
listener_registrations_.clear();
240243
}

firestore/src/common/listener_registration.cc

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -99,10 +99,7 @@ void ListenerRegistration::Remove() {
9999
}
100100

101101
void ListenerRegistration::Cleanup() {
102-
delete internal_;
103-
internal_ = nullptr;
104-
// `ListenerRegistration` additionally holds a pointer to `FirestoreInternal`,
105-
// which must also be set to null.
102+
Remove();
106103
firestore_ = nullptr;
107104
}
108105

firestore/src/ios/firestore_ios.cc

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
#include "firestore/src/ios/credentials_provider_ios.h"
1111
#include "firestore/src/ios/document_reference_ios.h"
1212
#include "firestore/src/ios/document_snapshot_ios.h"
13+
#include "firestore/src/ios/hard_assert_ios.h"
1314
#include "firestore/src/ios/listener_ios.h"
1415
#include "absl/memory/memory.h"
1516
#include "absl/types/any.h"
@@ -56,7 +57,12 @@ FirestoreInternal::FirestoreInternal(
5657
ApplyDefaultSettings();
5758
}
5859

59-
FirestoreInternal::~FirestoreInternal() { ClearListeners(); }
60+
FirestoreInternal::~FirestoreInternal() {
61+
std::lock_guard<std::mutex> lock(listeners_mutex_);
62+
HARD_ASSERT_IOS(listeners_.empty(),
63+
"Expected all listeners to be unregistered by the time "
64+
"FirestoreInternal is destroyed.");
65+
}
6066

6167
std::shared_ptr<api::Firestore> FirestoreInternal::CreateFirestore(
6268
App* app, std::unique_ptr<CredentialsProvider> credentials) {

0 commit comments

Comments
 (0)