Fix Core Data crash related to media errors #17759

twstokes · 2022-01-13T23:01:41Z

This PR aims to fix a Core Data crash when persisting Media errors due to underlying types in UserInfo that don't adopt NSSecureCoding.

The solution proposed here is to strip all but explicitly defined keys in the setter of Media.error.

Overview example:

An NSError is thrown by iOS that there's no Internet connectivity. Its UserInfo dictionary contains an NSUnderlyingError which contains an object whose class doesn't conform to NSSecureCoding. It also contains other standard keys such as NSLocalizedDescription, and NSErrorFailingURLKey.

Before this change:

When saving the context, Core Data would throw an exception once it reached the object that couldn't be encoded, because it didn't adopt NSSecureCoding.

After this change:

When saving the context, Core Data only encodes NSLocalizedDescription with no error. The persisted error is a stripped down encodable version of the original.

Fix UserInfo Crash #17488

To test:

On a self-hosted site not connected to Jetpack:

Create a new Post
Add an Image Block
Put the device in Airplane Mode
Tap "Add Image"
Choose a photo from the device or the camera
Observe that the image is added to the post with an overlay "Failed to insert media. Please tap for options."
Observe that when tapping on the overlay, a bottom sheet appears that reads "The Internet connection appears to be offline."
Tapping "Retry" does not crash the app.
Disabling Airplane Mode, tapping the image, and tapping "Retry" successfully uploads the image.

On a WordPress.com site:

Create a new Post
Add an Image Block
Put the device in Airplane Mode
Tap "Add Image"
Choose a photo from the device or the camera
Observe that the image is added to the post with an overlay "Failed to insert media. Please tap for options."
Observe that when tapping on the overlay, a bottom sheet appears that reads "The Internet connection appears to be offline."
Tapping "Retry" does not crash the app.
Disabling Airplane Mode, tapping the image, and tapping "Retry" successfully uploads the image.

Other context

Exact error that would be produced:

CoreData: error: SQLCore dispatchRequest: exception handling request: <NSSQLSaveChangesRequestContext: 0x151fc5b90> , <shared NSSecureUnarchiveFromData transformer> threw while encoding a value. with userInfo of {
    NSUnderlyingError = "Error Domain=NSCocoaErrorDomain Code=4866 \"The data couldn\U2019t be written because it isn\U2019t in the correct format.\" UserInfo={NSUnderlyingError=0x155c23f60 {Error Domain=NSCocoaErrorDomain Code=4864 \"This decoder will only decode classes that adopt NSSecureCoding. Class 'NWConcrete_nw_path' does not adopt it.\" UserInfo={NSDebugDescription=This decoder will only decode classes that adopt NSSecureCoding. Class 'NWConcrete_nw_path' does not adopt it.}}}";
}

Regression Notes

Potential unintended areas of impact

Different error scenarios:
- Uploading media to a self-hosted site that's low on storage
- Uploading media that's too large to a self-hosted site
- Uploading media to a WP.com / Jetpack connected site in Airplane mode

Because we're only cherrypicking one field from the error now (NSLocalizedDescription), there's a possibility other fields were used somewhere else in the app.

What I did to test those areas of impact (or what existing automated tests I relied on)
The testing steps above. I relied on the Core Data migration tests from version 103 to 104 of the data model.
What automated tests I added (or what prevented me from doing so)
I updated the Core Data migration tests to test that keys that aren't explicitly defined in the setter of Media.error aren't included.

PR submission checklist:

I have completed the Regression Notes.
I have considered adding unit tests for my changes.
I have considered adding accessibility improvements for my changes.
I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

peril-wordpress-mobile · 2022-01-13T23:01:46Z

You can trigger an installable build for these changes by visiting CircleCI here.

ttahmouch

The changes look good to me. I'm not sure if there is an easy way to test the sad path nor if it would be useful for these edge cases, but we probably should if it's not a large undertaking?

peril-wordpress-mobile · 2022-01-13T23:23:20Z

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

twstokes · 2022-01-18T03:27:30Z

The changes look good to me. I'm not sure if there is an easy way to test the sad path nor if it would be useful for these edge cases, but we probably should if it's not a large undertaking?

