-
Notifications
You must be signed in to change notification settings - Fork 262
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.
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
Fix IAM redisplay cache userdefaults crashes #894
Conversation
OS_IAM_REDISPLAY_DICTIONARY contains OSInAppMessage objects so we can't save it as a dictionary to userdefaults or we will crash.
Now that our unit tests are run inside a test app we no longer need to mock NSUserDefaults. This gets us detection of bad storage like trying to store a custom object in a dictionary instead of as a codeable. This commit also fixes the crash in InAppMessagingIntegrationTests by changing save/getDictionary to save/getCodeableData
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r1, 12 of 12 files at r2, 1 of 1 files at r3, 1 of 1 files at r4.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @emawby and @Jeasmine)
iOS_SDK/OneSignalSDK/Source/OSMigrationController.m, line 93 at r3 (raw file):
@try { __unused NSMutableDictionary *_ =[[NSMutableDictionary alloc] initWithDictionary:[OneSignalUserDefaults.initStandard
Why use __unused
? Do we get a warning if we ignore the input and put comment instead? If that is the case can we still give this a variable name instead of using _
as the name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice testing and fix!
If the OS_IAM_REDISPLAY_CACHE is successfully read as Codeable data we don't need to migrate. If that fails try to read it as a dictionary and then save it as Codeable data. If that also fails then clear the cache because we cannot determine what data type was stored for the key.
testing empty dictionary to codeable data testing codeabledata unchanged testing nil dictionary to nil
a8b774b
to
a51401d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 13 of 15 files reviewed, 1 unresolved discussion (waiting on @jkasten2)
iOS_SDK/OneSignalSDK/Source/OSMigrationController.m, line 93 at r3 (raw file):
Previously, jkasten2 (Josh Kasten) wrote…
Why use
__unused
? Do we get a warning if we ignore the input and put comment instead? If that is the case can we still give this a variable name instead of using_
as the name?
unused gets rid of the Xcdoe warning. I named it redisplayDict
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r5, 1 of 1 files at r6.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @emawby)
Fix for issue #848
OS_IAM_REDISPLAY_DICTIONARY
containsOSInAppMessage
objects so we can't save it as a dictionary to nsuserdefaults or it will crash. We were do this when deleting IAMs that were over 6 months old.We are now saving them properly as codeable data, but previously saved dictionary data will still cause a crash when trying to retrieve the cache as codeabledata so a migration was created to convert the dictionary data to codeable data.
To properly test this and catch crashes like this in the future I have removed the NSUserDefaultsOverrider. Since we are now running tests in a containing app, we can use the standard UserDefaults implementation that catches types issues like this one.
This change is