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

Fix crashes when failing to download Notification attachments #1029

Merged
merged 1 commit into from
Dec 14, 2021

Conversation

emawby
Copy link
Contributor

@emawby emawby commented Dec 14, 2021

Fixes #586 and Unity crash 411

Description

One Line Summary

This PR prevents crashes due to failures when writing downloaded notification attachments (images) to a temporary file location.

Details

These crashes are not easily reproduce-able, but can happen in the following scenarios according to Apple

This method raises NSFileHandleOperationException if the file descriptor is closed or isn’t valid, if the handle represents an unconnected pipe or socket endpoint, if there isn’t any free space on the file system, or if any other writing error occurs.

For iOS 13+ devices we will now use writeData:error: which returns a boolean and populates the NSError parameter with the failure reason if it fails.

For < iOS 13 we will place a try catch around writeData: to avoid crashes.

In both cases we will log the failure reason. Note that this is logged on the NotificationServiceExtension process not the main app process.

Motivation

Prevent the crashes below
#586 and Unity crash 411

Testing

Manual testing

I tested on an iOS 15 device that uses the new writeData:error: API. However I was unable to reproduce a failure.

Affected code checklist

  • Notifications
    • Display
    • Open
    • Push Processing
    • Confirm Deliveries
  • Outcomes
  • Sessions
  • In-App Messaging
  • REST API requests
  • Public API changes
  • Distribution

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

For iOS 13+ devices we will now use writeData:error: which returns a boolean and populates the NSError parameter with the failure reason if it  fails.

For < iOS 13 we will place a try catch around writeData: to avoid crashes.

In both cases we will log the failure reason. Note that this is logged on the NotificationServiceExtension process not the main app process.
@emawby emawby force-pushed the fix/attachment_handler_crashes branch from fdd50d2 to 021f274 Compare December 14, 2021 18:02
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.

:lgtm:

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @nan-li)

@emawby emawby merged commit d975c61 into main Dec 14, 2021
@emawby emawby deleted the fix/attachment_handler_crashes branch December 14, 2021 19:03
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.

Same issue as in January 2018, Apple reported numerous extension crashes.
3 participants