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

fix: Prevent daemonize mode from crashing upload process #1104

Merged
merged 2 commits into from
Jan 21, 2022

Conversation

kamilogorek
Copy link
Contributor

@kamilogorek kamilogorek commented Jan 21, 2022

In macOS 10.13, changes to Objective-C runtime forbid calling ObjC code before and after forking the process. If we want to do that, and we need to, due to moving sentry-cli process to the background during XCode artifacts upload, we need to perform all those calls after we daemonize ourselves. This forces us to not spawn any processes, nor call any ObjC-compiled code (like cURL) before we actually get to that branch.

The thing that's detaching us is MayDetach::wrap via MayDetach::may_detach function.

If we call any cURL code or spawn any process before-hand, it will result in the background process crashing with:

objc[80549]: +[__NSCFConstantString initialize] may have been in progress in another thread when fork() was called.
objc[80549]: +[__NSCFConstantString initialize] may have been in progress in another thread when fork() was called. We cannot safely call it or ignore it in the fork() child process. Crashing instead. Set a breakpoint on objc_initializeAfterForkError to debug.

This PR does the following:

  • disable signal handling for processes spawned via XCode (prevent direct process spawning); we can afford that, as our upload process is a standalone Build Script, and not part of the larger mechanism, thus we never expect it to be interrupted.
  • disable default built-in crash reporting (prevent process spawning in SDK transport constructors); this part was never used for years in the first place (see: 2b6bedb) and it was always meant to be used for us to track CLI issues. Original initialization code was still in place though, so it was executed despite lacking the necessary DSN (it initialized empty client).
  • move API initialization inside the wrapped function (prevent calling internal curl code);

ref: https://www.wefearchange.org/2018/11/forkmacos.rst.html
ref: http://sealiesoftware.com/blog/archive/2017/6/5/Objective-C_and_fork_in_macOS_1013.html

@kamilogorek kamilogorek requested a review from mitsuhiko January 21, 2022 10:44
@kamilogorek kamilogorek enabled auto-merge (squash) January 21, 2022 11:12
@kamilogorek kamilogorek merged commit 0ac7089 into master Jan 21, 2022
@kamilogorek kamilogorek deleted the xcode-daemon branch January 21, 2022 11:25
@marandaneto
Copy link
Contributor

@kamilogorek is it related to getsentry/sentry-react-native#1424 and the --force_foreground issue?

@kamilogorek
Copy link
Contributor Author

Yes, that's exactly that.

philipphofmann added a commit to getsentry/sentry-fastlane-plugin that referenced this pull request Jan 21, 2022
Bump minimum sentry-cli version to 1.72.0,
because of an important bug fix that sometimes
crashed the upload process,
getsentry/sentry-cli#1104.

Fixes GH-108
philipphofmann added a commit to getsentry/sentry-fastlane-plugin that referenced this pull request Jan 21, 2022
Bump minimum sentry-cli version to 1.72.0,
because of an important bug fix that sometimes
crashed the upload process,
getsentry/sentry-cli#1104.

Fixes GH-108
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