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

[dynamic_links]: fix initialization process #2817

Merged

Conversation

jacek-marchwicki
Copy link
Contributor

@jacek-marchwicki jacek-marchwicki commented Jun 22, 2020

Description:

This is my first contribution to this project so I'm sorry if I made this in improper way.

Description

Fix race conditions related to the dynamic links initialization process.

  1. During installation flow - (BOOL)application:(UIApplication *)application openURL:(NSURL *)url sourceApplication:(NSString *)sourceApplication annotation:(id)annotation might be called by OS after FirebaseDynamicLinks.instance.getInitialLink() is called. In such a case the app won't retrieve the deep-link.
  2. If FirebaseDynamicLinks.instance.getInitialLink() is called while processing [FIRDynamicLinks dynamicLinks] openURL:(NSURL *)url handleUniversalLink, the url is lost.
  3. Documentation fix: FirebaseDynamicLinks.instance.getInitialLink() call should be called after FirebaseDynamicLinks.instance.onLink() so url's aren't lost between those two calls.

More info in the comments in the PR:

Related Issues

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • If the pull request affects only one plugin, the PR title starts with the name of the plugin in brackets (e.g. [cloud_firestore])
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See Contributor Guide). - I haven't wrote any integration tests for the iOS stuff because i'd be a little complex.
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (flutter analyze) does not report any problems on my PR.
  • I read and followed the Flutter Style Guide.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy.
  • I updated CHANGELOG.md to add a description of the change.
  • I signed the CLA.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
  • No, this is not a breaking change.

}

- (BOOL)checkForDynamicLink:(NSURL *)url {
FIRDynamicLink *dynamicLink = [[FIRDynamicLinks dynamicLinks] dynamicLinkFromCustomSchemeURL:url];
if (dynamicLink) {
if (dynamicLink.url) _initialLink = dynamicLink;
[self onDeepLinkResult: dynamicLink error: nil];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

info: when the checkForDynamicLink method is executed, the getInitialLink() method had been already called. In such a case the app won't read _initialLink.
After the change, the link is delivered to onLink(onSuccess: xyz) callback

return [self onInitialLink:userActivity];
continueUserActivity:(NSUserActivity *)userActivity
restorationHandler:(void (^)(NSArray *))restorationHandler {
BOOL handled = [[FIRDynamicLinks dynamicLinks]
Copy link
Contributor Author

@jacek-marchwicki jacek-marchwicki Jun 22, 2020

Choose a reason for hiding this comment

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

info: previously _initiated were beeing checked before calling async function [[FIRDynamicLinks dynamicLinks] handleUniversalLink:userActivity. So previously if getInitialLink is called after the method is called but before it returns data. The link is lost.
After the change, the _initiated is checked after the callback returns.

Comment on lines 121 to 125
- (BOOL)application:(UIApplication *)application
openURL:(NSURL *)url
options:(NSDictionary<UIApplicationOpenURLOptionsKey, id> *)options {
return [self checkForDynamicLink:url];
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

info: nothing has changed, the method is moved lower where the rest of -application: xxx method are placed.

Description:
Fixing race-condition with getInitialLink and actuall setup
this initial link on iOS devices

Closes: firebase#100
Closes: firebase#1847
Closes: firebase#1861
@jacek-marchwicki jacek-marchwicki force-pushed the dynamic_links_initialization branch from 702f71c to c0f5fe5 Compare June 22, 2020 14:54
@jacek-sf
Copy link

This PR is ready for review by I can't change it from draft.

@jacek-marchwicki jacek-marchwicki marked this pull request as ready for review June 29, 2020 08:12
@jacek-marchwicki jacek-marchwicki changed the title (dynamic_links): fix initialization process [dynamic_links]: fix initialization process Jun 29, 2020
@jacek-marchwicki
Copy link
Contributor Author

Hi @bparrishMines!

Great that you taking care of this PR. Is it something that I can do for you related to this PR?

Copy link
Collaborator

@kroikie kroikie left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for this PR.

@kroikie
Copy link
Collaborator

kroikie commented Jun 30, 2020

@jacek-marchwicki could you please bump the version number and update the changelog?

Summary:
- Bump version number
- Added description to changelog
@jacek-marchwicki
Copy link
Contributor Author

@kroikie sorry it took me a while, but finally I did it!

@kroikie kroikie merged commit f4289e2 into firebase:master Jul 1, 2020
@sgehrman
Copy link

awesome fix! I spent 2 days trying to figure out why my links were acting weird.

@firebase firebase locked and limited conversation to collaborators Aug 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants