Fix deadlock in ExecutorLibdispatch destructor (#1835)

There is a deadlock that is triggered while running integration tests in XCode 10 (previous versions of XCode seem to work fine). During integration test case shutdown, two executors used in Firestore, the worker executor and the user executor, happen to be destroyed at the same time on each other's dispatch queues (worker executor is being destroyed on the main queue, which is also the user queue, and user executor is being destroyed on the worker queue). Each calls `RunSynchronized` in its destructor, with the result that they deadlock. The fix uses `std::atomic` as the synchronization mechanism instead.

Tested -- ran integration tests under TSan in XCode 10 with no issues.

Author: var-const

Firestore/core/src/firebase/firestore/util/executor_libdispatch.mm

@@ -16,6 +16,8 @@
 
 #include "Firestore/core/src/firebase/firestore/util/executor_libdispatch.h"
 
+#include <atomic>
+
 #include "Firestore/core/src/firebase/firestore/util/hard_assert.h"
 
 namespace firebase {
@@ -140,10 +142,9 @@ void MarkDone() {
 
   // True if the operation has either been run or canceled.
   //
-  // Note on thread-safety: because the precondition is that all member
-  // functions of this class are executed on the dispatch queue, no
-  // synchronization is required for `done_`.
-  bool done_ = false;
+  // Note on thread-safety: this variable is accessed both from the dispatch
+  // queue and in the destructor, which may run on any queue.
+  std::atomic<bool> done_;
 };
 
 TimeSlot::TimeSlot(ExecutorLibdispatch* const executor,
@@ -154,6 +155,9 @@ void MarkDone() {
                        std::chrono::steady_clock::now()) +
                    delay},
       tagged_{std::move(operation)} {
+  // Only assignment of std::atomic is atomic; initialization in its constructor
+  // isn't
+  done_ = false;
 }
 
 Executor::TaggedOperation TimeSlot::Unschedule() {
@@ -199,11 +203,11 @@ void MarkDone() {
   // the queue is serial, by the time libdispatch gets to the newly-enqueued
   // work, the pending operations that might have been in progress would have
   // already finished.
-  RunSynchronized(this, [this] {
-    for (auto slot : schedule_) {
-      slot->MarkDone();
-    }
-  });
+  // Note: this is thread-safe, because the underlying variable `done_` is
+  // atomic. `RunSynchronized` may result in a deadlock.
+  for (auto slot : schedule_) {
+    slot->MarkDone();
+  }
 }
 
 bool ExecutorLibdispatch::IsCurrentExecutor() const {

Firestore/core/test/firebase/firestore/util/executor_libdispatch_test.mm

@@ -33,6 +33,8 @@
       dispatch_queue_create("ExecutorLibdispatchTests", DISPATCH_QUEUE_SERIAL));
 }
 
+namespace chr = std::chrono;
+
 }  // namespace
 
 INSTANTIATE_TEST_CASE_P(ExecutorTestLibdispatch,
@@ -68,6 +70,37 @@
   EXPECT_TRUE(WaitForTestToFinish());
 }
 
+TEST_F(ExecutorLibdispatchOnlyTests, ScheduledOperationOutlivesExecutor) {
+  namespace chr = std::chrono;
+  const auto far_away = chr::milliseconds(100);
+  executor->Schedule(far_away, {Executor::Tag{1}, [] {}});
+  executor.reset();
+  // Try to wait until libdispatch invokes the scheduled operation. This is
+  // flaky but unlikely to not work in practice. The test is successful if
+  // there is no crash/data race under TSan.
+  std::this_thread::sleep_for(chr::milliseconds(500));
+}
+
+TEST_F(ExecutorLibdispatchOnlyTests,
+       ScheduledOperationOutlivesExecutor_DestroyedOnOwnQueue) {
+  const auto far_away = chr::milliseconds(100);
+  executor->Schedule(far_away, {Executor::Tag{1}, [] {}});
+
+  // Invoke destructor on the executor's own queue to make sure there is no
+  // deadlock.
+  std::function<void()> reset = [this] { executor.reset(); };
+  auto queue =
+      static_cast<ExecutorLibdispatch*>(executor.get())->dispatch_queue();
+  dispatch_sync_f(queue, &reset, [](void* const raw_reset) {
+    const auto unwrap = static_cast<std::function<void()>*>(raw_reset);
+    (*unwrap)();
+  });
+  // Try to wait until libdispatch invokes the scheduled operation. This is
+  // flaky but unlikely to not work in practice. The test is successful if
+  // there is no crash/data race under TSan.
+  std::this_thread::sleep_for(chr::milliseconds(500));
+}
+
 }  // namespace internal
 }  // namespace util
 }  // namespace firestore