Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(ios): crash on source change after coming back from background #4074

Conversation

blazlew
Copy link
Contributor

@blazlew blazlew commented Aug 9, 2024

Summary

This works around the crash on source change, if the app was previously put in the background. The crash occurs on iOS >= 16 and I blame apple for this 😅. They seem to be adding some observer logic on the player instance, causing it to explode in mentioned scenario.
This might not be a super sophisticated solution, perhaps there is a better more granular way to handle this, but on another hand it's simple and works well.

Motivation

Fix for #3900

Changes

Test plan

Author of the issue provided a crash demo app.
I was able to run it with:

yarn
(cd ios && pod update)
yarn ios --udid=<your_sim_udid>

Once succesfuly ran:

  1. play video
  2. go background
  3. go foreground
  4. change video
  5. notice the crash
  6. replace rn-video dependency with suggested change
  7. test again

@KrzysztofMoch
Copy link
Member

👀 Good catch! I think we can upgrade it with playInBackground check - WDYT ?

@KrzysztofMoch KrzysztofMoch self-requested a review August 12, 2024 11:53
@jaybayley
Copy link

Confirming this is still an issue in 6.4.4. Applying @blazlew fix via a patch file resolves the issue for me. @KrzysztofMoch Could we look at getting this merged please?

@blazlew
Copy link
Contributor Author

blazlew commented Aug 16, 2024

Thanks @KrzysztofMoch although the steps to reproduce mention backgrounding the app, the issue itself doesn't seem to be related to playInBackground flag. It's set to false in the demo
https://github.com/ammar20305/Dummy/blob/8273127732173223a221b9ee4feeccbf737b9dd7/App.tsx#L80

@freeboub freeboub self-requested a review August 22, 2024 08:29
Copy link
Member

@KrzysztofMoch KrzysztofMoch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, it LGTM so let's merge

@KrzysztofMoch KrzysztofMoch merged commit b05201a into TheWidlarzGroup:master Aug 22, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants