Merge pull request #219 from firebase/feature/js-merge-firestore-changes

Merge Firestore changes

Author: a-maurice

app/instance_id/instance_id_desktop_impl.cc

@@ -721,7 +721,7 @@ bool InstanceIdDesktopImpl::FetchServerToken(const char* scope, bool* retry) {
       size_t component_start = 0;
       size_t component_end;
       do {
-        component_end = error.find(":", component_start);
+        component_end = error.find(':', component_start);
         std::string error_component =
             error.substr(component_start, component_end - component_start);
         if (error_component == kPhoneRegistrationError) {

app/src/locale.cc

@@ -69,12 +69,12 @@ std::string GetLocale() {
   // Some of the environment variables have a "." after the name to specify the
   // terminal encoding, e.g.  "en_US.UTF-8", so we want to cut the string on the
   // ".".
-  size_t cut = output.find(".");
+  size_t cut = output.find('.');
   if (cut != std::string::npos) {
     output = output.substr(0, cut);
   }
   // Do the same with a "@" which some locales also have.
-  cut = output.find("@");
+  cut = output.find('@');
   if (cut != std::string::npos) {
     output = output.substr(0, cut);
   }

app/tests/locale_test.cc

@@ -49,7 +49,7 @@ TEST_F(LocaleTest, TestGetLocale) {
   // Make sure this looks like a locale, e.g. has at least 5 characters and
   // contains an underscore.
   EXPECT_GE(loc.size(), 5);
-  EXPECT_NE(loc.find("_"), std::string::npos);
+  EXPECT_NE(loc.find('_'), std::string::npos);
 }
 
 }  // namespace internal

auth/src/desktop/rpcs/error_codes.cc

@@ -225,7 +225,7 @@ AuthError GetAuthErrorCode(const std::string& error,
     // Couldn't find it, check again with just the first word.
     // Sometimes the backend will return the error code along with
     // some explanatory text (see kAuthErrorNoSuchProvider above).
-    size_t space = error.find(" ");
+    size_t space = error.find(' ');
     if (space != std::string::npos) {
       std::string first_word = error.substr(0, space);
       found = find_error(first_word);

firestore/src/android/firestore_android.cc

@@ -178,17 +178,24 @@ class JavaFirestoreMap {
   jni::Global<jni::HashMap> firestores_;
 };
 
-// Governed by init_mutex_below as well.
-JavaFirestoreMap* java_firestores_ = nullptr;
+// init_mutex protects all the globals below.
+Mutex init_mutex;  // NOLINT
+int initialize_count = 0;
+Loader* global_loader = nullptr;
+
+JavaFirestoreMap* java_firestores = nullptr;
+
+// The initial value for setLoggingEnabled.
+enum class InitialLogState {
+  kUnset,
+  kSetEnabled,
+  kSetDisabled,
+} initial_log_state = InitialLogState::kUnset;
 
 }  // namespace
 
 const char kApiIdentifier[] = "Firestore";
 
-Mutex FirestoreInternal::init_mutex_;  // NOLINT
-int FirestoreInternal::initialize_count_ = 0;
-Loader* FirestoreInternal::loader_ = nullptr;
-
 FirestoreInternal::FirestoreInternal(App* app) {
   FIREBASE_ASSERT(app != nullptr);
   if (!Initialize(app)) return;
@@ -200,7 +207,7 @@ FirestoreInternal::FirestoreInternal(App* app) {
   FIREBASE_ASSERT(java_firestore.get() != nullptr);
   obj_ = java_firestore;
 
-  java_firestores_->Put(env, java_firestore, this);
+  java_firestores->Put(env, java_firestore, this);
 
   // Mainly for enabling TimestampsInSnapshotsEnabled. The rest comes from the
   // default in native SDK. The C++ implementation relies on that for reading
@@ -218,21 +225,22 @@ FirestoreInternal::FirestoreInternal(App* app) {
 
 /* static */
 bool FirestoreInternal::Initialize(App* app) {
-  MutexLock init_lock(init_mutex_);
-  if (initialize_count_ == 0) {
+  MutexLock init_lock(init_mutex);
+  if (initialize_count == 0) {
     jni::Initialize(app->java_vm());
 
-    FIREBASE_DEV_ASSERT(java_firestores_ == nullptr);
-    java_firestores_ = new JavaFirestoreMap();
+    FIREBASE_DEV_ASSERT(java_firestores == nullptr);
+    java_firestores = new JavaFirestoreMap();
 
+    Env env = GetEnv();
     Loader loader(app);
     loader.AddEmbeddedFile(::firebase_firestore::firestore_resources_filename,
                            ::firebase_firestore::firestore_resources_data,
                            ::firebase_firestore::firestore_resources_size);
     loader.CacheEmbeddedFiles();
 
     jni::Object::Initialize(loader);
-
+    jni::String::Initialize(env, loader);
     jni::ArrayList::Initialize(loader);
     jni::Boolean::Initialize(loader);
     jni::Collection::Initialize(loader);
@@ -272,32 +280,42 @@ bool FirestoreInternal::Initialize(App* app) {
     TransactionInternal::Initialize(loader);
     WriteBatchInternal::Initialize(loader);
     if (!loader.ok()) {
-      ReleaseClasses(app);
+      ReleaseClassesLocked(env);
       return false;
     }
 
-    FIREBASE_DEV_ASSERT(loader_ == nullptr);
-    loader_ = new Loader(Move(loader));
+    FIREBASE_DEV_ASSERT(global_loader == nullptr);
+    global_loader = new Loader(Move(loader));
+
+    if (initial_log_state != InitialLogState::kUnset) {
+      bool enabled = initial_log_state == InitialLogState::kSetEnabled;
+      env.Call(kSetLoggingEnabled, enabled);
+    }
   }
-  initialize_count_++;
+  initialize_count++;
   return true;
 }
 
 /* static */
-void FirestoreInternal::ReleaseClasses(App* app) {
-  delete loader_;
-  loader_ = nullptr;
+void FirestoreInternal::ReleaseClassesLocked(Env& env) {
+  // Assumes `init_mutex` is held.
+  String::Terminate(env);
+
+  delete global_loader;
+  global_loader = nullptr;
 }
 
 /* static */
 void FirestoreInternal::Terminate(App* app) {
-  MutexLock init_lock(init_mutex_);
-  FIREBASE_ASSERT(initialize_count_ > 0);
-  initialize_count_--;
-  if (initialize_count_ == 0) {
-    ReleaseClasses(app);
-    delete java_firestores_;
-    java_firestores_ = nullptr;
+  MutexLock init_lock(init_mutex);
+  FIREBASE_ASSERT(initialize_count > 0);
+  initialize_count--;
+  if (initialize_count == 0) {
+    Env env(app->GetJNIEnv());
+    ReleaseClassesLocked(env);
+
+    delete java_firestores;
+    java_firestores = nullptr;
   }
 }
 
@@ -316,7 +334,7 @@ FirestoreInternal::~FirestoreInternal() {
 
   future_manager_.ReleaseFutureApi(this);
 
-  java_firestores_->Remove(env, obj_);
+  java_firestores->Remove(env, obj_);
 
   Terminate(app_);
   app_ = nullptr;
@@ -512,13 +530,28 @@ void Firestore::set_log_level(LogLevel log_level) {
   // "Info", "warning", "error", and "assert" map to logging disabled.
   bool logging_enabled = log_level < LogLevel::kLogLevelInfo;
 
+  {
+    MutexLock lock(init_mutex);
+
+    // Set the initial_log_state on every invocation, just in case Firestore
+    // is terminated for long enough to unload the Firestore classes within
+    // the JVM.
+    initial_log_state = logging_enabled ? InitialLogState::kSetEnabled
+                                        : InitialLogState::kSetDisabled;
+
+    if (initialize_count < 1) {
+      // Avoid invoking Java methods before Firestore has been initialized.
+      return;
+    }
+  }
+
   Env env = FirestoreInternal::GetEnv();
   env.Call(kSetLoggingEnabled, logging_enabled);
 }
 
 FirestoreInternal* FirestoreInternal::RecoverFirestore(
     Env& env, const Object& java_firestore) {
-  return java_firestores_->Get(env, java_firestore);
+  return java_firestores->Get(env, java_firestore);
 }
 
 void FirestoreInternal::SetClientLanguage(const std::string& language_token) {

firestore/src/android/firestore_android.h

@@ -178,13 +178,9 @@ class FirestoreInternal {
   void ShutdownUserCallbackExecutor(jni::Env& env);
 
   static bool Initialize(App* app);
-  static void ReleaseClasses(App* app);
+  static void ReleaseClassesLocked(jni::Env& env);
   static void Terminate(App* app);
 
-  static Mutex init_mutex_;
-  static int initialize_count_;
-  static jni::Loader* loader_;
-
   jni::Global<jni::Object> user_callback_executor_;
 
   App* app_ = nullptr;

firestore/src/jni/env.cc

@@ -1,7 +1,10 @@
 #include "firestore/src/jni/env.h"
 
+#include <cstring>
+
 #include "app/src/assert.h"
 #include "app/src/log.h"
+#include "firestore/src/jni/array.h"
 
 // Add constant declarations missing from the NDK's jni.h
 #ifndef JNI_ENOMEM
@@ -144,28 +147,36 @@ jmethodID Env::GetStaticMethodId(const Class& clazz, const char* name,
 }
 
 Local<String> Env::NewStringUtf(const char* bytes) {
+  if (!bytes) return {};
+
+  return NewStringUtf(bytes, strlen(bytes));
+}
+
+Local<String> Env::NewStringUtf(const char* bytes, size_t size) {
   if (!ok()) return {};
 
-  jstring result = env_->NewStringUTF(bytes);
-  RecordException();
-  return Local<String>(env_, result);
+  Local<Array<uint8_t>> java_bytes = NewArray<uint8_t>(size);
+  SetArrayRegion(java_bytes, 0, size, reinterpret_cast<const uint8_t*>(bytes));
+  if (!ok()) return {};
+
+  return String::Create(*this, java_bytes, String::GetUtf8());
 }
 
-std::string Env::GetStringUtfRegion(const String& string, size_t start,
-                                    size_t len) {
+std::string Env::ToStringUtf(const String& string) {
   if (!ok()) return "";
 
+  Local<Array<uint8_t>> bytes = string.GetBytes(*this, String::GetUtf8());
+  size_t len = GetArrayLength(bytes);
+
   // Copy directly into the std::string buffer. This is guaranteed to work as
   // of C++11, and also happens to work with STLPort.
   std::string result;
   result.resize(len);
 
-  env_->GetStringUTFRegion(string.get(), ToJni(start), ToJni(len), &result[0]);
-  RecordException();
-
-  // Ensure that if there was an exception, the contents of the buffer are
-  // disregarded.
+  auto* buffer = reinterpret_cast<uint8_t*>(&result[0]);
+  GetArrayRegion(bytes, 0, len, buffer);
   if (!ok()) return "";
+
   return result;
 }
 

firestore/src/jni/env.h

@@ -297,29 +297,28 @@ class Env {
   // MARK: String Operations
 
   /**
-   * Creates a new proxy for a Java String from a sequences of modified UTF-8
-   * bytes.
+   * Creates a new proxy for a Java String from a sequence of UTF-8 bytes.
+   *
+   * Note that unlike the underlying JNI method, these bytes should be encoded
+   * in standard UTF-8, and *not* in the modified UTF-8 customarily used in the
+   * JNI API.
+   *
+   * @param bytes The UTF-8 bytes from which to create a Java String.
+   * @param size The number of bytes in the UTF-8 sequence.
    */
   Local<String> NewStringUtf(const char* bytes);
+  Local<String> NewStringUtf(const char* bytes, size_t size);
   Local<String> NewStringUtf(const std::string& bytes) {
-    return NewStringUtf(bytes.c_str());
-  }
-
-  /** Returns the length of the string in modified UTF-8 bytes. */
-  size_t GetStringUtfLength(const String& string) {
-    if (!ok()) return 0;
-
-    jsize result = env_->GetStringUTFLength(string.get());
-    RecordException();
-    return static_cast<size_t>(result);
+    return NewStringUtf(bytes.c_str(), bytes.size());
   }
 
   /**
-   * Copies the contents of a region of a Java string to a C++ string. The
-   * resulting string has a modified UTF-8 encoding.
+   * Converts the given Java String to a C++ string encoded in UTF-8.
+   *
+   * Note that this string is encoded in standard UTF-8, and *not* in the
+   * modified UTF-8 customarily used in the JNI API.
    */
-  std::string GetStringUtfRegion(const String& string, size_t start,
-                                 size_t len);
+  std::string ToStringUtf(const String& string);
 
   // MARK: Array Operations
 

firestore/src/jni/loader.h

@@ -44,7 +44,7 @@ class Loader {
    * Returns true if the loader has succeeded. If not, any errors have already
    * been logged.
    */
-  bool ok() const { return ok_; }
+  bool ok() const { return ok_ && !env_->ExceptionCheck(); }
 
   /**
    * Adds metadata about embedded class files in the binary distribution.

firestore/src/jni/object.cc

@@ -11,23 +11,24 @@ namespace jni {
 namespace {
 
 Method<bool> kEquals("equals", "(Ljava/lang/Object;)Z");
-jclass g_clazz = nullptr;
+Method<String> kToString("toString", "()Ljava/lang/String;");
+jclass object_class = nullptr;
 
 }  // namespace
 
 void Object::Initialize(Loader& loader) {
-  g_clazz = util::object::GetClass();
-  loader.LoadFromExistingClass("java/lang/Object", g_clazz, kEquals);
+  object_class = util::object::GetClass();
+  loader.LoadFromExistingClass("java/lang/Object", object_class, kEquals,
+                               kToString);
 }
 
-Class Object::GetClass() { return Class(util::object::GetClass()); }
+Class Object::GetClass() { return Class(object_class); }
 
-std::string Object::ToString(JNIEnv* env) const {
-  return util::JniObjectToString(env, object_);
+std::string Object::ToString(Env& env) const {
+  Local<String> java_string = env.Call(*this, kToString);
+  return java_string.ToString(env);
 }
 
-std::string Object::ToString(Env& env) const { return ToString(env.get()); }
-
 bool Object::Equals(Env& env, const Object& other) const {
   return env.Call(*this, kEquals, other);
 }