[Android] Cleanup animated sensors in NativeProxy destructor #3954

jwajgelt · 2023-01-16T11:06:57Z

Summary

The reason for the crash when reloading was that, when the sensor listeners where cleaned up in the AnimatedSensorModule's destructor, the NativeProxy object used for calling JNI functions has already been destroyed, causing a null pointer dereference.

This PR moves the sensor listener cleanup to NativeProxy's destructor, where JNI functions are still safe to call.

Test plan

In the Example app:

go to the "Use Animated Sensor" example
trigger a reload (e.g. by typing "r" in metro)
🤞 for no crash

android/src/main/cpp/NativeProxy.cpp

DenianFossatti · 2023-01-27T17:32:37Z

Hi, sorry if this is not the right place for this, but you guys are planning to release a 2.x.x version for this fix ? @jwajgelt

I'm currently facing a issue similar to #2710 and #3645. When we reload the app after a codepush update, sometimes the app crashes and after making a patch using patch-package with almost all the changes that this PR contains, the app seems to not crash anymore.

One more thing, can you guys validate if this patch can break something on 2.14.4 ?

diff --git a/node_modules/react-native-reanimated/Common/cpp/AnimatedSensor/AnimatedSensorModule.cpp b/node_modules/react-native-reanimated/Common/cpp/AnimatedSensor/AnimatedSensorModule.cpp
index 83948c9..d6450f5 100644
--- a/node_modules/react-native-reanimated/Common/cpp/AnimatedSensor/AnimatedSensorModule.cpp
+++ b/node_modules/react-native-reanimated/Common/cpp/AnimatedSensor/AnimatedSensorModule.cpp
@@ -16,11 +16,7 @@ AnimatedSensorModule::AnimatedSensorModule(
       runtimeManager_(runtimeManager) {}
 
 AnimatedSensorModule::~AnimatedSensorModule() {
-  // It is called during app reload because app reload doesn't call hooks
-  // unmounting
-  for (auto sensorId : sensorsIds_) {
-    platformUnregisterSensorFunction_(sensorId);
-  }
+  assert(sensorsIds_.empty());
 }
 
 jsi::Value AnimatedSensorModule::registerSensor(
@@ -72,4 +68,11 @@ void AnimatedSensorModule::unregisterSensor(const jsi::Value &sensorId) {
   platformUnregisterSensorFunction_(sensorId.asNumber());
 }
 
+void AnimatedSensorModule::unregisterAllSensors() {
+  for (auto sensorId : sensorsIds_) {
+    platformUnregisterSensorFunction_(sensorId);
+  }
+  sensorsIds_.clear();
+}
+
 } // namespace reanimated
diff --git a/node_modules/react-native-reanimated/Common/cpp/AnimatedSensor/AnimatedSensorModule.h b/node_modules/react-native-reanimated/Common/cpp/AnimatedSensor/AnimatedSensorModule.h
index 76b652b..cffef9b 100644
--- a/node_modules/react-native-reanimated/Common/cpp/AnimatedSensor/AnimatedSensorModule.h
+++ b/node_modules/react-native-reanimated/Common/cpp/AnimatedSensor/AnimatedSensorModule.h
@@ -36,6 +36,7 @@ class AnimatedSensorModule {
       const jsi::Value &interval,
       const jsi::Value &sensorDataContainer);
   void unregisterSensor(const jsi::Value &sensorId);
+  void unregisterAllSensors();
 };
 
 } // namespace reanimated
diff --git a/node_modules/react-native-reanimated/Common/cpp/NativeModules/NativeReanimatedModule.cpp b/node_modules/react-native-reanimated/Common/cpp/NativeModules/NativeReanimatedModule.cpp
index 85cbe9c..6d9d686 100644
--- a/node_modules/react-native-reanimated/Common/cpp/NativeModules/NativeReanimatedModule.cpp
+++ b/node_modules/react-native-reanimated/Common/cpp/NativeModules/NativeReanimatedModule.cpp
@@ -368,4 +368,8 @@ void NativeReanimatedModule::unsubscribeFromKeyboardEvents(
   unsubscribeFromKeyboardEventsFunction(listenerId.asNumber());
 }
 
+void NativeReanimatedModule::cleanupSensors() {
+  animatedSensorModule.unregisterAllSensors();
+}
+
 } // namespace reanimated
