Tie up the loose ends from moving Firestore's integration tests into integration_test_internal (#351)

Author: dconeybe

firestore/integration_test_internal/src/android/jni_runnable_android_test.cc

@@ -1,5 +1,7 @@
 #include "firestore/src/android/jni_runnable_android.h"
 
+#include "app/memory/atomic.h"
+#include "app/src/mutex.h"
 #include "firestore/src/jni/declaration.h"
 #include "firestore/src/jni/object.h"
 #include "firestore/src/jni/ownership.h"
@@ -18,6 +20,7 @@ using jni::Global;
 using jni::Local;
 using jni::Method;
 using jni::Object;
+using jni::StaticField;
 using jni::StaticMethod;
 using jni::Task;
 using jni::Throwable;
@@ -27,14 +30,18 @@ Method<Object> kLooperGetThread("getThread", "()Ljava/lang/Thread;");
 Method<void> kRunnableRun("run", "()V");
 StaticMethod<Object> kCurrentThread("currentThread", "()Ljava/lang/Thread;");
 Method<jlong> kThreadGetId("getId", "()J");
+Method<Object> kThreadGetState("getState", "()Ljava/lang/Thread$State;");
+StaticField<Object> kThreadStateBlocked("BLOCKED", "Ljava/lang/Thread$State;");
 
 class JniRunnableTest : public FirestoreAndroidIntegrationTest {
  public:
   void SetUp() override {
     FirestoreAndroidIntegrationTest::SetUp();
     loader().LoadClass("android/os/Looper", kGetMainLooper, kLooperGetThread);
     loader().LoadClass("java/lang/Runnable", kRunnableRun);
-    loader().LoadClass("java/lang/Thread", kCurrentThread, kThreadGetId);
+    loader().LoadClass("java/lang/Thread", kCurrentThread, kThreadGetId,
+                       kThreadGetState);
+    loader().LoadClass("java/lang/Thread$State", kThreadStateBlocked);
     ASSERT_TRUE(loader().ok());
   }
 };
@@ -56,6 +63,16 @@ jlong GetMainThreadId(Env& env) {
   return env.Call(main_thread, kThreadGetId);
 }
 
+/**
+ * Returns whether or not the given thread is in the "blocked" state.
+ * See java.lang.Thread.State.BLOCKED.
+ */
+bool IsThreadBlocked(Env& env, Object& thread) {
+  Local<Object> actual_state = env.Call(thread, kThreadGetState);
+  Local<Object> expected_state = env.Get(kThreadStateBlocked);
+  return Object::Equals(env, expected_state, actual_state);
+}
+
 TEST_F(JniRunnableTest, JavaRunCallsCppRun) {
   Env env;
   bool invoked = false;
@@ -145,6 +162,27 @@ TEST_F(JniRunnableTest, DetachDetachesEvenIfAnExceptionIsPending) {
   EXPECT_TRUE(env.ok());
 }
 
+// Verify that b/181129657 does not regress; that is, calling `Detach()` from
+// `Run()` should not deadlock.
+TEST_F(JniRunnableTest, DetachCanBeCalledFromRun) {
+  Env env;
+  int run_count = 0;
+  auto runnable = MakeJniRunnable(env, [&run_count](JniRunnableBase& runnable) {
+    ++run_count;
+    Env env;
+    runnable.Detach(env);
+  });
+  Local<Object> java_runnable = runnable.GetJavaRunnable();
+
+  // Call `run()` twice to verify that the call to `Detach()` successfully
+  // detaches and the second `run()` invocation does not call C++ `Run()`.
+  env.Call(java_runnable, kRunnableRun);
+  env.Call(java_runnable, kRunnableRun);
+
+  EXPECT_TRUE(env.ok());
+  EXPECT_EQ(run_count, 1);
+}
+
 TEST_F(JniRunnableTest, DestructionCausesJavaRunToDoNothing) {
   Env env;
   bool invoked = false;
@@ -191,29 +229,21 @@ TEST_F(JniRunnableTest, RunOnMainThreadTaskFailsIfRunThrowsException) {
 }
 
 TEST_F(JniRunnableTest, RunOnMainThreadRunsSynchronouslyFromMainThread) {
-  class ChainedMainThreadJniRunnable : public JniRunnableBase {
-   public:
-    using JniRunnableBase::JniRunnableBase;
-
-    void Run() override {
-      Env env;
-      EXPECT_EQ(GetCurrentThreadId(env), GetMainThreadId(env));
-      if (is_nested_call_) {
-        return;
-      }
-      is_nested_call_ = true;
-      Local<Task> task = RunOnMainThread(env);
-      EXPECT_TRUE(task.IsComplete(env));
-      EXPECT_TRUE(task.IsSuccessful(env));
-      is_nested_call_ = false;
-    }
-
-   private:
-    bool is_nested_call_ = false;
-  };
-
   Env env;
-  ChainedMainThreadJniRunnable runnable(env);
+  bool is_recursive_call = false;
+  auto runnable =
+      MakeJniRunnable(env, [&is_recursive_call](JniRunnableBase& runnable) {
+        Env env;
+        EXPECT_EQ(GetCurrentThreadId(env), GetMainThreadId(env));
+        if (is_recursive_call) {
+          return;
+        }
+        is_recursive_call = true;
+        Local<Task> task = runnable.RunOnMainThread(env);
+        EXPECT_TRUE(task.IsComplete(env));
+        EXPECT_TRUE(task.IsSuccessful(env));
+        is_recursive_call = false;
+      });
 
   Local<Task> task = runnable.RunOnMainThread(env);
 
@@ -252,6 +282,59 @@ TEST_F(JniRunnableTest, RunOnNewThreadTaskFailsIfRunThrowsException) {
   EXPECT_TRUE(env.IsSameObject(exception, thrown_exception));
 }
 
+TEST_F(JniRunnableTest, DetachReturnsAfterLastRunOnAnotherThreadCompletes) {
+  Env env;
+  compat::Atomic<int32_t> runnable1_run_invoke_count;
+  runnable1_run_invoke_count.store(0);
+  Mutex detach_thread_mutex;
+  Global<Object> detach_thread;
+
+  auto runnable1 = MakeJniRunnable(
+      env, [&runnable1_run_invoke_count, &detach_thread, &detach_thread_mutex] {
+        runnable1_run_invoke_count.fetch_add(1);
+        Env env;
+        // Wait for `detach()` to be called and start blocking; then, return to
+        // allow `detach()` to unblock and do its job.
+        while (env.ok()) {
+          MutexLock lock(detach_thread_mutex);
+          if (detach_thread && IsThreadBlocked(env, detach_thread)) {
+            break;
+          }
+        }
+        EXPECT_TRUE(env.ok()) << "IsThreadBlocked() failed with an exception";
+      });
+
+  auto runnable2 =
+      MakeJniRunnable(env, [&runnable1, &detach_thread, &detach_thread_mutex] {
+        Env env;
+        {
+          MutexLock lock(detach_thread_mutex);
+          detach_thread = env.Call(kCurrentThread);
+        }
+        runnable1.Detach(env);
+        EXPECT_TRUE(env.ok()) << "Detach() failed with an exception";
+      });
+
+  // Wait for the `runnable1.Run()` to start to ensure that the lock is held.
+  Local<Task> task1 = runnable1.RunOnNewThread(env);
+  while (true) {
+    if (runnable1_run_invoke_count.load() != 0) {
+      break;
+    }
+  }
+
+  // Start a new thread to call `runnable1.Detach()`.
+  Local<Task> task2 = runnable2.RunOnNewThread(env);
+
+  Await(env, task1);
+  Await(env, task2);
+
+  // Invoke `run()` again and ensure that `Detach()` successfully did its job;
+  // that is, verify that `Run()` is not invoked.
+  env.Call(runnable1.GetJavaRunnable(), kRunnableRun);
+  EXPECT_EQ(runnable1_run_invoke_count.load(), 1);
+}
+
 }  // namespace
 }  // namespace firestore
 }  // namespace firebase

