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

iOS 17.2 crash when controls={true} #3329

Closed
abanobmikaeel opened this issue Nov 2, 2023 · 12 comments
Closed

iOS 17.2 crash when controls={true} #3329

abanobmikaeel opened this issue Nov 2, 2023 · 12 comments
Assignees

Comments

@abanobmikaeel
Copy link

Bug

Platform

Which player are you experiencing the problem on:

  • iOS

@react-navigation/native: 6.1.9
@react-navigation/native-stack: 6.9.16
expo: 49.0.15
react: 18.2.0
react-native: 0.72.6
react-native-safe-area-context: 4.7.4
react-native-screens: 3.27.0
react-native-video: 6.0.0-alpha.8

Library version: x.x.x
Device: iPhone 15 Pro Max, iOS 17.2

Background:
I do believe it's related to previous issues that happened like #3040 #3025 #2808 and #2773. I'm not well versed enough to narrow it down but I'm certain it's related to

Steps To Reproduce

  1. In the example I added, when controls={true} for video component, and we navigate away to another screen a crash happens on iOS 17.2
  2. The video takes 4-5 seconds to become visible but it's right under the navigate button.
  3. I will note that the video is an mp4
  4. Crash details as follows
'NSRangeException', reason: '*** -[__NSArray0 objectAtIndex:]: index
9223372036854775808 beyond bounds for empty array'
*** First throw call stack:
(
0   CoreFoundation                      0x000000010d308a09
__exceptionPreprocess + 242
1   libobjc.A.dylib                     0x0000000108d1d8b4
objc_exception_throw + 48
2   CoreFoundation                      0x000000010d1d92bf -[__NSArray0
objectEnumerator] + 0
3   AVKit                               0x000000010d982a31
-[AVMobileChromelessContentTabsViewController _activeContentTab] + 95
4   AVKit                               0x000000010d981fb6
-[AVMobileChromelessContentTabsViewController setCurrentContentTabIndex:]
+ 37
5   AVKit                               0x000000010d9827cf
-[AVMobileChromelessContentTabsViewController
scrollViewDidEndDecelerating:] + 244
6   videocrash                          0x00000001065e3e72 -[RNSScreen
traverseForScrollView:] + 706
7   videocrash                     <…>

Expected behaviour

  1. On Navigating away from a video
  2. No crash occurs whether or not controls={true}

Reproducible sample code

I created a minimal repro example. Can be found here https://github.com/abanobmikaeel/react-native-video-crash.

@maribeiroleya
Copy link

Same issue here

@hubertwang
Copy link

same here, any quick fix to this issue?

@KrzysztofMoch
Copy link
Member

Currently no 😵‍💫
it's heavily complicated - will let know if anything new comes up

@abanobmikaeel
Copy link
Author

@KrzysztofMoch I tried looking into the Swift file and I was completely lost unfortunately, you're our hope LOL. I do think this should be escalated in terms of priority since iOS 17.2 is being rolled out publicly and MANY users of this library will have crashes very soon unfortunately.

Thanks for all the work that you guys are doing💚

As an aside
@hubertwang One temporary fix I've rolled out is to add this logic, basically manually implementing controls which isn't ideal but it's what's working currently.

import { is_IOS_Under_17_2 } from "../helpers/Platform";
export default function VideoComponent({
  uri,
  showControls,
  onLoadStart,
  onLoadEnd,
  showThumbnailPlayButton,
}: any) {
  const [isPlaying, setIsPlaying] = useState(false);
  const togglePlayPause = () => {
    setIsPlaying(!isPlaying);
  };

  return (
    <>
      <Video
        source={{
          uri,
        }}
        resizeMode={ResizeMode.COVER}
        style={{ width: "100%", height: 300, flex: 1, zIndex: 1 }}
        paused={!isPlaying}
        controls={is_IOS_Under_17_2()}
        repeat={true}
        onLoadStart={() => onLoadStart()}
        onReadyForDisplay={() => onLoadEnd()}
      />

      <TouchableOpacity
        style={styles.topRightButtonStyle}
        onPress={togglePlayPause}
      >
        <Ionicons
          name={isPlaying ? "pause" : "play"}
          size={30}
          color={"white"}
        />
      </TouchableOpacity>
    </>
  );
}

@patrick-roon
Copy link

I went ahead an implemented some paired down custom controls for 17.2+. Wondering if there is an ETA on the long-term fix.

chrisleewilcox pushed a commit to MetaMask/metamask-mobile that referenced this issue Nov 10, 2023
## **Description**

<!--
Write a short description of the changes included in this pull request,
also include relevant motivation and context. Have in mind the following
questions:
1. What is the reason for the change?
2. What is the improvement/solution?
-->

It was discovered that the video begins crashing the app for iOS 17.2+
physical devices. While there is no solution in the latest
react-native-video package at the moment
(TheWidlarzGroup/react-native-video#3329),
we can patch it by removing the `controls` prop and temporarily use
custom controls (a play/pause + mute controls). The tradeoff is that
other features such as full screen and seeking will not be available on
iOS.

## **Related issues**

Fixes: #7729

## **Manual testing steps**

1. Go to this page...
2.
3.

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

### **After**
<img width="537" alt="Screenshot 2023-11-09 at 9 19 57 AM"
src="https://github.com/MetaMask/metamask-mobile/assets/10508597/134b6eb2-8017-4b2c-ae7d-12a28a47d10a">


<!-- [screenshots/recordings] -->

iOS 17.2 Interaction

https://github.com/MetaMask/metamask-mobile/assets/10508597/8976d5b7-d276-4834-9496-0ba8608e25e3

Video with play/pause + mute controls on settings

https://github.com/MetaMask/metamask-mobile/assets/10508597/492fb78d-3703-4714-bed1-8ddd7b631efa

Video with play/pause + mute controls on onboarding

https://github.com/MetaMask/metamask-mobile/assets/10508597/14780058-c595-4fb9-8aab-9987120c8126

Android remains the same
https://recordit.co/pTuloJmuIn

## **Pre-merge author checklist**

- [ ] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've clearly explained what problem this PR is solving and how it
is solved.
- [ ] I've linked related issues
- [ ] I've included manual testing steps
- [ ] I've included screenshots/recordings if applicable
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.
- [ ] I’ve properly set the pull request status:
  - [ ] In case it's not yet "ready for review", I've set it to "draft".
- [ ] In case it's "ready for review", I've changed it from "draft" to
"non-draft".

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.
@KrzysztofMoch
Copy link
Member

This issue is caused by react-native-screens. I opened an issue here.

chrisleewilcox pushed a commit to MetaMask/metamask-mobile that referenced this issue Nov 13, 2023
## **Description**

<!--
Write a short description of the changes included in this pull request,
also include relevant motivation and context. Have in mind the following
questions:
1. What is the reason for the change?
2. What is the improvement/solution?
-->

It was discovered that the video begins crashing the app for iOS 17.2+
physical devices. While there is no solution in the latest
react-native-video package at the moment
(TheWidlarzGroup/react-native-video#3329),
we can patch it by removing the `controls` prop and temporarily use
custom controls (a play/pause + mute controls). The tradeoff is that
other features such as full screen and seeking will not be available on
iOS.

## **Related issues**

Fixes: #7729

## **Manual testing steps**

1. Go to this page...
2.
3.

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

### **After**
<img width="537" alt="Screenshot 2023-11-09 at 9 19 57 AM"
src="https://github.com/MetaMask/metamask-mobile/assets/10508597/134b6eb2-8017-4b2c-ae7d-12a28a47d10a">


<!-- [screenshots/recordings] -->

iOS 17.2 Interaction

https://github.com/MetaMask/metamask-mobile/assets/10508597/8976d5b7-d276-4834-9496-0ba8608e25e3

Video with play/pause + mute controls on settings

https://github.com/MetaMask/metamask-mobile/assets/10508597/492fb78d-3703-4714-bed1-8ddd7b631efa

Video with play/pause + mute controls on onboarding

https://github.com/MetaMask/metamask-mobile/assets/10508597/14780058-c595-4fb9-8aab-9987120c8126

Android remains the same
https://recordit.co/pTuloJmuIn

## **Pre-merge author checklist**

- [ ] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've clearly explained what problem this PR is solving and how it
is solved.
- [ ] I've linked related issues
- [ ] I've included manual testing steps
- [ ] I've included screenshots/recordings if applicable
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.
- [ ] I’ve properly set the pull request status:
  - [ ] In case it's not yet "ready for review", I've set it to "draft".
- [ ] In case it's "ready for review", I've changed it from "draft" to
"non-draft".

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.
@KrzysztofMoch
Copy link
Member

As issue is not related with react-native-video I think we can close this. Progress can be followed at react-native-screen issue

chrisleewilcox added a commit to MetaMask/metamask-mobile that referenced this issue Nov 15, 2023
…7737) (#7785)

## **Description**

<!--
Write a short description of the changes included in this pull request,
also include relevant motivation and context. Have in mind the following
questions:
1. What is the reason for the change?
2. What is the improvement/solution? -->

It was discovered that the video begins crashing the app for iOS 17.2+
physical devices. While there is no solution in the latest
react-native-video package at the moment
(TheWidlarzGroup/react-native-video#3329),
we can patch it by removing the `controls` prop and temporarily use
custom controls (a play/pause + mute controls). The tradeoff is that
other features such as full screen and seeking will not be available on
iOS.

## **Related issues**

Fixes: #7729

## **Manual testing steps**

1. Go to this page... 2.
3.

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

### **After**
<img width="537" alt="Screenshot 2023-11-09 at 9 19 57 AM"
src="https://github.com/MetaMask/metamask-mobile/assets/10508597/134b6eb2-8017-4b2c-ae7d-12a28a47d10a">


<!-- [screenshots/recordings] -->

iOS 17.2 Interaction


https://github.com/MetaMask/metamask-mobile/assets/10508597/8976d5b7-d276-4834-9496-0ba8608e25e3

Video with play/pause + mute controls on settings


https://github.com/MetaMask/metamask-mobile/assets/10508597/492fb78d-3703-4714-bed1-8ddd7b631efa

Video with play/pause + mute controls on onboarding


https://github.com/MetaMask/metamask-mobile/assets/10508597/14780058-c595-4fb9-8aab-9987120c8126

Android remains the same
https://recordit.co/pTuloJmuIn

## **Pre-merge author checklist**

- [ ] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've clearly explained what problem this PR is solving and how it
is solved.
- [ ] I've linked related issues
- [ ] I've included manual testing steps
- [ ] I've included screenshots/recordings if applicable
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.
- [ ] I’ve properly set the pull request status:
  - [ ] In case it's not yet "ready for review", I've set it to "draft".
- [ ] In case it's "ready for review", I've changed it from "draft" to
"non-draft".

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.

## **Description**

<!--
Write a short description of the changes included in this pull request,
also include relevant motivation and context. Have in mind the following
questions:
1. What is the reason for the change?
2. What is the improvement/solution?
-->

## **Related issues**

Fixes: #

## **Manual testing steps**

1. Go to this page...
2.
3.

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

### **After**

<!-- [screenshots/recordings] -->

## **Pre-merge author checklist**

- [ ] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've clearly explained what problem this PR is solving and how it
is solved.
- [ ] I've linked related issues
- [ ] I've included manual testing steps
- [ ] I've included screenshots/recordings if applicable
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.
- [ ] I’ve properly set the pull request status:
  - [ ] In case it's not yet "ready for review", I've set it to "draft".
- [ ] In case it's "ready for review", I've changed it from "draft" to
"non-draft".

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.

Co-authored-by: Cal Leung <cleun007@gmail.com>
@KrzysztofMoch
Copy link
Member

Hey everyone, this issue will be fixed in next react-native-screens release!

@techlando
Copy link

techlando commented Dec 19, 2023

Just updated to iOS 17.2 and the crash started on my app. Updated to react native screens to 3.29.0 and the issue has been resolved 🙏

@prashant6768
Copy link

I have updated react-native-screens to 3.29.0 but still getting the same error.

@uzun0ff
Copy link

uzun0ff commented Jan 18, 2024

Just updated to iOS 17.2 and the crash started on my app. Updated to react native screens to 3.29.0 and the issue has been resolved 🙏

This has resolved the issue for me also. 🎉

I use Expo and also had to build a new build in order for the changes to have an effect.

@kmsayem12
Copy link

Just updated to iOS 17.2 and the crash started on my app. Updated to react native screens to 3.29.0 and the issue has been resolved 🙏

@techlando Thank you. Work for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants