Fix double execute for curried reject and double resolve for kAll

A followup to https://crrev.com/c/1637460

Bug: 906125
Change-Id: Ib9bb0ba030e001816c6b75d124060f8a04a8525d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1638540
Commit-Queue: Alex Clarke <alexclarke@chromium.org>
Reviewed-by: Etienne Pierre-Doray <etiennep@chromium.org>
Cr-Commit-Position: refs/heads/master@{#666214}

base/task/promise/abstract_promise.cc

@@ -80,7 +80,7 @@ bool AbstractPromise::InsertDependentOnAnyThread(DependentList::Node* node) {
       break;
 
     case DependentList::InsertResult::FAIL_PROMISE_RESOLVED:
-      dependent->OnPrerequisiteResolved();
+      dependent->OnPrerequisiteResolved(this);
       break;
 
     case DependentList::InsertResult::FAIL_PROMISE_REJECTED:
@@ -242,11 +242,12 @@ AbstractPromise::GetPrerequisitePolicy() {
   return executor->GetPrerequisitePolicy();
 }
 
-AbstractPromise* AbstractPromise::GetFirstRejectedPrerequisite() const {
+AbstractPromise* AbstractPromise::GetFirstSettledPrerequisite() const {
   if (!prerequisites_)
     return nullptr;
   return reinterpret_cast<AbstractPromise*>(
-      prerequisites_->first_rejecting_promise.load(std::memory_order_acquire));
+      prerequisites_->first_settled_prerequisite.load(
+          std::memory_order_acquire));
 }
 
 void AbstractPromise::Execute() {
@@ -264,14 +265,7 @@ void AbstractPromise::Execute() {
   }
 #endif
 
-  if (IsResolvedWithPromise()) {
-    bool settled = DispatchIfNonCurriedRootSettled();
-    DCHECK(settled);
-
-    prerequisites_->prerequisite_list.clear();
-    return;
-  }
-
+  DCHECK(!IsResolvedWithPromise());
   DCHECK(GetExecutor()) << from_here_.ToString() << " value_ contains "
                         << value_.type();
 
@@ -302,8 +296,12 @@ bool AbstractPromise::DispatchIfNonCurriedRootSettled() {
   return true;
 }
 
-void AbstractPromise::OnPrerequisiteResolved() {
+void AbstractPromise::OnPrerequisiteResolved(
+    AbstractPromise* resolved_prerequisite) {
+  DCHECK(resolved_prerequisite->IsResolved());
   if (IsResolvedWithPromise()) {
+    // The executor has already run so we don't need to call
+    // MarkPrerequisiteAsSettling.
     bool settled = DispatchIfNonCurriedRootSettled();
     DCHECK(settled);
     return;
@@ -317,7 +315,8 @@ void AbstractPromise::OnPrerequisiteResolved() {
 
     case Executor::PrerequisitePolicy::kAny:
       // PrerequisitePolicy::kAny should resolve immediately.
-      DispatchPromise();
+      if (prerequisites_->MarkPrerequisiteAsSettling(resolved_prerequisite))
+        DispatchPromise();
       break;
 
     case Executor::PrerequisitePolicy::kNever:
@@ -326,20 +325,17 @@ void AbstractPromise::OnPrerequisiteResolved() {
 }
 
 void AbstractPromise::OnPrerequisiteRejected(
-    AbstractPromise* rejected_promise) {
-  DCHECK(rejected_promise->IsRejected());
-  uintptr_t expected = 0;
-  // Promises::All (or Race if we add that) can have multiple prerequsites and
-  // it will reject as soon as any prerequsite rejects. Multiple prerequsites
-  // can reject, but we wish to record only the first one.
-  bool is_first_rejection =
-      prerequisites_->first_rejecting_promise.compare_exchange_strong(
-          expected, reinterpret_cast<uintptr_t>(rejected_promise),
-          std::memory_order_acq_rel);
-  // We only want to dispatch a promise the first time a prerequisite is
-  // rejected because the executors can only be invoked once.
-  if (is_first_rejection)
+    AbstractPromise* rejected_prerequisite) {
+  DCHECK(rejected_prerequisite->IsRejected());
+
+  // Promises::All (or Race if we add that) can have multiple prerequisites and
+  // it will reject as soon as any prerequisite rejects. Multiple prerequisites
+  // can reject, but we wish to record only the first one. Also we can only
+  // invoke executors once.
+  if (prerequisites_->MarkPrerequisiteAsSettling(rejected_prerequisite) &&
+      !DispatchIfNonCurriedRootSettled()) {
     DispatchPromise();
+  }
 }
 
 bool AbstractPromise::OnPrerequisiteCancelled() {
@@ -382,7 +378,7 @@ void AbstractPromise::OnResolveDispatchReadyDependents() {
     // OnPrerequisiteResolved might post a task which destructs |node| on
     // another thread so load |node->next| now.
     next = node->next.load(std::memory_order_relaxed);
-    dependent->OnPrerequisiteResolved();
+    dependent->OnPrerequisiteResolved(this);
   }
 }
 
@@ -468,10 +464,6 @@ void AbstractPromise::OnResolved() {
       // The curried promise isn't already settled we need to throw away any
       // existing dependencies and make |curried_promise| the only dependency of
       // this promise.
-
-      if (!curried_promise->prerequisites_)
-        curried_promise->prerequisites_ = std::make_unique<AdjacencyList>();
-
 #if DCHECK_IS_ON()
       {
         CheckedAutoLock lock(GetCheckedLock());
@@ -479,7 +471,11 @@ void AbstractPromise::OnResolved() {
         ancestor_that_could_reject_ = nullptr;
       }
 #endif
-      prerequisites_->ResetWithSingleDependency(curried_promise);
+      if (prerequisites_) {
+        prerequisites_->ResetWithSingleDependency(curried_promise);
+      } else {
+        prerequisites_ = std::make_unique<AdjacencyList>(curried_promise);
+      }
       AddAsDependentForAllPrerequisites();
     }
   } else {
@@ -559,6 +555,18 @@ bool AbstractPromise::AdjacencyList::
   return action_prerequisite_count.fetch_sub(1, std::memory_order_acq_rel) == 1;
 }
 
+// For PrerequisitePolicy::kAll this is called for the first rejected
+// prerequisite. For PrerequisitePolicy:kAny this is called for the first
+// resolving or rejecting prerequisite.
+bool AbstractPromise::AdjacencyList::MarkPrerequisiteAsSettling(
+    AbstractPromise* settled_prerequisite) {
+  DCHECK(settled_prerequisite->IsSettled());
+  uintptr_t expected = 0;
+  return first_settled_prerequisite.compare_exchange_strong(
+      expected, reinterpret_cast<uintptr_t>(settled_prerequisite),
+      std::memory_order_acq_rel);
+}
+
 void AbstractPromise::AdjacencyList::ResetWithSingleDependency(
     scoped_refptr<AbstractPromise> prerequisite) {
   prerequisite_list.clear();
@@ -572,7 +580,7 @@ AbstractPromise::Executor::~Executor() {
 
 AbstractPromise::Executor::PrerequisitePolicy
 AbstractPromise::Executor::GetPrerequisitePolicy() const {
-  return vtable_->get_prerequsite_policy(storage_);
+  return vtable_->get_prerequisite_policy(storage_);
 }
 
 bool AbstractPromise::Executor::IsCancelled() const {

base/task/promise/abstract_promise.h

@@ -280,7 +280,7 @@ class BASE_EXPORT AbstractPromise
 
     struct VTable {
       void (*destructor)(void* self);
-      PrerequisitePolicy (*get_prerequsite_policy)(const void* self);
+      PrerequisitePolicy (*get_prerequisite_policy)(const void* self);
       bool (*is_cancelled)(const void* self);
 #if DCHECK_IS_ON()
       ArgumentPassingType (*resolve_argument_passing_type)(const void* self);
@@ -390,6 +390,12 @@ class BASE_EXPORT AbstractPromise
 
     bool DecrementPrerequisiteCountAndCheckIfZero();
 
+    // Called for each prerequisites that resolves or rejects for
+    // PrerequisitePolicy::kAny and each prerequisite that rejects for
+    // PrerequisitePolicy::kAll. This saves |settled_prerequisite| and returns
+    // true iff called for the first time.
+    bool MarkPrerequisiteAsSettling(AbstractPromise* settled_prerequisite);
+
     std::vector<AdjacencyListNode> prerequisite_list;
 
     // PrerequisitePolicy::kAny waits for at most 1 resolve or N cancellations.
@@ -397,11 +403,11 @@ class BASE_EXPORT AbstractPromise
     // PrerequisitePolicy::kNever doesn't use this.
     std::atomic_int action_prerequisite_count;
 
-    // Stores the address of the first rejecting promise. The purpose of this is
-    // two-fold, first to ensure that Promises::All/Race return the first
-    // prerequisite that rejected and secondly to prevent the executor from
-    // being run multiple times if there's multiple rejection.
-    std::atomic<uintptr_t> first_rejecting_promise{0};
+    // For PrerequisitePolicy::kAll the address of the first rejected
+    // prerequisite if any.
+    // For PrerequisitePolicy::kAll the address of the first rejected or
+    // resolved rerequsite if any.
+    std::atomic<uintptr_t> first_settled_prerequisite{0};
   };
 
   const std::vector<AdjacencyListNode>* prerequisite_list() const {
@@ -418,7 +424,10 @@ class BASE_EXPORT AbstractPromise
     return prerequisites_->prerequisite_list[0].prerequisite.get();
   }
 
-  AbstractPromise* GetFirstRejectedPrerequisite() const;
+  // For PrerequisitePolicy::kAll returns the first rejected prerequisite if
+  // any. For PrerequisitePolicy::kAny returns the first rejected or resolved
+  // rerequsite if any.
+  AbstractPromise* GetFirstSettledPrerequisite() const;
 
   // Calls |RunExecutor()| or posts a task to do so if |from_here_| is not
   // nullopt.
@@ -498,10 +507,10 @@ class BASE_EXPORT AbstractPromise
 
   // Checks if the promise is now ready to be executed and if so posts it on the
   // given task runner.
-  void OnPrerequisiteResolved();
+  void OnPrerequisiteResolved(AbstractPromise* resolved_prerequisite);
 
   // Schedules the promise for execution.
-  void OnPrerequisiteRejected(AbstractPromise* rejected_promise);
+  void OnPrerequisiteRejected(AbstractPromise* rejected_prerequisite);
 
   // Returns true if we are still potentially eligible to run despite the
   // cancellation.

base/task/promise/all_container_executor.h

@@ -26,8 +26,10 @@ class AllContainerPromiseExecutor {
 
   void Execute(AbstractPromise* promise) {
     // All is rejected if any prerequisites are rejected.
-    if (AbstractPromise* rejected = promise->GetFirstRejectedPrerequisite()) {
-      AllPromiseRejectHelper<Rejected<RejectType>>::Reject(promise, rejected);
+    AbstractPromise* first_settled = promise->GetFirstSettledPrerequisite();
+    if (first_settled && first_settled->IsRejected()) {
+      AllPromiseRejectHelper<Rejected<RejectType>>::Reject(promise,
+                                                           first_settled);
       promise->OnRejected();
       return;
     }

base/task/promise/all_tuple_executor.h

@@ -62,8 +62,9 @@ class AllTuplePromiseExecutor {
 
   void Execute(AbstractPromise* promise) {
     // All is rejected if any prerequisites are rejected.
-    if (AbstractPromise* rejected = promise->GetFirstRejectedPrerequisite()) {
-      AllPromiseRejectHelper<RejectT>::Reject(promise, rejected);
+    AbstractPromise* first_settled = promise->GetFirstSettledPrerequisite();
+    if (first_settled && first_settled->IsRejected()) {
+      AllPromiseRejectHelper<RejectT>::Reject(promise, first_settled);
       promise->OnRejected();
       return;
     }