FCM: Fix crash on iOS 14 when multiple app delegate completion methods are called #6863

mikeger · 2020-10-29T13:06:07Z

Background

Since iOS 7 it is possible to execute the code in the background for iOS apps that receive silent push notifications. Once such push (most likely containing the content-available flag) is received, if the application is in the background, it has 30 seconds to complete the background activity and tell iOS that it's done by calling the completion block. More info on application(_:didReceiveRemoteNotification:fetchCompletionHandler:) here

Issue we observed

We discovered that on iOS 14 calling the completion block multiple times is leading to a crash. Based on my limited debugging capabilities, it looks like the API is now using dispatch groups to trace the moment when the completion is called, and in the completion block it invokes dispatch_group_leave. Once the completion block is called multiple times, the iOS is going to crash since this behavior is unexpected (calling dispatch_group_leave more times than dispatch_group_enter).

More people complained about the same issue here invertase/react-native-firebase#3944.

Worth noticing that in our experience the crash is only happening when the app is actually running in the foreground and in case the FCM is used together with the Braze SDK (they are also using GULAppDelegateSwizzler).

Proposed solution

Similia similibus curantur

It is sufficient to wrap the final completion block provided by iOS into yet another dispatch_group that would be entered for every observer of the application(_:didReceiveRemoteNotification:fetchCompletionHandler:) and left when the observer is calling to the provided callback.

google-cla · 2020-10-29T13:06:12Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

mikeger · 2020-10-29T13:07:04Z

@googlebot I signed it!

mikeger · 2020-10-29T14:06:00Z

I am afraid there are some CI errors in GitHub Actions, seems like something unrelated to my change. Should I rebase it on your recent release-xxx branch?

paulb777 · 2020-10-29T14:24:25Z

@mikeger Thanks for the PR! The CI failure is a result of GitHub Actions changing its default Xcode from 11.7 to 12.0.1 yesterday and being addressed in #6865. You can disregard for now. master is the correct target for this PR.

Assigning for review to @maksymmalyhin for the GoogleUtilities changes and @chliangGoogle for the FirebaseMessaging changes.

maksymmalyhin

Great idea, thank you for your contribution!

I added a couple comments to adjust the logic a bit. Feel free to ask clarification questions as needed.

Also we will need to add unit tests for these changes before merging. Please let us know if you help with it.

maksymmalyhin · 2020-10-29T14:30:18Z

GoogleUtilities/AppDelegateSwizzler/GULAppDelegateSwizzler.m

@@ -882,17 +883,30 @@ - (void)application:(GULApplication *)application
 didReceiveRemoteNotificationWithCompletionIMP =
 [didReceiveRemoteNotificationWithCompletionIMPPointer pointerValue];

+ dispatch_group_t __block callbackGroup = dispatch_group_create();
+ UIBackgroundFetchResult __block latestFetchResult = UIBackgroundFetchResultNoData;
+ dispatch_group_notify(callbackGroup, dispatch_get_main_queue(), ^() {


It looks like calling dispatch_group_notify on an empty group will lead to scheduling the block straight away. I think it should be moved to after line 916.

maksymmalyhin · 2020-10-29T14:37:39Z

GoogleUtilities/AppDelegateSwizzler/GULAppDelegateSwizzler.m

 NSInvocation *invocation = [GULAppDelegateSwizzler
 appDelegateInvocationForSelector:methodSelector];
 [invocation setTarget:interceptor];
 [invocation setSelector:methodSelector];
 [invocation setArgument:(void *)(&application) atIndex:2];
 [invocation setArgument:(void *)(&userInfo) atIndex:3];
- [invocation setArgument:(void *)(&completionHandler) atIndex:4];
+ [invocation setArgument:(void *)(&localCompletionHandler)
+ atIndex:4];
 [invocation invoke];
 }];
 // Call the real implementation if the real App Delegate has any.


