Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[PLAT-5729 ] Fix malformed NSError from BSGJSONSerialization #987

Merged
merged 1 commit into from
Feb 1, 2021
Merged

[PLAT-5729 ] Fix malformed NSError from BSGJSONSerialization #987

merged 1 commit into from
Feb 1, 2021

Conversation

nickdowell
Copy link
Contributor

@nickdowell nickdowell commented Jan 29, 2021 •
edited
Loading

Goal

Fixes a low-occurrence crash observed in a customer dashboard

NSInvalidArgumentException: -[NSException domain]: unrecognized selector sent to instance 0x29b7d8930

0  CoreFoundation    ___exceptionPreprocess
1  libobjc.A.dylib   _objc_exception_throw
2  CoreFoundation    -[NSObject(NSObject) doesNotRecognizeSelector:]
3  CoreFoundation    ____forwarding___
4  CoreFoundation    ___forwarding_prep_0___
5  CoreFoundation    _CFErrorGetDomain
6  CoreFoundation    __CFErrorCopyUserInfoKeyFromCallBack
7  CoreFoundation    __CFErrorFormatDebugDescriptionAux
8  CoreFoundation    _userInfoKeyValueShow
9  CoreFoundation    -[__NSDictionaryI __apply:context:]
10 CoreFoundation    __CFErrorFormatDebugDescriptionAux
11 CoreFoundation    __CFErrorCreateDebugDescription
12 Foundation        -[NSError description]
13 CoreFoundation    -[NSObject(NSObject) _copyDescription]
14 CoreFoundation    _CFCopyDescription
15 CoreFoundation    ___CFStringAppendFormatCore
16 CoreFoundation    __CFStringCreateWithFormatAndArgumentsAux2
17 App               bsg_i_kslog_logObjC (BSG_KSLogger.m:259:29)
18 App               -[BSG_KSCrash captureThreads:depth:recordAllThreads:] (BSG_KSCrash.m:348:9)
19 App               -[BugsnagClient notify:handledState:block:] (BugsnagClient.m:980:24)
20 App               -[BugsnagClient notifyError:block:] (BugsnagClient.m:870:5)
21 App               +[Bugsnag notifyError:block:] (Bugsnag.m:132:9)

I was not able to reproduce the crash in a test case, but the root cause was an NSException being asked for its domain (which it doesn't have) so something that was expecting to be passed an NSError was being passed an NSException.

From the involvement of -[BSG_KSCrash captureThreads:depth:recordAllThreads:] it can be seen that the error being logged came from [BSGJSONSerialization JSONObjectWithData:options:error:]

So the root cause is an exception object being passed as the value for NSUnderlyingErrorKey and the system libraries trying to interpret it as an NSError in accordance with the documentation;

// Key in userInfo. A recommended standard way to embed NSErrors from underlying calls.
// The value of this key should be an NSError.
FOUNDATION_EXPORT NSErrorUserInfoKey const NSUnderlyingErrorKey;

Changeset

  • Removed offending value for NSUnderlyingErrorKey
  • Added test case and renamed test class / file to match convention (<ClassBeingTested>Tests)

Testing

Was not able to reproduce the crash.

Added test case to verify that NSUnderlyingErrorKey is an NSError, if provided.

@github-actions
Copy link

Infer: No issues found 🎉

OCLint: No issues found 🎉

Bugsnag.framework binary size decreased by 72 bytes from 1,060,360 to 1,060,288 🎉

Generated by 🚫 Danger

@nickdowell nickdowell marked this pull request as ready for review January 29, 2021 14:58
@nickdowell nickdowell merged commit 2c68530 into next Feb 1, 2021
@nickdowell nickdowell deleted the nickdowell/fix-json-exception-errors branch February 1, 2021 08:43
@djskinner djskinner mentioned this pull request Feb 15, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kattrali kattrali kattrali approved these changes

@kstenerud kstenerud Awaiting requested review from kstenerud kstenerud is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@nickdowell @kattrali