Fix #3071: Load favicons on background thread, fix cell reusing.

Client/Frontend/Browser/Favicons/FaviconFetcher.swift

@@ -70,14 +70,18 @@ class FaviconFetcher {
     deinit {
         dataTasks.forEach { $0.cancel() }
         imageOps.forEach { $0.cancel() }
+        cancelLoadTask()
     }
 
-    private func faviconOnFileMatchesFetchKind(_ favicon: FaviconMO?) -> Bool {
-        guard let favicon = favicon else { return false }
-        return favicon.type == Int16(kind.iconType.rawValue) ||
-            IconType(rawValue: Int(favicon.type))?.isPreferredTo(kind.iconType) == true
+    private func faviconOnFileMatchesFetchKind(_ faviconType: Int16?) -> Bool {
+        guard let faviconType = faviconType else { return false }
+        return faviconType == Int16(kind.iconType.rawValue) ||
+            IconType(rawValue: Int(faviconType))?.isPreferredTo(kind.iconType) == true
     }
 
+    // fileprivate to use it in the UIImageExtension
+    fileprivate var loadTaskCancellable: DispatchWorkItem?
+
     /// Begin the search for a favicon for the site. `completion` will always
     /// be called on the main thread.
     ///
@@ -89,22 +93,40 @@ class FaviconFetcher {
     ///        downloaded and parsed for a favicon.
     ///     4. Monogram (letter + background color)
     func load(_ cachedOnly: Bool = false, _ completion: @escaping (URL, FaviconAttributes) -> Void) {
-        if let icon = customIcon {
-            completion(url, icon)
-            return
-        }
-        let matchesFetchKind = faviconOnFileMatchesFetchKind(domain.favicon)
-        if let icon = bundledIcon, domain.favicon == nil || !matchesFetchKind {
-            completion(url, icon)
-            return
-        }
-        fetchIcon(cachedOnly) { url, attributes in
-            DispatchQueue.main.async {
-                completion(url, attributes)
+        let faviconType = domain.favicon?.type
+
+        loadTaskCancellable = DispatchWorkItem(qos: .utility, block: { [weak self] in
+            guard let self = self else { return }
+            if let icon = self.customIcon {
+                DispatchQueue.main.async {
+                    completion(self.url, icon)
+                }
+                return
+            }
+            let matchesFetchKind = self.faviconOnFileMatchesFetchKind(faviconType)
+            if let icon = self.bundledIcon, faviconType == nil || !matchesFetchKind {
+                DispatchQueue.main.async {
+                    completion(self.url, icon)
+                }
+                return
+            }
+            self.fetchIcon(cachedOnly) { url, attributes in
+                DispatchQueue.main.async {
+                    completion(url, attributes)
+                }
             }
+        })
+
+        if let task = loadTaskCancellable {
+            DispatchQueue.global(qos: .utility).async(execute: task)
         }
     }
 
+    func cancelLoadTask() {
+        loadTaskCancellable?.cancel()
+        loadTaskCancellable = nil
+    }
+
     // MARK: - Custom Icons
 
     /// Icon attributes for any custom icon overrides
@@ -254,7 +276,7 @@ class FaviconFetcher {
         if let favicon = domainFavicon, let urlString = favicon.url, let url = URL(string: urlString) {
             // Verify that the favicon we have on file is what we want to pull
             // If not, we will just default to monogram to avoid blurry images
-            if faviconOnFileMatchesFetchKind(favicon) {
+            if faviconOnFileMatchesFetchKind(favicon.type) {
 
                 // If loading from cache only is specified,
                 // Return monogram image if there is no cache.
@@ -515,11 +537,9 @@ extension UIImageView {
     /// Removes any monogram labels that may have been added to the image in the
     /// past
     func clearMonogramFavicon() {
-        if let label = monogramLabel, label.superview != nil {
-            monogramLabel?.removeFromSuperview()
-            monogramLabel = nil
-            backgroundColor = nil
-        }
+        monogramLabel?.removeFromSuperview()
+        monogramLabel = nil
+        backgroundColor = nil
     }
 
     /// Load the favicon from a site URL directly into a `UIImageView`. If no
@@ -540,7 +560,13 @@ extension UIImageView {
         clearMonogramFavicon()
         faviconFetcher = FaviconFetcher(siteURL: siteURL, kind: .favicon, domain: domain)
         faviconFetcher?.load(cachedOnly) { [weak self] _, attributes in
-            guard let self = self else { return }
+            guard let self = self,
+                  let cancellable = self.faviconFetcher?.loadTaskCancellable,
+                  !cancellable.isCancelled  else {
+                completion?()
+                return
+            }
+
             if let image = attributes.image {
                 self.image = image
             } else {
@@ -568,4 +594,9 @@ extension UIImageView {
             completion?()
         }
     }
+
+    /// Cancel any pending favicon load task. This is to prevent race condition UI glitches with reusable table/collection view cells.
+    func cancelFaviconLoad() {
+        faviconFetcher?.cancelLoadTask()
+    }
 }

Client/Frontend/Browser/Toolbars/BottomToolbar/Menu/Bookmarks/BookmarksViewController.swift

@@ -79,6 +79,8 @@ class BookmarksViewController: SiteTableViewController, ToolbarUrlActionsProtoco
         super.viewDidLoad()
 
         tableView.allowsSelectionDuringEditing = true
+        tableView.register(BookmarkTableViewCell.self,
+                           forCellReuseIdentifier: String(describing: BookmarkTableViewCell.self))
 
         setUpToolbar()
         updateEditBookmarksButtonStatus()
@@ -268,16 +270,24 @@ class BookmarksViewController: SiteTableViewController, ToolbarUrlActionsProtoco
     return true
   }
 
-  fileprivate func configureCell(_ cell: UITableViewCell, atIndexPath indexPath: IndexPath) {
-
+  fileprivate func configureCell(_ cell: BookmarkTableViewCell, atIndexPath indexPath: IndexPath) {
     // Make sure Bookmark at index path exists,
     // `frc.object(at:)` crashes otherwise, doesn't fail safely with nil
     if let objectsCount = bookmarksFRC?.fetchedObjectsCount, indexPath.row >= objectsCount {
-        fatalError("Bookmarks FRC index out of bounds")
+        assertionFailure("Bookmarks FRC index out of bounds")
+        return
     }
 
     guard let item = bookmarksFRC?.object(at: indexPath) else { return }
     cell.tag = item.objectID
+    cell.imageView?.tag = indexPath.row
+
+    // See if the cell holds the same bookmark. If yes, we do not have to recreate its image view
+    // This makes scrolling through bookmarks better if there's many bookmarks with the same url
+    let domainOrFolderName = item.isFolder ? item.displayTitle : (item.domain?.url ?? item.url)
+    let shouldReuse = domainOrFolderName != cell.domainOrFolderName
+
+    cell.domainOrFolderName = domainOrFolderName
 
     func configCell(image: UIImage? = nil, icon: FaviconMO? = nil) {
       if !tableView.isEditing {
@@ -286,6 +296,11 @@ class BookmarksViewController: SiteTableViewController, ToolbarUrlActionsProtoco
         cell.addGestureRecognizer(lp)
       }
 
+        if !shouldReuse {
+            return
+        }
+
+        cell.imageView?.cancelFaviconLoad()
       cell.backgroundColor = .clear
       cell.imageView?.contentMode = .scaleAspectFit
       cell.imageView?.image = FaviconFetcher.defaultFaviconImage
@@ -457,7 +472,12 @@ class BookmarksViewController: SiteTableViewController, ToolbarUrlActionsProtoco
     }
 
     override func tableView(_ tableView: UITableView, cellForRowAt indexPath: IndexPath) -> UITableViewCell {
-        let cell = super.tableView(tableView, cellForRowAt: indexPath)
+        guard let cell = tableView
+                .dequeueReusableCell(withIdentifier: String(describing: BookmarkTableViewCell.self), for: indexPath) as? BookmarkTableViewCell else {
+            assertionFailure()
+            return UITableViewCell()
+        }
+
         configureCell(cell, atIndexPath: indexPath)
         return cell
     }
@@ -532,7 +552,10 @@ extension BookmarksViewController: BookmarksV2FetchResultsDelegate {
             // When Bookmark is moved to another folder, it can be interpreted as update action
             // (since the object is not deleted but updated to have a different parent Bookmark)
             // Make sure we are not out of bounds here.
-            if let path = path, let cell = self.tableView.cellForRow(at: path),
+            if let path = path,
+               let cell = self.tableView
+                .dequeueReusableCell(withIdentifier: String(describing: BookmarkTableViewCell.self),
+                                     for: path) as? BookmarkTableViewCell,
                 let fetchedObjectsCount = self.bookmarksFRC?.fetchedObjectsCount, path.row < fetchedObjectsCount {
                     self.configureCell(cell, atIndexPath: path)
             }

Client/Frontend/Widgets/TwoLineCell.swift

@@ -113,6 +113,32 @@ class SiteTableViewCell: TwoLineTableViewCell {
     }
 }
 
+class BookmarkTableViewCell: TwoLineTableViewCell {
+    /// Holds url domain for bookmarks or folder name for folders.
+    /// This property is used to see if the cell's image should be reused.
+    var domainOrFolderName: String?
+
+    override init(style: UITableViewCell.CellStyle, reuseIdentifier: String?) {
+        super.init(style: .subtitle, reuseIdentifier: reuseIdentifier)
+        guard let textLabel = textLabel, let detailTextLabel = detailTextLabel,
+              let imageView = imageView else { return }
+
+        twoLineHelper.setUpViews(contentView,
+                                 textLabel: textLabel,
+                                 detailTextLabel: detailTextLabel,
+                                 imageView: imageView)
+    }
+
+    override func layoutSubviews() {
+        super.layoutSubviews()
+        twoLineHelper.layoutSubviews(accessoryWidth: self.contentView.frame.origin.x)
+    }
+
+    required init?(coder aDecoder: NSCoder) {
+        fatalError("init(coder:) has not been implemented")
+    }
+}
+
 class TwoLineHeaderFooterView: UITableViewHeaderFooterView {
     fileprivate let twoLineHelper = TwoLineCellHelper()