-
Notifications
You must be signed in to change notification settings - Fork 24.3k
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
timing fix for RCTCxxBridge.executeApplicationScript #25991
Conversation
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed. If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
alternatively an additional null check could be added after creating the NSDataBigString, this seemed a little cleaner |
@ahimberg Hi, thanks for the PR! Would you mind show the crash stack backtrace? I think we already ensure read and write it on JS thread. |
Sure, this is the callstack where I see the crash (the JS thread is where the reset happened):
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
Will this change be merged in anytime soon? It resolves the timing crash for me on app start. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hramos has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
This pull request was successfully merged by @ahimberg in 0c2db32. When will my fix make it into a release? | Upcoming Releases |
Summary
In one of our test apps (actually on Mac not iOS, but same code) we very consistently crash in RCTCxxBridge.executeApplicationScript when js debugging, due to a timing issue where another thread has reset _reactInstance in between the null check on self->_reactInstance and usage of it on these lines:
The thread doing the reset is doing so switching the executorClass to WebSocketExecutor.
In the scenario we crash, the packager has a bundle ready and quickly returns it, though its a 34MB string being passed to NSDataBigString which must be taking long enough for the other thread to get a chance to reset.
Changelog
[iOS] [Fixed] - Fix crash in RCTCxxBridge.executeApplicationScript
Test Plan
Ran apple code path in normal from bundle file and js debugging scenarios.