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

Add a nil check to prevent a crash #1120

Merged
merged 3 commits into from
Apr 12, 2022

Conversation

amgleitman
Copy link
Member

Please select one of the following

  • I am removing an existing difference between facebook/react-native and microsoft/react-native-macos 👍
  • I am cherry-picking a change from Facebook's react-native into microsoft/react-native-macos 👍
  • I am making a fix / change for the macOS implementation of react-native
  • I am making a change required for Microsoft usage of react-native

Summary

We've seen a crash downstream where -[NSString stringByReplacingCharactersInRange:withString:] receives a nil value as the replacement string. This is not good, since we expect that argument to be non-null.

We believe that a cause of this is that -[RCTUITextField textView:shouldChangeTextInRange:replacementString:] is being called with nil as the replacement string. (This is legal, as per Apple's documentation.) Right now, the only check that this delegate method does is enforcing the maxLength parameter if it exists, and changes in attributes shouldn't affect the length of the string.

Changelog

[macOS] [Fixed] - -[RCTUITextField textView:shouldChangeTextInRange:replacementString:] no longer crashes when we pass in a nil replacement string

@amgleitman amgleitman requested a review from a team as a code owner April 11, 2022 23:50
@harrieshin
Copy link

should the predicted text be empty string if the replacement string is nil or backedTextInputView.attributedText.string?

@amgleitman
Copy link
Member Author

should the predicted text be empty string if the replacement string is nil or backedTextInputView.attributedText.string?

Passing a replacement string of nil means that we're not changing the text, just attributes (see the link to Apple's documentation above), so I think we should be good in leaving _predictedText unchanged in that case.

@amgleitman amgleitman merged commit 84be3e0 into microsoft:main Apr 12, 2022
@amgleitman amgleitman deleted the textinput-nil-check branch April 12, 2022 17:56
facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Jan 24, 2023
Summary:
This is a [change](microsoft#1120) we made in our fork (React Native macOS) that we are now upstreaming to reduce the number of diffs between React Native Core and React Native macOS. Also.. one less crash �!

Resolves microsoft#1679

Original PR notes:

> We've seen a crash downstream where -[NSString stringByReplacingCharactersInRange:withString:] receives a nil value as the replacement string. This is not good, since we expect that argument to be non-null.
>
>We believe that a cause of this is that -[RCTUITextField textView:shouldChangeTextInRange:replacementString:] is being called with nil as the replacement string. (This is legal, as per [Apple's documentation](https://developer.apple.com/documentation/appkit/nstextviewdelegate/1449325-textview?language=objc).) Right now, the only check that this delegate method does is enforcing the maxLength parameter if it exists, and changes in attributes shouldn't affect the length of the string.

## Changelog

[IOS] [FIXED] - `-[RCTUITextField textView:shouldChangeTextInRange:replacementString:]` no longer crashes when we pass in a `nil` replacement string

Pull Request resolved: #35941

Test Plan: Build should pass. This change has been running in our fork in production for a while so we're fairly confident of it.

Reviewed By: cipolleschi

Differential Revision: D42705382

Pulled By: jacdebug

fbshipit-source-id: 066cd8a4ba134a681f0f4c955594b1fcda61a30e
OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
Summary:
This is a [change](microsoft#1120) we made in our fork (React Native macOS) that we are now upstreaming to reduce the number of diffs between React Native Core and React Native macOS. Also.. one less crash �!

Resolves microsoft#1679

Original PR notes:

> We've seen a crash downstream where -[NSString stringByReplacingCharactersInRange:withString:] receives a nil value as the replacement string. This is not good, since we expect that argument to be non-null.
>
>We believe that a cause of this is that -[RCTUITextField textView:shouldChangeTextInRange:replacementString:] is being called with nil as the replacement string. (This is legal, as per [Apple's documentation](https://developer.apple.com/documentation/appkit/nstextviewdelegate/1449325-textview?language=objc).) Right now, the only check that this delegate method does is enforcing the maxLength parameter if it exists, and changes in attributes shouldn't affect the length of the string.

## Changelog

[IOS] [FIXED] - `-[RCTUITextField textView:shouldChangeTextInRange:replacementString:]` no longer crashes when we pass in a `nil` replacement string

Pull Request resolved: facebook#35941

Test Plan: Build should pass. This change has been running in our fork in production for a while so we're fairly confident of it.

Reviewed By: cipolleschi

Differential Revision: D42705382

Pulled By: jacdebug

fbshipit-source-id: 066cd8a4ba134a681f0f4c955594b1fcda61a30e
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.

3 participants