Fix Unicode handling on Android

The JNI support for Java Strings added in cl/321475293 did not handle Unicode
characters correctly, passing the number of bytes requested to a method that
actually wanted the number of chars. Switching to this broke reading strings
from Java if they have any non-ASCII Unicode characters in them.

Additionally, the old routines cl/321475293 was supposed to replace treat the
bytes coming into/out of Java as modified UTF-8, even though our product works
with standard UTF-8. This meant we had latent bugs around handling of null
bytes and characters beyond the basic multilingual plane.

This CL fixes both issues, by converting Strings to UTF-8 bytes using methods
on String itself. This avoids dealing in modified UTF-8 or the bug prone JNI
methods like `GetStringUTFRegion`.

PiperOrigin-RevId: 348050717

Author: wilhuff

firestore/src/android/firestore_android.cc

@@ -232,14 +232,15 @@ bool FirestoreInternal::Initialize(App* app) {
     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);
@@ -279,30 +280,27 @@ bool FirestoreInternal::Initialize(App* app) {
     TransactionInternal::Initialize(loader);
     WriteBatchInternal::Initialize(loader);
     if (!loader.ok()) {
-      ReleaseClassesLocked(app);
+      ReleaseClassesLocked(env);
       return false;
     }
 
     FIREBASE_DEV_ASSERT(global_loader == nullptr);
     global_loader = new Loader(Move(loader));
 
     if (initial_log_state != InitialLogState::kUnset) {
-      Env env;
       bool enabled = initial_log_state == InitialLogState::kSetEnabled;
       env.Call(kSetLoggingEnabled, enabled);
-
-      // If this fails it's unimportant. If there's some broad systemic failure
-      // the next real operation will trigger it too.
-      env.ExceptionClear();
     }
   }
   initialize_count++;
   return true;
 }
 
 /* static */
-void FirestoreInternal::ReleaseClassesLocked(App* app) {
+void FirestoreInternal::ReleaseClassesLocked(Env& env) {
   // Assumes `init_mutex` is held.
+  String::Terminate(env);
+
   delete global_loader;
   global_loader = nullptr;
 }
@@ -313,7 +311,9 @@ void FirestoreInternal::Terminate(App* app) {
   FIREBASE_ASSERT(initialize_count > 0);
   initialize_count--;
   if (initialize_count == 0) {
-    ReleaseClassesLocked(app);
+    Env env(app->GetJNIEnv());
+    ReleaseClassesLocked(env);
+
     delete java_firestores;
     java_firestores = nullptr;
   }

firestore/src/android/firestore_android.h

@@ -178,7 +178,7 @@ class FirestoreInternal {
   void ShutdownUserCallbackExecutor(jni::Env& env);
 
   static bool Initialize(App* app);
-  static void ReleaseClassesLocked(App* app);
+  static void ReleaseClassesLocked(jni::Env& env);
   static void Terminate(App* app);
 
   jni::Global<jni::Object> user_callback_executor_;

firestore/src/jni/env.cc

@@ -2,6 +2,7 @@
 
 #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 +145,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);
 }

firestore/src/jni/object.h

@@ -43,10 +43,12 @@ class Object {
   }
 
   /**
-   * Converts this object to a C++ String by calling the Java `toString` method
-   * on it.
+   * Converts this object to a C++ String encoded in UTF-8 by calling the Java
+   * `toString` method on it.
+   *
+   * 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 ToString(JNIEnv* env) const;
   std::string ToString(Env& env) const;
 
   bool Equals(Env& env, const Object& other) const;

firestore/src/jni/string.cc

@@ -1,20 +1,60 @@
 #include "firestore/src/jni/string.h"
 
+#include <cstring>
+
+#include "app/src/log.h"
 #include "app/src/util_android.h"
+#include "firestore/src/jni/array.h"
 #include "firestore/src/jni/class.h"
 #include "firestore/src/jni/env.h"
+#include "firestore/src/jni/loader.h"
 
 namespace firebase {
 namespace firestore {
 namespace jni {
+namespace {
+
+Constructor<String> kNewFromBytes("([BLjava/lang/String;)V");
+Method<Array<uint8_t>> kGetBytes("getBytes", "(Ljava/lang/String;)[B");
+jclass string_class = nullptr;
+jstring utf8_string = nullptr;
+
+}  // namespace
+
+void String::Initialize(Env& env, Loader& loader) {
+  string_class = util::string::GetClass();
+  loader.LoadFromExistingClass("java/lang/String", string_class, kNewFromBytes,
+                               kGetBytes);
 
-Class String::GetClass() { return Class(util::string::GetClass()); }
+  FIREBASE_DEV_ASSERT(utf8_string == nullptr);
+  Local<String> utf8(env.get(), env.get()->NewStringUTF("UTF-8"));
+  if (!env.ok()) return;
 
-std::string String::ToString(Env& env) const {
-  size_t len = env.GetStringUtfLength(*this);
-  return env.GetStringUtfRegion(*this, 0, len);
+  utf8_string = Global<String>(utf8).release();
 }
 
+void String::Terminate(Env& env) {
+  if (utf8_string) {
+    env.get()->DeleteGlobalRef(utf8_string);
+    utf8_string = nullptr;
+  }
+}
+
+Class String::GetClass() { return Class(string_class); }
+
+String String::GetUtf8() { return String(utf8_string); }
+
+Local<String> String::Create(Env& env, const Array<uint8_t>& bytes,
+                             const String& encoding) {
+  return env.New(kNewFromBytes, bytes, encoding);
+}
+
+Local<Array<uint8_t>> String::GetBytes(Env& env, const String& encoding) const {
+  return env.Call(*this, kGetBytes, encoding);
+}
+
+std::string String::ToString(Env& env) const { return env.ToStringUtf(*this); }
+
 }  // namespace jni
 }  // namespace firestore
 }  // namespace firebase

firestore/src/jni/string.h

@@ -22,11 +22,34 @@ class String : public Object {
   String() = default;
   explicit String(jstring string) : Object(string) {}
 
+  /**
+   * Creates a new Java String from the given bytes, using the given encoding.
+   * This matches the behavior of the Java `String(byte[], String)` constructor.
+   *
+   * @param bytes A Java array of encoded bytes.
+   * @param encoding A Java string naming the encoding of the bytes.
+   */
+  static Local<String> Create(Env& env, const Array<uint8_t>& bytes,
+                              const String& encoding);
+
   jstring get() const override { return static_cast<jstring>(object_); }
 
+  static void Initialize(Env& env, Loader& loader);
+  static void Terminate(Env& env);
+
   static Class GetClass();
 
-  /** Converts this Java String to a C++ string. */
+  /** Returns a Java String representing "UTF-8". */
+  static String GetUtf8();
+
+  Local<Array<uint8_t>> GetBytes(Env& env, const String& encoding) const;
+
+  /**
+   * Converts this 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 ToString(Env& env) const;
 };