[RNMobile] Prevent react-native-video crash that happens when an src changes quickly after first being set
#59841
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 3 commits
March 13, 2024 17:37
There is a bug with `react-native-video` that causes a crash if a video's `src` is changed quickly after setting it with some other value. Relevant GitHub discussion: wordpress-mobile/WordPress-iOS#20882 (comment) With this commit, we prevent the `src` from being quickly changed by not showing a video if it still has the `file:` protocol.
SiobhyB
added
[Type] Bug
SiobhyB
changed the title
react-native-video crash that happens when an src changes quickly after first being set
|
Size Change: 0 B Total Size: 1.71 MB ℹ️ View Unchanged
|
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @crazytonyli. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
jhnstn
approved these changes
Mar 13, 2024
4 tasks
carstingaxion
pushed a commit
to carstingaxion/gutenberg
that referenced
this pull request
Mar 27, 2024
…rc` changes quickly after first being set (WordPress#59841) Unlinked contributors: crazytonyli. Co-authored-by: SiobhyB <[email protected]> Co-authored-by: jhnstn <[email protected]> Co-authored-by: staskus <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes: wordpress-mobile/WordPress-iOS#20882
Gutenberg Mobile PR: wordpress-mobile/gutenberg-mobile#6729
What?
This PR prevents a
react-native-video-related crash that happens when ansrcchanges quickly after first being set in the Video block.Why?
As per wordpress-mobile/WordPress-iOS#20882, the crash impacts iOS users. Though the crash can't be reproduced consistently, it disrupts an important flow (uploading video) for those that do encounter it.
How?
A video may still have the
file:protocol for a short time after it's uploaded. In those cases the showVideo const will currently returntrueand thesrcwill be set. Shortly after, thesrcwill be set again with the video's actual URL. It is likely this quick change to thesrcis causing the crash, as per the description of thereact-native-videobug here.With the above in mind, a check for the
file:protocol has been added toshowVideoin 8cf9fd4. Now, thesrcwill not be changed quickly after it's first been set.A similar guard was added for video code in the Jetpack app, which resolved the crash in that instance and adds weight to the idea that the proposed solution here will work as intended.
The change should not have a noticeable impact on the time it takes for a video's thumbnail to be displayed, as the
file:protocol should only be in use for a very short time after the video has finished uploading (and we do not attempt to display the thumbnail while a video's uploading).Testing Instructions
The crash has been difficult to reproduce. However, we can verify that the changes don't contribute to a delay in the video's thumbnail displaying and that the
srcis not changed rapidly when first set.If testing locally, you can use the following diff to log changes to the
showVideoandsrcvariables:Screenshots or screencast
These side-by-side videos demonstrate that there isn't a noticeable delay in the video's thumbnail being shown. (I promise they're different videos, you can tell by the placement of the cursor 😂)
Screen.Recording.2024-03-13.at.17.26.05.mov
Screen.Recording.2024-03-13.at.17.28.28.mov