Fix ReferenceCountedFutureImpl to avoid using FirestoreInternal after its deletion.

firestore/integration_test_internal/CMakeLists.txt

@@ -110,6 +110,7 @@ set(FIREBASE_INTEGRATION_TEST_ANDROID_TEST_SRCS
     src/android/geo_point_android_test.cc
     src/android/jni_runnable_android_test.cc
     src/android/promise_android_test.cc
+    src/android/promise_factory_android_test.cc
     src/android/settings_android_test.cc
     src/android/snapshot_metadata_android_test.cc
     src/android/timestamp_android_test.cc

firestore/integration_test_internal/src/android/promise_android_test.cc

@@ -403,6 +403,24 @@ TEST_F(PromiseTest, FutureNonVoidShouldCallCompletionWhenTaskCancels) {
   EXPECT_EQ(completion.result(), nullptr);
 }
 
+TEST_F(PromiseTest, RegisterForTaskShouldNotCrashIfFirestoreWasDeleted) {
+  jni::Env env = GetEnv();
+  auto promise = promises().MakePromise<void>();
+  DeleteFirestore(TestFirestore());
+
+  promise.RegisterForTask(env, AsyncFn::kFn, GetTask());
+}
+
+TEST_F(PromiseTest, GetFutureShouldNotCrashIfFirestoreWasDeleted) {
+  jni::Env env = GetEnv();
+  auto promise = promises().MakePromise<void>();
+  promise.RegisterForTask(env, AsyncFn::kFn, GetTask());
+  DeleteFirestore(TestFirestore());
+
+  auto future = promise.GetFuture();
+  EXPECT_EQ(future.status(), FutureStatus::kFutureStatusInvalid);
+}
+
 }  // namespace
 }  // namespace firestore
 }  // namespace firebase

firestore/integration_test_internal/src/android/promise_factory_android_test.cc

@@ -0,0 +1,132 @@
+// Copyright 2021 Google LLC
+
+#include "firestore/src/android/promise_factory_android.h"
+
+#include <utility>
+
+#include "android/firestore_integration_test_android.h"
+#include "android/task_completion_source.h"
+#include "firebase/future.h"
+#include "firestore/src/android/converter_android.h"
+#include "firestore/src/android/firestore_android.h"
+#include "firestore/src/jni/env.h"
+#include "firestore/src/jni/integer.h"
+#include "firestore/src/jni/ownership.h"
+#include "firestore/src/jni/task.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace firebase {
+namespace firestore {
+namespace {
+
+using jni::Env;
+using jni::Integer;
+using jni::Local;
+using jni::Task;
+
+// Since `PromiseFactory` acts as a "constructor" of `Promise` objects, its
+// ability to create `Promise` objects is thoroughly tested in the unit tests
+// for `Promise` and therefore the tests here only cover the other aspects of
+// `PromiseFactory`, such as move semantics.
+
+class PromiseFactoryTest : public FirestoreAndroidIntegrationTest {
+ public:
+  // An enum of asynchronous functions to use in tests, as required by
+  // `FutureManager`.
+  enum class AsyncFn {
+    kFn,
+    kCount,  // Must be the last enum value.
+  };
+
+ protected:
+  void AssertCreatesValidFutures(Env& env,
+                                 PromiseFactory<AsyncFn>& promise_factory) {
+    Local<TaskCompletionSource> tcs = TaskCompletionSource::Create(env);
+    Local<Task> task = tcs.GetTask(env);
+    Future<void> future =
+        promise_factory.NewFuture<void>(env, AsyncFn::kFn, task);
+    EXPECT_EQ(future.status(), FutureStatus::kFutureStatusPending);
+    tcs.SetResult(env, jni::Integer::Create(env, 42));
+    Await(future);
+    EXPECT_EQ(future.status(), FutureStatus::kFutureStatusComplete);
+  }
+
+  void AssertCreatesInvalidFutures(Env& env,
+                                   PromiseFactory<AsyncFn>& promise_factory) {
+    Local<TaskCompletionSource> tcs = TaskCompletionSource::Create(env);
+    Local<Task> task = tcs.GetTask(env);
+    Future<void> future =
+        promise_factory.NewFuture<void>(env, AsyncFn::kFn, task);
+    EXPECT_EQ(future.status(), FutureStatus::kFutureStatusInvalid);
+  }
+};
+
+TEST_F(PromiseFactoryTest, CopyConstructor) {
+  Firestore* firestore = TestFirestore();
+  PromiseFactory<AsyncFn> promise_factory1(GetFirestoreInternal(firestore));
+
+  PromiseFactory<AsyncFn> promise_factory2(promise_factory1);
+
+  Env env;
+  {
+    SCOPED_TRACE("promise_factory1");
+    AssertCreatesValidFutures(env, promise_factory1);
+  }
+  {
+    SCOPED_TRACE("promise_factory2");
+    AssertCreatesValidFutures(env, promise_factory2);
+  }
+}
+
+TEST_F(PromiseFactoryTest, CopyConstructorWithDeletedFirestore) {
+  Firestore* firestore = TestFirestore();
+  PromiseFactory<AsyncFn> promise_factory1(GetFirestoreInternal(firestore));
+  DeleteFirestore(firestore);
+
+  PromiseFactory<AsyncFn> promise_factory2(promise_factory1);
+
+  Env env;
+  {
+    SCOPED_TRACE("promise_factory1");
+    AssertCreatesInvalidFutures(env, promise_factory1);
+  }
+  {
+    SCOPED_TRACE("promise_factory2");
+    AssertCreatesInvalidFutures(env, promise_factory2);
+  }
+}
+
+TEST_F(PromiseFactoryTest, MoveConstructor) {
+  Firestore* firestore = TestFirestore();
+  PromiseFactory<AsyncFn> promise_factory1(GetFirestoreInternal(firestore));
+
+  PromiseFactory<AsyncFn> promise_factory2(std::move(promise_factory1));
+
+  Env env;
+  AssertCreatesValidFutures(env, promise_factory2);
+}
+
+TEST_F(PromiseFactoryTest, MoveConstructorWithDeletedFirestore) {
+  Firestore* firestore = TestFirestore();
+  PromiseFactory<AsyncFn> promise_factory1(GetFirestoreInternal(firestore));
+  DeleteFirestore(firestore);
+
+  PromiseFactory<AsyncFn> promise_factory2(std::move(promise_factory1));
+
+  Env env;
+  AssertCreatesInvalidFutures(env, promise_factory2);
+}
+
+TEST_F(PromiseFactoryTest, ShouldCreateInvalidPromisesIfFirestoreIsDeleted) {
+  Firestore* firestore = TestFirestore();
+  PromiseFactory<AsyncFn> promise_factory(GetFirestoreInternal(firestore));
+  DeleteFirestore(firestore);
+  Env env;
+
+  AssertCreatesInvalidFutures(env, promise_factory);
+}
+
+}  // namespace
+}  // namespace firestore
+}  // namespace firebase

