fix(ndk): iterate over the Map.entrySet directly instead of copying…

… the keys to an `ArrayList` when copying the metadata into the NDK structures
bugsnag · Apr 29, 2024 · 874ee56 · 874ee56
1 parent 2d79f75
commit 874ee56
Show file tree

Hide file tree

Showing 6 changed files with 54 additions and 45 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -5,7 +5,9 @@
 ### Enhancements
 
 * `Configuration.maxBreadcrumbs` is now obeyed by `bugsnag-plugin-android-ndk`, so native events will include the correct number of breadcrumbs
-  []()
+  [#2020](https://github.com/bugsnag/bugsnag-android/pull/2020)
+* `bugsnag-plugin-android-ndk` will no longer create an `ArrayList` copy of metadata keys when leaving breadcrumbs
+  [#2022](https://github.com/bugsnag/bugsnag-android/pull/2022)
 
 ## 6.4.0 (2024-04-15)
 

diff --git a/bugsnag-plugin-android-ndk/src/main/java/com/bugsnag/android/ndk/NativeBridge.kt b/bugsnag-plugin-android-ndk/src/main/java/com/bugsnag/android/ndk/NativeBridge.kt
@@ -121,7 +121,7 @@ class NativeBridge(private val bgTaskService: BackgroundTaskService) : StateObse
                 event.message,
                 event.type.toNativeValue(),
                 event.timestamp,
-                makeSafeMetadata(event.metadata)
+                event.metadata
             )
 
             NotifyHandled -> addHandledEvent()
@@ -172,13 +172,6 @@ class NativeBridge(private val bgTaskService: BackgroundTaskService) : StateObse
         }
     }
 
