android: attempt to fix assertion errors in ReactInstanceManager

[saghul]

This is the main one:
https://github.com/facebook/react-native/blob/df4e67fe75d781d1eb264128cadf079989542755/ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManager.java#L512

Why this happens is a mystery wrapped in an enigma.

[paweldomas]

I don't like that it only masks the problem. Can you give more details on what device it happens and what version of the app ? Are we sure does not come from someone using the SDK in wrong way ?

[saghul]

facebook/react-native#24455
facebook/react-native#22241
facebook/react-native#22462

It's not clear on what devices it happens, but we are not alone in this one. This is an attempt to survive a crash, so avoinding the issue is 100% intentional.

[paweldomas]

One thing I've noticed is that we're calling onPause when fragment's onStop is called. Do you remember why ?
https://github.com/jitsi/jitsi-meet/blob/master/android/sdk/src/main/java/org/jitsi/meet/sdk/JitsiMeetFragment.java#L92

[saghul]

One thing I've noticed is that we're calling onPause when fragment's onStop is called. Do you remember why ?

Yes, that in order to support PiP properly. When in PiP the Activity gets onPause called, but we don't want to let React know, since it would emit the state that we are in the background, but we are not.

[paweldomas]

Yes, that in order to support PiP properly. When in PiP the Activity gets onPause called, but we don't want to let React know, since it would emit the state that we are in the background, but we are not.

Okay, makes sense. It's good to place things like that in comments in the code

[paweldomas]

@saghul here's the scenario to repro the issue. I am able to repro on Samsung with Android 8.0.0

1. Fresh install with no mic/camera permissions
2. Start a conference and wait for the permissions dialog
3. Do not accept the permissions, but press the home button instead
4. (This step is optional if JM failed to go PiP and the app disappears) Start JM again from the launcher icon. You should end up with the permissions dialog and the PiP window.
5. You should end up with the permissions dialog and the PiP window. Accept the permissions alert.
6. At this point the dialog will fade out and disappear, but the PiP window will still be there. Don't click the PiP, but use the multi-tasking button instead to bring back the Jitsi Meet activity (which should kind of show the permissions dialog snapshot image). This will load the welcome page once clicked.
7. Now close the PiP window and the app will crash.

[paweldomas]

I can not repro on One Plus 5

[paweldomas]

#4183 should fix the issue I am able to repro. Do we still want any parts of this PR ?

[saghul]

@paweldomas Thanks for #4183 ! I think we have 2 choices here:

1. Not land this and wait until the next release, then see if we get the crash again
2. Land it, because people who are not using PiP are running into it too: android: attempt to fix assertion errors in ReactInstanceManager #4160 (comment)

FTR, based on our Crashlytics reports, this issue is our top-most crash and the one that affects more users. So, I'd say let's go with 1) and keep this open and monitor the situation, if we see crashes still coming, we land this PR and release a .1 with it. WDYT?

[paweldomas]

Okay lets hold with this one then. I suppose that we should stop seeing this crash after the PiP fix, but also knowing what the issue is I think that not calling the pause would avoid the crash and maybe not cause any side issues. Because at the time when it happens react is already hooked up to new activity.

[paweldomas]

But then it's likely that this kind of situation will be causing leaks, because we never assume to have more than one instance of activity at a time.

[saghul]

I wouldn't worry too much. Looks like we are not alone and that somehow it's possible for that to happen: https://stackoverflow.com/questions/46985698/singleinstance-activity-launched-twice-intermittently-on-android-8-oreo but the OS will kill the other Activity. Our delegate will only keep track of the active one, so we should be good.

All of that ⬆️ applies if we do the hack this PR is, but let's wait until the next release.

[zbettenbuk]

Approved, but please let's wait to clear up Pawel"s concerns around leaks and pip

[jackyoo]

This PR has to be approved. ReactInstanceManager's lifecycle handling is not right.
This is a singleton and it's expecting each activity goes through
A's resume -> A's pause -> B's resume -> B's pause.
But it could go through
A's resume -> B's resume -> A's pause -> B's pause.
If it's taking the path ^, it will crash according to the assertion.

[saghul]

I’ll check if we have seen this assertion trip after our other changes, thanks for the reminder.

[jackyoo]

Thanks for the answer @saghul .
I have a question about this PR though. Looks like it's skipping the pause when the activity is not the currentActivity. Would it better to just let it go through the pause() path in reactInstanceManager with skipping assertion check?
With this PR, in case of A's resume -> B's resume -> A's pause -> B's pause, it would not perform neither A's pause nor B's pause. I'm not sure if it's ok that way.

[saghul]

@paweldomas I checked the crashlytics reports and I still see crashes because of this. Can you review again? For context: #4160 (comment)

[paweldomas]

With this PR, in case of A's resume -> B's resume -> A's pause -> B's pause, it would not perform neither A's pause nor B's pause. I'm not sure if it's ok that way.

I think it would do the B's pause in the case you describe above, because B would still be the current activity.

[saghul]

@jackyoo That scenario should not be possible, but it somehow happens. See https://stackoverflow.com/questions/46985698/singleinstance-activity-launched-twice-intermittently-on-android-8-oreo