From 6dcd0de96c8949cd45a07395556f58d08607c6ce Mon Sep 17 00:00:00 2001 From: Valentin Shergin Date: Thu, 21 Nov 2019 16:03:06 -0800 Subject: [PATCH] Fabric: Ressetting callable inside `JMessageQueueThread` Summary: We recently realized that `JNativeRunnable` instances that RN uses to pass C++ callables to Java land actually are GC managed objects. That makes their lifetime quite unpredictable (longer than necessary). Normally, it's fine but some C++ code explicitly relies on deallocation order. To make the behavior of `JMessageQueueThread` more predictable, now we clear/reset stored `std::function` object right after an invocation to explicitly free all associated resources (`JNativeRunnable` still holds some wrapper but that wrapper holds nothing). Changelog: [INTERNAL] Reviewed By: JoshuaGross Differential Revision: D18603390 fbshipit-source-id: 362f6cc0901cbe14d3360b928c98e204d277b1aa --- .../main/jni/react/jni/JMessageQueueThread.cpp | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/ReactAndroid/src/main/jni/react/jni/JMessageQueueThread.cpp b/ReactAndroid/src/main/jni/react/jni/JMessageQueueThread.cpp index c54b325257b..9c70f668e87 100644 --- a/ReactAndroid/src/main/jni/react/jni/JMessageQueueThread.cpp +++ b/ReactAndroid/src/main/jni/react/jni/JMessageQueueThread.cpp @@ -32,9 +32,20 @@ struct JavaJSException : jni::JavaClass { }; std::function wrapRunnable(std::function&& runnable) { - return [runnable=std::move(runnable)] { + return [runnable = std::move(runnable)]() mutable { + if (!runnable) { + // Runnable is empty, nothing to run. + return; + } + + auto localRunnable = std::move(runnable); + + // Clearing `runnable` to free all associated resources that stored lambda + // might retain. + runnable = nullptr; + try { - runnable(); + localRunnable(); } catch (const jsi::JSError& ex) { throwNewJavaException( JavaJSException::create(ex.getMessage().c_str(), ex.getStack().c_str(), ex)