Fix multiline TextInput crash when inserting/removing lots of text #489
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
ghost
added
It seems that the returned string that we use to set `previousText` can be mutated while it is waiting to be processed by RCTBridge.
tido64
force-pushed
the
fix-textinput-multiline-crash
branch
from
e01ab66 to
8c6da37
Compare
tom-un
approved these changes
Jul 8, 2020
facebook-github-bot
pushed a commit
to facebook/react-native
that referenced
this pull request
Jul 17, 2020
…29307) Summary: Multiline `TextInput` can crash when really long texts are inserted and removed quickly. This is caused by the fact that [`-[NSAttributedString string]`](https://developer.apple.com/documentation/foundation/nsattributedstring/1412616-string?language=objc) doesn't really return a copy, and may mutate the string while it is being used by `convertIdToFollyDynamic`. See microsoft#489 (comment) for more details on the issue. This issue was originally reported in microsoft#486 and was fixed in microsoft#489. ## Changelog [iOS] [Fixed] - Fix multiline TextInput crash when inserting/removing lots of text Pull Request resolved: #29307 Test Plan: 1. Open RNTester > TextInput 2. Search for a multiline example 3. Copy some large text and paste it into the text input view 4. Remove some (or all) text 5. Repeat steps 3-4 Reviewed By: shergin Differential Revision: D22488854 Pulled By: JoshuaGross fbshipit-source-id: 6fab7818d68538450d93460361ff5934caf86c10
christophpurrer
pushed a commit
to christophpurrer/react-native-macos
that referenced
this pull request
Jan 23, 2022
Summary: The crash in T90602856 is very similar to one fixed by D22488854 (see discussion [in PR](microsoft#489 (comment))), and it's possible this `replacement` string also suffers from sharing the same backing store as the attributed string (maybe only when the range encompasses the entire string?) and therefore should be copied as well. It's low risk, and hopefully we find it eliminates this crash, in which case I'll upstream into core. Test Plan: Smoke tested Zeratul Reviewers: lyahdav, mowang Reviewed By: lyahdav Subscribers: eliwhite Differential Revision: https://phabricator.intern.facebook.com/D28885871 Tasks: T90602856 Signature: 28885871:1622762608:ea6a17208d585ebd6b0668135be47ae914c19a45
christophpurrer
pushed a commit
to christophpurrer/react-native-macos
that referenced
this pull request
Jan 30, 2022
Summary: The crash in T90602856 is very similar to one fixed by D22488854 (see discussion [in PR](microsoft#489 (comment))), and it's possible this `replacement` string also suffers from sharing the same backing store as the attributed string (maybe only when the range encompasses the entire string?) and therefore should be copied as well. It's low risk, and hopefully we find it eliminates this crash, in which case I'll upstream into core. Test Plan: Smoke tested Zeratul Reviewers: lyahdav, mowang Reviewed By: lyahdav Subscribers: eliwhite Differential Revision: https://phabricator.intern.facebook.com/D28885871 Tasks: T90602856 Signature: 28885871:1622762608:ea6a17208d585ebd6b0668135be47ae914c19a45
christophpurrer
pushed a commit
to christophpurrer/react-native-macos
that referenced
this pull request
Mar 5, 2022
Summary: The crash in T90602856 is very similar to one fixed by D22488854 (see discussion [in PR](microsoft#489 (comment))), and it's possible this `replacement` string also suffers from sharing the same backing store as the attributed string (maybe only when the range encompasses the entire string?) and therefore should be copied as well. It's low risk, and hopefully we find it eliminates this crash, in which case I'll upstream into core. Test Plan: Smoke tested Zeratul Reviewers: lyahdav, mowang Reviewed By: lyahdav Subscribers: eliwhite Differential Revision: https://phabricator.intern.facebook.com/D28885871 Tasks: T90602856 Signature: 28885871:1622762608:ea6a17208d585ebd6b0668135be47ae914c19a45
facebook-github-bot
pushed a commit
to facebook/react-native
that referenced
this pull request
Jul 26, 2022
…w and RCTEventDispatcher Summary: A crash encountered in react-native-macOS is very similar to one fixed by microsoft#489 (comment) (see discussion), and it's possible this `replacement` string also suffers from sharing the same backing store as the attributed string (maybe only when the range encompasses the entire string?) and therefore should be copied as well. Changelog: [iOS][Fixed] - Possible fix for convertIdToFollyDynamic crash in RCTBaseTextInputView and RCTEventDispatcher Reviewed By: sammy-SC Differential Revision: D38064551 fbshipit-source-id: 9c15f2a980155ab3cbb3fde79fcb93b24ee2091a
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.
Summary
It seems that the returned string that we use to set
previousTextcan be mutated while it is waiting to be processed byRCTBridge.Resolves #486
Changelog
[macOS] [Fixed] - Fix multiline
TextInputcrash when inserting/removing lots of textTest Plan
Here's a screenshot showing that the length of
previousText(0x600000d952c0) was 29483 when first accessed, but is 0 when the exception is thrown.Microsoft Reviewers: Open in CodeFlow