firestore/integration_test_internal/src/android/promise_android_test.cc

@@ -41,17 +41,14 @@ namespace {
 
 class PromiseTest : public FirestoreAndroidIntegrationTest {
  public:
-  PromiseTest() : promises_(GetFirestoreInternal(TestFirestore())) {
+  PromiseTest() : promises_(GetFirestoreInternal(TestFirestore())) {}
+
+  void SetUp() override {
+    FirestoreAndroidIntegrationTest::SetUp();
     jni::Env env = GetEnv();
-    // TODO(b/183294303): Uncomment this initialization code when
-    // these tests are fixed on Android.
-    //
-    // Note: This usage of jni::Loader may be broken because this is
-    // running in a background thread. Requires further investigation.
-    //
-    // cancellation_token_source_ = CancellationTokenSource::Create(env);
-    // task_completion_source_ = TaskCompletionSource::Create(
-    //    env, cancellation_token_source_.GetToken(env));
+    cancellation_token_source_ = CancellationTokenSource::Create(env);
+    task_completion_source_ = TaskCompletionSource::Create(
+       env, cancellation_token_source_.GetToken(env));
   }
 
   // An enum of asynchronous functions to use in tests, as required by
@@ -223,8 +220,6 @@ class TestVoidCompletion : public TestCompletionBase<void, void> {
 };
 
 TEST_F(PromiseTest, FutureVoidShouldSucceedWhenTaskSucceeds) {
-  SKIP_TEST_ON_ANDROID;  // TODO(b/183294303): Fix this test on Android.
-
   jni::Env env = GetEnv();
   auto future = promises().NewFuture<void>(env, AsyncFn::kFn, GetTask());
   EXPECT_EQ(future.status(), FutureStatus::kFutureStatusPending);
@@ -238,8 +233,6 @@ TEST_F(PromiseTest, FutureVoidShouldSucceedWhenTaskSucceeds) {
 }
 
 TEST_F(PromiseTest, FutureNonVoidShouldSucceedWhenTaskSucceeds) {
-  SKIP_TEST_ON_ANDROID;  // TODO(b/183294303): Fix this test on Android.
-
   jni::Env env = GetEnv();
   auto future =
       promises().NewFuture<std::string, int>(env, AsyncFn::kFn, GetTask());
@@ -254,8 +247,6 @@ TEST_F(PromiseTest, FutureNonVoidShouldSucceedWhenTaskSucceeds) {
 }
 
 TEST_F(PromiseTest, FutureVoidShouldFailWhenTaskFails) {
-  SKIP_TEST_ON_ANDROID;  // TODO(b/183294303): Fix this test on Android.
-
   jni::Env env = GetEnv();
   auto future = promises().NewFuture<void>(env, AsyncFn::kFn, GetTask());
   EXPECT_EQ(future.status(), FutureStatus::kFutureStatusPending);
@@ -270,8 +261,6 @@ TEST_F(PromiseTest, FutureVoidShouldFailWhenTaskFails) {
 }
 
 TEST_F(PromiseTest, FutureNonVoidShouldFailWhenTaskFails) {
-  SKIP_TEST_ON_ANDROID;  // TODO(b/183294303): Fix this test on Android.
-
   jni::Env env = GetEnv();
   auto future =
       promises().NewFuture<std::string, int>(env, AsyncFn::kFn, GetTask());
@@ -287,8 +276,6 @@ TEST_F(PromiseTest, FutureNonVoidShouldFailWhenTaskFails) {
 }
 
 TEST_F(PromiseTest, FutureVoidShouldCancelWhenTaskCancels) {
-  SKIP_TEST_ON_ANDROID;  // TODO(b/183294303): Fix this test on Android.
-
   jni::Env env = GetEnv();
   auto future = promises().NewFuture<void>(env, AsyncFn::kFn, GetTask());
   EXPECT_EQ(future.status(), FutureStatus::kFutureStatusPending);
@@ -303,8 +290,6 @@ TEST_F(PromiseTest, FutureVoidShouldCancelWhenTaskCancels) {
 }
 
 TEST_F(PromiseTest, FutureNonVoidShouldCancelWhenTaskCancels) {
-  SKIP_TEST_ON_ANDROID;  // TODO(b/183294303): Fix this test on Android.
-
   jni::Env env = GetEnv();
   auto future =
       promises().NewFuture<std::string, int>(env, AsyncFn::kFn, GetTask());
@@ -320,8 +305,6 @@ TEST_F(PromiseTest, FutureNonVoidShouldCancelWhenTaskCancels) {
 }
 
 TEST_F(PromiseTest, FutureVoidShouldCallCompletionWhenTaskSucceeds) {
-  SKIP_TEST_ON_ANDROID;  // TODO(b/183294303): Fix this test on Android.
-
   jni::Env env = GetEnv();
   TestVoidCompletion completion;
   auto future = promises().NewFuture<void, void>(env, AsyncFn::kFn, GetTask(),
@@ -338,8 +321,6 @@ TEST_F(PromiseTest, FutureVoidShouldCallCompletionWhenTaskSucceeds) {
 }
 
 TEST_F(PromiseTest, FutureNonVoidShouldCallCompletionWhenTaskSucceeds) {
-  SKIP_TEST_ON_ANDROID;  // TODO(b/183294303): Fix this test on Android.
-
   jni::Env env = GetEnv();
   TestCompletion<std::string, int> completion;
   auto future = promises().NewFuture<std::string, int>(env, AsyncFn::kFn,
@@ -356,8 +337,6 @@ TEST_F(PromiseTest, FutureNonVoidShouldCallCompletionWhenTaskSucceeds) {
 }
 
 TEST_F(PromiseTest, FutureVoidShouldCallCompletionWhenTaskFails) {
-  SKIP_TEST_ON_ANDROID;  // TODO(b/183294303): Fix this test on Android.
-
   jni::Env env = GetEnv();
   TestVoidCompletion completion;
   auto future = promises().NewFuture<void, void>(env, AsyncFn::kFn, GetTask(),
@@ -374,8 +353,6 @@ TEST_F(PromiseTest, FutureVoidShouldCallCompletionWhenTaskFails) {
 }
 
 TEST_F(PromiseTest, FutureNonVoidShouldCallCompletionWhenTaskFails) {
-  SKIP_TEST_ON_ANDROID;  // TODO(b/183294303): Fix this test on Android.
-
   jni::Env env = GetEnv();
   TestCompletion<std::string, int> completion;
   auto future = promises().NewFuture<std::string, int>(env, AsyncFn::kFn,
@@ -392,8 +369,6 @@ TEST_F(PromiseTest, FutureNonVoidShouldCallCompletionWhenTaskFails) {
 }
 
 TEST_F(PromiseTest, FutureVoidShouldCallCompletionWhenTaskCancels) {
-  SKIP_TEST_ON_ANDROID;  // TODO(b/183294303): Fix this test on Android.
-
   jni::Env env = GetEnv();
   TestVoidCompletion completion;
   auto future = promises().NewFuture<void, void>(env, AsyncFn::kFn, GetTask(),
@@ -410,8 +385,6 @@ TEST_F(PromiseTest, FutureVoidShouldCallCompletionWhenTaskCancels) {
 }
 
 TEST_F(PromiseTest, FutureNonVoidShouldCallCompletionWhenTaskCancels) {
-  SKIP_TEST_ON_ANDROID;  // TODO(b/183294303): Fix this test on Android.
-
   jni::Env env = GetEnv();
   TestCompletion<std::string, int> completion;
   auto future = promises().NewFuture<std::string, int>(env, AsyncFn::kFn,

firestore/integration_test_internal/src/firestore_integration_test.cc

@@ -114,7 +114,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();
     }
   }
@@ -141,6 +141,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.");