fix(android): onHostPause crashes with multiple active activities

[hannojg]

Summary:

This PR addresses the problem outlined here:

• [android] java.lang.AssertionError: Pausing an activity that is not the current activity, this is incorrect! #54121

Problems tl;dr: ReactHost doesn't account for the fact that there can be multiple activities at the same time foregrounded.

There are scenarios where we can have multiple activities can be in resume, and we don't want to pause the ReactHost while the app is still in foreground (thats my assumption, maybe I am wrong here):

• ReactHost is held by the MainApplication (in the past it was the ReactActivity). It wants to be notified when the whole app gets paused to:
  • disable the hardware back btn handler to JS
  • call lifecycle methods for other classes subscribing to it

https://github.com/facebook/react-native/blob/e926ad04288b7e7ca35f228eae21874a0f9155f6/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/runtime/ReactHostImpl.kt#L258-L281

However, when the app is still in foreground by means of another ReactActivity, neither do we want to disable the back btn handling, nor notify that the app is full on pause.

Hence, the fix is to:

• Keep track of active activities
• Only pause when there is no active activity any longer

Note

Currently I removed all the checking code in onHostPause, as i believe its not needed, and potentially inherited from times where the ReactInstance was bound to a specific activity on the old arch.
I can ofc add it back, or guard my new approach around a feature flag if thats preferred.

Changelog:

[ANDROID] [FIXED] - Android can crash when multiple activities are in foreground and one pauses (fixes "Pausing an activity that is not the current activity, this is incorrect!")

Test Plan:

I tested these changes on top of the reproduction changes mentioned in the issue, and confirm that the app no longer crashes:

output_fixed.mp4

[hannojg]

After thoughts

Right now this would break the documentation on LifecycleEventListener a bit, however, multiple active activities is a case it didn't consider earlier:

https://github.com/facebook/react-native/blob/e926ad04288b7e7ca35f228eae21874a0f9155f6/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/bridge/LifecycleEventListener.kt#L12-L28

So I think either we should

• Update the documentation here as well to mention this case, or
• Maybe think of another fix that tracks the current activity by which activity was last interacted with, or
• We maybe want to revisit the design choices for LifecycleEventListener and how it lives on the ReactContext (MainApplication), while it tries to notify about lifecycle events of activities

[javache]

You'll see there's a featureflag in this code skipActivityIdentityAssertionOnHostPause, we're actively working on this!

Thanks for the PR, @fabriziocucci: does this approach make sense?

[fabriziocucci]

Thanks a lot for the PR @hannojg! 🙌

I'm gonna be looking at this (hopefully) next week, will let you know as soon as I've evaluated this and potentially other options.

[ArturBuja]

Thanks a lot for the PR @hannojg! 🙌

I'm gonna be looking at this (hopefully) next week, will let you know as soon as I've evaluated this and potentially other options.

Hey @fabriziocucci any updates about this PR?

[react-native-bot]

This PR is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days.