Catch Objective-C exceptions from Core Data object query

[crazytonyli]

Another attempt in fixing #20630.

I'm still not sure what's the root cause of the nil is not a valid object ID error. But catching the Objective-C exception should prevent the app from crashing.

Test Instructions

Open a few posts that have images from the post editor. Make sure images can be loaded.

Regression Notes

1. Potential unintended areas of impact
   None. The refactored loadObject function wasn't used anywhere, except the unit tests.

2. What I did to test those areas of impact (or what existing automated tests I relied on)
   What says in the "Test Instructions"

3. What automated tests I added (or what prevented me from doing so)
   The existing ones have been updated.

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.
• [ ]x I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

UI Changes testing checklist: N/A
Portrait and landscape orientations.
Light and dark modes.
Fonts: Larger, smaller and bold text.
High contrast.
VoiceOver.
Languages with large words or with letters/accents not frequently used in English.
Right-to-left languages. (Even if translation isn’t complete, formatting should still respect the right-to-left layout)
iPhone and iPad.
Multi-tasking: Split view and Slide over. (iPad)

[wpmobilebot]

📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.

	App Name	Jetpack Alpha
	Configuration	Release-Alpha
	Build Number	pr20755-5672b0d
	Version	22.5
	Bundle ID	com.jetpack.alpha
	Commit	5672b0d
	App Center Build	jetpack-installable-builds #4842

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

[wpmobilebot]

📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.

	App Name	WordPress Alpha
	Configuration	Release-Alpha
	Build Number	pr20755-5672b0d
	Version	22.5
	Bundle ID	org.wordpress.alpha
	Commit	5672b0d
	App Center Build	WPiOS - One-Offs #5811

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

[jkmassel]

I'm not sure I agree with the premise of this PR – IMHO it's preferable for the app to crash rather than give the mistaken impression that data was saved. If the app crashes, the user will validate that their data is safe. If it doesn't, they may discover later that it's missing, which is worse.

[crazytonyli]

I had a chat with Jeremy privately. Here is a recap:

The crash was introduced in this commit, where we changed accessing a Blog directly (without making sure the code is in the right queue) to querying the instance safely from a context object using the Blog.objectID.

The root cause of the crash is not caused by data not being saved. Because if that's indeed the case, existingObject(with:) would throw a Swift error which will be caught by existing error handling code.

As I mentioned in #20630 (comment), the crash reason "nil is not an object ID" can't be referencing the blogObjectID: NSManagedObjectID which is declared as non-optional.

The very first attempt of fixing this crash is checking if the object id is a temporary object ID in #20633. Unfortunately that didn't work.

However, I should add some code to send debugging logs to Sentry, so that we can gather some data around this NSInvalidArgumentException.

[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

[crazytonyli]

@jkmassel I have changed the approach to capture the exception at the crash site, instead of changing an generic "loadObject" function (which was done in 0a1829d).

[mokagio]

Approving to unblock, but I'd like to hear what Jeremy thinks because of the conversations here and offline between the two of you.

[mokagio]

Is this default value necessary? I don't see a code path where it's used, since if a try in the do-catch throws, the method rethrows.

[crazytonyli]

It's necessary in terms of making the code look nicer, but also kind of not necessary because the default value will never get used.

The alternative is, we don't give it a default value, then the blog variable needs to be defined as Optional. That means at line 49 where blog.get() is called to get the Blog instance, we need to unwrap the optional blog. Now, if the code ever reaches line 49 at runtime, the blog will never be nil, because no Objective-C exception was thrown and the blog must be a Result instance that wraps a blog object or a Swift Error.

Considering the alternative is unnecessarily unwrapping an Optional, I decided to just give it a default value, which is never going to be used.

[mokagio]

Gotcha. I was hoping we could do something like

                let x = true
                var foo: Int
                if x {
                    foo = 2
                } else {
                    foo = 3
                }

Alas, the construct work in the context of an if else but not in a closure

[crazytonyli]

@mokagio FYI, I've cherry-picked the UI test fix in #20752 to this PR. I checked the release/22.5 branch and don't think the fix commit was there.

[mokagio]

@crazytonyli good point! I cherry picked in what I thought was going to be my fix PR, #20751, but didn't progress on it because Sergiy's work addressed the issue.