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

Remove the Core Data workaround in EditorMediaUtility #21227

Merged
merged 2 commits into from
Aug 7, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions RELEASE-NOTES.txt
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
* [*] Fix occasional crashes when updating Notification, Posts, and Reader content [#21250]
* [*] Fix an issue in Reader topics cleanup that could cause the app to crash. [#21243]
* [*] [internal] Fix incorrectly terminated background task [#21254]
* [**] [internal] Refactor how image is downloaded in Gutenberg Editor and Aztec Edtiror. [#21227]

22.9
-----
Expand Down
7 changes: 7 additions & 0 deletions WordPress/Classes/Networking/MediaHost+Blog.swift
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,13 @@ extension MediaHost {
case baseInitializerError(error: Error, blog: Blog)
}

init(with blog: Blog) {
self.init(with: blog) { error in
// We'll log the error, so we know it's there, but we won't halt execution.
WordPressAppDelegate.crashLogging?.logError(error)
}
}

init(with blog: Blog, failure: (BlogError) -> ()) {
Comment on lines +11 to 18
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you considered using argument default for this over a dedicated init?

E.g.

init(
    with blog: Blog,
    failure: (BlogError) -> () = { error in
        // ...
    }
) {

Granted this approach assumes that the logging mechanism will remain a singleton shared instance, which might not be desirable in the future.

let isAtomic = blog.isAtomic()
self.init(with: blog, isAtomic: isAtomic, failure: failure)
Expand Down
2 changes: 1 addition & 1 deletion WordPress/Classes/Services/MediaThumbnailService.swift
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ class MediaThumbnailService: NSObject {
return
}

let download = AuthenticatedImageDownload(url: imageURL, blogObjectID: media.blog.objectID, callbackQueue: callbackQueue, onSuccess: onCompletion, onFailure: onError)
let download = AuthenticatedImageDownload(url: imageURL, mediaHost: MediaHost(with: media.blog), callbackQueue: callbackQueue, onSuccess: onCompletion, onFailure: onError)

download.start()
}
Expand Down
124 changes: 66 additions & 58 deletions WordPress/Classes/ViewRelated/Gutenberg/EditorMediaUtility.swift
Original file line number Diff line number Diff line change
Expand Up @@ -8,66 +8,24 @@ final class AuthenticatedImageDownload: AsyncOperation {
}

let url: URL
let blogObjectID: NSManagedObjectID
let mediaHost: MediaHost
Copy link
Contributor

Choose a reason for hiding this comment

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

From the PR description:

AuthenticatedImageDownload no long access NSManagedObject. It used to have code to query Blog. The code is now moved to its caller.

Just wanted to +1 this decision. Aside from the hunch that this will help with #20630, having the image download operation know about MediaHost over Blog seems like a better design because the surface of information it can access (thus that we need to be aware of when working with it) is now much smaller.

private let callbackQueue: DispatchQueue
private let onSuccess: (UIImage) -> ()
private let onFailure: (Error) -> ()

init(url: URL, blogObjectID: NSManagedObjectID, callbackQueue: DispatchQueue, onSuccess: @escaping (UIImage) -> (), onFailure: @escaping (Error) -> ()) {
assert(!blogObjectID.isTemporaryID)
init(url: URL, mediaHost: MediaHost, callbackQueue: DispatchQueue, onSuccess: @escaping (UIImage) -> (), onFailure: @escaping (Error) -> ()) {
self.url = url
self.blogObjectID = blogObjectID
self.mediaHost = mediaHost
self.callbackQueue = callbackQueue
self.onSuccess = onSuccess
self.onFailure = onFailure
}

override func main() {
let result = ContextManager.shared.performQuery { context in
Result {
// Can't fetch the blog using a temporary ID. This check is added as an attempt to prevent this crash:
// https://github.com/wordpress-mobile/WordPress-iOS/issues/20630
guard !self.blogObjectID.isTemporaryID else {
throw DownloadError.blogNotFound
}

var blog: Result<Blog, Error> = .failure(DownloadError.blogNotFound)
do {
// Catch an Objective-C `NSInvalidArgumentException` exception from `existingObject(with:)`.
// See https://github.com/wordpress-mobile/WordPress-iOS/issues/20630
try WPException.objcTry {
blog = Result {
try context.existingObject(with: self.blogObjectID) as! Blog
}
}
} catch {
// Send Objective-C exceptions to Sentry for further diagnosis.
WordPressAppDelegate.crashLogging?.logError(error)
throw error
}

return try MediaHost(with: blog.get()) { error in
// We'll log the error, so we know it's there, but we won't halt execution.
WordPressAppDelegate.crashLogging?.logError(error)
}
}
}

let host: MediaHost
do {
host = try result.get()
} catch {
self.state = .isFinished
self.callbackQueue.async {
self.onFailure(error)
}
return
}

let mediaRequestAuthenticator = MediaRequestAuthenticator()
mediaRequestAuthenticator.authenticatedRequest(
for: url,
from: host,
from: mediaHost,
onComplete: { request in
ImageDownloader.shared.downloadImage(for: request) { (image, error) in
self.state = .isFinished
Expand Down Expand Up @@ -165,11 +123,65 @@ class EditorMediaUtility {
success: @escaping (UIImage) -> Void,
onFailure failure: @escaping (Error) -> Void
) -> ImageDownloaderTask {
let postObjectID = post.objectID
let result = ContextManager.shared.performQuery { context in
Result {
try EditorMediaUtility.prepareForDownloading(url: url, size: requestSize, scale: scale, postObjectID: postObjectID, in: context)
}
}

let callbackQueue = DispatchQueue.main
switch result {
case let .failure(error):
callbackQueue.async {
failure(error)
}
return EmptyImageDownloaderTask()
case let .success((requestURL, mediaHost)):
let imageDownload = AuthenticatedImageDownload(
url: requestURL,
mediaHost: mediaHost,
callbackQueue: callbackQueue,
onSuccess: success,
onFailure: failure
)
imageDownload.start()
return imageDownload
}
}

private static func prepareForDownloading(
url: URL,
size requestSize: CGSize,
scale: CGFloat,
postObjectID: NSManagedObjectID,
in context: NSManagedObjectContext
) throws -> (URL, MediaHost) {
// This function is added to debug the issue linked below.
let safeExistingObject: (NSManagedObjectID) throws -> NSManagedObject = { objectID in
var object: Result<NSManagedObject, Error> = .failure(AuthenticatedImageDownload.DownloadError.blogNotFound)
do {
// Catch an Objective-C `NSInvalidArgumentException` exception from `existingObject(with:)`.
// See https://github.com/wordpress-mobile/WordPress-iOS/issues/20630
try WPException.objcTry {
object = Result {
try context.existingObject(with: objectID)
}
}
} catch {
// Send Objective-C exceptions to Sentry for further diagnosis.
WordPressAppDelegate.crashLogging?.logError(error)
throw error
}

return try object.get()
}

let post = try safeExistingObject(postObjectID) as! Post
Comment on lines +160 to +180
Copy link
Contributor

Choose a reason for hiding this comment

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

This is neat.

Hopefully we won't have to use it again, but just in case, I wonder how we could make it more discoverable / flexible so other people can reuse it? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I really hope that this PR fixes the issue and makes the extra Objective-C exception become unnecessary. That means we can delete this ugly try catch.

I don't want to move this into a shared places because I don't think we should catch the NSInvalidArgument exception, which to me feels like an indication of programming error, rather than Core Data data accessing error.


let imageMaxDimension = max(requestSize.width, requestSize.height)
//use height zero to maintain the aspect ratio when fetching
var size = CGSize(width: imageMaxDimension, height: 0)
let (requestURL, blogObjectID) = workaroundCoreDataConcurrencyIssue(accessing: post) {
let requestURL: URL
if url.isFileURL {
requestURL = url
Expand All @@ -185,18 +197,8 @@ class EditorMediaUtility {
// the size that PhotonImageURLHelper expects is points size
requestURL = PhotonImageURLHelper.photonURL(with: size, forImageURL: url)
}
return (requestURL, post.blog.objectID)
}

let imageDownload = AuthenticatedImageDownload(
url: requestURL,
blogObjectID: blogObjectID,
callbackQueue: .main,
onSuccess: success,
onFailure: failure)

imageDownload.start()
return imageDownload
return (requestURL, MediaHost(with: post.blog))
}

static func fetchRemoteVideoURL(for media: Media, in post: AbstractPost, withToken: Bool = false, completion: @escaping ( Result<(URL), Error> ) -> Void) {
Expand Down Expand Up @@ -247,3 +249,9 @@ class EditorMediaUtility {
})
}
}

private class EmptyImageDownloaderTask: ImageDownloaderTask {
func cancel() {
// Do nothing
}
}