Fix race between resolved promise construction and cross thread destruction

When a refcounted object is constructed, it's refcount is initally zero.
This is a problem if the constructor can post a task to another thread
bacause the scoped_refptr destructor on the other thread might run before
the posting thread has had a chance to increment the refcount.  This
results in a UAF on the posting thread.

The work around is to ensure the refcount is not zero before posting.

Bug: 966964, 906125
Change-Id: Idf3163f979a49abe39088f5af72e94dee4492833
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1628758
Commit-Queue: Alex Clarke <alexclarke@chromium.org>
Reviewed-by: François Doray <fdoray@chromium.org>
Cr-Commit-Position: refs/heads/master@{#664715}

base/task/promise/abstract_promise.cc

@@ -45,7 +45,8 @@ const AbstractPromise* AbstractPromise::FindNonCurriedAncestor() const {
 }
 
 void AbstractPromise::AddAsDependentForAllPrerequisites() {
-  DCHECK(prerequisites_);
+  if (!prerequisites_)
+    return;
 
   // Note a curried promise will eventually get to all its children and pass
   // them catch responsibility through AddAsDependentForAllPrerequisites,

base/task/promise/abstract_promise.h

@@ -115,48 +115,23 @@ class BASE_EXPORT AbstractPromise
   template <typename ConstructType,
             typename DerivedExecutorType,
             typename... ExecutorArgs>
-  AbstractPromise(scoped_refptr<TaskRunner>&& task_runner,
-                  const Location& from_here,
-                  std::unique_ptr<AdjacencyList> prerequisites,
-                  RejectPolicy reject_policy,
-                  ConstructWith<ConstructType, DerivedExecutorType>,
-                  ExecutorArgs&&... executor_args) noexcept
-      : task_runner_(std::move(task_runner)),
-        from_here_(std::move(from_here)),
-        value_(in_place_type_t<Executor>(),
-               in_place_type_t<DerivedExecutorType>(),
-               std::forward<ExecutorArgs>(executor_args)...),
-#if DCHECK_IS_ON()
-        reject_policy_(reject_policy),
-        resolve_argument_passing_type_(
-            GetExecutor()->ResolveArgumentPassingType()),
-        reject_argument_passing_type_(
-            GetExecutor()->RejectArgumentPassingType()),
-        executor_can_resolve_(GetExecutor()->CanResolve()),
-        executor_can_reject_(GetExecutor()->CanReject()),
-#endif
-        dependents_(ConstructType()),
-        prerequisites_(std::move(prerequisites)) {
-#if DCHECK_IS_ON()
-    {
-      CheckedAutoLock lock(GetCheckedLock());
-      if (executor_can_resolve_) {
-        this_resolve_ =
-            MakeRefCounted<DoubleMoveDetector>(from_here_, "resolve");
-      }
-
-      if (executor_can_reject_) {
-        this_reject_ = MakeRefCounted<DoubleMoveDetector>(from_here_, "reject");
-
-        if (reject_policy_ == RejectPolicy::kMustCatchRejection) {
-          this_must_catch_ = MakeRefCounted<LocationRef>(from_here_);
-        }
-      }
-    }
-#endif
-
-    if (prerequisites_)
-      AddAsDependentForAllPrerequisites();
+  static scoped_refptr<AbstractPromise> Create(
+      scoped_refptr<TaskRunner>&& task_runner,
+      const Location& from_here,
+      std::unique_ptr<AdjacencyList> prerequisites,
+      RejectPolicy reject_policy,
+      ConstructWith<ConstructType, DerivedExecutorType> tag,
+      ExecutorArgs&&... executor_args) {
+    scoped_refptr<AbstractPromise> promise = subtle::AdoptRefIfNeeded(
+        new internal::AbstractPromise(
+            std::move(task_runner), from_here, std::move(prerequisites),
+            reject_policy, tag, std::forward<ExecutorArgs>(executor_args)...),
+        AbstractPromise::kRefCountPreference);
+    // It's important this is called after |promise| has been initialized
+    // because otherwise it could trigger a scoped_refptr destructor on another
+    // thread before this thread has had a chance to increment the refcount.
+    promise->AddAsDependentForAllPrerequisites();
+    return promise;
   }
 
   AbstractPromise(const AbstractPromise&) = delete;
@@ -454,6 +429,50 @@ class BASE_EXPORT AbstractPromise
  private:
   friend base::RefCountedThreadSafe<AbstractPromise>;
 
+  template <typename ConstructType,
+            typename DerivedExecutorType,
+            typename... ExecutorArgs>
+  AbstractPromise(scoped_refptr<TaskRunner>&& task_runner,
+                  const Location& from_here,
+                  std::unique_ptr<AdjacencyList> prerequisites,
+                  RejectPolicy reject_policy,
+                  ConstructWith<ConstructType, DerivedExecutorType>,
+                  ExecutorArgs&&... executor_args) noexcept
+      : task_runner_(std::move(task_runner)),
+        from_here_(std::move(from_here)),
+        value_(in_place_type_t<Executor>(),
+               in_place_type_t<DerivedExecutorType>(),
+               std::forward<ExecutorArgs>(executor_args)...),
+#if DCHECK_IS_ON()
+        reject_policy_(reject_policy),
+        resolve_argument_passing_type_(
+            GetExecutor()->ResolveArgumentPassingType()),
+        reject_argument_passing_type_(
+            GetExecutor()->RejectArgumentPassingType()),
+        executor_can_resolve_(GetExecutor()->CanResolve()),
+        executor_can_reject_(GetExecutor()->CanReject()),
+#endif
+        dependents_(ConstructType()),
+        prerequisites_(std::move(prerequisites)) {
+#if DCHECK_IS_ON()
+    {
+      CheckedAutoLock lock(GetCheckedLock());
+      if (executor_can_resolve_) {
+        this_resolve_ =
+            MakeRefCounted<DoubleMoveDetector>(from_here_, "resolve");
+      }
+
+      if (executor_can_reject_) {
+        this_reject_ = MakeRefCounted<DoubleMoveDetector>(from_here_, "reject");
+
+        if (reject_policy_ == RejectPolicy::kMustCatchRejection) {
+          this_must_catch_ = MakeRefCounted<LocationRef>(from_here_);
+        }
+      }
+    }
+#endif
+  }
+
   NOINLINE ~AbstractPromise();
 
   // Returns the associated Executor if there is one.

base/task/promise/abstract_promise_unittest.cc

@@ -184,19 +184,17 @@ class AbstractPromiseTest : public testing::Test {
     }
 
     operator scoped_refptr<AbstractPromise>() {
-      return subtle::AdoptRefIfNeeded(
-          new AbstractPromise(
-              std::move(settings.task_runner), settings.from_here,
-              std::move(settings.prerequisites), settings.reject_policy,
-              AbstractPromise::ConstructWith<DependentList::ConstructUnresolved,
-                                             TestExecutor>(),
-              settings.prerequisite_policy,
+      return AbstractPromise::Create(
+          std::move(settings.task_runner), settings.from_here,
+          std::move(settings.prerequisites), settings.reject_policy,
+          AbstractPromise::ConstructWith<DependentList::ConstructUnresolved,
+                                         TestExecutor>(),
+          settings.prerequisite_policy,
 #if DCHECK_IS_ON()
-              settings.resolve_executor_type, settings.reject_executor_type,
-              settings.executor_can_resolve, settings.executor_can_reject,
+          settings.resolve_executor_type, settings.reject_executor_type,
+          settings.executor_can_resolve, settings.executor_can_reject,
 #endif
-              std::move(settings.callback)),
-          AbstractPromise::kRefCountPreference);
+          std::move(settings.callback));
     }
 
    private:

base/task/promise/all_container_executor.h

@@ -85,7 +85,7 @@ struct AllContainerHelper<Container, Promise<ResolveType, RejectType>> {
     for (auto& promise : promises) {
       prerequisite_list[i++].prerequisite = promise.abstract_promise_;
     }
-    return PromiseType(MakeRefCounted<AbstractPromise>(
+    return PromiseType(AbstractPromise::Create(
         nullptr, from_here,
         std::make_unique<AbstractPromise::AdjacencyList>(
             std::move(prerequisite_list)),

base/task/promise/no_op_promise_executor.cc

@@ -55,7 +55,7 @@ scoped_refptr<internal::AbstractPromise> NoOpPromiseExecutor::Create(
     bool can_resolve,
     bool can_reject,
     RejectPolicy reject_policy) {
-  return MakeRefCounted<internal::AbstractPromise>(
+  return internal::AbstractPromise::Create(
       nullptr, from_here, nullptr, reject_policy,
       internal::AbstractPromise::ConstructWith<
           internal::DependentList::ConstructUnresolved,

base/task/promise/promise.h

@@ -52,7 +52,7 @@ class Promise {
   Promise(scoped_refptr<TaskRunner> task_runner,
           const Location& location,
           RejectPolicy reject_policy)
-      : abstract_promise_(MakeRefCounted<internal::AbstractPromise>(
+      : abstract_promise_(internal::AbstractPromise::Create(
             std::move(task_runner),
             location,
             nullptr,
@@ -117,24 +117,23 @@ class Promise {
             std::is_const<std::remove_reference_t<RejectCallbackArgT>>::value,
         "Google C++ Style: References in function parameters must be const.");
 
-    return Promise<ReturnedPromiseResolveT, ReturnedPromiseRejectT>(
-        MakeRefCounted<internal::AbstractPromise>(
-            std::move(task_runner), from_here,
-            std::make_unique<internal::AbstractPromise::AdjacencyList>(
-                abstract_promise_),
-            RejectPolicy::kMustCatchRejection,
-            internal::AbstractPromise::ConstructWith<
-                internal::DependentList::ConstructUnresolved,
-                internal::ThenAndCatchExecutor<
-                    OnceClosure,  // Never called.
-                    OnceCallback<typename RejectCallbackTraits::SignatureType>,
-                    internal::NoCallback, RejectType,
-                    Resolved<ReturnedPromiseResolveT>,
-                    Rejected<ReturnedPromiseRejectT>>>(),
-            OnceClosure(),
-            static_cast<
-                OnceCallback<typename RejectCallbackTraits::SignatureType>>(
-                std::forward<RejectCb>(on_reject))));
+    return Promise<ReturnedPromiseResolveT,
+                   ReturnedPromiseRejectT>(internal::AbstractPromise::Create(
+        std::move(task_runner), from_here,
+        std::make_unique<internal::AbstractPromise::AdjacencyList>(
+            abstract_promise_),
+        RejectPolicy::kMustCatchRejection,
+        internal::AbstractPromise::ConstructWith<
+            internal::DependentList::ConstructUnresolved,
+            internal::ThenAndCatchExecutor<
+                OnceClosure,  // Never called.
+                OnceCallback<typename RejectCallbackTraits::SignatureType>,
+                internal::NoCallback, RejectType,
+                Resolved<ReturnedPromiseResolveT>,
+                Rejected<ReturnedPromiseRejectT>>>(),
+        OnceClosure(),
+        static_cast<OnceCallback<typename RejectCallbackTraits::SignatureType>>(
+            std::forward<RejectCb>(on_reject))));
   }
 
   template <typename RejectCb>
@@ -196,7 +195,7 @@ class Promise {
         "Google C++ Style: References in function parameters must be const.");
 
     return Promise<ReturnedPromiseResolveT, ReturnedPromiseRejectT>(
-        MakeRefCounted<internal::AbstractPromise>(
+        internal::AbstractPromise::Create(
             std::move(task_runner), from_here,
             std::make_unique<internal::AbstractPromise::AdjacencyList>(
                 abstract_promise_),
@@ -291,25 +290,24 @@ class Promise {
             std::is_const<std::remove_reference_t<RejectCallbackArgT>>::value,
         "Google C++ Style: References in function parameters must be const.");
 
-    return Promise<ReturnedPromiseResolveT, ReturnedPromiseRejectT>(
-        MakeRefCounted<internal::AbstractPromise>(
-            std::move(task_runner), from_here,
-            std::make_unique<internal::AbstractPromise::AdjacencyList>(
-                abstract_promise_),
-            RejectPolicy::kMustCatchRejection,
-            internal::AbstractPromise::ConstructWith<
-                internal::DependentList::ConstructUnresolved,
-                internal::ThenAndCatchExecutor<
-                    OnceCallback<typename ResolveCallbackTraits::SignatureType>,
-                    OnceCallback<typename RejectCallbackTraits::SignatureType>,
-                    ResolveType, RejectType, Resolved<ReturnedPromiseResolveT>,
-                    Rejected<ReturnedPromiseRejectT>>>(),
-            static_cast<
-                OnceCallback<typename ResolveCallbackTraits::SignatureType>>(
-                std::forward<ResolveCb>(on_resolve)),
-            static_cast<
-                OnceCallback<typename RejectCallbackTraits::SignatureType>>(
-                std::forward<RejectCb>(on_reject))));
+    return Promise<ReturnedPromiseResolveT,
+                   ReturnedPromiseRejectT>(internal::AbstractPromise::Create(
+        std::move(task_runner), from_here,
+        std::make_unique<internal::AbstractPromise::AdjacencyList>(
+            abstract_promise_),
+        RejectPolicy::kMustCatchRejection,
+        internal::AbstractPromise::ConstructWith<
+            internal::DependentList::ConstructUnresolved,
+            internal::ThenAndCatchExecutor<
+                OnceCallback<typename ResolveCallbackTraits::SignatureType>,
+                OnceCallback<typename RejectCallbackTraits::SignatureType>,
+                ResolveType, RejectType, Resolved<ReturnedPromiseResolveT>,
+                Rejected<ReturnedPromiseRejectT>>>(),
+        static_cast<
+            OnceCallback<typename ResolveCallbackTraits::SignatureType>>(
+            std::forward<ResolveCb>(on_resolve)),
+        static_cast<OnceCallback<typename RejectCallbackTraits::SignatureType>>(
+            std::forward<RejectCb>(on_reject))));
   }
 
   template <typename ResolveCb, typename RejectCb>
@@ -353,7 +351,7 @@ class Promise {
                   "|finally_callback| callback must have no arguments");
 
     return Promise<ReturnedPromiseResolveT, ReturnedPromiseRejectT>(
-        MakeRefCounted<internal::AbstractPromise>(
+        internal::AbstractPromise::Create(
             std::move(task_runner), from_here,
             std::make_unique<internal::AbstractPromise::AdjacencyList>(
                 abstract_promise_),
@@ -387,7 +385,7 @@ class Promise {
       const Location& from_here,
       Args&&... args) noexcept {
     scoped_refptr<internal::AbstractPromise> promise(
-        MakeRefCounted<internal::AbstractPromise>(
+        internal::AbstractPromise::Create(
             nullptr, from_here, nullptr, RejectPolicy::kMustCatchRejection,
             internal::AbstractPromise::ConstructWith<
                 internal::DependentList::ConstructResolved,
@@ -404,7 +402,7 @@ class Promise {
       const Location& from_here,
       Args&&... args) noexcept {
     scoped_refptr<internal::AbstractPromise> promise(
-        MakeRefCounted<internal::AbstractPromise>(
+        internal::AbstractPromise::Create(
             nullptr, from_here, nullptr, RejectPolicy::kMustCatchRejection,
             internal::AbstractPromise::ConstructWith<
                 internal::DependentList::ConstructRejected,
@@ -594,7 +592,7 @@ class Promises {
       prerequisite_list[i++].prerequisite = std::move(p);
     }
     return Promise<ReturnedPromiseResolveT, ReturnedPromiseRejectT>(
-        MakeRefCounted<internal::AbstractPromise>(
+        internal::AbstractPromise::Create(
             nullptr, from_here,
             std::make_unique<internal::AbstractPromise::AdjacencyList>(
                 std::move(prerequisite_list)),

base/task/promise/promise_unittest.cc

@@ -1635,8 +1635,7 @@ TEST_F(MultiThreadedPromiseTest, SimpleThreadHopping) {
   run_loop.Run();
 }
 
-// TODO(https://crbug.com/966964): Flakily crashes due to heap corruption.
-TEST_F(MultiThreadedPromiseTest, DISABLED_CrossThreadThens) {
+TEST_F(MultiThreadedPromiseTest, CrossThreadThens) {
   ManualPromiseResolver<void> promise_resolver(FROM_HERE);
 
   auto resolve_task =