mobile: check for pending exceptions after JNI call (#24361)

Commit Message: Check for pending exceptions after some of the Java calls from within the JNI layer. At Lyft, we noticed that we have a bunch of crashes related to us not checking for exceptions after on_request_headers or/and on_response_headers methods.
Additional description: I tried to use `ENVOY_LOG_EVENT_TO_LOGGER` to log when we detect a pending exception but ran into issues with deadlocks. Got rid of the extra logging to unblock my change.
Risk Level: Low, mostly additive change. New code executes for when the library crashes previously.
Testing: Integration test
Docs Changes:
Release Notes:
Platform Specific Features:

Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>

mobile/library/common/jni/BUILD

@@ -21,6 +21,7 @@ cc_library(
     deps = [
         "//library/common/jni/import:jni_import_lib",
         "//library/common/types:c_types_lib",
+        "//library/common/types:managed_types_lib",
         "@envoy//source/common/common:assert_lib",
     ],
 )
@@ -48,6 +49,7 @@ cc_library(
         ":ndk_jni_support",
         "//library/common:envoy_main_interface_lib",
         "//library/common/api:c_types",
+        "//library/common/types:managed_types_lib",
     ],
     # We need this to ensure that we link this into the .so even though there are no code references.
     alwayslink = True,
@@ -115,6 +117,7 @@ cc_binary(
         "//library/common:envoy_main_interface_lib",
         "//library/common/api:c_types",
         "//library/common/jni/import:jni_import_lib",
+        "//library/common/types:managed_types_lib",
     ],
 )
 
@@ -169,6 +172,7 @@ cc_library(
         "//library/common/jni/import:jni_import_lib",
         "//library/common:envoy_main_interface_lib",
         "//library/common/types:c_types_lib",
+        "//library/common/types:managed_types_lib",
         "@envoy//source/common/common:assert_lib",
     ] + TEST_EXTENSIONS,
 )

mobile/library/common/jni/jni_interface.cc

@@ -8,6 +8,7 @@
 #include "library/common/jni/jni_utility.h"
 #include "library/common/jni/jni_version.h"
 #include "library/common/main_interface.h"
+#include "library/common/types/managed_envoy_headers.h"
 
 // NOLINT(namespace-envoy)
 
@@ -278,22 +279,23 @@ Java_io_envoyproxy_envoymobile_engine_JniLibrary_recordHistogramValue(JNIEnv* en
 
 // JvmCallbackContext
 
-static void pass_headers(const char* method, envoy_headers headers, jobject j_context) {
+static void passHeaders(const char* method, const Envoy::Types::ManagedEnvoyHeaders& headers,
+                        jobject j_context) {
   JNIEnv* env = get_env();
   jclass jcls_JvmCallbackContext = env->GetObjectClass(j_context);
   jmethodID jmid_passHeader = env->GetMethodID(jcls_JvmCallbackContext, method, "([B[BZ)V");
   jboolean start_headers = JNI_TRUE;
 
-  for (envoy_map_size_t i = 0; i < headers.length; i++) {
+  for (envoy_map_size_t i = 0; i < headers.get().length; i++) {
     // Note: this is just an initial implementation, and we will pass a more optimized structure in
     // the future.
     // Note: the JNI function NewStringUTF would appear to be an appealing option here, except it
     // requires a null-terminated *modified* UTF-8 string.
 
     // Create platform byte array for header key
-    jbyteArray j_key = native_data_to_array(env, headers.entries[i].key);
+    jbyteArray j_key = native_data_to_array(env, headers.get().entries[i].key);
     // Create platform byte array for header value
-    jbyteArray j_value = native_data_to_array(env, headers.entries[i].value);
+    jbyteArray j_value = native_data_to_array(env, headers.get().entries[i].value);
 
     // Pass this header pair to the platform
     env->CallVoidMethod(j_context, jmid_passHeader, j_key, j_value, start_headers);
@@ -306,19 +308,18 @@ static void pass_headers(const char* method, envoy_headers headers, jobject j_co
   }
 
   env->DeleteLocalRef(jcls_JvmCallbackContext);
-  release_envoy_headers(headers);
 }
 
 // Platform callback implementation
 // These methods call jvm methods which means the local references created will not be
 // released automatically. Manual bookkeeping is required for these methods.
 
-static void* jvm_on_headers(const char* method, envoy_headers headers, bool end_stream,
-                            envoy_stream_intel stream_intel, void* context) {
+static void* jvm_on_headers(const char* method, const Envoy::Types::ManagedEnvoyHeaders& headers,
+                            bool end_stream, envoy_stream_intel stream_intel, void* context) {
   jni_log("[Envoy]", "jvm_on_headers");
   JNIEnv* env = get_env();
   jobject j_context = static_cast<jobject>(context);
-  pass_headers("passHeader", headers, j_context);
+  passHeaders("passHeader", headers, j_context);
 
   jclass jcls_JvmCallbackContext = env->GetObjectClass(j_context);
   jmethodID jmid_onHeaders =
@@ -327,24 +328,52 @@ static void* jvm_on_headers(const char* method, envoy_headers headers, bool end_
   jlongArray j_stream_intel = native_stream_intel_to_array(env, stream_intel);
   // Note: be careful of JVM types. Before we casted to jlong we were getting integer problems.
   // TODO: make this cast safer.
-  jobject result = env->CallObjectMethod(j_context, jmid_onHeaders, (jlong)headers.length,
+  jobject result = env->CallObjectMethod(j_context, jmid_onHeaders, (jlong)headers.get().length,
                                          end_stream ? JNI_TRUE : JNI_FALSE, j_stream_intel);
+  // TODO(Augustyniak): Pass the name of the filter in here so that we can instrument the origin of
+  // the JNI exception better.
+  bool exception_cleared = clear_pending_exceptions(env);
 
   env->DeleteLocalRef(j_stream_intel);
   env->DeleteLocalRef(jcls_JvmCallbackContext);
 
-  return result;
+  if (!exception_cleared) {
+    return result;
+  }
+
+  // Create a "no operation" result:
+  //  1. Tell the filter chain to continue the iteration.
+  //  2. Return headers received on as method's input as part of the method's output.
+  jclass jcls_object_array = env->FindClass("java/lang/Object");
+  jobjectArray noopResult = env->NewObjectArray(2, jcls_object_array, NULL);
+
+  jclass jcls_int = env->FindClass("java/lang/Integer");
+  jmethodID jmid_intInit = env->GetMethodID(jcls_int, "<init>", "(I)V");
+  jobject j_status = env->NewObject(jcls_int, jmid_intInit, 0);
+  // Set status to "0" (FilterHeadersStatus::Continue). Signal that the intent
+  // is to continue the iteration of the filter chain.
+  env->SetObjectArrayElement(noopResult, 0, j_status);
+
+  // Since the "on headers" call threw an exception set input headers as output headers.
+  env->SetObjectArrayElement(noopResult, 1, ToJavaArrayOfObjectArray(env, headers));
+
+  env->DeleteLocalRef(jcls_object_array);
+  env->DeleteLocalRef(jcls_int);
+
+  return noopResult;
 }
 
 static void* jvm_on_response_headers(envoy_headers headers, bool end_stream,
                                      envoy_stream_intel stream_intel, void* context) {
-  return jvm_on_headers("onResponseHeaders", headers, end_stream, stream_intel, context);
+  const auto managed_headers = Envoy::Types::ManagedEnvoyHeaders(headers);
+  return jvm_on_headers("onResponseHeaders", managed_headers, end_stream, stream_intel, context);
 }
 
 static envoy_filter_headers_status
-jvm_http_filter_on_request_headers(envoy_headers headers, bool end_stream,
+jvm_http_filter_on_request_headers(envoy_headers input_headers, bool end_stream,
                                    envoy_stream_intel stream_intel, const void* context) {
   JNIEnv* env = get_env();
+  const auto headers = Envoy::Types::ManagedEnvoyHeaders(input_headers);
   jobjectArray result = static_cast<jobjectArray>(jvm_on_headers(
       "onRequestHeaders", headers, end_stream, stream_intel, const_cast<void*>(context)));
 
@@ -363,9 +392,10 @@ jvm_http_filter_on_request_headers(envoy_headers headers, bool end_stream,
 }
 
 static envoy_filter_headers_status
-jvm_http_filter_on_response_headers(envoy_headers headers, bool end_stream,
+jvm_http_filter_on_response_headers(envoy_headers input_headers, bool end_stream,
                                     envoy_stream_intel stream_intel, const void* context) {
   JNIEnv* env = get_env();
+  const auto headers = Envoy::Types::ManagedEnvoyHeaders(input_headers);
   jobjectArray result = static_cast<jobjectArray>(jvm_on_headers(
       "onResponseHeaders", headers, end_stream, stream_intel, const_cast<void*>(context)));
 
@@ -484,7 +514,7 @@ static void* jvm_on_trailers(const char* method, envoy_headers trailers,
 
   JNIEnv* env = get_env();
   jobject j_context = static_cast<jobject>(context);
-  pass_headers("passHeader", trailers, j_context);
+  passHeaders("passHeader", trailers, j_context);
 
   jclass jcls_JvmCallbackContext = env->GetObjectClass(j_context);
   jmethodID jmid_onTrailers =
@@ -493,6 +523,7 @@ static void* jvm_on_trailers(const char* method, envoy_headers trailers,
   jlongArray j_stream_intel = native_stream_intel_to_array(env, stream_intel);
   // Note: be careful of JVM types. Before we casted to jlong we were getting integer problems.
   // TODO: make this cast safer.
+  // TODO(Augustyniak): check for pending exceptions after returning from JNI call.
   jobject result =
       env->CallObjectMethod(j_context, jmid_onTrailers, (jlong)trailers.length, j_stream_intel);
 
@@ -632,7 +663,7 @@ jvm_http_filter_on_resume(const char* method, envoy_headers* headers, envoy_data
   jlong headers_length = -1;
   if (headers) {
     headers_length = (jlong)headers->length;
-    pass_headers("passHeader", *headers, j_context);
+    passHeaders("passHeader", *headers, j_context);
   }
   jbyteArray j_in_data = nullptr;
   if (data) {
@@ -641,7 +672,7 @@ jvm_http_filter_on_resume(const char* method, envoy_headers* headers, envoy_data
   jlong trailers_length = -1;
   if (trailers) {
     trailers_length = (jlong)trailers->length;
-    pass_headers("passTrailer", *trailers, j_context);
+    passHeaders("passTrailer", *trailers, j_context);
   }
   jlongArray j_stream_intel = native_stream_intel_to_array(env, stream_intel);
 
@@ -714,6 +745,8 @@ static void* call_jvm_on_complete(envoy_stream_intel stream_intel,
   jobject result =
       env->CallObjectMethod(j_context, jmid_onComplete, j_stream_intel, j_final_stream_intel);
 
+  clear_pending_exceptions(env);
+
   env->DeleteLocalRef(j_stream_intel);
   env->DeleteLocalRef(j_final_stream_intel);
   env->DeleteLocalRef(jcls_JvmObserverContext);
@@ -737,6 +770,8 @@ static void* call_jvm_on_error(envoy_error error, envoy_stream_intel stream_inte
   jobject result = env->CallObjectMethod(j_context, jmid_onError, error.error_code, j_error_message,
                                          error.attempt_count, j_stream_intel, j_final_stream_intel);
 
+  clear_pending_exceptions(env);
+
   env->DeleteLocalRef(j_stream_intel);
   env->DeleteLocalRef(j_final_stream_intel);
   env->DeleteLocalRef(j_error_message);
@@ -769,6 +804,8 @@ static void* call_jvm_on_cancel(envoy_stream_intel stream_intel,
   jobject result =
       env->CallObjectMethod(j_context, jmid_onCancel, j_stream_intel, j_final_stream_intel);
 
+  clear_pending_exceptions(env);
+
   env->DeleteLocalRef(j_stream_intel);
   env->DeleteLocalRef(j_final_stream_intel);
   env->DeleteLocalRef(jcls_JvmObserverContext);

mobile/library/common/jni/jni_utility.cc

@@ -66,6 +66,16 @@ void jni_delete_const_global_ref(const void* context) {
   jni_delete_global_ref(const_cast<void*>(context));
 }
 
+bool clear_pending_exceptions(JNIEnv* env) {
+  if (env->ExceptionCheck() == JNI_TRUE) {
+    env->ExceptionClear();
+    // TODO(Augustyniak): Log exception details.
+    return true;
+  } else {
+    return false;
+  }
+}
+
 int unbox_integer(JNIEnv* env, jobject boxedInteger) {
   jclass jcls_Integer = env->FindClass("java/lang/Integer");
   jmethodID jmid_intValue = env->GetMethodID(jcls_Integer, "intValue", "()I");
@@ -278,6 +288,21 @@ envoy_map to_native_map(JNIEnv* env, jobjectArray entries) {
   return native_map;
 }
 
+jobjectArray ToJavaArrayOfObjectArray(JNIEnv* env, const Envoy::Types::ManagedEnvoyHeaders& map) {
+  jclass jcls_byte_array = env->FindClass("java/lang/Object");
+  jobjectArray javaArray = env->NewObjectArray(2 * map.get().length, jcls_byte_array, nullptr);
+
+  for (envoy_map_size_t i = 0; i < map.get().length; i++) {
+    jbyteArray key = native_data_to_array(env, map.get().entries[i].key);
+    jbyteArray value = native_data_to_array(env, map.get().entries[i].value);
+
+    env->SetObjectArrayElement(javaArray, 2 * i, key);
+    env->SetObjectArrayElement(javaArray, 2 * i + 1, value);
+  }
+
+  return javaArray;
+}
+
 jobjectArray ToJavaArrayOfByteArray(JNIEnv* env, const std::vector<std::string>& v) {
   jclass jcls_byte_array = env->FindClass("[B");
   jobjectArray joa = env->NewObjectArray(v.size(), jcls_byte_array, nullptr);

mobile/library/common/jni/jni_utility.h

@@ -5,6 +5,7 @@
 
 #include "library/common/jni/import/jni_import.h"
 #include "library/common/types/c_types.h"
+#include "library/common/types/managed_envoy_headers.h"
 
 // NOLINT(namespace-envoy)
 
@@ -43,6 +44,15 @@ void jni_delete_global_ref(void* context);
 
 void jni_delete_const_global_ref(const void* context);
 
+/**
+ * Clears any pending exceptions that may have been rides in result to a call into Java code.
+ *
+ * @param env, the JNI env pointer.
+ *
+ * @return Whether any pending JNI exception was cleared.
+ */
+bool clear_pending_exceptions(JNIEnv* env);
+
 int unbox_integer(JNIEnv* env, jobject boxedInteger);
 
 envoy_data array_to_native_data(JNIEnv* env, jbyteArray j_data);
@@ -100,6 +110,8 @@ jbyteArray ToJavaByteArray(JNIEnv* env, const uint8_t* bytes, size_t len);
 
 jbyteArray ToJavaByteArray(JNIEnv* env, const std::string& str);
 
+jobjectArray ToJavaArrayOfObjectArray(JNIEnv* env, const Envoy::Types::ManagedEnvoyHeaders& map);
+
 void JavaArrayOfByteArrayToStringVector(JNIEnv* env, jobjectArray array,
                                         std::vector<std::string>* out);
 

mobile/library/common/types/BUILD

@@ -18,3 +18,15 @@ envoy_cc_library(
         "@envoy//source/common/common:assert_lib",
     ],
 )
+
+envoy_cc_library(
+    name = "managed_types_lib",
+    srcs = [
+        "managed_envoy_headers.h",
+    ],
+    repository = "@envoy",
+    visibility = ["//visibility:public"],
+    deps = [
+        "//library/common/types:c_types_lib",
+    ],
+)

mobile/library/common/types/managed_envoy_headers.h

@@ -0,0 +1,32 @@
+#pragma once
+
+#include "library/common/types/c_types.h"
+
+namespace Envoy {
+namespace Types {
+
+/**
+ * A wrapper around envoy_headers that's responsible for freeing
+ * the underlying headers when they are not needed anymore.
+ */
+class ManagedEnvoyHeaders {
+public:
+  /**
+   * Initialize a new instance of the receiver using a given instance of envoy headers.
+   *
+   * @param headers, that should be wrapped by the receiver. The wrapper will hold onto
+   *                 the passed headers and free them once the receiver is not used anymore.
+   */
+  ManagedEnvoyHeaders(envoy_headers headers) : headers_(headers){};
+  ~ManagedEnvoyHeaders() { release_envoy_headers(headers_); }
+  const envoy_headers& get() const { return headers_; }
+
+private:
+  envoy_headers headers_;
+  // Make copy and assignment operators private to prevent copying of the receiver.
+  ManagedEnvoyHeaders(const ManagedEnvoyHeaders&);
+  ManagedEnvoyHeaders& operator=(const ManagedEnvoyHeaders&);
+};
+
+} // namespace Types
+} // namespace Envoy

mobile/test/kotlin/integration/BUILD

@@ -1,4 +1,5 @@
-load("@envoy_mobile//bazel:kotlin_test.bzl", "envoy_mobile_jni_kt_test")
+load("@envoy_mobile//bazel:kotlin_test.bzl", "envoy_mobile_android_test", "envoy_mobile_jni_kt_test")
+load("@envoy_mobile//bazel:kotlin_lib.bzl", "envoy_mobile_kt_library")
 
 envoy_mobile_jni_kt_test(
     name = "engine_start_test",
@@ -200,3 +201,23 @@ envoy_mobile_jni_kt_test(
         "//library/kotlin/io/envoyproxy/envoymobile:envoy_interfaces_lib",
     ],
 )
+
+envoy_mobile_android_test(
+    name = "filter_throwing_exception_test",
+    srcs = [
+        "FilterThrowingExceptionTest.kt",
+    ],
+    exec_properties = {
+        # TODO(lfpino): Remove this once the sandboxNetwork=off works for ipv4 localhost addresses.
+        "sandboxNetwork": "standard",
+        "dockerNetwork": "standard",
+    },
+    native_deps = [
+        "//library/common/jni:libndk_envoy_jni.so",
+        "//library/common/jni:libndk_envoy_jni.jnilib",
+    ],
+    deps = [
+        "//library/kotlin/io/envoyproxy/envoymobile:envoy_interfaces_lib",
+        "//library/kotlin/io/envoyproxy/envoymobile:envoy_lib",
+    ],
+)