Thanks @ttahmouch! I'm going to take a little more time on this PR because the scope of how this crash can be triggered is potentially larger than just what I've identified here.

ttahmouch · 2022-01-20T04:29:45Z

WordPress/WordPressTest/CoreDataMigrationTests.m

@@ -1322,7 +1322,7 @@ - (void)testMigrationFrom103To104CustomTransformers
    // Media has an Error transformer
    Media *media1 = (Media *)[NSEntityDescription insertNewObjectForEntityForName:[Media entityName] inManagedObjectContext:context];
    media1.blog = blog1;
-    NSError *error1 = [NSError errorWithDomain:NSURLErrorDomain code:100 userInfo:@{ @"reason": @"test" }];
+    NSError *error1 = [NSError errorWithDomain:NSURLErrorDomain code:100 userInfo:@{ NSLocalizedDescriptionKey: @"test" }];


twstokes · 2022-01-20T16:56:33Z

WordPress/Classes/Services/MediaService.m

    if (customErrorMessage) {
-        NSMutableDictionary *userInfo = [[NSMutableDictionary alloc] init];
+        NSMutableDictionary *userInfo = [error.userInfo mutableCopy];


This reverts the fix introduced here in favor of passing all UserInfo values up to the new setter, which could cherry pick more if needed in the future.

twstokes · 2022-01-20T17:26:55Z

WordPress/Classes/Models/Media.m

+        error = [[NSError alloc] initWithDomain:value.domain code:value.code userInfo:userInfo];
+    }
+
+    [self willChangeValueForKey:@"error"];


The setter boilerplate was generated from Xcode, so it makes me feel better about its legitimacy. 😄

twstokes · 2022-01-20T17:38:05Z

@frosty, would you mind giving this PR a look over since you'd worked on the transformers part? I'm totally open to a more optimal solution than the setter method if there are better ideas. 🙇

ttahmouch

The changes look good to me overall.

I'm just not positive replacing the originally emitted NSError is the best approach because I don't know if the state that was being encoded and decoded conforming to NSSecureCoding was needed at all, or if it could have simply been ignored if non-conformant.

WordPress/Classes/Services/MediaService.m

WordPress/Classes/Models/Media.m

peril-wordpress-mobile · 2022-01-20T18:48:24Z

	Warnings
⚠️	This PR is assigned to a milestone which is closing in less than 4 days Please, make sure to get it merged by then or assign it to a later expiring milestone

Generated by 🚫 dangerJS

twstokes · 2022-01-20T18:56:22Z

I'm just not positive replacing the originally emitted NSError is the best approach because I don't know if the state that was being encoded and decoded conforming to NSSecureCoding was needed at all, or if it could have simply been ignored if non-conformant.

I'm not 100% sure on this myself but it may be a requirement to conform now. #15167 has some information where warnings appeared starting with Xcode 12.

frosty

The code changes look good to me, and I can't think of any other places we use the errors other than those you've identified.

However, when testing I followed the steps on both trunk and this branch, and I was unable to cause the original crash to happen on trunk so I can't 100% verify that these changes have made a difference there!

twstokes · 2022-01-21T14:41:35Z

However, when testing I followed the steps on both trunk and this branch, and I was unable to cause the original crash to happen on trunk so I can't 100% verify that these changes have made a difference there!

Thanks @frosty! I'll double-check this to make sure nothing has changed since the PR started.

twstokes · 2022-01-21T16:19:52Z

I just wanted to follow up that I was still able to reproduce the crash in trunk on a self-hosted site that didn't have Jetpack connected. It's expected that .com sites won't crash due to the previous fix.

Thanks to both of you for the reviews. 🙇

ttahmouch reviewed Jan 13, 2022

View reviewed changes

twstokes added the Core Data Issues related to Core Data label Jan 14, 2022

twstokes force-pushed the fix/media-error-crash branch from 68e6289 to 917cadf Compare January 14, 2022 18:54

ttahmouch mentioned this pull request Jan 14, 2022

Release 1.69.0 wordpress-mobile/gutenberg-mobile#4438

Merged

4 tasks

twstokes added the [Type] Crash label Jan 14, 2022

