Bugfix FXIOS-10149, FXIOS-10940 Migrate to Kingfisher 8 and fix iOS 18 crash on launch (continuations again?)

BrowserKit/Package.swift

@@ -51,7 +51,7 @@ let package = Package(
             branch: "master"),
         .package(
             url: "https://github.com/onevcat/Kingfisher.git",
-            exact: "7.12.0"),
+            exact: "8.1.3"),
         .package(
             url: "https://github.com/AliSoftware/Dip.git",
             exact: "7.1.1"),

BrowserKit/Sources/SiteImageView/ImageProcessing/SiteImageFetcher/DefaultSiteImageDownloader.swift

@@ -6,28 +6,12 @@ import Foundation
 import Kingfisher
 import Common
 
-class DefaultSiteImageDownloader: ImageDownloader, SiteImageDownloader {
-    var continuation: CheckedContinuation<SiteImageLoadingResult, Error>?
-    var timeoutDelay: UInt64 { return 10 }
+class DefaultSiteImageDownloader: ImageDownloader, @unchecked Sendable, SiteImageDownloader {
+    var timeoutDelay: Double { return 10 }
     var logger: Logger
 
     init(name: String = "default", logger: Logger = DefaultLogger.shared) {
         self.logger = logger
         super.init(name: name)
     }
-
-    func downloadImage(with url: URL,
-                       completionHandler: ((Result<SiteImageLoadingResult, Error>) -> Void)?
-    ) -> DownloadTask? {
-        return downloadImage(with: url,
-                             options: [.processor(SVGImageProcessor())],
-                             completionHandler: { result in
-            switch result {
-            case .success(let value):
-                completionHandler?(.success(value))
-            case .failure(let error):
-                completionHandler?(.failure(error))
-            }
-        })
-    }
 }

BrowserKit/Sources/SiteImageView/ImageProcessing/SiteImageFetcher/SiteImageDownloader.swift

@@ -11,72 +11,47 @@ import Common
 /// Image downloader wrapper around Kingfisher image downloader
 /// Used in FaviconFetcher
 protocol SiteImageDownloader: AnyObject {
-    /// Provides the KingFisher ImageDownloader with a Timeout in case the completion isn't called
-    var timeoutDelay: UInt64 { get }
-    var continuation: CheckedContinuation<any SiteImageLoadingResult, any Error>? { get set }
+    /// Specifies the timeout on image downloads
+    var timeoutDelay: Double { get }
     var logger: Logger { get }
 
-    @discardableResult
-    func downloadImage(
-        with url: URL,
-        completionHandler: ((Result<SiteImageLoadingResult, Error>) -> Void)?
-    ) -> DownloadTask?
-
     func downloadImage(with url: URL) async throws -> SiteImageLoadingResult
 }
 
 extension SiteImageDownloader {
+    /// Downloads an image at the given URL. Throws `SiteImageError` errors.
     func downloadImage(with url: URL) async throws -> SiteImageLoadingResult {
-        return try await withThrowingTaskGroup(of: SiteImageLoadingResult.self) { group in
-            // Use task groups to have a timeout when downloading an image from Kingfisher
-            // due to https://sentry.io/share/issue/951b878416374dd98eccb6fd88fd8427
-            group.addTask {
-                return try await self.handleImageDownload(url: url)
-            }
-
-            group.addTask {
-                try await self.handleTimeout()
-            }
-
-            // wait for the first task and cancel the other one
-            let result = try await group.next()
-            group.cancelAll()
-            guard let result = result else {
-                throw SiteImageError.unableToDownloadImage("Result not present")
-            }
-            return result
+        // Override Kingfisher's default timeout (which is 15s)
+        let modifier = AnyModifier { [weak self] request in
+            var modifiedRequest = request
+            modifiedRequest.timeoutInterval = self?.timeoutDelay ?? 15
+            return modifiedRequest
         }
-    }
-
-    private func handleImageDownload(url: URL) async throws -> any SiteImageLoadingResult {
-        return try await withCheckedThrowingContinuation { continuation in
-            // Store a copy of the continuation to act on in the case the sleep finishes first
-            self.continuation = continuation
 
-            _ = self.downloadImage(with: url) { result in
-                guard let continuation = self.continuation else { return }
-                switch result {
-                case .success(let imageResult):
-                    continuation.resume(returning: imageResult)
-                case .failure(let error):
-                    continuation.resume(throwing: error)
-                }
-                self.continuation = nil
+        do {
+            let result = try await ImageDownloader.default.downloadImage(
+                with: url,
+                options: [
+                    .processor(SVGImageProcessor()),
+                    .requestModifier(modifier)
+                ]
+            )
+            return result
+        } catch let error as KingfisherError {
+            // Log telemetry for Kingfisher timeout errors
+            if case .responseError(let reason) = error,
+               case .URLSessionError(let sessionError) = reason,
+               let error = sessionError as? URLError,
+               error.code == .timedOut {
+                self.logger.log("Timeout when downloading image reached",
+                                level: .warning,
+                                category: .images)
             }
-        }
-    }
-
-    private func handleTimeout() async throws -> any SiteImageLoadingResult {
-        try await Task.sleep(nanoseconds: self.timeoutDelay * NSEC_PER_SEC)
-        try Task.checkCancellation()
-        let error = SiteImageError.unableToDownloadImage("Timeout reached")
-        self.continuation?.resume(throwing: error)
-        self.continuation = nil
 
-        self.logger.log("Timeout when downloading image reached",
-                        level: .warning,
-                        category: .images)
-        throw error
+            throw SiteImageError.unableToDownloadImage(error.errorDescription ?? "No description")
+        } catch {
+            throw SiteImageError.unableToDownloadImage(error.localizedDescription)
+        }
     }
 }
 

BrowserKit/Tests/SiteImageViewTests/ImageProcessing/FaviconFetcherTests.swift

@@ -25,7 +25,7 @@ final class FaviconFetcherTests: XCTestCase {
         let subject = DefaultFaviconFetcher()
 
         do {
-            _ = try await subject.fetchFavicon(from: URL(string: "www.mozilla.com")!,
+            _ = try await subject.fetchFavicon(from: URL(string: "https://www.mozilla.org")!,
                                                imageDownloader: mockImageDownloader)
             XCTFail("Should have failed with error")
         } catch let error as SiteImageError {
@@ -49,42 +49,24 @@ final class FaviconFetcherTests: XCTestCase {
             XCTFail("Should have succeeded with image")
         }
     }
-
-    func testTimeout_completesWithoutImageOrError() async {
-        mockImageDownloader.timeoutDelay = 1
-        let subject = DefaultFaviconFetcher()
-
-        do {
-            _ = try await subject.fetchFavicon(from: URL(string: "www.mozilla.com")!,
-                                               imageDownloader: mockImageDownloader)
-            XCTFail("Should have failed with error")
-        } catch let error as SiteImageError {
-            XCTAssertEqual("Unable to download image with reason: Timeout reached", error.description)
-        } catch {
-            XCTFail("Should have failed with SiteImageError type")
-        }
-    }
 }
 
 // MARK: - MockSiteImageDownloader
 private class MockSiteImageDownloader: SiteImageDownloader {
     var logger: Logger = DefaultLogger.shared
-    var timeoutDelay: UInt64 = 10
-    var continuation: CheckedContinuation<SiteImageLoadingResult, Error>?
+    var timeoutDelay: Double = 10
 
     var image: UIImage?
     var error: KingfisherError?
 
-    func downloadImage(with url: URL,
-                       completionHandler: ((Result<SiteImageLoadingResult, Error>) -> Void)?
-    ) -> DownloadTask? {
-        if let error = error {
-            completionHandler?(.failure(error))
+    func downloadImage(with url: URL) async throws -> SiteImageLoadingResult {
+        if let error {
+            throw error
         } else if let image = image {
-            completionHandler?(.success(MockSiteImageLoadingResult(image: image)))
+            return MockSiteImageLoadingResult(image: image)
         }
 
-        return nil // not using download task
+        throw SiteImageError.unableToDownloadImage("Bad mock setup")
     }
 }
 

firefox-ios/Client.xcodeproj/project.pbxproj

@@ -26014,7 +26014,7 @@
 			repositoryURL = "https://github.com/onevcat/Kingfisher.git";
 			requirement = {
 				kind = exactVersion;
-				version = 7.12.0;
+				version = 8.1.3;
 			};
 		};
 		8AB30EC62B6C038600BD9A9B /* XCRemoteSwiftPackageReference "lottie-ios" */ = {

firefox-ios/Client.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved

@@ -68,8 +68,8 @@
       "kind" : "remoteSourceControl",
       "location" : "https://github.com/onevcat/Kingfisher.git",
       "state" : {
-        "revision" : "2ef543ee21d63734e1c004ad6c870255e8716c50",
-        "version" : "7.12.0"
+        "revision" : "e6749919f9761d573d37a7d7e78b1b854c191d7f",
+        "version" : "8.1.3"
       }
     },
     {