Skip to content

Commit

Permalink
mobile: Fix memory leak in the terminate JNI code (envoyproxy#33311)
Browse files Browse the repository at this point in the history 
This PR fixes a memory leak by deleting the InternalEngine * after calling the InternalEngine::terminate.

This PR also fixes an issue with CronetUrlRequestTest.testInternetDisconnectedError() to always load a new singleton instance of AndroidNetworkMonitor by calling AndroidNetworkMonitor.shutdown(). Prior to this, AndroidNetworkInstance singleton instance might be holding on an EnvoyEngine that had been destroyed. Ideally, we should fix this by exposing the EngineCallbacks.on_exit_ callback to automatically reset the instance of AndroidNetworkMonitor to null instead of manually calling AndroidNetworkMonitor.shutdown(). However, that EngineCallbacks.on_exit_ callback has not been exposed yet into the Java API.

Risk Level: low
Testing: unit test
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: mobile

Signed-off-by: Fredy Wijaya <[email protected]>
  • Loading branch information
@fredyw
fredyw authored Apr 4, 2024
1 parent 9711b5b commit 6e4e7ee
Show file tree
Hide file tree
Showing 3 changed files with 43 additions and 33 deletions.
9 changes: 9 additions & 0 deletions mobile/library/java/io/envoyproxy/envoymobile/engine/AndroidNetworkMonitor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,15 @@ public static void load(Context context, EnvoyEngine envoyEngine) {
}
}

/**
* Sets the {@link AndroidNetworkMonitor} singleton instance to null, so that it can be recreated
* when a new EnvoyEngine is created.
*/
@VisibleForTesting
public static void shutdown() {
instance = null;
}