twstokes force-pushed the fix/media-error-crash branch from 917cadf to 33eb87c Compare January 20, 2022 02:57

ttahmouch reviewed Jan 20, 2022

View reviewed changes

twstokes commented Jan 20, 2022

View reviewed changes

twstokes added this to the 19.1 milestone Jan 20, 2022

twstokes commented Jan 20, 2022

View reviewed changes

twstokes added 3 commits January 20, 2022 12:34

Apply a setter for media errors to prevent Core Data crashes.

f805a86

Update migration tests to account for new Media.error setter.

dd4728e

Add release notes.

873aaf8

twstokes force-pushed the fix/media-error-crash branch from 85f783d to 873aaf8 Compare January 20, 2022 17:34

twstokes marked this pull request as ready for review January 20, 2022 17:35

twstokes requested review from ttahmouch and frosty January 20, 2022 17:35

ttahmouch approved these changes Jan 20, 2022

View reviewed changes

WordPress/Classes/Services/MediaService.m Outdated Show resolved Hide resolved

WordPress/Classes/Services/MediaService.m Show resolved Hide resolved

WordPress/Classes/Models/Media.m Outdated Show resolved Hide resolved

WordPress/Classes/Models/Media.m Outdated Show resolved Hide resolved

twstokes added 2 commits January 20, 2022 13:33

Clean up setter for Media error.

d81f54d

Use cleaner init style for NSError.

f614c18

frosty approved these changes Jan 21, 2022

View reviewed changes

twstokes merged commit 8eda448 into trunk Jan 21, 2022

twstokes deleted the fix/media-error-crash branch January 21, 2022 16:20

twstokes self-assigned this Jan 21, 2022

ttahmouch mentioned this pull request Jan 21, 2022

Retrying failed media uploads without Internet connectivity leads to Core Data exception #17802

Closed

ttahmouch linked an issue Jan 21, 2022 that may be closed by this pull request

Retrying failed media uploads without Internet connectivity leads to Core Data exception #17802

Closed

momo-ozawa added Offline Mode (Sync Issues) and removed Offline Mode (Sync Issues) labels Apr 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Core Data crash related to media errors #17759

Fix Core Data crash related to media errors #17759

twstokes commented Jan 13, 2022 •

edited

Loading

peril-wordpress-mobile bot commented Jan 13, 2022 •

edited

Loading

ttahmouch left a comment

peril-wordpress-mobile bot commented Jan 13, 2022 •

edited

Loading

twstokes commented Jan 18, 2022

ttahmouch Jan 20, 2022

twstokes Jan 20, 2022

twstokes Jan 20, 2022

twstokes commented Jan 20, 2022

ttahmouch left a comment

peril-wordpress-mobile bot commented Jan 20, 2022

twstokes commented Jan 20, 2022

frosty left a comment

twstokes commented Jan 21, 2022

twstokes commented Jan 21, 2022

Fix Core Data crash related to media errors #17759

Fix Core Data crash related to media errors #17759

Conversation

twstokes commented Jan 13, 2022 • edited Loading

Overview example:

Other context

Regression Notes

peril-wordpress-mobile bot commented Jan 13, 2022 • edited Loading

ttahmouch left a comment

Choose a reason for hiding this comment

peril-wordpress-mobile bot commented Jan 13, 2022 • edited Loading

twstokes commented Jan 18, 2022

ttahmouch Jan 20, 2022

Choose a reason for hiding this comment

twstokes Jan 20, 2022

Choose a reason for hiding this comment

twstokes Jan 20, 2022

Choose a reason for hiding this comment

twstokes commented Jan 20, 2022

ttahmouch left a comment

Choose a reason for hiding this comment

peril-wordpress-mobile bot commented Jan 20, 2022

twstokes commented Jan 20, 2022

frosty left a comment

Choose a reason for hiding this comment

twstokes commented Jan 21, 2022

twstokes commented Jan 21, 2022

twstokes commented Jan 13, 2022 •

edited

Loading

peril-wordpress-mobile bot commented Jan 13, 2022 •

edited

Loading

peril-wordpress-mobile bot commented Jan 13, 2022 •

edited

Loading