Appboy / appboy-ios-sdk

Public repo for the Braze iOS SDK
https://www.braze.com
Other
165 stars 142 forks source link

Silent Push Notification Receive Crashes App for iOS 14 devices #256

Closed kolibrigames-ajimine-vahid closed 3 years ago

kolibrigames-ajimine-vahid commented 3 years ago

https://github.com/Appboy/appboy-ios-sdk/blob/0e1e249c621fc38530541c23915630205d995891/AppboyKit/headers/AppboyKitLibrary/Appboy.h#L521

This crashes for Unity when invoked upon receiving a silent push notification on iOS 14. It works fine for iOS 13, and iOS 12 devices.

Bucimis commented 3 years ago

@kolibrigames-ajimine-vahid can you post a stack trace of the crash?

kolibrigames-ajimine-vahid commented 3 years ago

idleminertycoon AppboyUnityManager.mm - Line 316 -[AppboyUnityManager registerApplication:didReceiveRemoteNotification:fetchCompletionHandler:]

this was pretty much it

kolibrigames-ajimine-vahid commented 3 years ago

Crashed: com.apple.main-thread 0 libdispatch.dylib 0x1aedeb7f4 dispatch_group_leave.cold.1 + 36 1 libdispatch.dylib 0x1aedb98c4 _dispatch_group_wake + 126 2 ??? 0x35557f0108ea4000 (Missing) 3 idleminertycoon 0x10117563c -[AppboyUnityManager registerApplication:didReceiveRemoteNotification:fetchCompletionHandler:] + 316 (AppboyUnityManager.mm:316)

Bucimis commented 3 years ago

Thanks for the info. What version of the Unity SDK are you on? We resolved some issues in that method in 3.22.0 of the iOS SDK (Unity 2.3+). Also, is this consistently reproducible?

kolibrigames-ajimine-vahid commented 3 years ago

This is for Unity Version 2.5.0 when building with Xcode 12. This happens everytime a silent push notification is received.

Another note, we are building on Xcode 12 and on Unity 2018.4.17f1 LTS

johannth commented 3 years ago

We're not using the Unity SDK but still seeing the same. Building on Xcode 12, first saw it on 3.26.1 and then updated to 3.28.0 but seems to be still happening. We haven't been able to reproduce locally but our crash reporting tools tell another story.

Bucimis commented 3 years ago

@johannth can you check whether the completionHandler is getting called multiple times (note that Braze will call it if passed into our delegates, you can also pass in nil)? That seems to cause issues in iOS 14.

johannth commented 3 years ago

HI @Bucimis, that might be the case - we're looking into if Firebase Auth Phone Number is conflicting with Braze in this code path. Will report back when we figure it out.

johannth commented 3 years ago

But are you suggesting that we should pass in nil for the silent notification? i.e. does Braze not need to call it if the silent notification is from Braze?

Bucimis commented 3 years ago

@johannth You should always pass the notification object into Braze's push delegates, however, you are not strictly required to pass in a completionHandler (though best practice is to).

You can check if a push is from Braze using the following utility: https://appboy.github.io/appboy-ios-sdk/docs/interface_a_b_k_push_utils.html#a9510314a1e1f6cf457872505b2af1599

You'll need to ensure that between your code and any third-party libraries that you have installed, that the original completionHandler provided by the OS is always called at most once.

johannth commented 3 years ago

@Bucimis - you were right and we managed to reproduce. A silent push notification sent from Firebase called the completion handler but then we passed the completion handler into braze and it got called again. We fixed it by adding the following defences:

- (void)application:(UIApplication *)application didReceiveRemoteNotification:(NSDictionary *)userInfo fetchCompletionHandler:(void (^)(UIBackgroundFetchResult result))completionHandler {
  if ([ABKPushUtils isAppboyRemoteNotification:userInfo]
      || [ABKPushUtils isAppboyInternalRemoteNotification:userInfo]
      || [ABKPushUtils isGeofencesSyncRemoteNotification:userInfo]
      || [ABKPushUtils isPushStoryRemoteNotification:userInfo]
      || [ABKPushUtils isUninstallTrackingRemoteNotification:userInfo]) {
    [[Appboy sharedInstance] registerApplication:application
                    didReceiveRemoteNotification:userInfo
                          fetchCompletionHandler:completionHandler];
  }
}

Does this make sense as a fix to you?

I think it might make sense for the Braze SDK to be extra defensive on this (i.e. include this if statement in the SDK and don't call the completion handler unless it's your push) because it seems iOS 14 is sensitive to these multiple triggers.

Bucimis commented 3 years ago

@johannth that fix looks good. One quick optimization you can make is to remove the check for isGeofencesSyncRemoteNotification and isUninstallTrackingRemoteNotification since those are also checked within isAppboyInternalRemoteNotification (apologies for not having that be super clear from the docs)

re Not calling the completionHandler for non-Appboy pushes, I agree and we'll look into getting a fix released in the near future.

hokstuff commented 3 years ago

I'm closing out this issue since it appears to be fixed and has been inactive for a while. If you are still having problems, feel free to contact support@braze.com with more info.

Thanks!