-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Update FIRDynamicLinkNetworking.m #5941
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
Conversation
Recreate the session to avoid crash when opening another or the same dynamic link (app already opened by a dynamic or have already processed one)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for making this PR! Not sure how FB team feels about it but I think we could add a short comment here explaining this approach. I think otherwise it would be too easy for someone to come in, say “This looks inefficient, let me use a shared session here” and revert it back and introduce a regression. Thoughts?
|
Changes looks good to me. Deferring to @paulb777 for approval |
|
Thanks for the fix @marqroldan and thanks for the review @eldhosembabu. Before merging I'd like to get a resolution to @ncooke3's question about reproduction. Do either of you have that? Thanks! |
@paulb777 Ironically enough, I was able to accidentally reproduce this issue while testing another dynamic links issue (on iOS14)! I was then able to apply @marqroldan 's proposed fix and the crash was avoided. 👍 |
|
Sounds good. I'll merge. @ncooke3 (or anyone else on the thread). I'd a appreciate another PR with the appropriate update to https://github.com/firebase/firebase-ios-sdk/blob/master/FirebaseDynamicLinks/CHANGELOG.md |
Recreate the session to avoid crash when opening another or the same dynamic link (app already opened by a dynamic or have already processed one)
Recreate the session to avoid crash when opening another or the same dynamic link (app already opened by a dynamic or have already processed one)
Thanks @rszalski for the solution!
Fix #5880