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

[exec-ctx] Remove ScopedTimeCache from ExecCtx on iOS #34416

Merged
merged 2 commits into from
Sep 25, 2023

Conversation

dconeybe
Copy link
Contributor

@dconeybe dconeybe commented Sep 20, 2023

Fix a crash on older iOS versions due to problematic thread-local variable initialization.

See firebase/firebase-ios-sdk#11509

Basically, there appears to be a bug in Xcode where it generates assembly for thread-local variable initialization that is susceptible to a crash. For example, on arm64 the generated assembly relies on registers like x8 and x10 being preserved by the thread-local variable initialization routine; however, in some cases this thread-local variable initialization calls functions like ImageLoaderMachOCompressed::doBindFastLazySymbol which clobber these registers, leaving their values indeterminate when the caller resumes. When those indeterminate values are later used as memory addresses they are invalid and result in a crash.

This PR works around this bug by removing the ScopedTimeCache member variable from the ExecCtx class on iOS. This is a reasonable workaround because ScopedTimeCache is only a slight optimization for data centers that entirely doesn't matter for mobile.

See https://github.com/dconeybe/TlsCrashIos12 for a demo of this crash.

Googlers see b/300501963 for full details.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Sep 20, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@dconeybe
Copy link
Contributor Author

I don't have permissions to access the kokoro test results. Would you be able to give me access to the GCP project so I can view the test results?

@drfloob drfloob assigned ctiller and unassigned drfloob Sep 22, 2023
@sampajano sampajano added the release notes: no Indicates if PR should not be in release notes label Sep 22, 2023
@sampajano
Copy link
Contributor

I don't have permissions to access the kokoro test results. Would you be able to give me access to the GCP project so I can view the test results?

@dconeybe Thanks so much for working on this!

I've seeing 2 failures right now but they don't really seem related to your change. (I don't know how to get your GCP access.. I think maybe you have to part of the grpc "organization" to see it? But i'm not sure i can add you to it.. @ctiller FYI in case you have an idea here :))

There is a merge conflict now though so you probably need to rebase your PR.

I'll run the tests for you again and we can probably merge it if the tests seem non-related to this.

Thanks!

Copy link
Member

@ctiller ctiller left a comment

Choose a reason for hiding this comment

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

Leaving an LGTM here and letting Eryu sort out any build/test issues.

Also noting that I'm ok with the ifdef mess here primarily because we're planning on removing ExecCtx in the medium term - otherwise I'd be hoping we could find a cleaner approach.

@dconeybe
Copy link
Contributor Author

Thank you for the review, @ctiller. I completely agree that the #if added by this PR are highly undesirable. We (Firestore users and the Firestore SDK development team) appreciate your cooperation in solving this unusual issue.

@HannahShiSFB
Copy link
Collaborator

LGTM, Thanks

@sampajano sampajano enabled auto-merge (squash) September 25, 2023 20:39
Copy link
Contributor

@sampajano sampajano left a comment

Choose a reason for hiding this comment

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

Thanks everyone!!

I've enabled auto merge and hope it'll be merged soon! 😃

@sampajano sampajano merged commit df4c0c6 into grpc:master Sep 25, 2023
56 of 70 checks passed
@dconeybe dconeybe deleted the ExecCtxScopedTimeCacheRemoval branch September 26, 2023 00:58
@copybara-service copybara-service bot added the imported Specifies if the PR has been imported to the internal repository label Sep 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bloat/none imported Specifies if the PR has been imported to the internal repository lang/core per-call-memory/neutral per-channel-memory/neutral release notes: no Indicates if PR should not be in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants