superwall / Superwall-iOS

Remotely configure every aspect of your paywall and double your revenue.
https://superwall.com
MIT License
86 stars 22 forks source link

[BUG] Fatal error: Unexpectedly found nil while unwrapping an Optional value in getSortedLocalizations() (iOS 17) #144

Closed tkafka closed 1 year ago

tkafka commented 1 year ago

New issue checklist

General information

Describe the bug

I have built and ran an app with Superwallkit 3.0.1 in Xcode 15 for iOS 17 simulator. The app crashed with Thread 1: Fatal error: Unexpectedly found nil while unwrapping an Optional value on LocalizationLogic.swift:22 while unpacking the currentLocale.localizedString(forLanguageCode: localeId) when localeId = bgc.

It seems to be that either there should not be a need for Superwall to enumerate hundreds of locales, or that the enumeration should be coded defensively, with an if let instead of force unwrap.

Other Information

image

Scheme settings:

image
tkafka commented 1 year ago

A repro code. When Locale.autoupdatingCurrent is cs_001, the currentLocale.localizedString(forLanguageCode: localeId) returns nil for bgc localeId.

When I do the same with localeCs = NSLocale(localeIdentifier: "cs_001"), it works.

So, Superwall enumeration now crashes the whole app on devices with Czech locale on iOS 17. I reported this to Apple, but this needs to be changed into a defensive code.

    func printLocales() {
        let localeIds = Locale.availableIdentifiers

        let currentLocale = Locale.autoupdatingCurrent
        let localeUs = NSLocale(localeIdentifier: "en_US")
        let localeCs = NSLocale(localeIdentifier: "cs_001")

        print("localeIdentifier, Current (\(currentLocale.identifier)), US, CS")
        localeIds.forEach { localeId in
            /// Get language
            let localizedLanguageCurrentLocale = currentLocale.localizedString(
                forLanguageCode: localeId
            )
            let localizedLanguageUs = localeUs.localizedString(
                forLanguageCode: localeId
            )
            let localizedLanguageCs = localeCs.localizedString(
                forLanguageCode: localeId
            )

            print("\(localeId),\"\(localizedLanguageCurrentLocale ?? "NO LOCALIZED STRING IN CURRENT LOCALE (\(currentLocale.identifier))")\",\"\(localizedLanguageUs ?? "NO US LOCALIZED STRING")\",\"\(localizedLanguageCs ?? "NO CS LOCALIZED STRING")\"")
        }
    }
tkafka commented 1 year ago

I fixed this for myself in https://github.com/tkafka/Superwall-iOS/commit/ccf077f35300429e694fe154b0ed8bf505f7b07d

tkafka commented 1 year ago

Guys, I looked at how much swiftlint:disable:next force_unwrapping is used in Superwallkit. ! definitely feels like a code smell to me - each place is a potential crash site for the app.

yusuftor commented 1 year ago

Thanks for this report, I've fixed this on the develop branch now. I've also moved the localization manager to be lazily loaded only when debugging paywalls, which means that we now won't loop through the localizations every time the app is launched from a cold start. Good point about the disabling of force_unwrapping, I've reduced these instances as well as force_cast.

tkafka commented 1 year ago

Thanks, this definitely happens for multiple languages, as I had crash reports from multiple people around the world. Will be happy to move back to official SDK once this is merged into some release.

yusuftor commented 1 year ago

Thanks, we've had some reports from others too since. Again keeping this open for visibility until it's merged into the next release.

yusuftor commented 1 year ago

Released in 3.0.2!