Skip to content

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

Merged
sampajano merged 2 commits intogrpc:masterfrom
dconeybe:ExecCtxScopedTimeCacheRemoval
Sep 25, 2023
Merged

[exec-ctx] Remove ScopedTimeCache from ExecCtx on iOS#34416
sampajano merged 2 commits intogrpc:masterfrom
dconeybe:ExecCtxScopedTimeCacheRemoval

Conversation

@dconeybe
Copy link
Copy Markdown
Contributor

@dconeybe dconeybe commented Sep 20, 2023 •
edited
Loading

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
Copy Markdown

linux-foundation-easycla bot commented Sep 20, 2023 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@dconeybe dconeybe changed the title exec_ctx.h: remove ScopedTimeCache on iOS [exec-ctx] Remove ScopedTimeCache from ExecCtx on iOS Sep 20, 2023
@dconeybe dconeybe mentioned this pull request Sep 21, 2023
Closed
@dconeybe
Copy link
Copy Markdown
Contributor Author

dconeybe commented Sep 22, 2023

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
Copy Markdown
Contributor

sampajano commented Sep 22, 2023

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!

@dconeybe dconeybe mentioned this pull request Sep 25, 2023
Merged
ctiller
ctiller reviewed Sep 25, 2023
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

dconeybe commented Sep 25, 2023

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
Copy Markdown
Collaborator

HannahShiSFB commented Sep 25, 2023

LGTM, Thanks

@sampajano sampajano enabled auto-merge (squash) September 25, 2023 20:39
sampajano
sampajano approved these changes Sep 25, 2023
View reviewed changes
Copy link
Copy Markdown
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
@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
@dconeybe dconeybe mentioned this pull request Sep 28, 2023
Closed
@dconeybe dconeybe mentioned this pull request Nov 2, 2023
Closed
@ti-chi-bot ti-chi-bot bot mentioned this pull request Jul 29, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sampajano sampajano sampajano approved these changes

@ctiller ctiller ctiller approved these changes

@HannahShiSFB HannahShiSFB HannahShiSFB approved these changes

Assignees

@ctiller ctiller

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@dconeybe @sampajano @HannahShiSFB @ctiller @drfloob @grpc-kokoro