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

@@ -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_));

folly/futures/test/SelfDestructTest.cpp

+/*
+ * 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);
+}

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 \