Fix crash when perform is called on a context with deprecated concurrency type

[dvdchr]

Refs pcdRpT-1i3-p2#comment-2354

This fixes a crash that happens during the database export process after previously saving a Reader post. Here are some findings during my investigation:

• Saving a post updates a property on the ReaderPost managed object, and triggers the didSave method that updates another model, ReaderCard. The export process makes use of NSPersistentStoreCoordinator's migratePersistentStore(_:to:options:withType:) method¹.

• While reading through the stack trace, it seems that the migratePersistentStoremethod creates its own NSManagedObjectContext and calls save. This call somehow triggers ReaderPost's didSave method on the saved post. Note that I've tried to modify the save post method such that it's persisted in the store before the export process is initiated (i.e., the post's isUpdated is false), but the didSave still gets called during migratePersistentStore.

  📜 Click to see the a snippet of the stack trace

  Terminating app due to uncaught exception 'NSInvalidArgumentException', reason: 'Can only use -performBlock: on an NSManagedObjectContext that was created with a queue'
  
  Last Exception Backtrace:
  0   CoreFoundation                         0x1d8795e88 __exceptionPreprocess + 164
  1   libobjc.A.dylib                        0x1d1ac38d8 objc_exception_throw + 60
  2   CoreData                               0x1dfff8914 -[NSSQLiteAdapter newCreateTableStatementForEntity:] + 0
  3   WordPress                              0x10585e030 ContainerContextFactory.save(_:andWait:withCompletionBlock:) + 732
  4   WordPress                              0x10585e668 @objc ContainerContextFactory.save(_:andWait:withCompletionBlock:) + 244
  5   WordPress                              0x104ae5aec -[ContextManager saveContext:] + 116
  6   WordPress                              0x104c03840 -[ReaderPost didSave] + 488
  7   CoreData                               0x1dffcde74 -[NSManagedObjectContext _didSaveChanges] + 1664
  8   CoreData                               0x1dfff7308 -[NSManagedObjectContext save:] + 3024
  9   CoreData                               0x1e00feb04 __84-[NSPersistentStoreCoordinator migratePersistentStore:toURL:options:withType:error:]_block_invoke.492 + 2072
  10  CoreData                               0x1e0048840 gutsOfBlockToNSPersistentStoreCoordinatorPerform + 204
  11  libdispatch.dylib                      0x1dfd65fdc _dispatch_client_callout + 20
  12  libdispatch.dylib                      0x1dfd75574 _dispatch_lane_barrier_sync_invoke_and_complete + 56
  13  CoreData                               0x1dffa1bb0 -[NSPersistentStoreCoordinator performBlockAndWait:] + 184
  14  CoreData                               0x1e00fddd0 __84-[NSPersistentStoreCoordinator migratePersistentStore:toURL:options:withType:error:]_block_invoke + 916
  

• Within the ReaderPost's didSave, there's another save call to the managed object context that ends up in ContainerContextFactory.

  WordPress-iOS/WordPress/Classes/Models/ReaderPost.m

  Lines 513 to 522 in be75d66

  	- (void) didSave {
  	[super didSave];
  	// A ReaderCard can have either a post, or a list of topics, but not both.
  	// Since this card has a post, we can confidently set `topics` to NULL.
  	if ([self respondsToSelector:@selector(card)] && self.card.count > 0) {
  	self.card.allObjects[0].topics = NULL;
  	[[ContextManager sharedInstance] saveContext:self.managedObjectContext];
  	}
  	}

• Finally: in the ContainerContextFactory's save method, the completion block is wrapped in a performBlock:

  WordPress-iOS/WordPress/Classes/Utility/ContainerContextFactory.swift

  Lines 30 to 34 in be75d66

  	if wait {
  	context.performAndWait(block)
  	} else {
  	context.perform(block)
  	}

  And this is where the issue is. The NSManagedObjectContext that's internally created by the migratePersistentStore method is initialized with .confinementConcurrencyType — which, apparently, does NOT have a queue (as seen on the stack trace above). So, the solution is to wrap the block with perform ONLY when the context has the correct concurrency types.

Note: I'm adding @crazytonyli as a reviewer since this touches the core part of our Core Data setup. That said, I'm quite sure this is safe² since nowhere in our codebase creates NSManagedObjectContext with the deprecated concurrency type.

To test

Verify that the crash issue is fixed

• Install and run WordPress.
• Log in using any dotcom account.
• Ensure that the contentMigration flag is enabled.
• Go to Reader > Discover.
• Save a post or two that has tags/topics on them.
• Go to Notifications tab.
• Tap on the Jetpack-powered banner, and tap on the "Try the new Jetpack app" button.
• 🔍 Verify that the app does not crash.
  • Note: Depending on whether you have the Jetpack app installed, it should either open the Jetpack app or redirect you to the Jetpack page in App Store. but that's out of the scope of this PR.

Verify that everywhere else is business as usual

Do a light smoke test to ensure that everything works as before and is not impacted by this change:

• Create a post.
• Add a self-hosted site.
• Like a post.
• Write a comment.
• Edit an existing post.
• Moderate a comment.
• Modify selected Topics in Reader settings.
• Save a post.
• View Notifications.
• Like a comment from Notifications.

Regression Notes

1. Potential unintended areas of impact
   There may be some unexpected thread-related issues? But this should be very very unlikely since there's nothing from our side that creates an NSManagedObjectContext with the now deprecated .confinedConcurrencyType.

2. What I did to test those areas of impact (or what existing automated tests I relied on)
   Manually tested the changes.

3. What automated tests I added (or what prevented me from doing so)
   N/A.

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.

Footnotes

1. The method is deprecated, but the recommended method is only available on iOS 15.0+. We are still supporting iOS 14. ↩

2. Famous last words. 🙃 ↩

[peril-wordpress-mobile]

	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

[dvdchr]

I can do if context.concurrencyType == .confinementConcurrencyType {, but Xcode will throw out a warning if we type a deprecated enum value. 🙃

[wpmobilebot]

You can test the changes in WordPress from this Pull Request by:

• Clicking here or scanning the QR code below to access App Center
• Then installing the build number pr19765-0caa881 on your iPhone

If you need access to App Center, please ask a maintainer to add you.

[wpmobilebot]

You can test the changes in Jetpack from this Pull Request by:

• Clicking here or scanning the QR code below to access App Center
• Then installing the build number pr19765-0caa881 on your iPhone

If you need access to App Center, please ask a maintainer to add you.

[wargcm]

This appears to work fine but I feel like we should be changing the ReaderPost's didSave instead. It's a little strange to me to be double saving an object.

After playing around with it a bit, it seems like we can delete the didSave and then update this section to:

Updated code

    [self.managedObjectContext performBlockAndWait:^{

        // Get a the post in our own context
        NSError *error;
        ReaderPost *readerPost = (ReaderPost *)[self.managedObjectContext existingObjectWithID:post.objectID error:&error];
        if (error) {
            dispatch_async(dispatch_get_main_queue(), ^{
                if (failure) {
                    failure(error);
                }
            });
            return;
        }

        readerPost.isSavedForLater = !readerPost.isSavedForLater;
        if ([readerPost respondsToSelector:@selector(card)] && readerPost.card.count > 0) {
            readerPost.card.allObjects[0].topics = NULL;
        }
        
        [[ContextManager sharedInstance] saveContext:self.managedObjectContext];

        success();
    }];

The main change being performBlock → performBlockAndWait. There was a race condition with the UI. It'd toggle the flag and then immediately try updating the UI without waiting for the object to save. I also re-added the code from the didSave here but I'm not really sure if that's doing much. The original PR (#17117) that added the didSave appears to be trying to fix the UI state.

[dvdchr]

Thanks for the review, @wargcm ! 👍🏼

it seems like we can delete the didSave
...
There was a race condition with the UI. It'd toggle the flag and then immediately try updating the UI without waiting for the object to save. I also re-added the code from the didSave here but I'm not really sure if that's doing much.

Good point. The crash no longer happens after the didSave hook is removed from that model. I've also tried performBlockAndWait previously, but it was still crashing for me because I didn't remove delete the code in didSave.

we should be changing the ReaderPost's didSave instead. It's a little strange to me to be double saving an object.

Good observation, and your suggestion makes sense 👍🏼 . I've thought of doing this instead of modifying the logic in ContainerContextFactory, but seeing that it's placed in the didSave method, the scope is wider than saving a post. However, the original PR #17117 does look very specific to resolving UI issues surrounding saving posts, as you've said.

That said, the root cause for the crash was not because we're performing a double save¹; Rather, we're calling a method unsupported by the context's concurrency type. Assume we have another scenario that unknowingly leads to an NSManagedObjectContext with the deprecated concurrency type, and save is called; This will end up going through ContainerContextFactory and crash anyway because the method either calls perform or performAndWait. This fix will act as a safeguard for that.

If my assumption is correct, this change would act as an additional safeguard instead of introducing side effects, and I'd prefer going ahead with this for 21.4. However, I also think that the double save part is a technical debt that needs to be addressed, so I'm thinking to open a separate issue and PR for it. What do you think?

Footnotes

1. Although, I agree with you. That bit feels like a hacky solution that should be fixed. ↩

[wargcm]

If my assumption is correct, this change would act as an additional safeguard instead of introducing side effects, and I'd prefer going ahead with this for 21.4. However, I also think that the double save part is a technical debt that needs to be addressed, so I'm thinking to open a separate issue and PR for it. What do you think?

Yep, that works! It's a good idea to safeguard this in case there are other areas that aren't as obvious as this is. I'm also not sure about the side effects of moving the topics being set to NULL so it'd be a good idea to look at it a bit more.

[crazytonyli]

TIL about confinementConcurrencyType. Thanks for making the change!

I agree with @wargcm , this saving changes code in a didSave function looks a bit strange.

One small question: do you think it's be possible to add an unit test to cover this crash?

[dvdchr]

I've added tests for ContainerContextFactory in 0caa881 and have enabled auto-merge. Thank you both for reviewing! 👍🏼