NaN crash Encodable.toString(pretty:)

[cprince-foreflight]

Describe the bug
We are getting production app crash reports here with the toString. Crashlytics is reporting it as:

Encodable.toString(pretty:)
NSInvalidArgumentException - NaN number in JSON write

With more detail:

To Reproduce
So far I don't have a clear repro case. But, we have 34 crash events affecting 30 users.

It looks like some our custom data being added to dictionary payloads has NaN numeric (NSNumber) values. And that is making it through the JSON tests (e.g., here), but later somehow causing the JSONDecoder to crash.

I can't actually show that above idea with an actual crash in our development build though, so that's just my best guess so far.

Expected behavior
I wouldn't expect the JSONDecoder usage in JSON.swift to cause an app crash.

Screenshots
See above.

Platform (please complete the following information):

• Library Version in use: 1.4.7
• Platform being tested: iOS and iPadOS-- most crashes are on iPad, but many of our users are on iPad also.
• Integrations in use: Firebase

Additional context
I'd like to resolve this ASAP. I'm hoping to get a patch into our production app. Thanks.

[bsneed]

I can reproduce this in a test here:

    func testJSONNaN() throws {
        struct TestStruct: Codable {
            let str: String
            let decimal: Double
            let nan: Double
        }
        let nan = NSNumber.FloatLiteralType(nan: 1, signaling: true)
        
        do {
            let o = try JSON(nan)
            XCTAssertNotNil(o)
        } catch {
            print(error)
            XCTFail()
        }
        
        let test = TestStruct(
            str: "hello",
            decimal: 333.9999,
            nan: nan
        )
        
        do {
            let o = try JSON(with: test)
            XCTAssertNotNil(o)
        } catch {
            print(error)
            XCTFail()
        }
    }

Lemme think on the best way to handle it... or if you have any thoughts lemme know.

[cprince-foreflight]

Thanks for the quick response. When I try your function, the catch blocks do execute-- i.e., JSON throws an error. I don't think JSON can be throwing an error in our case because the error should get caught here, the props would be nil, and a NaN from our side couldn't cause an issue with toString.

Have you been able to repro a crash in the JSONEncoder?

(I just noticed I mis-wrote above and said JSONDecoder when I meant JSONEncoder).

[cprince-foreflight]

I do now note that:

        let nanNumber = NSNumber(value: 0.0/0.0)
        let nan = NSNumber.FloatLiteralType(nan: 1, signaling: true)

        let info: [String: CodableValue] = [
            "my-key1" : nanNumber,
            "my-key2" : nan
        ]

        analytics.track(name: name, properties: info)

Doesn't throw an error in JSON.

[bsneed]

Thanks for the quick response. When I try your function, the catch blocks do execute-- i.e., JSON throws an error. I don't think JSON can be throwing an error in our case because the error should get caught here, the props would be nil, and a NaN from our side couldn't cause an issue with toString.

Have you been able to repro a crash in the JSONEncoder?

(I just noticed I mis-wrote above and said JSONDecoder when I meant JSONEncoder).

I'm pretty sure we're hitting NSNumber in objc here on the original stack trace, in which case, you're getting an ObjC exception, not a swift one.

[bsneed]

What is "CodableValue" in your example above?

[cprince-foreflight]

CodableValue

typealias CodableValue = Any

:)

[cprince-foreflight]

I now have a repro. Running as an Xcode debug build on iPad 10th generation, iPadOS 16.2 simulator. If I add NaN values to the properties in a track call via Objective-c. E.g.,

    NSNumber *NaN1 = [NSDecimalNumber notANumber];
    NSNumber *NaN2 = [NSNumber numberWithDouble: 0./0.];
    [augmentedParams setValue:NaN1 forKey:@"myNaN1"];
    [augmentedParams setValue:NaN2 forKey:@"myNaN2"];

Or if I do this via Swift, e.g.,

    let nanNumber = NSNumber(value: 0.0/0.0)
    let nan = NSNumber.FloatLiteralType(nan: 1, signaling: true)

    let info: [String: CodableValue] = [
        "my-key1" : nanNumber,
        "my-key2" : nan
    ]

I get the NaN crash as indicated above.

This also happens if I use an iPad Mini, actual hardware running iPadOS 16.6.1.

So far all of our reported crashes are on iPadOS 16 or lower. I've not been able to repro on iPadOS 17 simulator.

[cprince-foreflight]

Here is a smaller repro, and a fix as well. This only crashes on iOS 16 or lower (i..e, not iOS 17).
TestNaNCrash.zip

[bsneed]

I can't use that fix unfortunately. It introduces side effects that would be unknown to others without reading the code. Since NaN, Infinity, -Infinity aren't allowed in JSON, it makes the most sense for us to throw on data that contains it. That puts the onus on the developer to do whatever interpretations (convert to 0 in your case), maybe type it as a string with "NaN" etc. on their own. What do you think?

[cprince-foreflight]

This seems like a migration difference, from the prior Objective-C library. Not sure what was being logged there with NaN values, but I suspect we've had those instances logging to Segment in our code for a while now. Not saying that's the best on our side, but just a fact. It's a large lift for us to go through all our Segment analytics logging and change it.

I wonder if you could add a setting something like what JSONEncoder has. And put this setting into your Configuration. The default of configuration value could be to throw, but you could allow for clients to also decide on some other less exceptional behavior, such as a 0 value.

[bsneed]

It's not really. They're likely just getting stripped out silently due to the checks it does during JSON generation. I wrote most of that code for the JSON generation in analytics-ios, and i wasn't thinking about NaN/Infinity so it wasn't on purpose. It's another instance of technically correct vs. convenience. I'll keep noodling on it for the time being since you have a work around in place.

[bsneed]

PR for this pending legal review. Stay tuned.