Skip to content

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

Merged
ncooke3 merged 6 commits into
masterfrom
nc/m1-ios13-fix
Apr 28, 2021
Merged

DynamicLinks: Skip retrieving web locale when running targeting below iOS14 (only on simulator & Apple Silicon)#7989
ncooke3 merged 6 commits into
masterfrom
nc/m1-ios13-fix

Conversation

@ncooke3

@ncooke3 ncooke3 commented Apr 27, 2021

Copy link
Copy Markdown
Member

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

Fixes #7618

@google-oss-bot

Copy link
Copy Markdown
Collaborator
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

TIL... thanks!

@paulb777 paulb777 left a comment

Copy link
Copy Markdown
Member

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

google-oss-bot commented Apr 27, 2021

Copy link
Copy Markdown
Collaborator

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

@eldhosembabu eldhosembabu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for addressing this.

@paulb777 paulb777 left a comment

Copy link
Copy Markdown
Member

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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Crash on .configure() using M1 chip, iOS 13 simulator and Rosetta

4 participants