Reland: Fix leaks related to DependentList retaining owning AbstractPromise

(Fix was to make SingleRejectPrerequisitePolicyALLModified not use
RunUntilIdle)

Original patch: https://crrev.com/c/1640639

The idea (thanks Etienne) is to not retain any prerequisites (including
curried ones) until they've settled.  This fixes ref counting cycles
while ensuring results are retained for dependencies.

This patch also changes the way curried promises are handled which was
necessary to break refcounting cycles. Now when a promise resolves or
rejects with a promise, its dependents are modified to use the curried
promise as their prerequisite instead.

TBR=fdoray@chromium.org

Bug: 968302, 906125
Change-Id: I1532faff6fe4f55dc236210389db62eb6a03212f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1680264Reviewed-by: Alex Clarke <alexclarke@chromium.org>
Commit-Queue: Alex Clarke <alexclarke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#673293}

base/task/promise/all_container_executor.h

@@ -74,17 +74,17 @@ class AllContainerPromiseExecutor {
     using NonVoidResolveType = ToNonVoidT<ResolveType>;
     Resolved<std::vector<NonVoidResolveType>> result;
 
-    const std::vector<AbstractPromise::AdjacencyListNode>* prerequisite_list =
+    const std::vector<DependentList::Node>* prerequisite_list =
         promise->prerequisite_list();
     DCHECK(prerequisite_list);
     result.value.reserve(prerequisite_list->size());
 
     for (const auto& node : *prerequisite_list) {
-      DCHECK(node.prerequisite->IsResolved());
+      DCHECK(node.prerequisite()->IsResolved());
       result.value.push_back(
           ArgMoveSemanticsHelper<
               NonVoidResolveType,
-              Resolved<NonVoidResolveType>>::Get(node.prerequisite.get()));
+              Resolved<NonVoidResolveType>>::Get(node.prerequisite()));
     }
 
     promise->emplace(std::move(result));