-    private fun makeSafeMetadata(metadata: Map<String, Any?>): Map<String, Any?> {
-        if (metadata.isEmpty()) return metadata
-        return object : Map<String, Any?> by metadata {
-            override fun get(key: String): Any? = OpaqueValue.makeSafe(metadata[key])
-        }
-    }
-
     private fun isInvalidMessage(msg: Any?): Boolean {
         if (msg == null || msg !is StateEvent) {
             return true

diff --git a/bugsnag-plugin-android-ndk/src/main/java/com/bugsnag/android/ndk/OpaqueValue.kt b/bugsnag-plugin-android-ndk/src/main/java/com/bugsnag/android/ndk/OpaqueValue.kt
@@ -7,12 +7,12 @@ import java.io.StringWriter
  * Marker class for values that are `BSG_METADATA_OPAQUE_VALUE` in the C layer
  */
 internal class OpaqueValue(val json: String) {
-    companion object {
+    internal companion object {
         private const val MAX_NDK_STRING_LENGTH = 64
         private const val US_ASCII_MAX_CODEPOINT = 127
         private const val INITIAL_BUFFER_SIZE = 256
 
-        private fun isStringNDKSupported(value: String): Boolean {
+        fun isStringNDKSupported(value: String): Boolean {
             // anything over 63 characters is definitely not supported
             if (value.length >= MAX_NDK_STRING_LENGTH) return false
 
@@ -27,7 +27,7 @@ internal class OpaqueValue(val json: String) {
             return value.toByteArray().size < MAX_NDK_STRING_LENGTH
         }
 
-        private fun encode(value: Any): String {
+        fun encode(value: Any): String {
             val writer = StringWriter(INITIAL_BUFFER_SIZE)
             writer.use { JsonStream(it).value(value, false) }
             return writer.toString()
@@ -38,6 +38,7 @@ internal class OpaqueValue(val json: String) {
          * is both a compatible type and fits into the available space. This method can return
          * any one of: `Boolean`, `Number`, `String`, `OpaqueValue` or `null`.
          */
+        @JvmStatic
         fun makeSafe(value: Any?): Any? = when {
             value is Boolean -> value
             value is Number -> value

diff --git a/bugsnag-plugin-android-ndk/src/main/jni/jni_cache.c b/bugsnag-plugin-android-ndk/src/main/jni/jni_cache.c
@@ -218,6 +218,8 @@ bool bsg_jni_cache_init(JNIEnv *env) {
   CACHE_CLASS(OpaqueValue, "com/bugsnag/android/ndk/OpaqueValue");
   CACHE_METHOD(OpaqueValue, OpaqueValue_getJson, "getJson",
                "()Ljava/lang/String;");
+  CACHE_STATIC_METHOD(OpaqueValue, OpaqueValue_makeSafe, "makeSafe",
+                      "(Ljava/lang/Object;)Ljava/lang/Object;");
 
   CACHE_ENUM_CONSTANT(ErrorType_C, "com/bugsnag/android/ErrorType", "C");
 

diff --git a/bugsnag-plugin-android-ndk/src/main/jni/jni_cache.h b/bugsnag-plugin-android-ndk/src/main/jni/jni_cache.h
@@ -89,6 +89,7 @@ typedef struct {
 
   jclass OpaqueValue;
   jmethodID OpaqueValue_getJson;
+  jmethodID OpaqueValue_makeSafe;
 
   jobject ErrorType_C;
 } bsg_jni_cache_t;

diff --git a/bugsnag-plugin-android-ndk/src/main/jni/metadata.c b/bugsnag-plugin-android-ndk/src/main/jni/metadata.c
@@ -343,24 +343,29 @@ static void populate_metadata_value(JNIEnv *env, bugsnag_metadata *dst,
     return;
   }
 
-  if (bsg_safe_is_instance_of(env, _value, bsg_jni_cache->number)) {
+  jobject safe_value = bsg_safe_call_static_object_method(
+      env, bsg_jni_cache->OpaqueValue, bsg_jni_cache->OpaqueValue_makeSafe,
+      _value);
+
+  if (bsg_safe_is_instance_of(env, safe_value, bsg_jni_cache->number)) {
     // add a double metadata value
     double value = bsg_safe_call_double_method(
-        env, _value, bsg_jni_cache->number_double_value);
+        env, safe_value, bsg_jni_cache->number_double_value);
     bsg_add_metadata_value_double(dst, section, name, value);
-  } else if (bsg_safe_is_instance_of(env, _value, bsg_jni_cache->Boolean)) {
+  } else if (bsg_safe_is_instance_of(env, safe_value, bsg_jni_cache->Boolean)) {
     // add a boolean metadata value
     bool value = bsg_safe_call_boolean_method(
-        env, _value, bsg_jni_cache->Boolean_booleanValue);
+        env, safe_value, bsg_jni_cache->Boolean_booleanValue);
     bsg_add_metadata_value_bool(dst, section, name, value);
-  } else if (bsg_safe_is_instance_of(env, _value, bsg_jni_cache->String)) {
-    const char *value = bsg_safe_get_string_utf_chars(env, _value);
+  } else if (bsg_safe_is_instance_of(env, safe_value, bsg_jni_cache->String)) {
+    const char *value = bsg_safe_get_string_utf_chars(env, safe_value);
     if (value != NULL) {
       bsg_add_metadata_value_str(dst, section, name, value);
     }
-  } else if (bsg_safe_is_instance_of(env, _value, bsg_jni_cache->OpaqueValue)) {
+  } else if (bsg_safe_is_instance_of(env, safe_value,
+                                     bsg_jni_cache->OpaqueValue)) {
     jstring _json = bsg_safe_call_object_method(
-        env, _value, bsg_jni_cache->OpaqueValue_getJson);
+        env, safe_value, bsg_jni_cache->OpaqueValue_getJson);
     const char *json = bsg_safe_get_string_utf_chars(env, _json);
 
     if (json != NULL) {
@@ -513,8 +518,8 @@ void bsg_populate_metadata(JNIEnv *env, bugsnag_metadata *dst,
 
 void bsg_populate_crumb_metadata(JNIEnv *env, bugsnag_breadcrumb *crumb,
                                  jobject metadata) {
-  jobject keyset = NULL;
-  jobject keylist = NULL;
+  jobject entryset = NULL;
+  jobject entries = NULL;
 
   if (metadata == NULL) {
     goto exit;
@@ -526,43 +531,48 @@ void bsg_populate_crumb_metadata(JNIEnv *env, bugsnag_breadcrumb *crumb,
   // get size of metadata map
   jint map_size =
       bsg_safe_call_int_method(env, metadata, bsg_jni_cache->Map_size);
-  if (map_size == -1) {
+  if (map_size <= 0) {
     goto exit;
   }
 
   // create a list of metadata keys
-  keyset =
-      bsg_safe_call_object_method(env, metadata, bsg_jni_cache->Map_keySet);
-  if (keyset == NULL) {
+  entryset =
+      bsg_safe_call_object_method(env, metadata, bsg_jni_cache->Map_entrySet);
+  if (entryset == NULL) {
     goto exit;
   }
-  keylist = bsg_safe_new_object(env, bsg_jni_cache->ArrayList,
-                                bsg_jni_cache->ArrayList_constructor_collection,
-                                keyset);
-  if (keylist == NULL) {
+  entries =
+      bsg_safe_call_object_method(env, entryset, bsg_jni_cache->Set_iterator);
+  if (entries == NULL) {
     goto exit;
   }
 
-  for (int i = 0; i < map_size; i++) {
-    jstring _key = bsg_safe_call_object_method(
-        env, keylist, bsg_jni_cache->ArrayList_get, (jint)i);
-    jobject _value = bsg_safe_call_object_method(env, metadata,
-                                                 bsg_jni_cache->Map_get, _key);
-
-    if (_key != NULL && _value != NULL) {
-      const char *key = bsg_safe_get_string_utf_chars(env, _key);
-      if (key != NULL) {
-        populate_metadata_value(env, &crumb->metadata, "metaData", key, _value);
-        bsg_safe_release_string_utf_chars(env, _key, key);
+  while (bsg_safe_call_boolean_method(env, entries,
+                                      bsg_jni_cache->Iterator_hasNext)) {
+    (*env)->PushLocalFrame(env, 3);
+    {
+      jobject entry = bsg_safe_call_object_method(env, entries,
+                                                  bsg_jni_cache->Iterator_next);
+      jstring _key = bsg_safe_call_object_method(
+          env, entry, bsg_jni_cache->MapEntry_getKey);
+      jobject _value = bsg_safe_call_object_method(
+          env, entry, bsg_jni_cache->MapEntry_getValue);
+
+      if (_key != NULL && _value != NULL) {
+        const char *key = bsg_safe_get_string_utf_chars(env, _key);
+        if (key != NULL) {
+          populate_metadata_value(env, &crumb->metadata, "metaData", key,
+                                  _value);
+          bsg_safe_release_string_utf_chars(env, _key, key);
+        }
       }
     }
-    bsg_safe_delete_local_ref(env, _key);
-    bsg_safe_delete_local_ref(env, _value);
+    (*env)->PopLocalFrame(env, NULL);
   }
 
 exit:
-  bsg_safe_delete_local_ref(env, keyset);
-  bsg_safe_delete_local_ref(env, keylist);
+  bsg_safe_delete_local_ref(env, entries);
+  bsg_safe_delete_local_ref(env, entryset);
 }
 
 void bsg_populate_event(JNIEnv *env, bugsnag_event *event) {