Nil check our run loop thread #771
Merged
Conversation
This file contains hidden or 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
Merge upstream into fork
|
Saadnajmi
approved these changes
May 13, 2021
| { | ||
| if (!_scheduledRunloops.count) { | ||
| [self scheduleInRunLoop:[NSRunLoop RCTSR_networkRunLoop] forMode:NSDefaultRunLoopMode]; | ||
| // [TODO(macOS ISS#2323203): `scheduleInRunLoop:forMode:` takes in a non-null run loop parameter so let's be safe and verify that |
Collaborator
There was a problem hiding this comment.
Do we have a better bug number or can we create one in git then this generic ISS number?
Author
There was a problem hiding this comment.
I looked and one didn't exist. I figure since we need to go through and update all of these anyway, making it one that we can easily track/update all at once might be easier than making a new one and not tracking it well/forgetting about it
harrieshin
approved these changes
May 13, 2021
HeyImChris
added a commit
to HeyImChris/react-native-macos
that referenced
this pull request
May 13, 2021
* Update RCTCxxBridge.mm * Update RCTCxxBridge.mm * run loop nil check Co-authored-by: Chris Hogan <chrishogan@Chriss-MacBook-Pro-2.local>
HeyImChris
added a commit
to HeyImChris/react-native-macos
that referenced
this pull request
May 13, 2021
* Update RCTCxxBridge.mm * Update RCTCxxBridge.mm * run loop nil check Co-authored-by: Chris Hogan <chrishogan@Chriss-MacBook-Pro-2.local>
HeyImChris
added a commit
that referenced
this pull request
May 14, 2021
* Add nullability checks (#704) * Update RCTCxxBridge.mm * add nullability checks * Nil check our run loop thread (#771) * Update RCTCxxBridge.mm * Update RCTCxxBridge.mm * run loop nil check Co-authored-by: Chris Hogan <chrishogan@Chriss-MacBook-Pro-2.local> * podfile lock Co-authored-by: Chris Hogan <chrishogan@Chriss-MacBook-Pro-2.local>
HeyImChris
added a commit
that referenced
this pull request
May 14, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Please select one of the following
Summary
Our logs show intermittent crashes with the stack ending here. It's not obvious what's failing as everything in the stack is pretty safe, but the only potentially suspicious behavior is we blindly pass in an NSRunLoop object into a non-null parameter of a method.
It's not obvious how the custom run loop could fail to instantiate, but there's no harm in being safe here and adding a nil check and log to see if we ever hit this.
After this gets in I can bring the same change over to our stable branches.
Changelog
[General] [Bug] - Nil check our custom run loop
Test Plan
It's just a simple nil check so nothing to test here. I can't repro the failure but in a test app was able to verify we'll throw an exception if we pass a nil object in as a non-null parameter.