@@ -106,10 +106,9 @@ struct AllContainerHelper<Container, Promise<ResolveType, RejectType>> {
 
   static PromiseType All(const Location& from_here, const Container& promises) {
     size_t i = 0;
-    std::vector<AbstractPromise::AdjacencyListNode> prerequisite_list(
-        promises.size());
+    std::vector<DependentList::Node> prerequisite_list(promises.size());
     for (auto& promise : promises) {
-      prerequisite_list[i++].prerequisite = promise.abstract_promise_;
+      prerequisite_list[i++].SetPrerequisite(promise.abstract_promise_.get());
     }
     return PromiseType(AbstractPromise::Create(
         nullptr, from_here,

base/task/promise/all_tuple_executor.h

@@ -29,13 +29,13 @@ struct TupleConstructor<Tuple, std::index_sequence<Indices...>> {
   // Resolves |result| with a std::tuple of the promise results of the dependent
   // promises.
   static void ConstructTuple(
-      const std::vector<AbstractPromise::AdjacencyListNode>* prerequisite_list,
+      const std::vector<DependentList::Node>* prerequisite_list,
       AbstractPromise* result) {
     DCHECK_EQ(sizeof...(Indices), prerequisite_list->size());
     result->emplace(
         in_place_type_t<Resolved<Tuple>>(),
         GetResolvedValueFromPromise<std::tuple_element_t<Indices, Tuple>>(
-            (*prerequisite_list)[Indices].prerequisite.get())...);
+            (*prerequisite_list)[Indices].prerequisite())...);
   }
 };
 
@@ -69,7 +69,7 @@ class AllTuplePromiseExecutor {
       return;
     }
 
-    const std::vector<AbstractPromise::AdjacencyListNode>* prerequisite_list =
+    const std::vector<DependentList::Node>* prerequisite_list =
         promise->prerequisite_list();
     DCHECK(prerequisite_list);
     TupleConstructor<ResolveTuple>::ConstructTuple(prerequisite_list, promise);

base/task/promise/dependent_list.cc

@@ -13,42 +13,55 @@
 
 namespace base {
 namespace internal {
-namespace {
 
-DependentList::Node* ReverseList(DependentList::Node* list) {
+// static
+DependentList::Node* DependentList::ReverseList(DependentList::Node* list) {
   DependentList::Node* prev = nullptr;
   while (list) {
-    DependentList::Node* next = list->next;
-    list->next = prev;
+    DependentList::Node* next = list->next_;
+    list->next_ = prev;
     prev = list;
     list = next;
   }
   return prev;
 }
 
-// Goes through the list starting at |head| consuming node->dependent and
-// passing it to the provided |visitor|.
-void DispatchAll(DependentList::Node* head, DependentList::Visitor* visitor) {
+// static
+void DependentList::DispatchAll(DependentList::Node* head,
+                                DependentList::Visitor* visitor,
+                                bool retain_prerequsites) {
   head = ReverseList(head);
   DependentList::Node* next = nullptr;
   while (head) {
-    next = head->next;
-    // consume_fn might delete the node, so no access to node past this
+    next = head->next_;
+    if (retain_prerequsites)
+      head->RetainSettledPrerequisite();
+    // |visitor| might delete the node, so no access to node past this
     // call!
-    visitor->Visit(std::move(head->dependent));
+    visitor->Visit(std::move(head->dependent_));
     head = next;
   }
 }
 
-}  // namespace
-
 DependentList::Visitor::~Visitor() = default;
 
 DependentList::Node::Node() = default;
-DependentList::Node::Node(DependentList::Node&& other) = default;
-DependentList::Node& DependentList::Node::operator=(
-    DependentList::Node&& other) = default;
-DependentList::Node::~Node() = default;
+
+DependentList::Node::Node(Node&& other) {
+  prerequisite_ = other.prerequisite_.load(std::memory_order_relaxed);
+  other.prerequisite_ = 0;
+  dependent_ = std::move(other.dependent_);
+  DCHECK_EQ(other.next_, nullptr);
+}
+
+DependentList::Node::Node(AbstractPromise* prerequisite,
+                          scoped_refptr<AbstractPromise> dependent)
+    : prerequisite_(reinterpret_cast<intptr_t>(prerequisite)),
+      dependent_(std::move(dependent)) {}
+
+DependentList::Node::~Node() {
+  ClearPrerequisite();
+}
 
 DependentList::DependentList(State initial_state)
     : data_(CreateData(nullptr,
@@ -67,8 +80,50 @@ DependentList::DependentList(ConstructRejected)
 
 DependentList::~DependentList() = default;
 
+void DependentList::Node::Reset(AbstractPromise* prerequisite,
+                                scoped_refptr<AbstractPromise> dependent) {
+  SetPrerequisite(prerequisite);
+  dependent_ = std::move(dependent);
+  next_ = nullptr;
+}
+
+void DependentList::Node::SetPrerequisite(AbstractPromise* prerequisite) {
+  DCHECK(prerequisite);
+  intptr_t prev_value = prerequisite_.exchange(
+      reinterpret_cast<intptr_t>(prerequisite), std::memory_order_acq_rel);
+
+  if (prev_value & kIsRetained)
+    reinterpret_cast<AbstractPromise*>(prev_value & ~kIsRetained)->Release();
+}
+
+AbstractPromise* DependentList::Node::prerequisite() const {
+  return reinterpret_cast<AbstractPromise*>(
+      prerequisite_.load(std::memory_order_acquire) & ~kIsRetained);
+}
+
+void DependentList::Node::RetainSettledPrerequisite() {
+  intptr_t prerequisite = prerequisite_.load(std::memory_order_acquire);
+  DCHECK((prerequisite & kIsRetained) == 0) << "May only be called once";
+  if (!prerequisite)
+    return;
+
+  // Mark as retained, note we could have another thread trying to call
+  // ClearPrerequisite.
+  if (prerequisite_.compare_exchange_strong(
+          prerequisite, prerequisite | kIsRetained, std::memory_order_release,
+          std::memory_order_acquire)) {
+    reinterpret_cast<AbstractPromise*>(prerequisite)->AddRef();
+  }
+}
+
+void DependentList::Node::ClearPrerequisite() {
+  intptr_t prerequisite = prerequisite_.exchange(0, std::memory_order_acq_rel);
+  if (prerequisite & kIsRetained)
+    reinterpret_cast<AbstractPromise*>(prerequisite & ~kIsRetained)->Release();
+}
+
 DependentList::InsertResult DependentList::Insert(Node* node) {
-  DCHECK(!node->next);
+  DCHECK(!node->next_);
 
   // std::memory_order_acquire for hapens-after relation with
   // SettleAndDispatchAllDependents completing and thus this this call returning
@@ -76,7 +131,7 @@ DependentList::InsertResult DependentList::Insert(Node* node) {
   uintptr_t prev_data = data_.load(std::memory_order_acquire);
   bool did_insert = false;
   while (IsAllowingInserts(prev_data) && !did_insert) {
-    node->next = ExtractHead(prev_data);
+    node->next_ = ExtractHead(prev_data);
 
     // On success std::memory_order_release so that all memory operations become
     // visible in SettleAndDispatchAllDependents when iterating the list.
@@ -90,11 +145,11 @@ DependentList::InsertResult DependentList::Insert(Node* node) {
     // new node but with the same address so node->next is still valid).
     if (data_.compare_exchange_weak(
             prev_data, CreateData(node, ExtractState(prev_data), kAllowInserts),
-            std::memory_order_release, std::memory_order_acquire)) {
+            std::memory_order_seq_cst, std::memory_order_seq_cst)) {
       did_insert = true;
     } else {
       // Cleanup in case the loop terminates
-      node->next = nullptr;
+      node->next_ = nullptr;
     }
   }
 
@@ -132,7 +187,7 @@ bool DependentList::SettleAndDispatchAllDependents(const State settled_state,
   // This load, and the ones in for compare_exchange_weak failures can be
   // std::memory_order_relaxed as we do not make any ordering guarantee when
   // this method returns false.
-  uintptr_t prev_data = data_.load(std::memory_order_relaxed);
+  uintptr_t prev_data = data_.load(std::memory_order_seq_cst);
   while (true) {
     if (!did_set_state && ExtractState(prev_data) != State::kUnresolved) {
       // Somebody else set the state.
@@ -156,12 +211,14 @@ bool DependentList::SettleAndDispatchAllDependents(const State settled_state,
       // On success std::memory_order_acquire for happens-after relation with
       // with the last successful Insert().
       if (!data_.compare_exchange_weak(prev_data, new_data,
-                                       std::memory_order_acquire,
+                                       std::memory_order_seq_cst,
                                        std::memory_order_relaxed)) {
         continue;
       }
       did_set_state = true;
-      DispatchAll(ExtractHead(prev_data), visitor);
+      // We don't want to retain prerequisites when cancelling.
+      DispatchAll(ExtractHead(prev_data), visitor,
+                  settled_state != State::kCanceled);
       prev_data = new_data;
     }
 
@@ -175,7 +232,7 @@ bool DependentList::SettleAndDispatchAllDependents(const State settled_state,
     // Insert returning an error.
     if (data_.compare_exchange_weak(
             prev_data, CreateData(nullptr, settled_state, kBlockInserts),
-            std::memory_order_release, std::memory_order_relaxed)) {
+            std::memory_order_seq_cst, std::memory_order_relaxed)) {
       // Inserts no longer allowed, state settled and list is empty. We are
       // done!
       return true;
@@ -190,24 +247,36 @@ bool DependentList::SettleAndDispatchAllDependents(const State settled_state,
 // std::memory_order_relaxed.
 
 bool DependentList::IsSettled() const {
-  return ExtractState(data_.load(std::memory_order_relaxed)) !=
+  return ExtractState(data_.load(std::memory_order_seq_cst)) !=
          State::kUnresolved;
 }
 
 bool DependentList::IsResolved() const {
-  return ExtractState(data_.load(std::memory_order_relaxed)) ==
+  DCHECK(IsSettled()) << "This check is racy";
+  return ExtractState(data_.load(std::memory_order_seq_cst)) ==
          State::kResolved;
 }
 
 bool DependentList::IsRejected() const {
-  return ExtractState(data_.load(std::memory_order_relaxed)) ==
+  DCHECK(IsSettled()) << "This check is racy";
+  return ExtractState(data_.load(std::memory_order_seq_cst)) ==
          State::kRejected;
 }
 
 bool DependentList::IsCanceled() const {
-  return ExtractState(data_.load(std::memory_order_relaxed)) ==
+  return ExtractState(data_.load(std::memory_order_seq_cst)) ==
          State::kCanceled;
 }
 
+bool DependentList::IsResolvedForTesting() const {
+  return ExtractState(data_.load(std::memory_order_seq_cst)) ==
+         State::kResolved;
+}
+
+bool DependentList::IsRejectedForTesting() const {
+  return ExtractState(data_.load(std::memory_order_seq_cst)) ==
+         State::kRejected;
+}
+
 }  // namespace internal
 }  // namespace base

base/task/promise/dependent_list.h

@@ -59,14 +59,68 @@ class BASE_EXPORT DependentList {
 
   // Align Node on an 8-byte boundary to ensure the first 3 bits are 0 and can
   // be used to store additional state (see static_asserts below).
-  struct BASE_EXPORT alignas(8) Node {
+  class BASE_EXPORT alignas(8) Node {
+   public:
     Node();
     explicit Node(Node&& other) noexcept;
-    Node& operator=(Node&& other) noexcept;
+
+    // Constructs a Node, |prerequisite| will not be retained unless
+    // RetainSettledPrerequisite is called.
+    Node(AbstractPromise* prerequisite,
+         scoped_refptr<AbstractPromise> dependent);
     ~Node();
 
-    scoped_refptr<AbstractPromise> dependent;
-    Node* next = nullptr;
+    // Caution this is not thread safe.
+    void Reset(AbstractPromise* prerequisite,
+               scoped_refptr<AbstractPromise> dependent);
+
+    // Expected prerequisite usage:
+    // 1. prerequisite = null on creation (or is constructed with a value)
+    // 2. (optional, once only) SetPrerequisite(value)
+    // 3. (maybe, once only) RetainSettledPrerequisite();
+    // 4. (maybe) ClearPrerequisite()
+    // 5. Destructor called
+
+    // Can be called on any thread.
+    void SetPrerequisite(AbstractPromise* prerequisite);
+
+    // Can be called on any thread.
+    AbstractPromise* prerequisite() const;
+
+    scoped_refptr<AbstractPromise>& dependent() { return dependent_; }
+
+    const scoped_refptr<AbstractPromise>& dependent() const {
+      return dependent_;
+    }
+
+    Node* next() const { return next_; }
+
+    // Calls AddRef on |prerequisite()| and marks the prerequisite as being
+    // retained. The |prerequisite()| will be released by Node's destructor or
+    // a call to ClearPrerequisite. Does nothing if called more than once.
+    // Can be called on any thread at any time. Can be called once only.
+    void RetainSettledPrerequisite();
+
+    // Calls Release() if the rerequsite was retained and then sets
+    // |prerequisite_| to zero. Can be called on any thread at any time. Can be
+    // called more than once.
+    void ClearPrerequisite();
+
+   private:
+    friend class DependentList;
+
+    void MarkAsRetained() { prerequisite_ |= kIsRetained; }
+
+    // An AbstractPromise* where the LSB is a flag which specified if it's
+    // retained or not.
+    // A reference for |prerequisite_| is acquired with an explicit call to
+    // AddRef() if it's resolved or rejected.
+    std::atomic<intptr_t> prerequisite_{0};
+
+    scoped_refptr<AbstractPromise> dependent_;
+    Node* next_ = nullptr;
+
+    static constexpr intptr_t kIsRetained = 1;
   };
 
   // Insert will only succeed if neither ResolveAndConsumeAllDependents nor
@@ -128,9 +182,17 @@ class BASE_EXPORT DependentList {
   //
   // ATTENTION: No guarantees are made as of whether the
   // (Resolve/Reject/Cancel)AndConsumeAllDependents method is still executing.
+  bool IsCanceled() const;
+
+  // DCHECKs if not settled.
   bool IsResolved() const;
+
+  // DCHECKs if not settled.
   bool IsRejected() const;
-  bool IsCanceled() const;
+
+  // Like the above but doesn't DCHECK if unsettled.
+  bool IsResolvedForTesting() const;
+  bool IsRejectedForTesting() const;
 
  private:
   // The data for this class is:
@@ -224,6 +286,14 @@ class BASE_EXPORT DependentList {
   // already settled it does nothing and returns false, true otherwise.
   bool SettleAndDispatchAllDependents(State settled_state, Visitor* visitor);
 
+  static DependentList::Node* ReverseList(DependentList::Node* list);
+
+  // Goes through the list starting at |head| consuming node->dependent and
+  // passing it to the provided |visitor|.
+  static void DispatchAll(DependentList::Node* head,
+                          DependentList::Visitor* visitor,
+                          bool retain_prerequsites);
+
   std::atomic<uintptr_t> data_;
 };
 

base/task/promise/dependent_list_unittest.cc

@@ -9,6 +9,7 @@
 
 #include "base/memory/scoped_refptr.h"
 #include "base/task/promise/abstract_promise.h"
+#include "base/test/do_nothing_promise.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -45,9 +46,9 @@ TEST(DependentList, ConstructUnresolved) {
   DependentList list(DependentList::ConstructUnresolved{});
   DependentList::Node node;
   EXPECT_EQ(DependentList::InsertResult::SUCCESS, list.Insert(&node));
-  EXPECT_FALSE(list.IsRejected());
+  EXPECT_FALSE(list.IsRejectedForTesting());
   EXPECT_FALSE(list.IsCanceled());
-  EXPECT_FALSE(list.IsResolved());
+  EXPECT_FALSE(list.IsResolvedForTesting());
   EXPECT_FALSE(list.IsSettled());
 }
 
@@ -82,11 +83,12 @@ TEST(DependentList, ResolveAndConsumeAllDependents) {
   EXPECT_EQ(DependentList::InsertResult::SUCCESS, list.Insert(&node2));
   EXPECT_EQ(DependentList::InsertResult::SUCCESS, list.Insert(&node3));
 
-  EXPECT_FALSE(list.IsResolved());
+  EXPECT_FALSE(list.IsResolvedForTesting());
   EXPECT_FALSE(list.IsSettled());
 
-  std::vector<AbstractPromise*> expected_dependants = {
-      node1.dependent.get(), node2.dependent.get(), node3.dependent.get()};
+  std::vector<AbstractPromise*> expected_dependants = {node1.dependent().get(),
+                                                       node2.dependent().get(),
+                                                       node3.dependent().get()};
 
   PushBackVisitor visitor;
   list.ResolveAndConsumeAllDependents(&visitor);
@@ -110,10 +112,11 @@ TEST(DependentList, RejectAndConsumeAllDependents) {
   EXPECT_EQ(DependentList::InsertResult::SUCCESS, list.Insert(&node2));
   EXPECT_EQ(DependentList::InsertResult::SUCCESS, list.Insert(&node3));
 
-  EXPECT_FALSE(list.IsResolved());
+  EXPECT_FALSE(list.IsResolvedForTesting());
   EXPECT_FALSE(list.IsSettled());
-  std::vector<AbstractPromise*> expected_dependants = {
-      node1.dependent.get(), node2.dependent.get(), node3.dependent.get()};
+  std::vector<AbstractPromise*> expected_dependants = {node1.dependent().get(),
+                                                       node2.dependent().get(),
+                                                       node3.dependent().get()};
 
   PushBackVisitor visitor;
   list.RejectAndConsumeAllDependents(&visitor);
@@ -137,10 +140,11 @@ TEST(DependentList, CancelAndConsumeAllDependents) {
   EXPECT_EQ(DependentList::InsertResult::SUCCESS, list.Insert(&node2));
   EXPECT_EQ(DependentList::InsertResult::SUCCESS, list.Insert(&node3));
 
-  EXPECT_FALSE(list.IsResolved());
+  EXPECT_FALSE(list.IsResolvedForTesting());
   EXPECT_FALSE(list.IsSettled());
-  std::vector<AbstractPromise*> expected_dependants = {
-      node1.dependent.get(), node2.dependent.get(), node3.dependent.get()};
+  std::vector<AbstractPromise*> expected_dependants = {node1.dependent().get(),
+                                                       node2.dependent().get(),
+                                                       node3.dependent().get()};
 
   PushBackVisitor visitor;
   EXPECT_TRUE(list.CancelAndConsumeAllDependents(&visitor));
@@ -181,5 +185,25 @@ TEST(DependentList, NextPowerOfTwo) {
                 "");
 }
 
+TEST(DependentListNode, Simple) {
+  DependentList::Node node;
+  EXPECT_EQ(nullptr, node.prerequisite());
+
+  scoped_refptr<AbstractPromise> p = DoNothingPromiseBuilder(FROM_HERE);
+  EXPECT_TRUE(p->HasOneRef());
+  node.SetPrerequisite(p.get());
+  EXPECT_EQ(p.get(), node.prerequisite());
+  EXPECT_TRUE(p->HasOneRef());
+
+  EXPECT_TRUE(p->HasOneRef());
+  node.RetainSettledPrerequisite();
+  EXPECT_EQ(p.get(), node.prerequisite());
+  EXPECT_FALSE(p->HasOneRef());
+
+  node.ClearPrerequisite();
+  EXPECT_EQ(nullptr, node.prerequisite());
+  EXPECT_TRUE(p->HasOneRef());
+}
+
 }  // namespace internal
 }  // namespace base

base/task/promise/promise.h

@@ -116,12 +116,12 @@ class Promise {
 
   bool IsResolvedForTesting() const {
     DCHECK(abstract_promise_);
-    return abstract_promise_->IsResolved();
+    return abstract_promise_->IsResolvedForTesting();
   }
 
   bool IsRejectedForTesting() const {
     DCHECK(abstract_promise_);
-    return abstract_promise_->IsRejected();
+    return abstract_promise_->IsRejectedForTesting();
   }
 
   // A task to execute |on_reject| is posted on |task_runner| as soon as this
@@ -173,7 +173,7 @@ class Promise {
                    ReturnedPromiseRejectT>(internal::AbstractPromise::Create(
         std::move(task_runner), from_here,
         std::make_unique<internal::AbstractPromise::AdjacencyList>(
-            abstract_promise_),
+            abstract_promise_.get()),
         RejectPolicy::kMustCatchRejection,
         internal::AbstractPromise::ConstructWith<
             internal::DependentList::ConstructUnresolved,
@@ -250,7 +250,7 @@ class Promise {
         internal::AbstractPromise::Create(
             std::move(task_runner), from_here,
             std::make_unique<internal::AbstractPromise::AdjacencyList>(
-                abstract_promise_),
+                abstract_promise_.get()),
             RejectPolicy::kMustCatchRejection,
             internal::AbstractPromise::ConstructWith<
                 internal::DependentList::ConstructUnresolved,
@@ -346,7 +346,7 @@ class Promise {
                    ReturnedPromiseRejectT>(internal::AbstractPromise::Create(
         std::move(task_runner), from_here,
         std::make_unique<internal::AbstractPromise::AdjacencyList>(
-            abstract_promise_),
+            abstract_promise_.get()),
         RejectPolicy::kMustCatchRejection,
         internal::AbstractPromise::ConstructWith<
             internal::DependentList::ConstructUnresolved,
@@ -406,7 +406,7 @@ class Promise {
         internal::AbstractPromise::Create(
             std::move(task_runner), from_here,
             std::make_unique<internal::AbstractPromise::AdjacencyList>(
-                abstract_promise_),
+                abstract_promise_.get()),
             RejectPolicy::kMustCatchRejection,
             internal::AbstractPromise::ConstructWith<
                 internal::DependentList::ConstructUnresolved,
@@ -514,17 +514,16 @@ class ManualPromiseResolver {
       RejectPolicy reject_policy = RejectPolicy::kMustCatchRejection)
       : promise_(SequencedTaskRunnerHandle::Get(), from_here, reject_policy) {}
 
-  ~ManualPromiseResolver() {
-    // If the promise wasn't resolved or rejected, then cancel it to make sure
-    // we don't leak memory.
-    if (!promise_.abstract_promise_->IsSettled())
-      promise_.abstract_promise_->OnCanceled();
+  ~ManualPromiseResolver() = default;
+
+  void Resolve(Promise<ResolveType, RejectType> promise) noexcept {
+    promise_.abstract_promise_->emplace(std::move(promise.abstract_promise_));
+    promise_.abstract_promise_->OnResolved();
   }
 
   template <typename... Args>
   void Resolve(Args&&... arg) noexcept {
-    DCHECK(!promise_.abstract_promise_->IsResolved());
-    DCHECK(!promise_.abstract_promise_->IsRejected());
+    DCHECK(!promise_.abstract_promise_->IsSettled());
     static_assert(!std::is_same<NoResolve, ResolveType>::value,
                   "Can't resolve a NoResolve promise.");
     promise_.abstract_promise_->emplace(
@@ -534,8 +533,7 @@ class ManualPromiseResolver {
 
   template <typename... Args>
   void Reject(Args&&... arg) noexcept {
-    DCHECK(!promise_.abstract_promise_->IsResolved());
-    DCHECK(!promise_.abstract_promise_->IsRejected());
+    DCHECK(!promise_.abstract_promise_->IsSettled());
     static_assert(!std::is_same<NoReject, RejectType>::value,
                   "Can't reject a NoReject promise.");
     promise_.abstract_promise_->emplace(
@@ -637,11 +635,11 @@ class Promises {
         std::tuple<internal::ToNonVoidT<Resolve>...>;
     using ReturnedPromiseRejectT = Reject;
 
-    std::vector<internal::AbstractPromise::AdjacencyListNode> prerequisite_list(
+    std::vector<internal::DependentList::Node> prerequisite_list(
         sizeof...(promises));
     int i = 0;
     for (auto&& p : {promises.abstract_promise_...}) {
-      prerequisite_list[i++].prerequisite = std::move(p);
+      prerequisite_list[i++].SetPrerequisite(p.get());
     }
     return Promise<ReturnedPromiseResolveT, ReturnedPromiseRejectT>(
         internal::AbstractPromise::Create(

base/task/promise/promise_unittest.cc

@@ -15,27 +15,11 @@
 #include "base/test/scoped_task_environment.h"
 #include "base/test/test_mock_time_task_runner.h"
 #include "base/threading/thread.h"
+#include "base/threading/thread_task_runner_handle.h"
 #include "base/values.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
-// TODO(crbug.com/968302): Fix memory leaks in tests and re-enable on LSAN.
-#ifdef LEAK_SANITIZER
-#define MAYBE_ThenRejectWithTuple DISABLED_ThenRejectWithTuple
-#define MAYBE_TargetTaskRunnerClearsTasks DISABLED_TargetTaskRunnerClearsTasks
-#define MAYBE_MoveOnlyTypeMultipleThensNotAllowed \
-  DISABLED_MoveOnlyTypeMultipleThensNotAllowed
-#define MAYBE_MoveOnlyTypeMultipleCatchesNotAllowed \
-  DISABLED_MoveOnlyTypeMultipleCatchesNotAllowed
-#else
-#define MAYBE_ThenRejectWithTuple ThenRejectWithTuple
-#define MAYBE_TargetTaskRunnerClearsTasks TargetTaskRunnerClearsTasks
-#define MAYBE_MoveOnlyTypeMultipleThensNotAllowed \
-  MoveOnlyTypeMultipleThensNotAllowed
-#define MAYBE_MoveOnlyTypeMultipleCatchesNotAllowed \
-  MoveOnlyTypeMultipleCatchesNotAllowed
-#endif
-
 using testing::ElementsAre;
 
 namespace base {
@@ -93,7 +77,7 @@ class PromiseTest : public testing::Test {
   test::ScopedTaskEnvironment scoped_task_environment_;
 };
 
-TEST(PromiseMemoryLeakTest, MAYBE_TargetTaskRunnerClearsTasks) {
+TEST(PromiseMemoryLeakTest, TargetTaskRunnerClearsTasks) {
   scoped_refptr<TestMockTimeTaskRunner> post_runner =
       MakeRefCounted<TestMockTimeTaskRunner>();
   scoped_refptr<TestMockTimeTaskRunner> reply_runner =
@@ -277,7 +261,7 @@ TEST_F(PromiseTest, CreateResolvedThen) {
   run_loop.Run();
 }
 
-TEST_F(PromiseTest, MAYBE_ThenRejectWithTuple) {
+TEST_F(PromiseTest, ThenRejectWithTuple) {
   ManualPromiseResolver<void> p(FROM_HERE);
   p.Resolve();
 
@@ -1048,6 +1032,95 @@ TEST_F(PromiseTest, CurriedIntPromise) {
   run_loop.Run();
 }
 
+TEST_F(PromiseTest, CurriedIntPromiseChain) {
+  Promise<int> p = Promise<int>::CreateResolved(FROM_HERE, 1000);
+
+  ManualPromiseResolver<int> promise_resolver_1(FROM_HERE);
+  ManualPromiseResolver<int> promise_resolver_2(FROM_HERE);
+  promise_resolver_2.Resolve(promise_resolver_1.promise());
+  promise_resolver_1.Resolve(123);
+
+  RunLoop run_loop;
+  p.ThenHere(FROM_HERE, BindLambdaForTesting([&](int result) {
+               EXPECT_EQ(1000, result);
+               return promise_resolver_2.promise();
+             }))
+      .ThenHere(FROM_HERE, BindLambdaForTesting([&](int result) {
+                  EXPECT_EQ(123, result);
+                  run_loop.Quit();
+                }));
+
+  run_loop.Run();
+}
+
+TEST_F(PromiseTest, CurriedIntPromiseChain2) {
+  Promise<int> p1 = Promise<int>::CreateResolved(FROM_HERE, 1000);
+  Promise<int> p2 = Promise<int>::CreateResolved(FROM_HERE, 789);
+  Promise<int> then2;
+
+  {
+    Promise<int> then1 =
+        Promise<int>::CreateResolved(FROM_HERE, 789)
+            .ThenHere(FROM_HERE, BindLambdaForTesting([&]() { return p2; }));
+    then2 = Promise<int>::CreateResolved(FROM_HERE, 789)
+                .ThenHere(
+                    FROM_HERE,
+                    BindOnce([&](Promise<int> then1) { return then1; }, then1));
+  }
+
+  RunLoop run_loop;
+  p1.ThenHere(FROM_HERE, BindLambdaForTesting([&](int result) {
+                EXPECT_EQ(1000, result);
+                return then2;
+              }))
+      .ThenHere(FROM_HERE, BindLambdaForTesting([&](int result) {
+                  EXPECT_EQ(789, result);
+                  run_loop.Quit();
+                }));
+
+  run_loop.Run();
+}
+
+TEST_F(PromiseTest, CurriedIntPromiseChainThenAddedAfterInitialResolve) {
+  ManualPromiseResolver<int> promise_resolver_1(FROM_HERE);
+  ManualPromiseResolver<int> promise_resolver_2(FROM_HERE);
+  ManualPromiseResolver<int> promise_resolver_3(FROM_HERE);
+  promise_resolver_2.Resolve(promise_resolver_1.promise());
+  promise_resolver_3.Resolve(promise_resolver_2.promise());
+
+  RunLoop run_loop;
+  promise_resolver_3.promise().ThenHere(FROM_HERE,
+                                        BindLambdaForTesting([&](int result) {
+                                          EXPECT_EQ(123, result);
+                                          run_loop.Quit();
+                                        }));
+
+  ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE,
+      BindLambdaForTesting([&]() { promise_resolver_1.Resolve(123); }));
+
+  run_loop.Run();
+}
+
+TEST_F(PromiseTest, CurriedVoidPromiseModified) {
+  for (size_t i = 0; i < 1000; ++i) {
+    Promise<void> p = Promise<void>::CreateResolved(FROM_HERE);
+    std::unique_ptr<ManualPromiseResolver<int>> promise_resolver =
+        std::make_unique<ManualPromiseResolver<int>>(FROM_HERE);
+    RunLoop run_loop;
+    p.ThenHere(FROM_HERE, BindOnce([](Promise<int> promise) { return promise; },
+                                   promise_resolver->promise()))
+        .ThenHere(FROM_HERE, base::BindOnce([](int v) { EXPECT_EQ(v, 42); }))
+        .ThenHere(FROM_HERE, run_loop.QuitClosure());
+    base::PostTaskWithTraits(FROM_HERE, {}, base::BindLambdaForTesting([&]() {
+                               promise_resolver->Resolve(42);
+                               promise_resolver.reset();
+                             }));
+    run_loop.Run();
+    scoped_task_environment_.RunUntilIdle();
+  }
+}
+
 TEST_F(PromiseTest, PromiseResultReturningAPromise) {
   Promise<int> p = Promise<int>::CreateResolved(FROM_HERE, 1000);
   ManualPromiseResolver<int> promise_resolver(FROM_HERE);
@@ -1476,7 +1549,7 @@ TEST_F(PromiseTest, CatchNotRequired) {
   run_loop.Run();
 }
 
-TEST_F(PromiseTest, MAYBE_MoveOnlyTypeMultipleThensNotAllowed) {
+TEST_F(PromiseTest, MoveOnlyTypeMultipleThensNotAllowed) {
 #if DCHECK_IS_ON()
   Promise<std::unique_ptr<int>> p =
       Promise<std::unique_ptr<int>>::CreateResolved(FROM_HERE,
@@ -1492,7 +1565,7 @@ TEST_F(PromiseTest, MAYBE_MoveOnlyTypeMultipleThensNotAllowed) {
 #endif
 }
 
-TEST_F(PromiseTest, MAYBE_MoveOnlyTypeMultipleCatchesNotAllowed) {
+TEST_F(PromiseTest, MoveOnlyTypeMultipleCatchesNotAllowed) {
 #if DCHECK_IS_ON()
   auto p = Promise<void, std::unique_ptr<int>>::CreateRejected(
       FROM_HERE, std::make_unique<int>(123));