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

Leak mutex to avoid a crashing issue #22607

Closed
wants to merge 1 commit into from

Conversation

chuganzy
Copy link
Contributor

@chuganzy chuganzy commented Dec 11, 2018

Try to solve #13588
ref: facebook/componentkit#906

Test Plan:

Regression / smoke tests should be enough.

Changelog:

[iOS] [Fixed] - Fix a crashing issue on RCTFont / mutex

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Dec 11, 2018
@DimitryDushkin
Copy link

Ping. Eliminating crash is really important.

@DimitryDushkin
Copy link

We've released out app with this patch and this crash gone away.

@chuganzy
Copy link
Contributor Author

@DimitryDushkin
Copy link

Where is my manners. Thank you @chuganzy!

@shergin
Copy link
Contributor

shergin commented Feb 14, 2019

@chuganzy
Wait, wait, what does make you think that the linked crash happens when application is closing? Should we leak all static mutexes?

@shergin
Copy link
Contributor

shergin commented Feb 14, 2019

Here's a quote from Oliver:

Historically, TextKit has had lots of hidden thread-safety issues despite being stated as thread safe. CK locks around all access to TextKit, but there are still issues that have cropped up in the recent past. We built the same text locking system in both CK and ASDK, and they both work this way.

As I can see, that's not how RN works here (and everywhere else) with fonts and co. So, maybe this is the problem.

@chuganzy
Copy link
Contributor Author

chuganzy commented Feb 14, 2019

Here's also a quote from @ocrickard. You can see the commit log here.
facebook/componentkit@37bcfdb

OK, looks like std::mutex is not trivially destructible in Clang, so we should probably just leak that mutex. Anyway, I believe this crash is happening on exit() as @gkassabli said above, so it probably has no impact on user experience.

@shergin
Copy link
Contributor

shergin commented Feb 14, 2019

Yes, exactly. My point is that the fix in CK fixes different type of the crash, not the one that promised in this PR's description.
I believe this change will not fix #13588 .

@shergin
Copy link
Contributor

shergin commented Feb 14, 2019

Just chatted with Oliver and... I was a bit wrong. :)
So, this fix seems to fix the one of the crashes mentioned in the assigned issue (but probably not that one which in the description).

As Oliver pointed out, all TextKit-related crashes wich has something like '0x185dfc238 std::__1::mutex::try_lock() + 34' will be fixed with this fix.

@facebook-github-bot facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Feb 14, 2019
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shergin is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@chuganzy
Copy link
Contributor Author

@shergin Thank you, but seems like the issue in its description also indicates that it's crashing when it tries to lock from its line so I am hoping that this solves the issue🤞

@react-native-bot
Copy link
Collaborator

@chuganzy merged commit 6082431 into facebook:master.

@facebook facebook locked as resolved and limited conversation to collaborators Feb 14, 2019
@react-native-bot react-native-bot added the Merged This PR has been merged. label Feb 14, 2019
@chuganzy chuganzy deleted the rctfont-crash-fix branch February 15, 2019 00:35
JAStanton pushed a commit to convoyinc/react-native that referenced this pull request Feb 19, 2019
Summary:
Try to solve facebook#13588
ref: facebook/componentkit#906
Pull Request resolved: facebook#22607

Differential Revision: D14084056

Pulled By: shergin

fbshipit-source-id: 585aef758e1a215e825ae12914c5011d790a69b0
JAStanton pushed a commit to convoyinc/react-native that referenced this pull request Feb 19, 2019
Summary:
Try to solve facebook#13588
ref: facebook/componentkit#906
Pull Request resolved: facebook#22607

Differential Revision: D14084056

Pulled By: shergin

fbshipit-source-id: 585aef758e1a215e825ae12914c5011d790a69b0
JAStanton pushed a commit to convoyinc/react-native that referenced this pull request Feb 19, 2019
Summary:
Try to solve facebook#13588
ref: facebook/componentkit#906
Pull Request resolved: facebook#22607

Differential Revision: D14084056

Pulled By: shergin

fbshipit-source-id: 585aef758e1a215e825ae12914c5011d790a69b0
JAStanton pushed a commit to convoyinc/react-native that referenced this pull request Feb 19, 2019
Summary:
Try to solve facebook#13588
ref: facebook/componentkit#906
Pull Request resolved: facebook#22607

Differential Revision: D14084056

Pulled By: shergin

fbshipit-source-id: 585aef758e1a215e825ae12914c5011d790a69b0
@hramos hramos removed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Feb 23, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. Platform: iOS iOS applications.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants