Bugfix FXIOS-9830 Do not duplicate favicon URL and image requests on UITableViewCell reload [Favicon Refactor]

BrowserKit/Sources/SiteImageView/FaviconURLProcessing/FaviconURLHandler.swift

@@ -20,6 +20,9 @@ struct DefaultFaviconURLHandler: FaviconURLHandler {
         self.urlCache = urlCache
     }
 
+    /// Attempts to get the favicon URL associated with this site. First checks the URL cache. If the URL can't be obtained
+    /// from the cache, then a network request is made to hopefully scrape the favicon URL from a webpage's metadata.
+    /// **Note**: This is a slow call when the URL is not cached.
     func getFaviconURL(site: SiteImageModel) async throws -> SiteImageModel {
         // Don't fetch favicon URL if we don't have a URL or domain for it
         guard let siteURL = site.siteURL else {

BrowserKit/Sources/SiteImageView/ImageProcessing/ImageHandler.swift

@@ -6,11 +6,10 @@ import UIKit
 
 protocol ImageHandler {
     /// The ImageHandler will fetch the favicon with the following precedence:
-    ///     1. Tries to fetch from the bundle.
-    ///     2. Tries to fetch from the cache.
-    ///     3. Tries to fetch from the favicon fetcher (from the web) if there's a URL. 
+    ///     1. Tries to fetch from the cache.
+    ///     2. Tries to fetch from the favicon fetcher (from the web) if there's a URL.
     ///        If there's no URL it fallbacks to the letter favicon.
-    ///     4. When all fails it returns the letter favicon.
+    ///     3. When all fails it returns the letter favicon.
     ///
     /// Any time the favicon is fetched, it will be cache for future usage.
     ///
@@ -37,18 +36,15 @@ protocol ImageHandler {
 }
 
 class DefaultImageHandler: ImageHandler {
-    private let bundleImageFetcher: BundleImageFetcher
     private let imageCache: SiteImageCache
     private let faviconFetcher: FaviconFetcher
     private let letterImageGenerator: LetterImageGenerator
     private let heroImageFetcher: HeroImageFetcher
 
-    init(bundleImageFetcher: BundleImageFetcher = DefaultBundleImageFetcher(),
-         imageCache: SiteImageCache = DefaultSiteImageCache(),
+    init(imageCache: SiteImageCache = DefaultSiteImageCache(),
          faviconFetcher: FaviconFetcher = DefaultFaviconFetcher(),
          letterImageGenerator: LetterImageGenerator = DefaultLetterImageGenerator(),
          heroImageFetcher: HeroImageFetcher = DefaultHeroImageFetcher()) {
-        self.bundleImageFetcher = bundleImageFetcher
         self.imageCache = imageCache
         self.faviconFetcher = faviconFetcher
         self.letterImageGenerator = letterImageGenerator
@@ -57,9 +53,9 @@ class DefaultImageHandler: ImageHandler {
 
     func fetchFavicon(site: SiteImageModel) async -> UIImage {
         do {
-            return try bundleImageFetcher.getImageFromBundle(domain: site.domain)
+            return try await imageCache.getImageFromCache(cacheKey: site.cacheKey, type: site.expectedImageType)
         } catch {
-            return await fetchFaviconFromCache(site: site)
+            return await fetchFaviconFromFetcher(site: site)
         }
     }
 
@@ -79,14 +75,6 @@ class DefaultImageHandler: ImageHandler {
 
     // MARK: Private
 
-    private func fetchFaviconFromCache(site: SiteImageModel) async -> UIImage {
-        do {
-            return try await imageCache.getImageFromCache(cacheKey: site.cacheKey, type: site.expectedImageType)
-        } catch {
-            return await fetchFaviconFromFetcher(site: site)
-        }
-    }
-
     private func fetchFaviconFromFetcher(site: SiteImageModel) async -> UIImage {
         do {
             guard let url = site.faviconURL else {

BrowserKit/Sources/SiteImageView/SiteImageHandler.swift

@@ -15,12 +15,11 @@ public class DefaultSiteImageHandler: SiteImageHandler {
     private let urlHandler: FaviconURLHandler
     private let imageHandler: ImageHandler
 
-    private let serialQueue = DispatchQueue(label: "com.mozilla.DefaultSiteImageHandler")
-    private var _currentInFlightRequest: String?
-    private var currentInFlightRequest: String? {
-        get { return serialQueue.sync { _currentInFlightRequest } }
-        set { serialQueue.sync { _currentInFlightRequest = newValue } }
-    }
+    /// Right now, multiple `SiteImageView`s each have their own `DefaultSiteImageHandler`. Ideally they'd all share a
+    /// reference to the same `DefaultSiteImageHandler` so we could properly queue and throttle requests to get favicon
+    /// URLs, download images, etc. Since that's a large architectural change, for now lets use a static queue so we can
+    /// prevent too many duplicate calls to remotely fetching URLs and images. (FXIOS-9830, revised FXIOS-9427 bugfix)
+    private(set) static var requestQueue: [String: Task<UIImage, Never>] = [:]
 
     public static func factory() -> DefaultSiteImageHandler {
         return DefaultSiteImageHandler()
@@ -46,15 +45,15 @@ public class DefaultSiteImageHandler: SiteImageHandler {
                                                faviconURL: imageModel.faviconURL,
                                                type: imageModel.expectedImageType)
 
-        do {
-            switch site.expectedImageType {
-            case .heroImage:
+        switch site.expectedImageType {
+        case .heroImage:
+            do {
                 imageModel.heroImage = try await getHeroImage(imageModel: imageModel)
-            case .favicon:
+            } catch {
+                // If hero image fails, we return a favicon image
                 imageModel.faviconImage = await getFaviconImage(imageModel: imageModel)
             }
-        } catch {
-            // If hero image fails, we return a favicon image
+        case .favicon:
             imageModel.faviconImage = await getFaviconImage(imageModel: imageModel)
         }
 
@@ -107,29 +106,40 @@ public class DefaultSiteImageHandler: SiteImageHandler {
         }
     }
 
+    // Note: Must be synchronized on an actor to avoid crashes (managing the activeRequest queue).
+    @MainActor
     private func getFaviconImage(imageModel: SiteImageModel) async -> UIImage {
-        do {
-            while let currentSiteRequest = currentInFlightRequest,
-               imageModel.siteURLString == currentSiteRequest {
-                // We are already processing a favicon request for this site
-                // Sleep this task until the previous request is completed
-                try? await Task.sleep(nanoseconds: 50_000_000) // 50ms
-            }
+        // Check if we're already awaiting a request to get the favicon image for this cacheKey.
+        // If so, simply await that request rather than begin a new one.
+        let requestKey = imageModel.cacheKey
+        if let activeRequest = DefaultSiteImageHandler.requestQueue[requestKey] {
+            return await activeRequest.value
+        }
 
-            currentInFlightRequest = imageModel.siteURLString
-            var faviconURLImageModel = imageModel
-            if faviconURLImageModel.faviconURL == nil {
-                // Try to fetch the favicon URL
-                faviconURLImageModel = try await urlHandler.getFaviconURL(site: imageModel)
+        let requestHandle = Task {
+            var faviconImageModel = imageModel
+
+            do {
+                if faviconImageModel.faviconURL == nil {
+                    // Try to obtain the favicon URL if needed (ideally from cache, otherwise scrape the webpage)
+                    faviconImageModel = try await urlHandler.getFaviconURL(site: imageModel)
+                }
+
+                // Try to load the favicon image from the cache, or make a request to the favicon URL
+                // if it's not in the cache
+                let icon = await imageHandler.fetchFavicon(site: faviconImageModel)
+                return icon
+            } catch {
+                // If no favicon URL, generate favicon without it
+                let letter = await imageHandler.fetchFavicon(site: imageModel)
+                return letter
             }
-            let icon = await imageHandler.fetchFavicon(site: faviconURLImageModel)
-            currentInFlightRequest = nil
-            return icon
-        } catch {
-            // If no favicon URL, generate favicon without it
-            currentInFlightRequest = nil
-            return await imageHandler.fetchFavicon(site: imageModel)
         }
+
+        DefaultSiteImageHandler.requestQueue[requestKey] = requestHandle
+        let image = await requestHandle.value
+        DefaultSiteImageHandler.requestQueue[requestKey] = nil
+        return image
     }
 
     private func generateDomainURL(siteURL: URL) -> ImageDomain {

BrowserKit/Sources/SiteImageView/SiteImageView.swift

@@ -29,11 +29,11 @@ extension SiteImageView {
     }
 
     func updateImage(site: SiteImageModel) {
-        Task {
-            let imageModel = await imageFetcher.getImage(site: site)
+        Task { [weak self] in
+            let imageModel = await self?.imageFetcher.getImage(site: site)
 
             DispatchQueue.main.async { [weak self] in
-                guard let self, uniqueID == imageModel.id else { return }
+                guard let self, let imageModel, uniqueID == imageModel.id else { return }
                 setImage(imageModel: imageModel)
             }
         }