Port cl/361792354: Call Terminate() from ~FirebaseFirestore() so that the cached Java instance will be cleared. (#329)

Author: dconeybe

firestore/src/android/firestore_android.cc

@@ -137,6 +137,15 @@ void InitializeUserCallbackExecutor(Loader& loader) {
                    kExecutorShutdown);
 }
 
+constexpr char kFirestoreTasksClassName[] = PROGUARD_KEEP_CLASS
+    "com/google/firebase/firestore/internal/cpp/FirestoreTasks";
+StaticMethod<void> kAwaitCompletion("awaitCompletion",
+                                    "(Lcom/google/android/gms/tasks/Task;)V");
+
+void InitializeFirestoreTasks(Loader& loader) {
+  loader.LoadClass(kFirestoreTasksClassName, kAwaitCompletion);
+}
+
 /**
  * A map of Java Firestore instance to C++ Firestore instance.
  */
@@ -258,6 +267,7 @@ bool FirestoreInternal::Initialize(App* app) {
     jni::Map::Initialize(loader);
 
     InitializeFirestore(loader);
+    InitializeFirestoreTasks(loader);
     InitializeUserCallbackExecutor(loader);
 
     BlobInternal::Initialize(loader);
@@ -337,6 +347,13 @@ FirestoreInternal::~FirestoreInternal() {
   ClearListeners();
 
   Env env = GetEnv();
+
+  // Call `terminate()` on the Java `FirebaseFirestore` object and wait for it
+  // to complete to guarantee that the next instance of `FirestoreInternal` will
+  // be backed by a new Java `FirebaseFirestore` instance.
+  Local<Task> terminate_task = env.Call(obj_, kTerminate);
+  env.Call(kAwaitCompletion, terminate_task);
+
   ShutdownUserCallbackExecutor(env);
 
   promises_.reset(nullptr);

firestore/src/tests/firestore_integration_test.cc

@@ -107,7 +107,7 @@ Firestore* FirestoreIntegrationTest::TestFirestore(
     const std::string& name) const {
   for (const auto& entry : firestores_) {
     const FirestoreInfo& firestore_info = entry.second;
-    if (firestore_info.cached() && firestore_info.name() == name) {
+    if (firestore_info.name() == name) {
       return firestore_info.firestore();
     }
   }
@@ -135,6 +135,14 @@ void FirestoreIntegrationTest::DeleteFirestore(Firestore* firestore) {
   firestores_.erase(found);
 }
 
+void FirestoreIntegrationTest::DisownFirestore(Firestore* firestore) {
+  auto found = firestores_.find(firestore);
+  FIREBASE_ASSERT_MESSAGE(found != firestores_.end(),
+                          "The given Firestore was not found.");
+  found->second.ReleaseFirestore();
+  firestores_.erase(found);
+}
+
 void FirestoreIntegrationTest::DeleteApp(App* app) {
   auto found = apps_.find(app);
   FIREBASE_ASSERT_MESSAGE(found != apps_.end(), "The given App was not found.");

firestore/src/tests/firestore_integration_test.h

@@ -152,7 +152,7 @@ class TestEventListener : public EventListener<T> {
 };
 
 // Base class for Firestore integration tests.
-// Note it keeps a cached of created Firestore instances, and is thread-unsafe.
+// Note it keeps a cache of created Firestore instances, and is thread-unsafe.
 class FirestoreIntegrationTest : public testing::Test {
   friend class TransactionTester;
 
@@ -170,18 +170,32 @@ class FirestoreIntegrationTest : public testing::Test {
   // Returns a Firestore instance for an app with the given name.
   // If this method is invoked again with the same `name`, then the same pointer
   // will be returned. The only exception is if the `Firestore` was removed
-  // from the cache by a call to `DeleteFirestore()` or `DeleteApp()` with the
-  // `App` of the returned `Firestore`.
+  // from the cache by a call to `DeleteFirestore()` or `DisownFirestore()`, or
+  // if `DeleteApp()` is called with the `App` of the returned `Firestore`.
   Firestore* TestFirestore(const std::string& name = kDefaultAppName) const;
 
   // Deletes the given `Firestore` instance, which must have been returned by a
-  // previous invocation of `TestFirestore()`. If the given instance was in the
-  // cache, then it will be removed from the cache. Note that all `Firestore`
+  // previous invocation of `TestFirestore()`, and removes it from the cache of
+  // instances returned from `TestFirestore()`. Note that all `Firestore`
   // instances returned from `TestFirestore()` will be automatically deleted at
   // the end of the test case; therefore, this method is only needed if the test
-  // requires that the instance be deleted earlier than that.
+  // requires that the instance be deleted earlier than that. If the given
+  // `Firestore` instance has already been removed from the cache, such as by a
+  // previous invocation of this method, then the behavior of this method is
+  // undefined.
   void DeleteFirestore(Firestore* firestore);
 
+  // Relinquishes ownership of the given `Firestore` instance, which must have
+  // been returned by a previous invocation of `TestFirestore()`, and removes it
+  // from the cache of instances returned from `TestFirestore()`. Note that all
+  // `Firestore` instances returned from `TestFirestore()` will be automatically
+  // deleted at the end of the test case; therefore, this method is only needed
+  // if the test requires that the instance be excluded from this automatic
+  // deletion. If the given `Firestore` instance has already been removed from
+  // the cache, such as by a previous invocation of this method, then the
+  // behavior of this method is undefined.
+  void DisownFirestore(Firestore* firestore);
+
   // Deletes the given `App` instance. The given `App` must have been the `App`
   // associated with a `Firestore` instance returned by a previous invocation of
   // `TestFirestore()`. Normally the `App` is deleted at the end of the test, so
@@ -304,13 +318,11 @@ class FirestoreIntegrationTest : public testing::Test {
 
     const std::string& name() const { return name_; }
     Firestore* firestore() const { return firestore_.get(); }
-    bool cached() const { return cached_; }
-    void ClearCached() { cached_ = false; }
+    void ReleaseFirestore() { firestore_.release(); }
 
    private:
     std::string name_;
     UniquePtr<Firestore> firestore_;
-    bool cached_ = true;
   };
 
   // The Firestore and App instance caches.

firestore/src/tests/firestore_test.cc

@@ -11,6 +11,11 @@
 
 #if defined(__ANDROID__)
 #include "firestore/src/android/exception_android.h"
+#include "firestore/src/android/jni_runnable_android.h"
+#include "firestore/src/jni/env.h"
+#include "firestore/src/jni/ownership.h"
+#include "firestore/src/jni/task.h"
+#include "firestore/src/tests/android/firestore_integration_test_android.h"
 #endif  // defined(__ANDROID__)
 
 #include "app/memory/unique_ptr.h"
@@ -1429,13 +1434,6 @@ TEST_F(FirestoreIntegrationTest, CanClearPersistenceOnANewFirestoreInstance) {
   const std::string path = document.path();
   WriteDocument(document, MapFieldValue{{"foo", FieldValue::Integer(42)}});
 
-#if defined(__ANDROID__)
-  // TODO(b/168628900) Remove this call to Terminate() once deleting the
-  // Firestore* instance removes the underlying Java object from the instance
-  // cache in Android.
-  EXPECT_THAT(db->Terminate(), FutureSucceeds());
-#endif
-
   // Call DeleteFirestore() to ensure that both the App and Firestore instances
   // are deleted, which emulates the way an end user would experience their
   // application being killed and later re-launched by the user.
@@ -1543,6 +1541,21 @@ TEST_F(FirestoreIntegrationTest, FirestoreCanBeDeletedFromTransaction) {
 }
 #endif  // #if !defined(__ANDROID__)
 
+#if defined(__ANDROID__)
+TEST_F(FirestoreAndroidIntegrationTest,
+       CanDeleteFirestoreInstanceOnJavaMainThread) {
+  jni::Env env;
+  Firestore* db = TestFirestore();
+  auto runnable = MakeJniRunnable(env, [db] { delete db; });
+
+  jni::Local<jni::Task> task = runnable.RunOnMainThread(env);
+
+  Await(env, task);
+  EXPECT_TRUE(task.IsSuccessful(env));
+  DisownFirestore(db);  // Avoid double-deletion of the `db`.
+}
+#endif  // defined(__ANDROID__)
+
 #endif  // defined(FIRESTORE_STUB_BUILD)
 
 }  // namespace firestore

firestore/src_java/com/google/firebase/firestore/internal/cpp/FirestoreTasks.java

@@ -0,0 +1,52 @@
+package com.google.firebase.firestore.internal.cpp;
+
+import com.google.android.gms.tasks.OnCompleteListener;
+import com.google.android.gms.tasks.Task;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+
+/** Helper methods for working with {@link Task} objects from C++. */
+public final class FirestoreTasks {
+
+  private FirestoreTasks() {}
+
+  /**
+   * Blocks the calling thread until the given {@link Task} has completed.
+   *
+   * <p>This method is identical to {@link com.google.android.gms.tasks.Tasks#await} except that it
+   * does <em>not</em> throw an exception if invoked from the main thread. Since it is technically
+   * possible to wait for a {@link Task} from any thread in C++, throwing is undesirable if called
+   * from the main thread.
+   *
+   * <p>The result of the given {@link Task} (i.e. success or failure) is not considered by this
+   * method; whenever the task completes, either successfully or unsuccessfully, this method will
+   * return.
+   *
+   * @param task The task whose completion to await.
+   * @throws InterruptedException if waiting for the task to complete is interrupted.
+   */
+  public static <T> void awaitCompletion(Task<T> task) throws InterruptedException {
+    CountDownLatch countDownLatch = new CountDownLatch(1);
+    ExecutorService executor = Executors.newSingleThreadExecutor();
+    try {
+      task.addOnCompleteListener(executor, new CountDownOnCompleteListener<T>(countDownLatch));
+      countDownLatch.await();
+    } finally {
+      executor.shutdown();
+    }
+  }
+
+  private static final class CountDownOnCompleteListener<T> implements OnCompleteListener<T> {
+    private final CountDownLatch countDownLatch;
+
+    CountDownOnCompleteListener(CountDownLatch countDownLatch) {
+      this.countDownLatch = countDownLatch;
+    }
+
+    @Override
+    public void onComplete(Task<T> task) {
+      countDownLatch.countDown();
+    }
+  }
+}