diff --git a/node_modules/react-native-reanimated/Common/cpp/NativeModules/NativeReanimatedModule.h b/node_modules/react-native-reanimated/Common/cpp/NativeModules/NativeReanimatedModule.h
index ec06185..2740bf5 100644
--- a/node_modules/react-native-reanimated/Common/cpp/NativeModules/NativeReanimatedModule.h
+++ b/node_modules/react-native-reanimated/Common/cpp/NativeModules/NativeReanimatedModule.h
@@ -90,6 +90,9 @@ class NativeReanimatedModule : public NativeReanimatedModuleSpec,
       const jsi::Value &interval,
       const jsi::Value &sensorDataContainer) override;
   void unregisterSensor(jsi::Runtime &rt, const jsi::Value &sensorId) override;
+
+  void cleanupSensors();
+
   jsi::Value subscribeForKeyboardEvents(
       jsi::Runtime &rt,
       const jsi::Value &keyboardEventContainer) override;
diff --git a/node_modules/react-native-reanimated/android/src/main/cpp/NativeProxy.cpp b/node_modules/react-native-reanimated/android/src/main/cpp/NativeProxy.cpp
index fff5022..9080cbe 100644
--- a/node_modules/react-native-reanimated/android/src/main/cpp/NativeProxy.cpp
+++ b/node_modules/react-native-reanimated/android/src/main/cpp/NativeProxy.cpp
@@ -47,6 +47,11 @@ NativeProxy::NativeProxy(
 NativeProxy::~NativeProxy() {
   // removed temporary, new event listener mechanism need fix on the RN side
   // reactScheduler_->removeEventListener(eventListener_);
+
+  // cleanup all animated sensors here, since NativeProxy
+  // has already been destroyed when AnimatedSensorModule's
+  // destructor is ran
+  _nativeReanimatedModule->cleanupSensors();
 }
 
 jni::local_ref<NativeProxy::jhybriddata> NativeProxy::initHybrid(

I didn't find where to put this lines of code:

volatile bool uiRuntimeDestroyed = false;
this file (Common/cpp/SharedItems/Shareables.h) doesn't exist in this version

and this part, the function doesn't seems to have the same signature, so I didn't make the change

the line on 2.14.4:

  int sensorId = platformRegisterSensorFunction_(
      sensorType.asNumber(), interval.asNumber(), setter);

and in this PR:

int sensorId = platformRegisterSensorFunction_( sensorType, interval.asNumber(), [uiRuntime, sensorType, shareableHandler](double newValues[]) {

Common/cpp/AnimatedSensor/AnimatedSensorModule.cpp

Common/cpp/NativeModules/NativeReanimatedModule.cpp

jwajgelt · 2023-01-30T10:00:37Z

Hi, sorry if this is not the right place for this, but you guys are planning to release a 2.x.x version for this fix ? @jwajgelt

I'm currently facing a issue similar to #2710 and #3645. When we reload the app after a codepush update, sometimes the app crashes and after making a patch using patch-package with almost all the changes that this PR contains, the app seems to not crash anymore.

One more thing, can you guys validate if this patch can break something on 2.14.4 ?

diff --git a/node_modules/react-native-reanimated/Common/cpp/AnimatedSensor/AnimatedSensorModule.cpp b/node_modules/react-native-reanimated/Common/cpp/AnimatedSensor/AnimatedSensorModule.cpp
index 83948c9..d6450f5 100644
--- a/node_modules/react-native-reanimated/Common/cpp/AnimatedSensor/AnimatedSensorModule.cpp
+++ b/node_modules/react-native-reanimated/Common/cpp/AnimatedSensor/AnimatedSensorModule.cpp
@@ -16,11 +16,7 @@ AnimatedSensorModule::AnimatedSensorModule(
       runtimeManager_(runtimeManager) {}
 
 AnimatedSensorModule::~AnimatedSensorModule() {
-  // It is called during app reload because app reload doesn't call hooks
-  // unmounting
-  for (auto sensorId : sensorsIds_) {
-    platformUnregisterSensorFunction_(sensorId);
-  }
+  assert(sensorsIds_.empty());
 }
 
 jsi::Value AnimatedSensorModule::registerSensor(
@@ -72,4 +68,11 @@ void AnimatedSensorModule::unregisterSensor(const jsi::Value &sensorId) {
   platformUnregisterSensorFunction_(sensorId.asNumber());
 }
 
+void AnimatedSensorModule::unregisterAllSensors() {
+  for (auto sensorId : sensorsIds_) {
+    platformUnregisterSensorFunction_(sensorId);
+  }
+  sensorsIds_.clear();
+}
+
 } // namespace reanimated
diff --git a/node_modules/react-native-reanimated/Common/cpp/AnimatedSensor/AnimatedSensorModule.h b/node_modules/react-native-reanimated/Common/cpp/AnimatedSensor/AnimatedSensorModule.h
index 76b652b..cffef9b 100644
--- a/node_modules/react-native-reanimated/Common/cpp/AnimatedSensor/AnimatedSensorModule.h
+++ b/node_modules/react-native-reanimated/Common/cpp/AnimatedSensor/AnimatedSensorModule.h
@@ -36,6 +36,7 @@ class AnimatedSensorModule {
       const jsi::Value &interval,
       const jsi::Value &sensorDataContainer);
   void unregisterSensor(const jsi::Value &sensorId);
+  void unregisterAllSensors();
 };
 
 } // namespace reanimated
diff --git a/node_modules/react-native-reanimated/Common/cpp/NativeModules/NativeReanimatedModule.cpp b/node_modules/react-native-reanimated/Common/cpp/NativeModules/NativeReanimatedModule.cpp
index 85cbe9c..6d9d686 100644
--- a/node_modules/react-native-reanimated/Common/cpp/NativeModules/NativeReanimatedModule.cpp
+++ b/node_modules/react-native-reanimated/Common/cpp/NativeModules/NativeReanimatedModule.cpp
@@ -368,4 +368,8 @@ void NativeReanimatedModule::unsubscribeFromKeyboardEvents(
   unsubscribeFromKeyboardEventsFunction(listenerId.asNumber());
 }
 
+void NativeReanimatedModule::cleanupSensors() {
+  animatedSensorModule.unregisterAllSensors();
+}
+
 } // namespace reanimated
diff --git a/node_modules/react-native-reanimated/Common/cpp/NativeModules/NativeReanimatedModule.h b/node_modules/react-native-reanimated/Common/cpp/NativeModules/NativeReanimatedModule.h
index ec06185..2740bf5 100644
--- a/node_modules/react-native-reanimated/Common/cpp/NativeModules/NativeReanimatedModule.h
+++ b/node_modules/react-native-reanimated/Common/cpp/NativeModules/NativeReanimatedModule.h
@@ -90,6 +90,9 @@ class NativeReanimatedModule : public NativeReanimatedModuleSpec,
       const jsi::Value &interval,
       const jsi::Value &sensorDataContainer) override;
   void unregisterSensor(jsi::Runtime &rt, const jsi::Value &sensorId) override;
+
+  void cleanupSensors();
+
   jsi::Value subscribeForKeyboardEvents(
       jsi::Runtime &rt,
       const jsi::Value &keyboardEventContainer) override;
diff --git a/node_modules/react-native-reanimated/android/src/main/cpp/NativeProxy.cpp b/node_modules/react-native-reanimated/android/src/main/cpp/NativeProxy.cpp
index fff5022..9080cbe 100644
--- a/node_modules/react-native-reanimated/android/src/main/cpp/NativeProxy.cpp
+++ b/node_modules/react-native-reanimated/android/src/main/cpp/NativeProxy.cpp
@@ -47,6 +47,11 @@ NativeProxy::NativeProxy(
 NativeProxy::~NativeProxy() {
   // removed temporary, new event listener mechanism need fix on the RN side
   // reactScheduler_->removeEventListener(eventListener_);
+
+  // cleanup all animated sensors here, since NativeProxy
+  // has already been destroyed when AnimatedSensorModule's
+  // destructor is ran
+  _nativeReanimatedModule->cleanupSensors();
 }
 
 jni::local_ref<NativeProxy::jhybriddata> NativeProxy::initHybrid(

I didn't find where to put this lines of code:

volatile bool uiRuntimeDestroyed = false; this file (Common/cpp/SharedItems/Shareables.h) doesn't exist in this version

and this part, the function doesn't seems to have the same signature, so I didn't make the change

the line on 2.14.4:

  int sensorId = platformRegisterSensorFunction_(
      sensorType.asNumber(), interval.asNumber(), setter);

and in this PR:

int sensorId = platformRegisterSensorFunction_( sensorType, interval.asNumber(), [uiRuntime, sensorType, shareableHandler](double newValues[]) {

Hi @DenianFossatti !
Thanks for investigating this issue on your own and coming up with this patch.

At the moment we didn't plan on backporting this PR to v2.
That said, we might consider it in the future since it doesn't seem to rely on other changes made in v3, but it isn't a priority at this moment.

Have you tried upgrading to v3? While the current version is marked as a "release candidate" and isn't as battle-tested as v2, we believe it shouldn't have any major regressions compared to v2, so if it's possible, you might want to try it out (after this PR has been released, probably in the next rc).

gp3gp3gp3 · 2023-02-22T20:03:29Z

This commit is quite important for our project, since we have to force a reload in production to support RTL languages, and we are seeing a high amount of crashes that I am hoping this should address.

A backport to v2 would be ideal, but we can update our version to 3 if it includes this commit. When can we expect an RC cut with this?

tomekzaw · 2023-02-23T08:22:02Z

Hey @gp3gp3gp3, actually we plan to release 3.0.0 early next week 🤞

spsaucier · 2023-04-11T21:36:13Z

We are not able to upgrade to version 3 due to other dependencies which break with removal of _setGlobalConsole (mrousavy/react-native-vision-camera#1514). A patch to version 2 would be immensely appreciated.

…e-mansion#3954)  ## Summary Fixes software-mansion#3645 The reason for the crash when reloading was that, when the sensor listeners where cleaned up in the `AnimatedSensorModule`'s destructor, the `NativeProxy` object used for calling JNI functions has already been destroyed, causing a null pointer dereference. This PR moves the sensor listener cleanup to `NativeProxy`'s destructor, where JNI functions are still safe to call.  ## Test plan In the Example app: - go to the "Use Animated Sensor" example - trigger a reload (e.g. by typing "r" in metro) - 🤞 for no crash

jwajgelt added the Platform: Android label Jan 16, 2023

jwajgelt requested a review from tomekzaw January 16, 2023 11:06

jwajgelt self-assigned this Jan 16, 2023

cleanup animated sensors in NativeProxy destructor

9cce3f0

jwajgelt force-pushed the @jwajgelt/animated-sensor-crash branch from 82a883d to 9cce3f0 Compare January 16, 2023 11:10

revert unnecessary change

8ed658e

piaskowyk reviewed Jan 16, 2023

View reviewed changes

android/src/main/cpp/NativeProxy.cpp Show resolved Hide resolved

tomekzaw reviewed Jan 30, 2023

View reviewed changes

Common/cpp/AnimatedSensor/AnimatedSensorModule.cpp Outdated Show resolved Hide resolved

Common/cpp/NativeModules/NativeReanimatedModule.cpp Show resolved Hide resolved

don't capture raw ptr to ui runtime

70f057a

jwajgelt force-pushed the @jwajgelt/animated-sensor-crash branch from 6255ec0 to 70f057a Compare January 30, 2023 10:13

tomekzaw approved these changes Jan 30, 2023

View reviewed changes

jwajgelt merged commit 806c662 into main Jan 30, 2023

jwajgelt deleted the @jwajgelt/animated-sensor-crash branch January 30, 2023 14:33

pepf mentioned this pull request Mar 14, 2023

App crashes with SIGABRT #1912

Closed

3 tasks

lag-of-death mentioned this pull request Mar 20, 2023

[android] app crash -> JNI DETECTED ERROR IN APPLICATION: java_object == null #2710

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Android] Cleanup animated sensors in NativeProxy destructor #3954

[Android] Cleanup animated sensors in NativeProxy destructor #3954

jwajgelt commented Jan 16, 2023

Uh oh!

Uh oh!

DenianFossatti commented Jan 27, 2023 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

jwajgelt commented Jan 30, 2023

Uh oh!

gp3gp3gp3 commented Feb 22, 2023

Uh oh!

tomekzaw commented Feb 23, 2023

Uh oh!

spsaucier commented Apr 11, 2023

Uh oh!

[Android] Cleanup animated sensors in NativeProxy destructor #3954

[Android] Cleanup animated sensors in NativeProxy destructor #3954

Conversation

jwajgelt commented Jan 16, 2023

Summary

Test plan

Uh oh!

Uh oh!

DenianFossatti commented Jan 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jwajgelt commented Jan 30, 2023

Uh oh!

gp3gp3gp3 commented Feb 22, 2023

Uh oh!

tomekzaw commented Feb 23, 2023

Uh oh!

spsaucier commented Apr 11, 2023

Uh oh!

DenianFossatti commented Jan 27, 2023 •

edited

Loading