The current implementation doesn't add the originally implemented method to the group. I think we should do the same for it: add enter/leave group calls and wrap the completion. Potentially we may use a single completion handler wrapper to be passed to all interceptors and the original implementations.

maksymmalyhin · 2020-10-29T14:42:00Z

GoogleUtilities/AppDelegateSwizzler/GULAppDelegateSwizzler.m

+ dispatch_group_enter(callbackGroup);
+ void (^localCompletionHandler)(UIBackgroundFetchResult) =
+ ^void(UIBackgroundFetchResult fetchResult) {
+ latestFetchResult = fetchResult;


I think we can improve combining the fetch results. Perhaps if there is at least on UIBackgroundFetchResultNewData the combined result should be also UIBackgroundFetchResultNewData. Also I would pass UIBackgroundFetchResultFailed only if all calls failed.

mikeger · 2020-10-29T15:00:20Z

@maksymmalyhin thanks for the review, all valid comments.

paulb777 · 2020-10-29T17:33:50Z

#6865 has merged so rebasing or merging master should now enable a green CI run.

AlexPritchin · 2020-11-04T21:14:41Z

Hello. Are there any updates here? I'm facing exact same issue in Firebase 6.18, 6.31.1, 6.34.0 and even 7.0.0. However I'm not using Braze SDK. According to Crashlytics 80% of such crashes occur on iOS 14.1. Others on other versions of iOS 14. So I'm waiting for this fix in next release version of Firebase. BTW the project I'm supporting now is written on Objective C.

maksymmalyhin · 2020-11-04T22:08:12Z

@mikeger Will you have time to address the comments? We may finish your PR if not. We will make sure your contribution will be visible in Firebase repo. Please let us know.

mikeger · 2020-11-05T09:25:59Z

@maksymmalyhin hey, I would address the comments today. I got stuck since the tests (./test.sh) were not running for me (build failed), I will retry rebased on the current master.

maksymmalyhin · 2020-11-05T15:02:04Z

@mikeger Ah, ./test.sh is an obsolete script that should've been removed. Sorry about that. Currently we mostly use the following ways to run tests:

Follow the development guide and run tests from the generated project. This is the most convenient way for active development.
Running pod lib lint for a podspec. See CI for examples, e.g. for FirebaseMessaging or GoogleUtilities. This is useful to debug CI issues locally.

mikeger · 2020-11-05T16:00:13Z

@maksymmalyhin thanks so much for the testing tip, this allowed me to progress with the tests. Please re-review the changes, I updated it based on your comments.

maksymmalyhin

Great, thank you for addressing the comments! It mostly looks good to me. I added a couple minor comments. The comments are mostly optional and I'm fine proceeding with proceeding as is.

@chliangGoogle Could you please take a look at FCM part.

maksymmalyhin · 2020-11-05T16:30:24Z

GoogleUtilities/Tests/Unit/Swizzler/GULAppDelegateSwizzlerTest.m

+
+ XCTestExpectation *completionExpectation =
+ [[XCTestExpectation alloc] initWithDescription:@"Completion called once"];
+ completionExpectation.assertForOverFulfill = YES;


nit: I think this line is not required as YES is the default value of the property

maksymmalyhin · 2020-11-05T17:06:26Z

GoogleUtilities/Tests/Unit/Swizzler/GULAppDelegateSwizzlerTest.m

+ fetchCompletionHandler:completion];
+ testAppDelegate.remoteNotificationCompletionHandler(UIBackgroundFetchResultNewData);
+ OCMVerifyAll(interceptor);
+ [self waitForExpectations:@[ completionExpectation ] timeout:0.1];


nit: Looks like these 3 tests are mostly identical except the background fetch result. Could you please extract the test logic to a helper function like assertApplicationDidReceiveRemoteNotificationWithCompletionCompletionIsCalledOnceWithResult:(UIBackgroundFetchResult) to reduce code duplication.

I thought it's paid by the number of lines

maksymmalyhin · 2020-11-05T17:11:29Z

GoogleUtilities/Tests/Unit/Swizzler/GULAppDelegateSwizzlerTest.m

+ [self waitForExpectations:@[ completionExpectation ] timeout:0.1];
+}
+
+- (void)


It would be nice to add tests for cases with different fetch results or at least add a TODO to add them.

mikeger · 2020-11-05T18:20:19Z

@maksymmalyhin updated, thanks for the comments!

maksymmalyhin

👍

maksymmalyhin

I think we've missed on place where we need to call the completion handler after the change: could you please add it to auth as well.

mikeger · 2020-11-05T20:01:32Z

@maksymmalyhin added, to my understanding there it's sufficient to always indicate NoData.

charlotteliang

LGTM on messaging

maksymmalyhin · 2020-11-05T20:19:00Z

All looks good to me, I'm going to merge the PR on CI green.

maksymmalyhin · 2020-11-05T22:32:57Z

@mikeger Thank you again for your contribution!

We will try to include the fix into 7.1.0 release.

…s are called (#6863) * Fix crash on iOS 14 when multiple completion methods are called * Fixed lint * Addressed the PR comments & implemented unit tests * Reverted auto-extracted method * Re-organized tests * Added completion handler for FIRAuth

* FCM: Fix crash on iOS 14 when multiple app delegate completion methods are called (#6863) * Fix crash on iOS 14 when multiple completion methods are called * Fixed lint * Addressed the PR comments & implemented unit tests * Reverted auto-extracted method * Re-organized tests * Added completion handler for FIRAuth * Manifest: GDT 8.0.1 * Update Changelogs for 7.1.0 (#6913) * Changelogs Co-authored-by: Mike Gerasimenko <[email protected]> Co-authored-by: Paul Beusterien <[email protected]>

* Update versions for Release 7.1.0 * 7.1.0 update version of close source podspecs * Release 7.1.0 cherry picks (#6917) * FCM: Fix crash on iOS 14 when multiple app delegate completion methods are called (#6863) * Fix crash on iOS 14 when multiple completion methods are called * Fixed lint * Addressed the PR comments & implemented unit tests * Reverted auto-extracted method * Re-organized tests * Added completion handler for FIRAuth * Manifest: GDT 8.0.1 * Update Changelogs for 7.1.0 (#6913) * Changelogs Co-authored-by: Mike Gerasimenko <[email protected]> Co-authored-by: Paul Beusterien <[email protected]> * Bump GDT podspec version to 8.0.1 * 7.1.0 SPM Analytics (#6933) * Carthage update 7.1.0 (#6943) Co-authored-by: Mike Gerasimenko <[email protected]> Co-authored-by: Paul Beusterien <[email protected]>

google-cla bot added the cla: no label Oct 29, 2020

google-cla bot added cla: yes and removed cla: no labels Oct 29, 2020

paulb777 assigned maksymmalyhin and charlotteliang Oct 29, 2020

maksymmalyhin suggested changes Oct 29, 2020

View reviewed changes

paulb777 added the api: messaging label Oct 29, 2020

mikeger added 2 commits November 5, 2020 13:25

Fix crash on iOS 14 when multiple completion methods are called

81fbde4

Fixed lint

f56db59

Addressed the PR comments & implemented unit tests

2c9ff1a

mikeger force-pushed the master branch from cb3699e to 2c9ff1a Compare November 5, 2020 15:53

Reverted auto-extracted method

2710c04

maksymmalyhin reviewed Nov 5, 2020

View reviewed changes

maksymmalyhin approved these changes Nov 5, 2020

View reviewed changes

Re-organized tests

1988b4c

maksymmalyhin approved these changes Nov 5, 2020

View reviewed changes

maksymmalyhin suggested changes Nov 5, 2020

View reviewed changes

Added completion handler for FIRAuth

1556a49

charlotteliang approved these changes Nov 5, 2020

View reviewed changes

maksymmalyhin approved these changes Nov 5, 2020

View reviewed changes

maksymmalyhin merged commit c8de4fc into firebase:master Nov 5, 2020

maksymmalyhin mentioned this pull request Nov 6, 2020

Release 7.1.0 cherry picks #6917

Merged

mauriciosoares01 mentioned this pull request Nov 12, 2020

🔥 iOS App running on iOS 14 crashes every-time new push comes invertase/react-native-firebase#3944

Closed

5 tasks

bang9 mentioned this pull request Nov 13, 2020

fix: avoid crash on iOS 14 when push comes on foreground react-native-push-notification/ios#234

Merged

maksymmalyhin added the GoogleUtilities label Nov 13, 2020

firebase locked and limited conversation to collaborators Dec 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FCM: Fix crash on iOS 14 when multiple app delegate completion methods are called #6863

FCM: Fix crash on iOS 14 when multiple app delegate completion methods are called #6863

mikeger commented Oct 29, 2020

google-cla bot commented Oct 29, 2020

mikeger commented Oct 29, 2020

mikeger commented Oct 29, 2020

paulb777 commented Oct 29, 2020

maksymmalyhin left a comment

maksymmalyhin Oct 29, 2020

maksymmalyhin Oct 29, 2020

maksymmalyhin Oct 29, 2020

mikeger commented Oct 29, 2020

paulb777 commented Oct 29, 2020

AlexPritchin commented Nov 4, 2020

maksymmalyhin commented Nov 4, 2020

mikeger commented Nov 5, 2020

maksymmalyhin commented Nov 5, 2020

mikeger commented Nov 5, 2020

maksymmalyhin left a comment

maksymmalyhin Nov 5, 2020

maksymmalyhin Nov 5, 2020

mikeger Nov 5, 2020

maksymmalyhin Nov 5, 2020

mikeger commented Nov 5, 2020

maksymmalyhin left a comment

maksymmalyhin left a comment

mikeger commented Nov 5, 2020

charlotteliang left a comment

maksymmalyhin commented Nov 5, 2020

maksymmalyhin commented Nov 5, 2020

FCM: Fix crash on iOS 14 when multiple app delegate completion methods are called #6863

FCM: Fix crash on iOS 14 when multiple app delegate completion methods are called #6863

Conversation

mikeger commented Oct 29, 2020

Background

Issue we observed

Proposed solution

google-cla bot commented Oct 29, 2020

What to do if you already signed the CLA

Individual signers

Corporate signers

mikeger commented Oct 29, 2020

mikeger commented Oct 29, 2020

paulb777 commented Oct 29, 2020

maksymmalyhin left a comment

Choose a reason for hiding this comment

maksymmalyhin Oct 29, 2020

Choose a reason for hiding this comment

maksymmalyhin Oct 29, 2020

Choose a reason for hiding this comment

maksymmalyhin Oct 29, 2020

Choose a reason for hiding this comment

mikeger commented Oct 29, 2020

paulb777 commented Oct 29, 2020

AlexPritchin commented Nov 4, 2020

maksymmalyhin commented Nov 4, 2020

mikeger commented Nov 5, 2020

maksymmalyhin commented Nov 5, 2020

mikeger commented Nov 5, 2020

maksymmalyhin left a comment

Choose a reason for hiding this comment

maksymmalyhin Nov 5, 2020

Choose a reason for hiding this comment

maksymmalyhin Nov 5, 2020

Choose a reason for hiding this comment

mikeger Nov 5, 2020

Choose a reason for hiding this comment

maksymmalyhin Nov 5, 2020

Choose a reason for hiding this comment

mikeger commented Nov 5, 2020

maksymmalyhin left a comment

Choose a reason for hiding this comment

maksymmalyhin left a comment

Choose a reason for hiding this comment

mikeger commented Nov 5, 2020

charlotteliang left a comment

Choose a reason for hiding this comment

maksymmalyhin commented Nov 5, 2020

maksymmalyhin commented Nov 5, 2020