OneSignal is a free push notification service for mobile apps. This plugin makes it easy to integrate your native iOS app with OneSignal. https://onesignal.com
Other
493
stars
262
forks
source link
[Bug] Make null values safe for IAM requests #1457
Update, see below under Manual Testing for a reproduction scenario.
Motivation
Fix crashes, reported in #1453 and others. The crash reports all look like they are for player ID.
Scope
Correctly handle and pass along null values.
Testing
Unit testing
Ported over 2 existing IAM-related request tests, and add a few more with null values and check it does not crash
Manual testing
Physical iPhone 13 on iOS 17.4
reproduced crashes reported by manually passing nil subscription ID to the OSRequestInAppMessageViewed and OSRequestInAppMessageClicked
does not crash after these changes, no request are made.
I was able to recreate this scenario "naturally" with this flow:
SDK has perviously fetched IAMs so there are IAMs in the cache
Kill app and then delete the user via REST API
Manipulate the SDK code so that Create User requests cannot be sent
Open app, and notice that before IAMs are fetched, we actually evaluate IAMs on onApplicationDidBecomeActive, which at this point subscription ID still exists locally so the shouldShowInAppMessage check passes
Then the SDK fetches user to refresh and finds the user is gone, nullifies the local subscription ID, and enqueues a Create User request (that I prevent from being sent).
The IAM displays in a context with null subscription ID.
Affected code checklist
[ ] Notifications
[ ] Display
[ ] Open
[ ] Push Processing
[ ] Confirm Deliveries
[ ] Outcomes
[ ] Sessions
[ ] In-App Messaging
[ ] REST API requests
[ ] Public API changes
Checklist
Overview
[x] I have filled out all REQUIRED sections above
[x] PR does one thing
[x] Any Public API changes are explained in the PR details and conform to existing APIs
Testing
[x] I have included test coverage for these changes, or explained why they are not needed
[x] All automated tests pass, or I explained why that is not possible
[x] I have personally tested this on my device, or explained why that is not possible
Final pass
[x] Code is as readable as possible.
[x] I have reviewed this PR myself, ensuring it meets each checklist item
Description
One Line Summary
Make null values safe for IAM payload such as subscription ID (most common) or other data required in the payload is
nil
.Details
nil
, the app crashes when requests for IAM impressions or clicks are created.[__NSPlaceholderDictionary initWithObjects:forKeys:count:]: attempt to insert nil object from objects[1]
""
and receives a 400 response.How could this happen?
null
subscription ID, but to get IAMs, we need a subscription IDshouldShowInAppMessage
, there is an additional check for subscription ID before presenting an IAM. No IAM should be shown when subscription ID isnull
.Motivation
Fix crashes, reported in #1453 and others. The crash reports all look like they are for player ID.
Scope
Correctly handle and pass along null values.
Testing
Unit testing
Ported over 2 existing IAM-related request tests, and add a few more with null values and check it does not crash
Manual testing
Physical iPhone 13 on iOS 17.4
nil
subscription ID to theOSRequestInAppMessageViewed
andOSRequestInAppMessageClicked
I was able to recreate this scenario "naturally" with this flow:
shouldShowInAppMessage
check passesAffected code checklist
Checklist
Overview
Testing
Final pass
This change is