private AndroidNetworkMonitor(Context context, EnvoyEngine envoyEngine) {
int permission =
ContextCompat.checkSelfPermission(context, Manifest.permission.ACCESS_NETWORK_STATE);
Expand Down
59 changes: 28 additions & 31 deletions mobile/library/jni/jni_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,27 +36,23 @@ Java_io_envoyproxy_envoymobile_engine_JniLibrary_setLogLevel(JNIEnv* /*env*/, jc
}

extern "C" JNIEXPORT jlong JNICALL Java_io_envoyproxy_envoymobile_engine_JniLibrary_initEngine(
JNIEnv* env, jclass, jobject on_start_context, jobject envoy_logger_context,
jobject event_tracker_context) {
JNIEnv* env, jclass, jobject on_engine_running, jobject envoy_logger,
jobject envoy_event_tracker) {
//================================================================================================
// EngineCallbacks
//================================================================================================
std::unique_ptr<Envoy::EngineCallbacks> callbacks = std::make_unique<Envoy::EngineCallbacks>();
if (on_start_context != nullptr) {
jobject retained_on_start_context =
env->NewGlobalRef(on_start_context); // Required to keep context in memory
callbacks->on_engine_running_ = [retained_on_start_context] {
if (on_engine_running != nullptr) {
jobject on_engine_running_global_ref = env->NewGlobalRef(on_engine_running);
callbacks->on_engine_running_ = [on_engine_running_global_ref] {
Envoy::JNI::JniHelper jni_helper(Envoy::JNI::getEnv());
Envoy::JNI::LocalRefUniquePtr<jclass> java_on_engine_running_class =
jni_helper.getObjectClass(retained_on_start_context);
jni_helper.getObjectClass(on_engine_running_global_ref);
jmethodID java_on_engine_running_method_id = jni_helper.getMethodId(
java_on_engine_running_class.get(), "invokeOnEngineRunning", "()Ljava/lang/Object;");
Envoy::JNI::LocalRefUniquePtr<jobject> unused =
jni_helper.callObjectMethod(retained_on_start_context, java_on_engine_running_method_id);

// TODO(goaway): This isn't re-used by other engine callbacks, so it's safe to delete here.
// This will need to be updated for https://github.com/envoyproxy/envoy-mobile/issues/332
jni_helper.getEnv()->DeleteGlobalRef(retained_on_start_context);
Envoy::JNI::LocalRefUniquePtr<jobject> unused = jni_helper.callObjectMethod(
on_engine_running_global_ref, java_on_engine_running_method_id);
jni_helper.getEnv()->DeleteGlobalRef(on_engine_running_global_ref);
};
callbacks->on_exit_ = [] {
// Note that this is not dispatched because the thread that
Expand All @@ -70,50 +66,49 @@ extern "C" JNIEXPORT jlong JNICALL Java_io_envoyproxy_envoymobile_engine_JniLibr
// EnvoyLogger
//================================================================================================
std::unique_ptr<Envoy::EnvoyLogger> logger = std::make_unique<Envoy::EnvoyLogger>();
if (envoy_logger_context != nullptr) {
const jobject retained_logger_context = env->NewGlobalRef(envoy_logger_context);
logger->on_log_ = [retained_logger_context](Envoy::Logger::Logger::Levels level,
if (envoy_logger != nullptr) {
const jobject envoy_logger_global_ref = env->NewGlobalRef(envoy_logger);
logger->on_log_ = [envoy_logger_global_ref](Envoy::Logger::Logger::Levels level,
const std::string& message) {
Envoy::JNI::JniHelper jni_helper(Envoy::JNI::getEnv());
Envoy::JNI::LocalRefUniquePtr<jstring> java_message =
jni_helper.newStringUtf(message.c_str());
Envoy::JNI::cppStringToJavaString(jni_helper, message);
jint java_level = static_cast<jint>(level);
Envoy::JNI::LocalRefUniquePtr<jclass> java_envoy_logger_class =
jni_helper.getObjectClass(retained_logger_context);
jni_helper.getObjectClass(envoy_logger_global_ref);
jmethodID java_log_method_id =
jni_helper.getMethodId(java_envoy_logger_class.get(), "log", "(ILjava/lang/String;)V");
jni_helper.callVoidMethod(retained_logger_context, java_log_method_id, java_level,
jni_helper.callVoidMethod(envoy_logger_global_ref, java_log_method_id, java_level,
java_message.get());
};
logger->on_exit_ = [retained_logger_context] {
Envoy::JNI::getEnv()->DeleteGlobalRef(retained_logger_context);
logger->on_exit_ = [envoy_logger_global_ref] {
Envoy::JNI::getEnv()->DeleteGlobalRef(envoy_logger_global_ref);
};
}
//================================================================================================
// EnvoyEventTracker
//================================================================================================
std::unique_ptr<Envoy::EnvoyEventTracker> event_tracker =
std::make_unique<Envoy::EnvoyEventTracker>();
if (event_tracker_context != nullptr) {
const jobject retained_event_tracker_context = env->NewGlobalRef(event_tracker_context);
event_tracker->on_track_ = [retained_event_tracker_context](
if (envoy_event_tracker != nullptr) {
const jobject event_tracker_global_ref = env->NewGlobalRef(envoy_event_tracker);
event_tracker->on_track_ = [event_tracker_global_ref](
const absl::flat_hash_map<std::string, std::string>& events) {
Envoy::JNI::JniHelper jni_helper(Envoy::JNI::getEnv());
Envoy::JNI::LocalRefUniquePtr<jobject> java_events =
Envoy::JNI::cppMapToJavaMap(jni_helper, events);
Envoy::JNI::LocalRefUniquePtr<jclass> java_envoy_event_tracker_class =
jni_helper.getObjectClass(retained_event_tracker_context);
jni_helper.getObjectClass(event_tracker_global_ref);
jmethodID java_track_method_id = jni_helper.getMethodId(java_envoy_event_tracker_class.get(),
"track", "(Ljava/util/Map;)V");
jni_helper.callVoidMethod(retained_event_tracker_context, java_track_method_id,
java_events.get());
jni_helper.callVoidMethod(event_tracker_global_ref, java_track_method_id, java_events.get());
};
event_tracker->on_exit_ = [retained_event_tracker_context] {
Envoy::JNI::getEnv()->DeleteGlobalRef(retained_event_tracker_context);
event_tracker->on_exit_ = [event_tracker_global_ref] {
Envoy::JNI::getEnv()->DeleteGlobalRef(event_tracker_global_ref);
};
}

return reinterpret_cast<intptr_t>(
return reinterpret_cast<jlong>(
new Envoy::InternalEngine(std::move(callbacks), std::move(logger), std::move(event_tracker)));
}

Expand Down Expand Up @@ -144,7 +139,9 @@ extern "C" JNIEXPORT jint JNICALL Java_io_envoyproxy_envoymobile_engine_JniLibra

extern "C" JNIEXPORT void JNICALL Java_io_envoyproxy_envoymobile_engine_JniLibrary_terminateEngine(
JNIEnv* /*env*/, jclass, jlong engine_handle) {
reinterpret_cast<Envoy::InternalEngine*>(engine_handle)->terminate();
Envoy::InternalEngine* internal_engine = reinterpret_cast<Envoy::InternalEngine*>(engine_handle);
internal_engine->terminate();
delete internal_engine;
}

extern "C" JNIEXPORT jint JNICALL Java_io_envoyproxy_envoymobile_engine_JniLibrary_recordCounterInc(
Expand Down
8 changes: 6 additions & 2 deletions mobile/test/java/org/chromium/net/CronetUrlRequestTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,11 @@
import java.util.regex.Pattern;
import io.envoyproxy.envoymobile.engine.AndroidNetworkMonitor;
import org.chromium.net.impl.CronvoyUrlRequest;
import org.chromium.net.impl.CronvoyUrlRequestContext;
import org.chromium.net.impl.Errors.EnvoyMobileError;
import org.chromium.net.impl.Errors.NetError;
import org.chromium.net.impl.CronvoyUrlResponseInfoImpl;
import org.chromium.net.testing.CronetTestRule;
import org.chromium.net.testing.CronetTestRule.CronetTestFramework;
import org.chromium.net.testing.CronetTestRule.RequiresMinApi;
import org.chromium.net.testing.FailurePhase;
import org.chromium.net.testing.Feature;
Expand Down Expand Up @@ -77,7 +77,6 @@ public class CronetUrlRequestTest {
@Rule
public final RuleChain chain = RuleChain.outerRule(mTestRule).around(mRuntimePermissionRule);

private CronetTestFramework mTestFramework;
private MockUrlRequestJobFactory mMockUrlRequestJobFactory;

@Before
Expand All @@ -91,6 +90,11 @@ public void setUp() {
public void tearDown() {
mMockUrlRequestJobFactory.shutdown();
NativeTestServer.shutdownNativeTestServer();
// Calling AndroidNetworkMonitor.shutdown() will set the AndroidNetworkMonitor singleton
// instance to null so that the next EnvoyEngine creation will have a new AndroidNetworkMonitor
// instance instead of holding on a dangling EnvoyEngine because AndroidNetworkMonitor.load
// does not update the singleton instance to a new one if there is already an existing instance.
AndroidNetworkMonitor.shutdown();
}

private TestUrlRequestCallback startAndWaitForComplete(CronetEngine engine, String url)
Expand Down

0 comments on commit 6e4e7ee

Please sign in to comment.