Fix removing old versions of filter lists from ad-block manager

brave · Aug 13, 2022 · cf365a3 · cf365a3
1 parent ca3c738
commit cf365a3
Show file tree

Hide file tree

Showing 3 changed files with 35 additions and 28 deletions.
diff --git a/Client/WebFilters/AdBlockEngineManager.swift b/Client/WebFilters/AdBlockEngineManager.swift
@@ -15,7 +15,8 @@ private let log = Logger.browserLogger
 public class AdBlockEngineManager {
   /// The source of a resource. In some cases we need to remove all resources for a given source.
   enum Source: Hashable {
-    case general
+    case adBlock
+    case cosmeticFilters
     case filterList(uuid: String)
   }
 
@@ -79,9 +80,10 @@ public class AdBlockEngineManager {
   }
 
   /// Tells this manager to remove all resources for the given source next time it compiles this engine
-  func removeResources(for source: Source) {
+  func removeResources(for source: Source, resourceTypes: Set<ResourceType> = Set(ResourceType.allCases)) {
     self.enabledResources = self.enabledResources.filter { resourceWithVersion in
-      guard resourceWithVersion.resource.source == source else { return true }
+      let resource = resourceWithVersion.resource
+      guard resource.source == source && resourceTypes.contains(resource.type) else { return true }
       compileResults.removeValue(forKey: resourceWithVersion)
       return false
     }
@@ -175,8 +177,7 @@ public class AdBlockEngineManager {
       .map { resourceWithVersion -> String in
         let resource = resourceWithVersion.resource
         let type: String
-        let sourceTypeString: String
-        let uuidString: String?
+        let sourceString: String
         let resultString: String
 
         switch resource.type {
@@ -190,11 +191,11 @@ public class AdBlockEngineManager {
 
         switch resource.source {
         case .filterList(let uuid):
-          sourceTypeString = "filterList"
-          uuidString = uuid
-        case .general:
-          sourceTypeString = "general"
-          uuidString = nil
+          sourceString = "filterList(\(uuid))"
+        case .adBlock:
+          sourceString = "adBlock"
+        case .cosmeticFilters:
+          sourceString = "cosmeticFilters"
         }
 
         switch compileResults[resourceWithVersion] {
@@ -209,8 +210,7 @@ public class AdBlockEngineManager {
         return [
           "TEST \t{",
           "TEST \t\tfileName: \(resource.fileURL.lastPathComponent)",
-          "TEST \t\tuuid: \(uuidString ?? "nil")",
-          "TEST \t\tsourceType: \(sourceTypeString)",
+          "TEST \t\tsource: \(sourceString)",
           "TEST \t\tversion: \(resourceWithVersion.version ?? "nil")",
           "TEST \t\ttype: \(type)",
           "TEST \t\tresult: \(resultString)",

diff --git a/Client/WebFilters/AdblockResourceDownloader.swift b/Client/WebFilters/AdblockResourceDownloader.swift
@@ -81,7 +81,7 @@ public class AdblockResourceDownloader {
       AdBlockEngineManager.shared.add(resource: AdBlockEngineManager.Resource(
         fileURL: downloadedFileURL,
         type: .dat,
-        source: .general
+        source: .cosmeticFilters
       ), version: version)
     case .genericContentBlockingBehaviors:
       ContentBlockerManager.shared.set(resource: ContentBlockerManager.Resource(

diff --git a/Client/WebFilters/FilterListResourceDownloader.swift b/Client/WebFilters/FilterListResourceDownloader.swift
@@ -58,33 +58,26 @@ public class FilterListResourceDownloader: ObservableObject {
     }
   }
 
-  #if DEBUG
-  /// A method available only for tests since we cannot use the AdblockService in our tests.
-  /// It simulates a part of the `start` function that does not require an adBlockService.
-  func startForTests(filterListSettings: [FilterListSetting], filterLists: [FilterList]) {
-    prepareData(filterListSettings: filterListSettings, filterLists: filterLists)
-
-    Task(priority: .userInitiated) {
-      await subscribeToFilterListChanges()
-      await registerAllEnabledFilterLists()
-    }
-  }
-  #endif
-
   /// Load shields with the given `AdblockService` folder `URL`
   private func loadShields(fromFolderURL folderURL: URL) {
     let version = folderURL.lastPathComponent
 
+    // Make sure we remove the old resource if there is one
+    AdBlockEngineManager.shared.removeResources(
+      for: .adBlock,
+      resourceTypes: [.dat, .jsonResources]
+    )
+
     AdBlockEngineManager.shared.add(resource: AdBlockEngineManager.Resource(
       fileURL: folderURL.appendingPathComponent("rs-ABPFilterParserData.dat"),
       type: .dat,
-      source: .general
+      source: .adBlock
     ), version: version)
 
     AdBlockEngineManager.shared.add(resource: AdBlockEngineManager.Resource(
       fileURL: folderURL.appendingPathComponent("resources.json"),
       type: .jsonResources,
-      source: .general
+      source: .adBlock
     ), version: version)
   }
 
@@ -265,6 +258,13 @@ public class FilterListResourceDownloader: ObservableObject {
       ), for: .filterList(uuid: filterList.uuid))
     case .dataFile:
       // TODO: Compile rulelist to blocklist
+      // Make sure we remove the old resource if there is one
+      AdBlockEngineManager.shared.removeResources(
+        for: .filterList(uuid: filterList.uuid),
+        resourceTypes: [.ruleList]
+      )
+
+      // Add the new one back in
       AdBlockEngineManager.shared.add(resource: AdBlockEngineManager.Resource(
         fileURL: downloadedFileURL,
         type: .ruleList,
@@ -285,6 +285,13 @@ public class FilterListResourceDownloader: ObservableObject {
     let resourceURL = downloadedFolderURL.appendingPathComponent("resources.json")
     let version = downloadedFolderURL.lastPathComponent
 
+    // Make sure we remove the old resource if there is one
+    AdBlockEngineManager.shared.removeResources(
+      for: .filterList(uuid: filterList.uuid),
+      resourceTypes: [.jsonResources]
+    )
+
+    // Let's add the new one in
     AdBlockEngineManager.shared.add(resource: AdBlockEngineManager.Resource(
       fileURL: resourceURL,
       type: .jsonResources,