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

DynamicLinks: Skip retrieving web locale when running targeting below iOS14 (only on simulator & Apple Silicon) #7989

Merged
merged 6 commits into from
Apr 28, 2021

Conversation

ncooke3
Copy link
Member

@ncooke3 ncooke3 commented Apr 27, 2021

Manually tested fix on M1 machine and it successfully ran without crashing.

Fixes #7618

@google-oss-bot
Copy link

1 Warning
⚠️ Did you forget to add a changelog entry? (Add #no-changelog to the PR description to silence this warning.)

Generated by 🚫 Danger

// Perform an early exit if the process is running under Rosetta translation and targeting
// under iOS 14.
if (processIsTranslated() && !systemVersionAtLeastiOS14) {
[self handleExecutionError:nil];
Copy link
Member Author

Choose a reason for hiding this comment

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

Calling the delegate method in the early exit case avoids interrupting the dynamic links flow and the locale property in the delegate will be set to empty string. See here

// and -1 when an error occurs.
// From:
// https://developer.apple.com/documentation/apple-silicon/about-the-rosetta-translation-environment
int processIsTranslated() {
Copy link
Member

Choose a reason for hiding this comment

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

Make this C function static so that it does not pollute the global symbol namespace.

Copy link
Member Author

Choose a reason for hiding this comment

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

TIL... thanks!

Copy link
Member

@paulb777 paulb777 left a comment

Choose a reason for hiding this comment

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

LGTM with static fix.

Deferring approval to @eldhosembabu

@google-oss-bot
Copy link

google-oss-bot commented Apr 27, 2021

Coverage Report

Affected SDKs

  • FirebaseDynamicLinks-iOS-FirebaseDynamicLinks.framework

    SDK overall coverage changed from 75.64% (b780014) to 76.62% (2269c59) by +0.98%.

    Filename Base (b780014) Head (2269c59) Diff
    FDLUtilities.m 90.15% 97.35% +7.20%
    FIRDLDefaultRetrievalProcessV2.m 72.92% 73.96% +1.04%
    FIRDLJavaScriptExecutor.m 85.22% 82.48% -2.74%
    FIRDynamicLink.m 80.17% 81.90% +1.72%
    FIRDynamicLinkNetworking.m 80.83% 81.67% +0.83%

Test Logs

Copy link
Contributor

@eldhosembabu eldhosembabu left a comment

Choose a reason for hiding this comment

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

Thanks for addressing this.

Copy link
Member

@paulb777 paulb777 left a comment

Choose a reason for hiding this comment

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

Please update the changelog

@ncooke3 ncooke3 merged commit 8be7fa5 into master Apr 28, 2021
@ncooke3 ncooke3 deleted the nc/m1-ios13-fix branch April 28, 2021 00:55
@firebase firebase locked and limited conversation to collaborators May 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.