Fix a crash that happens when Firestore is being terminated when the last shared pointer to it is in a listener

Firestore/CHANGELOG.md

@@ -1,3 +1,7 @@
+# Unreleased
+- [fixed] Fixed a crash that would happen when the app is being deleted and
+  immediately disposed of and there's an active listener (#6909).
+
 # v7.5.0
 - [changed] A write to a document that contains FieldValue transforms is no
   longer split up into two separate operations. This reduces the number of

Firestore/Example/Firestore.xcodeproj/xcshareddata/xcschemes/Firestore_IntegrationTests_iOS.xcscheme

@@ -27,6 +27,7 @@
       selectedDebuggerIdentifier = "Xcode.DebuggerFoundation.Debugger.LLDB"
       selectedLauncherIdentifier = "Xcode.DebuggerFoundation.Launcher.LLDB"
       shouldUseLaunchSchemeArgsEnv = "YES"
+      enableAddressSanitizer = "YES"
       enableASanStackUseAfterReturn = "YES">
       <MacroExpansion>
          <BuildableReference

Firestore/Example/Tests/Integration/API/FIRDatabaseTests.mm

@@ -1441,6 +1441,30 @@ - (void)testAppDeleteLeadsToFirestoreTermination {
   XCTAssertTrue(firestore.wrapped->client()->is_terminated());
 }
 
+// Ensures b/172958106 doesn't regress.
+- (void)testDeleteAppWorksWhenLastReferenceToFirestoreIsInListener {
+  FIRApp *app = testutil::AppForUnitTesting(util::MakeString([FSTIntegrationTestCase projectID]));
+  FIRFirestore *firestore = [FIRFirestore firestoreForApp:app];
+
+  FIRDocumentReference *doc = [firestore documentWithPath:@"abc/123"];
+  // Make sure there is a listener.
+  [doc addSnapshotListener:^(FIRDocumentSnapshot *, NSError *){
+  }];
+  NSDictionary<NSString *, id> *data =
+      @{@"owner" : @{@"name" : @"Jonny", @"email" : @"[email protected]"}};
+  // Make sure the client is initialized.
+  [self writeDocumentRef:doc data:data];
+
+  XCTestExpectation *expectation = [self expectationWithDescription:@"App is deleted"];
+  [app deleteApp:^(BOOL) {
+    [expectation fulfill];
+  }];
+  // Let go of the last app reference.
+  app = nil;
+
+  [self awaitExpectations];
+}
+
 - (void)testTerminateCanBeCalledMultipleTimes {
   FIRApp *app = testutil::AppForUnitTesting(util::MakeString([FSTIntegrationTestCase projectID]));
   FIRFirestore *firestore = [FIRFirestore firestoreForApp:app];

Firestore/core/src/api/firestore.cc

@@ -219,7 +219,8 @@ void Firestore::EnsureClientConfigured() {
     HARD_ASSERT(worker_queue_, "Expected non-null worker queue");
     client_ = FirestoreClient::Create(
         MakeDatabaseInfo(), settings_, std::move(credentials_provider_),
-        user_executor_, worker_queue_, std::move(firebase_metadata_provider_));
+        user_executor_, worker_queue_, std::move(firebase_metadata_provider_),
+        shared_from_this());
   }
 }
 

Firestore/core/src/core/firestore_client.cc

@@ -64,6 +64,7 @@ namespace core {
 using api::DocumentReference;
 using api::DocumentSnapshot;
 using api::DocumentSnapshotListener;
+using api::Firestore;
 using api::ListenerRegistration;
 using api::QuerySnapshot;
 using api::QuerySnapshotListener;
@@ -112,11 +113,13 @@ std::shared_ptr<FirestoreClient> FirestoreClient::Create(
     std::shared_ptr<CredentialsProvider> credentials_provider,
     std::shared_ptr<Executor> user_executor,
     std::shared_ptr<AsyncQueue> worker_queue,
-    std::unique_ptr<FirebaseMetadataProvider> firebase_metadata_provider) {
+    std::unique_ptr<FirebaseMetadataProvider> firebase_metadata_provider,
+    std::shared_ptr<Firestore> firestore) {
   // Have to use `new` because `make_shared` cannot access private constructor.
   std::shared_ptr<FirestoreClient> shared_client(new FirestoreClient(
       database_info, std::move(credentials_provider), std::move(user_executor),
-      std::move(worker_queue), std::move(firebase_metadata_provider)));
+      std::move(worker_queue), std::move(firebase_metadata_provider),
+      std::move(firestore)));
 
   std::weak_ptr<FirestoreClient> weak_client(shared_client);
   auto credential_change_listener = [weak_client, settings](User user) mutable {
@@ -158,12 +161,14 @@ FirestoreClient::FirestoreClient(
     std::shared_ptr<CredentialsProvider> credentials_provider,
     std::shared_ptr<Executor> user_executor,
     std::shared_ptr<AsyncQueue> worker_queue,
-    std::unique_ptr<FirebaseMetadataProvider> firebase_metadata_provider)
+    std::unique_ptr<FirebaseMetadataProvider> firebase_metadata_provider,
+    std::shared_ptr<Firestore> firestore)
     : database_info_(database_info),
       credentials_provider_(std::move(credentials_provider)),
       worker_queue_(std::move(worker_queue)),
       user_executor_(std::move(user_executor)),
-      firebase_metadata_provider_(std::move(firebase_metadata_provider)) {
+      firebase_metadata_provider_(std::move(firebase_metadata_provider)),
+      weak_firestore_(std::move(firestore)) {
 }
 
 void FirestoreClient::Initialize(const User& user, const Settings& settings) {
@@ -231,21 +236,27 @@ FirestoreClient::~FirestoreClient() {
 }
 
 void FirestoreClient::Dispose() {
-  // Prevent new API invocations from enqueueing further work.
-  worker_queue_->EnterRestrictedMode();
-
-  // Clean up internal resources. It's possible that this can race with a call
-  // to `Firestore::ClearPersistence` or `Firestore::Terminate`, but that's OK
-  // because that operation does not rely on any state in this FirestoreClient.
   std::promise<void> signal_disposing;
-  bool enqueued = worker_queue_->EnqueueEvenWhileRestricted([&, this] {
-    // Once this task has started running, AsyncQueue::Dispose will block on its
-    // completion. Signal as early as possible to lock out even restricted tasks
-    // as early as possible.
-    signal_disposing.set_value();
 
-    TerminateInternal();
-  });
+  bool enqueued = false;
+  // If `remote_store_` is null, it means termination has finished already. In
+  // that case, enqueing termination is not only unnecessary, but also
+  // dangerous in the case when `Dispose` is invoked immediately after the
+  // termination, still on the worker queue -- it would break the sequential
+  // order invariant of the queue.
+  if (!is_terminated()) {
+    // Prevent new API invocations from enqueueing further work.
+    worker_queue_->EnterRestrictedMode();
+
+    enqueued = worker_queue_->EnqueueEvenWhileRestricted([&, this] {
+      // Once this task has started running, AsyncQueue::Dispose will block on
+      // its completion. Signal as early as possible to lock out even restricted
+      // tasks as early as possible.
+      signal_disposing.set_value();
+
+      TerminateInternal();
+    });
+  }
 
   // If we successfully enqueued the TerminateInternal task then wait for it to
   // start.
@@ -263,7 +274,13 @@ void FirestoreClient::Dispose() {
 
 void FirestoreClient::TerminateAsync(StatusCallback callback) {
   worker_queue_->EnterRestrictedMode();
-  worker_queue_->EnqueueEvenWhileRestricted([this, callback] {
+  worker_queue_->EnqueueEvenWhileRestricted([&, this, callback] {
+    // Make sure this `FirestoreClient` is not destroyed during
+    // `TerminateInternal`, so that `user_executor_` stays valid.
+    auto self = shared_from_this();
+    // Make sure that `api::Firestore` is not destroyed during the call to
+    // `TerminateInternal`.
+    auto firestore = weak_firestore_.lock();
     TerminateInternal();
 
     if (callback) {

Firestore/core/src/core/firestore_client.h

@@ -80,7 +80,8 @@ class FirestoreClient : public std::enable_shared_from_this<FirestoreClient> {
       std::shared_ptr<util::Executor> user_executor,
       std::shared_ptr<util::AsyncQueue> worker_queue,
       std::unique_ptr<remote::FirebaseMetadataProvider>
-          firebase_metadata_provider);
+          firebase_metadata_provider,
+      std::shared_ptr<api::Firestore> firestore);
 
   ~FirestoreClient();
 
@@ -185,7 +186,8 @@ class FirestoreClient : public std::enable_shared_from_this<FirestoreClient> {
       std::shared_ptr<util::Executor> user_executor,
       std::shared_ptr<util::AsyncQueue> worker_queue,
       std::unique_ptr<remote::FirebaseMetadataProvider>
-          firebase_metadata_provider);
+          firebase_metadata_provider,
+      std::shared_ptr<api::Firestore> firestore);
 
   void Initialize(const auth::User& user, const api::Settings& settings);
 
@@ -223,6 +225,9 @@ class FirestoreClient : public std::enable_shared_from_this<FirestoreClient> {
   bool credentials_initialized_ = false;
   local::LruDelegate* _Nullable lru_delegate_;
   util::DelayedOperation lru_callback_;
+
+  // Used during shutdown to guarantee lifetimes.
+  std::weak_ptr<api::Firestore> weak_firestore_;
 };
 
 }  // namespace core

Firestore/core/src/util/async_queue.cc

@@ -86,6 +86,7 @@ void AsyncQueue::ExecuteBlocking(const Operation& operation) {
               "ExecuteBlocking may not be called "
               "before the previous operation finishes executing");
 
+  auto self = shared_from_this();
   is_operation_in_progress_ = true;
   operation();
   is_operation_in_progress_ = false;

Firestore/core/test/unit/util/ordered_code_test.cc

@@ -350,7 +350,7 @@ TEST(OrderedCodeInvalidEncodingsTest, NonCanonical) {
 
 #if defined(NDEBUG)
     EXPECT_ANY_THROW(TestRead<uint64_t>(INCREASING, non_minimal));
-#else  // defined(NDEBUG)
+#else   // defined(NDEBUG)
     absl::string_view s(non_minimal);
     EXPECT_ANY_THROW(OrderedCode::ReadNumIncreasing(&s, NULL));
 #endif  // defined(NDEBUG)
@@ -372,7 +372,7 @@ TEST(OrderedCodeInvalidEncodingsTest, NonCanonical) {
 
 #if defined(NDEBUG)
     EXPECT_ANY_THROW(TestRead<int64_t>(INCREASING, non_minimal));
-#else  // defined(NDEBUG)
+#else   // defined(NDEBUG)
     absl::string_view s(non_minimal);
     EXPECT_ANY_THROW(OrderedCode::ReadSignedNumIncreasing(&s, NULL));
     s = non_minimal;