firebase / firebase-ios-sdk

Firebase SDK for Apple App Development
https://firebase.google.com
Apache License 2.0
5.59k stars 1.46k forks source link

Crash rejection from App Store Review: watchdog and Security framework timeout #2843

Closed christianselig closed 5 years ago

christianselig commented 5 years ago

Describe your environment

Steps to reproduce:

I'm unfortunately unable to reproduce this personally, but App Store review was able to and provided the following 3 crash logs (I've symbolicated them). They were able to produce this simply by trying to launch the app.

Crash Log 1: https://gist.github.com/christianselig/1c278c522e41c9ba0d7bf7d70c21158a Crash Log 2: https://gist.github.com/christianselig/629f846ab0f4a1e4df2c84ae19ee9516 Crash Log 3: https://gist.github.com/christianselig/51d216b3c1afd593b898d496e64be6cb

Relevant Code:

The following is Swift, I allow users to opt out of crash reporting and analytics completely so I start the app up with the relevant Info.plist disablers (eg FIREBASE_ANALYTICS_COLLECTION_ENABLED), and then check in applicationDidFinishLaunching if they still have it enabled, and if so start them up:

// It's a weird dance for which parts of Firebase have to be enabled in what order, Performance Monitoring has to be before, but Analytics has to be after. Crashlytics doesn't seem to care as it seems to start up independently (doesn't require `Firebaseapp.configure()`).
let analyticsEnabled = UserDefaults.standard.bool(forKey: DefaultsKeys.crashAnalyticsEnabled)

if analyticsEnabled {
    Performance.sharedInstance().isInstrumentationEnabled = true
    Performance.sharedInstance().isDataCollectionEnabled = true
}

FirebaseApp.configure()

if analyticsEnabled {
    Analytics.setAnalyticsCollectionEnabled(true)

    Fabric.with([Crashlytics.self])

    if let userIdentifier = UIDevice.current.identifierForVendor?.uuidString {
        Crashlytics.sharedInstance().setUserIdentifier(userIdentifier)
    }
}

Notes/Workaround

I know the crashlogs don't particularly outline Firebase (rather Lockbox, a library for accessing the keychain), but I've had Lockbox in the app for dozens of submissions (and approvals) and this is effectively my first build with Firebase. Further after reading #1399 there was some questions around other keychain-accessing code conflicting with Firebase, which seems to apply here, so I thought it would be relevant to report as my assumption would be Firebase and Lockbox deadlocked.

Lockbox also starts up at launch, because user credentials are stored in the keychain and restored when the app launches.

Because of this I'm pretty sure I can get around this by delaying the execution of Firebaseapp.configure() by a few seconds (a good ol' DispatchQueue.main.asyncAfter), but it's not the best workaround obviously as it won't catch crashes that occur within the first three seconds.

If there's some really stupid I'm forgetting or overlooking, please publicly shame me!

maksymmalyhin commented 5 years ago

@jamesdaniels Thank you for sharing your issue.

We can confirm that Firebase may perform requests to Keychain in the background as result of calling FirebaseApp.configure(). In the current Firebase version we make sure that Keychain is accessed asynchronously, so we don't block application start. I assume, these requests may interfere with the requests from Lockbox, so the Lockbox requests are performed slower.

As far as I can see from the provided crash logs, your application loads data from Keychain synchronously from UITableViewDataSource of a table view which, I guess, is a part of the app initial view controller. As a result, your initial view controller view is not loaded until the data from Keychain is loaded, which delays the app start.

In general, accessing Keychain is a relatively expensive operation, so it is not recommended to be done synchronously at the app startup. I would recommend to consider a small refactoring of your initial view controller, so Keychain is accessed from e.g. viewWillAppear or viewDidAppear and don't block the application start.

christianselig commented 5 years ago

@maksymmalyhin So to be clear I'm a moron and this has nothing to do with Firebase and is just a coincidence? If that's the case I'm so sorry for wasting your time! And I appreciate you answering!

maksymmalyhin commented 5 years ago

@christianselig Well, it is not exactly what I meant 😄. We've had to fix the issue with accessing Keychain synchronously in FirebaseApp.configure() because of similar issues just recently. It looks to me that in this particular case Firebase helps to reveals your issue.

Please let us know if you find more details pointing out to Firebase as a source of the issue.

christianselig commented 5 years ago

Thanks so much, I definitely will! Appreciate you shining a light on the problem even when it had nothing to do with you. :P