From 914e877b032b402e31990f80dfef1bf489daf311 Mon Sep 17 00:00:00 2001
From: Sven Over <over@fb.com>
Date: Tue, 23 Feb 2016 23:46:14 -0800
Subject: [PATCH] folly/futures: keep Core alive until callback has returned

Summary:Callbacks passed to e.g. folly::Future::then (or
folly::Future::ensure) may delete the promise that keeps the
Core of the same future alive. The Core must protect itself from
destruction during callback execution.

This commit also adds a unit test to check for correct behaviour
in the "self destruction" scenario. The test should usually pass
even without the fix in Core.h. However, when you run the test
in Valgrind or ASAN, it will report problems unless the fix in
Core.h is applied.

Reviewed By: mhx

Differential Revision: D2938094

fb-gh-sync-id: 22796e168e1876ef2e3c7d7619da020be6a22073
shipit-source-id: 22796e168e1876ef2e3c7d7619da020be6a22073
---
 folly/futures/detail/Core.h             |  6 ++--
 folly/futures/test/SelfDestructTest.cpp | 38 +++++++++++++++++++++++++
 folly/test/Makefile.am                  |  1 +
 3 files changed, 43 insertions(+), 2 deletions(-)
 create mode 100644 folly/futures/test/SelfDestructTest.cpp

diff --git a/folly/futures/detail/Core.h b/folly/futures/detail/Core.h
index ecacb280..74c9ef28 100644
--- a/folly/futures/detail/Core.h
+++ b/folly/futures/detail/Core.h
@@ -327,9 +327,10 @@ class Core {
       executorLock_.unlock();
     }
 
+    // keep Core alive until callback did its thing
+    ++attached_;
+
     if (x) {
-      // keep Core alive until executor did its thing
-      ++attached_;
       try {
         if (LIKELY(x->getNumPriorities() == 1)) {
           x->add([this]() mutable {
@@ -354,6 +355,7 @@ class Core {
         callback_(std::move(*result_));
       }
     } else {
+      SCOPE_EXIT { detachOne(); };
       RequestContext::setContext(context_);
       SCOPE_EXIT { callback_ = {}; };
       callback_(std::move(*result_));
diff --git a/folly/futures/test/SelfDestructTest.cpp b/folly/futures/test/SelfDestructTest.cpp
new file mode 100644
index 00000000..52842f44
--- /dev/null
+++ b/folly/futures/test/SelfDestructTest.cpp
@@ -0,0 +1,38 @@
+/*
+ * Copyright 2016 Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include <gtest/gtest.h>
+
+#include <folly/futures/Future.h>
+
+using namespace folly;
+
+TEST(SelfDestruct, then) {
+  auto* p = new Promise<int>();
+  auto future = p->getFuture().then([p](int x) {
+    delete p;
+    return x + 1;
+  });
+  p->setValue(123);
+  EXPECT_EQ(future.get(), 124);
+}
+
+TEST(SelfDestruct, ensure) {
+  auto* p = new Promise<int>();
+  auto future = p->getFuture().ensure([p] { delete p; });
+  p->setValue(123);
+  EXPECT_EQ(future.get(), 123);
+}
diff --git a/folly/test/Makefile.am b/folly/test/Makefile.am
index 02e57278..e5414567 100644
--- a/folly/test/Makefile.am
+++ b/folly/test/Makefile.am
@@ -229,6 +229,7 @@ futures_test_SOURCES = \
     ../futures/test/PromiseTest.cpp \
     ../futures/test/ReduceTest.cpp \
     ../futures/test/RetryingTest.cpp \
+    ../futures/test/SelfDestructTest.cpp \
     ../futures/test/SharedPromiseTest.cpp \
     ../futures/test/ThenCompileTest.cpp \
     ../futures/test/ThenTest.cpp \
-- 
2.34.1