Avoid destroying the Task's operation while holding a lock (#5658)

This prevents deadlock that can occur if the currently executing task
happens hold the last `shared_ptr<Firestore>` for an instance.

The deadlock looked like this: the destruction of the closure would
trigger the destruction of the instance, which would dispose the
executor, triggering cancellation of all tasks, including the currently
executing one. Since the `operation_` member was reassigned while
holding the task's lock, the `Cancel` operation would block trying to
acquire it.

This fixes the deadlock by forcing all interaction with the `operation_`
outside the lock.

Author: wilhuff

Firestore/core/src/util/task.cc

@@ -101,11 +101,15 @@ void Task::ExecuteAndRelease() {
         Defer relock([&] { lock.lock(); });
         operation_();
 
+        // Clear the operation while not holding the lock to avoid the
+        // possibility that the destructor of something in the closure might
+        // try to reference this task.
+        operation_ = {};
+
         TASK_TRACE("Task::Execute %s (completing)", this);
       }
 
       state_ = State::kDone;
-      operation_ = {};
 
       // The callback to the executor must be performed after the operation
       // completes, otherwise the executor's destructor cannot reliably block
@@ -165,9 +169,17 @@ void Task::Cancel() {
   TASK_TRACE("Task::Cancel %s", this);
 
   if (state_ == State::kInitial) {
+    // Do not clear the `operation_` here because that might indirectly trigger
+    // an interaction with this task through its destructor.
     state_ = State::kCancelled;
     executor_ = nullptr;
+
+    // Clear the operation while not holding the lock to avoid the
+    // possibility that the destructor of something in the closure might
+    // try to reference this task.
+    lock.unlock();
     operation_ = {};
+
     is_complete_.notify_all();
 
   } else if (state_ == State::kRunning) {

Firestore/core/src/util/task.h

@@ -201,8 +201,11 @@ class Task {
   Executor::Tag tag_ = 0;
   Executor::Id id_ = 0;
 
-  Executor::Operation operation_;
   std::thread::id executing_thread_;
+
+  // The operation to run, supplied by the caller. Make this the last member
+  // just in case it refers to this task during its own destruction.
+  Executor::Operation operation_;
 };
 
 /**

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

@@ -210,6 +210,7 @@ TEST_F(TaskTest, OwnedExecuteThenRelease) {
   ASSERT_EQ(state.task_destroyed, 0);
 
   task->Release();
+  ASSERT_EQ(state.op_destroyed, 1);
   ASSERT_EQ(state.task_destroyed, 1);
 }
 
@@ -269,6 +270,52 @@ TEST_F(TaskTest, OwnedExecuteThenExecute) {
   ASSERT_EQ(state.task_destroyed, 1);
 }
 
+TEST_F(TaskTest, AvoidsDeadlockDuringOperationDestruction) {
+  std::string steps;
+
+  // Arrange for a value to be captured inside the lambda capture of the Task's
+  // operation.
+  //
+  // On destruction, Holder will try to cancel the task it owns.
+  struct Holder {
+    explicit Holder(std::string* steps) : steps_(steps) {
+    }
+
+    ~Holder() {
+      *steps_ += "4";
+      if (!copied_from_ && task_) {
+        task_->Cancel();
+      }
+    }
+
+    Holder(const Holder& other) : task_(other.task_), steps_(other.steps_) {
+      other.copied_from_ = true;
+    }
+
+    mutable bool copied_from_ = false;
+    Task* task_ = nullptr;
+    std::string* steps_ = nullptr;
+  };
+  auto holder = std::make_shared<Holder>(&steps);
+
+  Task* task = Task::Create(nullptr, [holder, &steps] { steps += "3"; });
+
+  // Give the task to holder and remove our local copy of the holder.
+  holder->task_ = task;
+  holder.reset();
+  steps += "1";
+
+  Expectation ran;
+  Async([&] {
+    steps += "2";
+    task->ExecuteAndRelease();
+    ran.Fulfill();
+  });
+
+  Await(ran);
+  ASSERT_EQ(steps, "1234");
+}
+
 }  // namespace util
 }  // namespace firestore
 }  // namespace firebase