-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
fix(messaging): Fixes a race condition between `FIRAuth/didReceiveRem… #6392
fix(messaging): Fixes a race condition between `FIRAuth/didReceiveRem… #6392
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
Codecov Report
@@ Coverage Diff @@
## main #6392 +/- ##
===========================================
- Coverage 72.31% 25.83% -46.48%
===========================================
Files 109 97 -12
Lines 4655 4291 -364
Branches 1048 1048
===========================================
- Hits 3366 1108 -2258
- Misses 1211 2583 +1372
- Partials 78 600 +522 |
Oh wow! This is Ninja level work, thank you! I'm travelling and only mobile phone at the moment but will merge as soon as I can give it a good look |
haha thanks @mikehardy. Lmk if I can be of any help! |
@mikehardy any news about this? |
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.
Hey there - sorry for the delay, when I say traveling I meant really traveling, I'm changing city about every 2 days on average, have been for a few weeks and will be for a few weeks more, with quite intermittent network connections. So I do appreciate your patience while you bear with me not being a great collaborator at the moment
That said, I left a comment where I'm basically just looking for some reference as I can't do a good review without understanding how that conditional is working. In general it seems like a fine idea but only with an understanding of the conditional
@@ -123,6 +123,18 @@ - (void)application:(UIApplication *)application | |||
completionHandler(UIBackgroundFetchResultNoData); | |||
return; | |||
} | |||
|
|||
// If the notification is a probe notification, always call the completion |
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.
is there some documentation about what a probe notification is? First I have heard of it! It seems to be detectable (via your data && data[@"warning"]
test but I'd love a canonical source for that test as nothing came up in my attempt at a search
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.
Good question & sorry if I put pressure on you :)
This can wait for you to be back to normal, I just would hate this bug to be in the codebase.
Regarding the probe notification, I haven't found it through the documentation but rather through inspecting the Firebase iOS SDK code.
I looked at the blame log and this was part of the iOS Firebase SDK's first commit ever.
The fix has been found through dissecting the code until I found most minimal cause of the bug, and then got a matching fix.
Perhaps @paulb777 (the original author of the SDK) could give us some details?
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.
I did the original open sourcing, but not the authoring so I can't help with the purpose of FirebaseAuth probe notifications, but maybe @renkelvin can?
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.
Hi all, I'm not the author either, while my understanding is that this key is to help Firebase Auth to decide whether they should handle the notification or let the App handle it. See https://github.com/firebase/firebase-ios-sdk/blob/eb30b6fdd809cbfe3a9eb7f3ccf1dd46326bef2d/FirebaseAuth/Sources/SystemService/FIRAuthNotificationManager.m#L150
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.
thanks @paulb777 @renkelvin (that was some awesomely quick feedback)! This definitely brings me back to when I found the fix.
@mikehardy I should have taken notes when I made the original fix, but my understanding was that (thank you Paul & Chuanr for jogging my memory back) we should always callback when we receive a probe notification, since that is what the SDK does when it has pending callbacks:
Moreover, even if we redundantly call there would be no side effect to call the completion handler since it is effectively no-op:
https://github.com/firebase/firebase-ios-sdk/blob/eb30b6fdd809cbfe3a9eb7f3ccf1dd46326bef2d/FirebaseAuth/Sources/SystemService/FIRAuthNotificationManager.m#L116-L117
Therefore the thinking is:
- we get a probe notification; and RNFB can't handle it.
- assuming that detox waits for completion handlers to be called back to say things are complete (not an unfair assumption for us & detox); we can't just drop the notification and never call the completion handler.
- Even if we redundantly call back the completion handler --> there is no side effect.
therefore just always call back the completion handler when it's a probe notification.
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.
@Salakar or @Ehesp just dropping you a notification that links you into some really interesting firebase messaging / auth race condition that is deep enough / subtle enough it probably lives in FlutterFire too - you might take a look. I hate to create new notification pings just for gratitude when we probably get too many, but I'll do it anyway, thanks @paulb777 and @renkelvin for the assist here, much appreciated.
941128b
to
41c7805
Compare
…oteNotification` and this module causing detox to hang when `FIRAuth/didReceiveRemoteNotification` is called first. see https://stackoverflow.com/questions/72044950/detox-tests-hang-with-pending-items-on-dispatch-queue/72989494
41c7805
to
6a9d3d3
Compare
@mikehardy just pushed a 2nd commit fixing an issue when the received notification data is a JSON string. I noticed some crashes in our production build related to the fix in the PR. I then realized we were missing the deserialization logic: fced716 fixes that. |
I'm so sorry I've been a slow collaborator on this - I'm still traveling and haven't found a sufficient time slice to be useful digitally for a couple weeks. I really appreciate you've moved this forward but I probably won't have time to integrate for maybe a week. Have you tried the patch package zip set for it off the GitHub actions? There is a small problem halting full release at the moment but it's in new functionality ( firestore geopoint toJSON, a really minor new API) so there should be no problems integrating it safely |
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.
this looks good, I appreciate the digging so I could understand it ("Today I learned...") and the second commit seems to make it as safe as I can currently imagine. Got some free in flight wifi and a quick moment to run through react-native-firebase PRs pending merge so hopefully I can get everything through CI and kick off the release with this in it. Thanks!
Hey - nothing personal, I just don't have commit rights to this branch on your repo (frequently PR authors assign those to maintainers but it's totally fair play not to, no worries) and I need to commit result of |
CI went green on the superceding PR, commits carried on to main now via squash merge - launching shortly - thanks again |
No problem! I'll set those rights for next time. Thanks for the merge @mikehardy! |
Description
Fixes a race condition between
FIRAuth/didReceiveRemoteNotification
and the RNFB/messaging module causing detox to hang whenFIRAuth/didReceiveRemoteNotification
is called first.This problem is caused by a bug in the @react-native-firebase/messaging implementation of
application: didReceiveRemoteNotification: fetchCompletionHandler:
(in @react-native-firebase/messaging/ios/RNFBMessaging/RNFBMessaging+AppDelegate.m).The implementation gets into a race with
FIRAuth/didReceiveRemoteNotification
which causes react-native-firebase/messaging to not callback the completion handler if FIRAuth's function ran first.This in turns causes detox to randomly (and often) hang with the following error:
Related issues
Release Summary
The patch fixes a race condition between
FIRAuth/didReceiveRemoteNotification
and the RNFB/messaging module causing detox to hang whenFIRAuth/didReceiveRemoteNotification
is called first.Checklist
Android
iOS
e2e
tests added or updated inpackages/\*\*/e2e
jest
tests added or updated inpackages/\*\*/__tests__
Test Plan
This is another tale from the deep-internals of Detox / RNFB / Firebase sdks playing not so well together -- and making tests extremely flaky. It will be hard to make a test harness for this.
However...
Unit tests are passing (with the exception of two tests that had to be skipped bc they had team/env specific values that made them fail):
As well as e2e tests:
Think
react-native-firebase
is great? Please consider supporting the project with any of the below:React Native Firebase
andInvertase
on Twitter🔥