From ff1de68e67651d6e7ed85f663a5085abb06e9a05 Mon Sep 17 00:00:00 2001 From: Joshua Gross Date: Fri, 3 May 2019 15:47:41 -0700 Subject: [PATCH] Java objects should own C++ State Summary: When passing StateWrapper objects across the JNI, we were not ensuring that the Java objects would own the C++ state. This was initially done because I assumed that in Java, State would either be used immediately or discarded, so this wouldn't be unsafe. As it turns out, it makes sense in some cases to store the StateWrapper in Java and use it later, potentially even in other threads, so we need to make sure we maintain ownership of the C++ object from the Java object. Reviewed By: shergin Differential Revision: D15206194 fbshipit-source-id: a437d921ba00b194cf08bad80666bd99baf11d52 --- .../java/com/facebook/react/fabric/jsi/jni/Binding.cpp | 8 +++----- .../com/facebook/react/fabric/jsi/jni/StateWrapperImpl.h | 2 +- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/jsi/jni/Binding.cpp b/ReactAndroid/src/main/java/com/facebook/react/fabric/jsi/jni/Binding.cpp index 5f761938751..bff1dc8b3f1 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/jsi/jni/Binding.cpp +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/jsi/jni/Binding.cpp @@ -287,14 +287,12 @@ local_ref createUpdateStateMountItem( auto state = mutation.newChildShadowView.state; - // We use state.get() to pass a raw pointer through the JNI - // We don't need to access the state ptr in Java, but we need to be able to - // pass a state object back through the JNI for state updates - // Do not hold onto Java object from C + // We DO want to hold onto C object from Java, since we don't know the + // lifetime of the Java object auto javaStateWrapper = StateWrapperImpl::newObjectJavaArgs(); StateWrapperImpl* cStateWrapper = cthis(javaStateWrapper); - cStateWrapper->state_ = state.get(); + cStateWrapper->state_ = state; return updateStateInstruction( javaUIManager, diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/jsi/jni/StateWrapperImpl.h b/ReactAndroid/src/main/java/com/facebook/react/fabric/jsi/jni/StateWrapperImpl.h index 0796f5ec836..82239ef0635 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/jsi/jni/StateWrapperImpl.h +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/jsi/jni/StateWrapperImpl.h @@ -23,7 +23,7 @@ class StateWrapperImpl : public jni::HybridClass { jni::local_ref getState(); void updateStateImpl(NativeMap *map); - const State* state_; + State::Shared state_; private: jni::alias_ref jhybridobject_;