Facebook/react-native
1
0
Fork 0
mirror of https://github.com/facebook/react-native.git synced 2025-11-01 09:14:26 +00:00
Code Issues Packages Projects Releases Wiki Activity

Do not strongly own State from Java (#36699)

Browse Source 
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
This commit is contained in:
Samuel Susla
2023-03-29 09:11:47 -07:00
committed by Facebook GitHub Bot
parent fec5658a32
commit a9e7da12af
2 changed files with 19 additions and 9 deletions
Download Patch File Download Diff File Expand all files Collapse all files
packages/react-native/ReactAndroid/src/main/jni/react/fabric/StateWrapperImpl.cpp
+18 -8
View File
@@ -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() {
packages/react-native/ReactAndroid/src/main/jni/react/fabric/StateWrapperImpl.h
+1 -1
View File
@@ -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_;