From a9e7da12af3b3fcf20ac786f3b3540dad01b56bf Mon Sep 17 00:00:00 2001
From: Samuel Susla <samuelsusla@meta.com>
Date: Wed, 29 Mar 2023 09:11:47 -0700
Subject: [PATCH] Do not strongly own State from Java (#36699)

Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/36699

Changelog: [internal]

This fixes a crash introduced in D43692171 where `jsi::Pointer` outlives `jsi::Runtime` in which it was created. This leads to a crash.
The diff changed ownership model, retaining `TextLayoutManager` strongly from `ParagraphState`.

To resolve this, in this diff we change how `StateWrapperImpl` owns state, moving from shared_ptr to weak_ptr. We have tried to do it previously in D26815275 but it had to be reverted because it broke end to end tests. I made sure the tests are fine. However it is the right ownership model. Java objects should not strongly hold onto anything in ShadowTree. If ShadowTree is destroyed, Java calls should noop, not keep objects in memory and have them handle the case when runtime was destroyed.

jest_e2e[run_all_tests]

Reviewed By: cipolleschi

Differential Revision: D44472121

fbshipit-source-id: 83b79329440ac1211902ea9511c0dde9a77ab9e9
---
 .../jni/react/fabric/StateWrapperImpl.cpp     | 26 +++++++++++++------
 .../main/jni/react/fabric/StateWrapperImpl.h  |  2 +-
 2 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/packages/react-native/ReactAndroid/src/main/jni/react/fabric/StateWrapperImpl.cpp b/packages/react-native/ReactAndroid/src/main/jni/react/fabric/StateWrapperImpl.cpp
index 49e3f3ef825..e79d25f55df 100644
--- a/packages/react-native/ReactAndroid/src/main/jni/react/fabric/StateWrapperImpl.cpp
+++ b/packages/react-native/ReactAndroid/src/main/jni/react/fabric/StateWrapperImpl.cpp
@@ -26,21 +26,31 @@ jni::local_ref<StateWrapperImpl::jhybriddata> StateWrapperImpl::initHybrid(
 
 jni::local_ref<ReadableNativeMap::jhybridobject>
 StateWrapperImpl::getStateDataImpl() {
-  folly::dynamic map = state_->getDynamic();
-  return ReadableNativeMap::newObjectCxxArgs(std::move(map));
+  if (auto state = state_.lock()) {
+    folly::dynamic map = state->getDynamic();
+    return ReadableNativeMap::newObjectCxxArgs(std::move(map));
+  } else {
+    return nullptr;
+  }
 }
 
 jni::local_ref<JReadableMapBuffer::jhybridobject>
 StateWrapperImpl::getStateMapBufferDataImpl() {
-  MapBuffer map = state_->getMapBuffer();
-  return JReadableMapBuffer::createWithContents(std::move(map));
+  if (auto state = state_.lock()) {
+    MapBuffer map = state->getMapBuffer();
+    return JReadableMapBuffer::createWithContents(std::move(map));
+  } else {
+    return nullptr;
+  }
 }
 
 void StateWrapperImpl::updateStateImpl(NativeMap *map) {
-  // Get folly::dynamic from map
-  auto dynamicMap = map->consume();
-  // Set state
-  state_->updateState(std::move(dynamicMap));
+  if (auto state = state_.lock()) {
+    // Get folly::dynamic from map
+    auto dynamicMap = map->consume();
+    // Set state
+    state->updateState(std::move(dynamicMap));
+  }
 }
 
 void StateWrapperImpl::registerNatives() {
diff --git a/packages/react-native/ReactAndroid/src/main/jni/react/fabric/StateWrapperImpl.h b/packages/react-native/ReactAndroid/src/main/jni/react/fabric/StateWrapperImpl.h
index 637efd7a83b..92ec87e9bb2 100644
--- a/packages/react-native/ReactAndroid/src/main/jni/react/fabric/StateWrapperImpl.h
+++ b/packages/react-native/ReactAndroid/src/main/jni/react/fabric/StateWrapperImpl.h
@@ -30,7 +30,7 @@ class StateWrapperImpl : public jni::HybridClass<StateWrapperImpl> {
   jni::local_ref<ReadableNativeMap::jhybridobject> getStateDataImpl();
   void updateStateImpl(NativeMap *map);
 
-  State::Shared state_;
+  std::weak_ptr<State const> state_;
 
  private:
   jni::alias_ref<StateWrapperImpl::jhybriddata> jhybridobject_;