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.

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

[Bug] Make null values safe for IAM requests #1457

Merged
merged 3 commits into from
Jul 1, 2024

Conversation

nan-li
Copy link
Contributor

@nan-li nan-li commented Jun 26, 2024

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

  • If subscription ID (most common) or other properties such as app ID are nil, the app crashes when requests for IAM impressions or clicks are created.
  • The app will crash with Exception [__NSPlaceholderDictionary initWithObjects:forKeys:count:]: attempt to insert nil object from objects[1]
  • If we omit these properties in the payload, the request will return a 400, which allows us to track these events
  • Note that Android already omit the property in the payload or pass an empty string "" and receives a 400 response.

How could this happen?

  • All the crash reports point to null subscription ID, but to get IAMs, we need a subscription ID
  • It is possible the user or subscription has been deleted?
  • When the Messaging Controller tries to get IAMs from the server but there is no subscription ID, we use IAMs from the cache
  • However, in shouldShowInAppMessage, there is an additional check for subscription ID before presenting an IAM. No IAM should be shown when subscription ID is null.
  • 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:

  1. SDK has perviously fetched IAMs so there are IAMs in the cache
  2. Kill app and then delete the user via REST API
  3. Manipulate the SDK code so that Create User requests cannot be sent
  4. 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
  5. 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).
  6. 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

  • I have filled out all REQUIRED sections above
  • PR does one thing
  • Any Public API changes are explained in the PR details and conform to existing APIs

Testing

  • I have included test coverage for these changes, or explained why they are not needed
  • All automated tests pass, or I explained why that is not possible
  • I have personally tested this on my device, or explained why that is not possible

Final pass

  • Code is as readable as possible.
  • I have reviewed this PR myself, ensuring it meets each checklist item

This change is Reviewable

Copy link
Member

@jkasten2 jkasten2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we instead still make the request even if we are missing fields? Doing so allows the backend to have data on how many requests are missing what data. Only logging to the device doesn't give us this insight.

@nan-li nan-li force-pushed the fix/iam_nil_subscription_id_crashes branch from 1910593 to 5f793b8 Compare June 27, 2024 23:03
* If subscription ID (most common) or other properties such as app ID are `nil`, the app crashes when requests for IAM impressions or clicks are created.
* The app will crash with Exception `[__NSPlaceholderDictionary initWithObjects:forKeys:count:]: attempt to insert nil object from objects[1]`
* Use a null-safe dictionary that will omit the entry if the value is null
* We choose to send these requests so we have record of 400-ing requests
* Add test module for In App Messages
* Add request tests for In App Messages including when some properties can be null
* And port over 2 existing IAM request tests
@nan-li nan-li force-pushed the fix/iam_nil_subscription_id_crashes branch from 5f793b8 to fcf9496 Compare June 27, 2024 23:21
@nan-li nan-li changed the title [Bug] Don't make IAM requests with no subscription ID [Bug] Make null values safe for IAM requests Jun 27, 2024
@nan-li nan-li requested a review from jkasten2 June 27, 2024 23:23
@nan-li nan-li requested a review from jkasten2 June 28, 2024 19:24
@nan-li nan-li merged commit ca4ef2a into main Jul 1, 2024
4 checks passed
@nan-li nan-li deleted the fix/iam_nil_subscription_id_crashes branch July 1, 2024 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants