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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

fix(messaging): Fixes a race condition between `FIRAuth/didReceiveRem… #6392

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6a9d3d3
fix(messaging): Fixes a race condition between `FIRAuth/didReceiveRem…
Jul 15, 2022
fced716
fix(messaging): messaging crashing if receiving JSON data for firebas…
Aug 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions packages/messaging/ios/RNFBMessaging/RNFBMessaging+AppDelegate.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,23 @@ - (void)application:(UIApplication *)application
completionHandler(UIBackgroundFetchResultNoData);
return;
}

// If the notification is a probe notification, always call the completion
Copy link
Collaborator

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

Copy link
Contributor Author

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.

https://github.com/firebase/firebase-ios-sdk/blob/eb30b6fdd809cbfe3a9eb7f3ccf1dd46326bef2d/FirebaseAuth/Sources/SystemService/FIRAuthNotificationManager.m#L105-L110

https://github.com/firebase/firebase-ios-sdk/blob/eb30b6fdd809cbfe3a9eb7f3ccf1dd46326bef2d/FirebaseAuth/Sources/SystemService/FIRAuthNotificationManager.m#L45-L48

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?

Copy link
Contributor

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?

Copy link

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

Copy link
Contributor Author

@glesperance glesperance Aug 3, 2022 •
edited
Loading

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:

https://github.com/firebase/firebase-ios-sdk/blob/eb30b6fdd809cbfe3a9eb7f3ccf1dd46326bef2d/FirebaseAuth/Sources/SystemService/FIRAuthNotificationManager.m#L150-L158

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:

  1. we get a probe notification; and RNFB can't handle it.
  2. 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.
  3. 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.

Copy link
Collaborator

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.

// handler with UIBackgroundFetchResultNoData.
//
// This fixes a race condition between `FIRAuth/didReceiveRemoteNotification` 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
NSDictionary *data = userInfo[@"com.google.firebase.auth"];
if ([data isKindOfClass:[NSString class]]) {
// Deserialize in case the data is a JSON string.
NSData *JSONData = [((NSString *)data) dataUsingEncoding:NSUTF8StringEncoding];
data = [NSJSONSerialization JSONObjectWithData:JSONData options:0 error:NULL];
}
if ([data isKindOfClass:[NSDictionary class]] && data[@"warning"]) {
completionHandler(UIBackgroundFetchResultNoData);
return;
}
#endif

[[NSNotificationCenter defaultCenter]
Expand Down