Fix #8191: Fix crashes due to compiling too many engines, too quickly (…

…#8224)
brave · Oct 12, 2023 · 604a858 · 604a858
1 parent 8bec171
commit 604a858
Show file tree

Hide file tree

Showing 9 changed files with 477 additions and 245 deletions.
diff --git a/Sources/Brave/Frontend/Browser/BrowserViewController.swift b/Sources/Brave/Frontend/Browser/BrowserViewController.swift
@@ -420,7 +420,7 @@ public class BrowserViewController: UIViewController {
     ScriptFactory.shared.clearCaches()
 
     Task {
-      await AdBlockStats.shared.clearCaches()
+      await AdBlockStats.shared.didReceiveMemoryWarning()
     }
 
     for tab in tabManager.tabsForCurrentMode where tab.id != tabManager.selectedTab?.id {

diff --git a/Sources/Brave/Frontend/Settings/Features/ShieldsPrivacy/FilterLists/FilterListsView.swift b/Sources/Brave/Frontend/Settings/Features/ShieldsPrivacy/FilterLists/FilterListsView.swift
@@ -16,53 +16,17 @@ struct FilterListsView: View {
   @ObservedObject private var customFilterListStorage = CustomFilterListStorage.shared
   @Environment(\.editMode) private var editMode
   @State private var showingAddSheet = false
+  @State private var expectedEnabledSources: Set<CachedAdBlockEngine.Source> = Set(AdBlockStats.shared.enabledSources)
   private let dateFormatter = RelativeDateTimeFormatter()
 
+  private var reachedMaxLimit: Bool {
+    expectedEnabledSources.count >= AdBlockStats.maxNumberOfAllowedFilterLists
+  }
+
   var body: some View {
     List {
       Section {
-        ForEach($customFilterListStorage.filterListsURLs) { $filterListURL in
-          VStack(alignment: .leading, spacing: 4) {
-            Toggle(isOn: $filterListURL.setting.isEnabled) {
-              VStack(alignment: .leading, spacing: 4) {
-                Text(filterListURL.title)
-                  .foregroundColor(Color(.bravePrimary))
-                  .truncationMode(.middle)
-                  .lineLimit(1)
-
-                switch filterListURL.downloadStatus {
-                case .downloaded(let downloadDate):
-                  Text(String.localizedStringWithFormat(
-                    Strings.filterListsLastUpdated,
-                    dateFormatter.localizedString(for: downloadDate, relativeTo: Date())))
-                    .font(.caption)
-                    .foregroundColor(Color(.braveLabel))
-                case .failure:
-                  Text(Strings.filterListsDownloadFailed)
-                    .font(.caption)
-                    .foregroundColor(.red)
-                case .pending:
-                  Text(Strings.filterListsDownloadPending)
-                    .font(.caption)
-                    .foregroundColor(Color(.braveLabel))
-                }
-              }
-            }
-            .disabled(editMode?.wrappedValue.isEditing == true)
-            .toggleStyle(SwitchToggleStyle(tint: .accentColor))
-            .onChange(of: filterListURL.setting.isEnabled) { value in
-              Task {
-                CustomFilterListSetting.save(inMemory: !customFilterListStorage.persistChanges)
-              }
-            }
-
-          Text(filterListURL.setting.externalURL.absoluteDisplayString)
-            .font(.caption)
-            .foregroundColor(Color(.secondaryBraveLabel))
-            .allowsTightening(true)
-          }.listRowBackground(Color(.secondaryBraveGroupedBackground))
-        }
-        .onDelete(perform: onDeleteHandling)
+        customFilterListView
 
         Button {
           showingAddSheet = true
@@ -71,27 +35,16 @@ struct FilterListsView: View {
             .foregroundColor(Color(.braveBlurpleTint))
         }
           .disabled(editMode?.wrappedValue.isEditing == true)
-          .listRowBackground(Color(.secondaryBraveGroupedBackground))
           .popover(isPresented: $showingAddSheet, content: {
             FilterListAddURLView()
           })
       } header: {
         Text(Strings.customFilterLists)
       }
+      .toggleStyle(SwitchToggleStyle(tint: .accentColor))
 
       Section {
-        ForEach($filterListStorage.filterLists) { $filterList in
-          Toggle(isOn: $filterList.isEnabled) {
-            VStack(alignment: .leading) {
-              Text(filterList.entry.title)
-                .foregroundColor(Color(.bravePrimary))
-              Text(filterList.entry.desc)
-                .font(.caption)
-                .foregroundColor(Color(.secondaryBraveLabel))
-            }
-          }.toggleStyle(SwitchToggleStyle(tint: .accentColor))
-            .listRowBackground(Color(.secondaryBraveGroupedBackground))
-        }
+        filterListView
       } header: {
         VStack(alignment: .leading, spacing: 4) {
           Text(Strings.defaultFilterLists)
@@ -101,6 +54,8 @@ struct FilterListsView: View {
         }
       }
     }
+    .toggleStyle(SwitchToggleStyle(tint: .accentColor))
+    .listRowBackground(Color(.secondaryBraveGroupedBackground))
     .animation(.default, value: customFilterListStorage.filterListsURLs)
     .listBackgroundColor(Color(UIColor.braveGroupedBackground))
     .listStyle(.insetGrouped)
@@ -111,6 +66,84 @@ struct FilterListsView: View {
         editMode?.wrappedValue.isEditing == false
       )
     }
+    .onDisappear {
+      Task.detached {
+        await AdBlockStats.shared.removeDisabledEngines()
+        await AdBlockStats.shared.ensureEnabledEngines()
+      }
+    }
+  }
+
+  @ViewBuilder private var filterListView: some View {
+    ForEach($filterListStorage.filterLists) { $filterList in
+      Toggle(isOn: $filterList.isEnabled) {
+        VStack(alignment: .leading) {
+          Text(filterList.entry.title)
+            .foregroundColor(Color(.bravePrimary))
+          Text(filterList.entry.desc)
+            .font(.caption)
+            .foregroundColor(Color(.secondaryBraveLabel))
+        }
+      }
+      .disabled(!filterList.isEnabled && reachedMaxLimit)
+      .onChange(of: filterList.isEnabled) { isEnabled in
+        if isEnabled {
+          expectedEnabledSources.insert(filterList.engineSource)
+        } else {
+          expectedEnabledSources.remove(filterList.engineSource)
+        }
+      }
+    }
+  }
+
+  @ViewBuilder private var customFilterListView: some View {
+    ForEach($customFilterListStorage.filterListsURLs) { $filterListURL in
+      VStack(alignment: .leading, spacing: 4) {
+        Toggle(isOn: $filterListURL.setting.isEnabled) {
+          VStack(alignment: .leading, spacing: 4) {
+            Text(filterListURL.title)
+              .foregroundColor(Color(.bravePrimary))
+              .truncationMode(.middle)
+              .lineLimit(1)
+
+            switch filterListURL.downloadStatus {
+            case .downloaded(let downloadDate):
+              Text(String.localizedStringWithFormat(
+                Strings.filterListsLastUpdated,
+                dateFormatter.localizedString(for: downloadDate, relativeTo: Date())))
+                .font(.caption)
+                .foregroundColor(Color(.braveLabel))
+            case .failure:
+              Text(Strings.filterListsDownloadFailed)
+                .font(.caption)
+                .foregroundColor(.red)
+            case .pending:
+              Text(Strings.filterListsDownloadPending)
+                .font(.caption)
+                .foregroundColor(Color(.braveLabel))
+            }
+          }
+        }
+        .disabled(reachedMaxLimit && !filterListURL.setting.isEnabled)
+        .onChange(of: filterListURL.setting.isEnabled) { isEnabled in
+          if isEnabled {
+            expectedEnabledSources.insert(filterListURL.setting.engineSource)
+          } else {
+            expectedEnabledSources.remove(filterListURL.setting.engineSource)
+          }
+
+          Task {
+            CustomFilterListSetting.save(inMemory: !customFilterListStorage.persistChanges)
+          }
+        }
+
+      Text(filterListURL.setting.externalURL.absoluteDisplayString)
+        .font(.caption)
+        .foregroundColor(Color(.secondaryBraveLabel))
+        .allowsTightening(true)
+      }
+    }
+    .onDelete(perform: onDeleteHandling)
   }
 
   private func onDeleteHandling(offsets: IndexSet) {

diff --git a/Sources/Brave/WebFilters/FilterListCustomURLDownloader.swift b/Sources/Brave/WebFilters/FilterListCustomURLDownloader.swift
@@ -58,22 +58,29 @@ actor FilterListCustomURLDownloader: ObservableObject {
     self.startedService = true
     await CustomFilterListStorage.shared.loadCachedFilterLists()
 
-    await CustomFilterListStorage.shared.filterListsURLs.asyncConcurrentForEach { customURL in
-      let resource = await customURL.setting.resource
-
-      do {
-        if let cachedResult = try resource.cachedResult() {
-          await self.handle(downloadResult: cachedResult, for: customURL)
+    do {
+      try await CustomFilterListStorage.shared.filterListsURLs.asyncConcurrentForEach { customURL in
+        let resource = await customURL.setting.resource
+
+        do {
+          if let cachedResult = try resource.cachedResult() {
+            await self.handle(downloadResult: cachedResult, for: customURL)
+          }
+        } catch {
+          let uuid = await customURL.setting.uuid
+          ContentBlockerManager.log.error(
+            "Failed to cached data for custom filter list `\(uuid)`: \(error)"
+          )
         }
-      } catch {
-        let uuid = await customURL.setting.uuid
-        ContentBlockerManager.log.error(
-          "Failed to cached data for custom filter list `\(uuid)`: \(error)"
-        )
+
+        // Always fetch this resource so it's ready if the user enables it.
+        await self.startFetching(filterListCustomURL: customURL)
+
+        // Sleep for 1ms. This drastically reduces memory usage without much impact to usability
+        try await Task.sleep(nanoseconds: 1000000)
       }
-
-      // Always fetch this resource so it's ready if the user enables it.
-      await self.startFetching(filterListCustomURL: customURL)
+    } catch {
+      // Ignore cancellation errors
     }
   }
 
@@ -105,32 +112,33 @@ actor FilterListCustomURLDownloader: ObservableObject {
     }
 
     // Add/remove the resource depending on if it is enabled/disabled
-    if await filterListCustomURL.setting.isEnabled {
-      guard let resourcesInfo = await FilterListResourceDownloader.shared.resourcesInfo else {
-        assertionFailure("This should not have been called if the resources are not ready")
-        return
-      }
-
-      let version = fileVersionDateFormatter.string(from: downloadResult.date)
-      let filterListInfo = CachedAdBlockEngine.FilterListInfo(
-        source: .filterListURL(uuid: uuid),
-        localFileURL: downloadResult.fileURL,
-        version: version, fileType: .text
+    let source = CachedAdBlockEngine.Source.filterListURL(uuid: uuid)
+    guard let resourcesInfo = await FilterListResourceDownloader.shared.resourcesInfo else {
+      assertionFailure("This should not have been called if the resources are not ready")
+      return
+    }
+
+    let version = fileVersionDateFormatter.string(from: downloadResult.date)
+    let filterListInfo = CachedAdBlockEngine.FilterListInfo(
+      source: .filterListURL(uuid: uuid),
+      localFileURL: downloadResult.fileURL,
+      version: version, fileType: .text
+    )
+
+    guard await AdBlockStats.shared.isEagerlyLoaded(source: source) else {
+      // Don't compile unless eager
+      await AdBlockStats.shared.updateIfNeeded(resourcesInfo: resourcesInfo)
+      await AdBlockStats.shared.updateIfNeeded(filterListInfo: filterListInfo, isAlwaysAggressive: true)
+      return
+    }
+
+    do {
+      try await AdBlockStats.shared.compileDelayed(
+        filterListInfo: filterListInfo, resourcesInfo: resourcesInfo,
+        isAlwaysAggressive: true, delayed: true
       )
-
-      do {
-        let engine = try await CachedAdBlockEngine.compile(
-          filterListInfo: filterListInfo, resourcesInfo: resourcesInfo,
-          isAlwaysAggressive: true
-        )
-
-        await AdBlockStats.shared.add(engine: engine)
-        ContentBlockerManager.log.debug("Compiled engine for custom filter list `\(uuid)` v\(version)")
-      } catch {
-        ContentBlockerManager.log.error("Failed to compile engine for custom filter list `\(uuid)` v\(version): \(String(describing: error))")
-      }
-    } else {
-      await AdBlockStats.shared.removeEngine(for: .filterListURL(uuid: uuid))
+    } catch {
+      // Don't handle cancellation errors
     }
   }