firestore/integration_test_internal/src/firestore_test.cc

@@ -3,11 +3,8 @@
 #include "firebase/firestore.h"
 
 #include <algorithm>
+#include <future>
 #include <memory>
-
-#if !defined(__ANDROID__)
-#include <future>  // NOLINT(build/c++11)
-#endif
 #include <stdexcept>
 
 #if defined(__ANDROID__)
@@ -1489,12 +1486,11 @@ TEST_F(FirestoreIntegrationTest, AuthWorks) {
   WriteDocument(doc, MapFieldValue{{"foo", FieldValue::Integer(43)}});
 }
 
-#if !defined(__ANDROID__)
 // This test is to ensure b/172986326 doesn't regress.
 // Note: this test only exists in C++.
-TEST_F(FirestoreIntegrationTest, FirestoreCanBeDeletedFromTransaction) {
-  auto* app = App::Create(this->app()->options(), "foo");
-  auto* db = Firestore::GetInstance(app);
+TEST_F(FirestoreIntegrationTest, FirestoreCanBeDeletedFromTransactionAsync) {
+  Firestore* db = TestFirestore();
+  DisownFirestore(db);
 
   auto future = db->RunTransaction(
       [](Transaction&, std::string&) { return Error::kErrorOk; });
@@ -1511,7 +1507,26 @@ TEST_F(FirestoreIntegrationTest, FirestoreCanBeDeletedFromTransaction) {
   callback_done.wait();
   deletion.wait();
 }
-#endif  // #if !defined(__ANDROID__)
+
+// This test is to ensure b/172986326 doesn't regress.
+// Note: this test only exists in C++.
+TEST_F(FirestoreIntegrationTest, FirestoreCanBeDeletedFromTransaction) {
+  Firestore* db = TestFirestore();
+  DisownFirestore(db);
+
+  auto future = db->RunTransaction(
+      [](Transaction&, std::string&) { return Error::kErrorOk; });
+
+  std::promise<void> callback_done_promise;
+  auto callback_done = callback_done_promise.get_future();
+  future.AddOnCompletion([&](const Future<void>&) mutable {
+    delete db;
+    callback_done_promise.set_value();
+  });
+
+  Await(future);
+  callback_done.wait();
+}
 
 #if defined(__ANDROID__)
 TEST_F(FirestoreAndroidIntegrationTest,