Js/engine memory fixes #8200

cuba · 2023-10-05T20:53:55Z

Summary of Changes

This pull request fixes #

Submitter Checklist:

Unit Tests are updated to cover new or changed functionality
User-facing strings use NSLocalizableString()
New or updated UI has been tested across:
- Light & dark mode
- Different size classes (iPhone, landscape, iPad)
- Different dynamic type sizes

Test Plan:

Screenshots:

Reviewer Checklist:

Issues include necessary QA labels:
- QA/(Yes|No)
- bug / enhancement
Necessary security reviews have taken place.
Adequate unit test coverage exists to prevent regressions.
Adequate test plan exists for QA to validate (if applicable).
Issue and pull request is assigned to a milestone (should happen at merge time).

cuba · 2023-10-06T05:40:00Z

Sources/Brave/Frontend/Browser/Helpers/LaunchHelper.swift

+      }
+    }
+  }
+}


Here I realized I was unnecessarily removing content blockers when filter lists were not yet ready.

cuba · 2023-10-06T05:40:35Z

Sources/Brave/Frontend/Browser/UserScriptManager.swift

+        " #\(scriptType.order) \(scriptType.debugDescription)"
+      }.joined(separator: "\n")
+    ]
+    ContentBlockerManager.log.debug("Loaded \(userScripts.count + customScripts.count) script(s): \n\(logComponents.joined(separator: "\n"))")    


just some logging to help me debug the code and ensure everything is working properly

cuba · 2023-10-06T05:41:06Z

Sources/Brave/Frontend/Settings/Features/ShieldsPrivacy/FilterLists/FilterListsView.swift

+        Toggle("All", isOn: allToggled)
+          .toggleStyle(SwitchToggleStyle(tint: .accentColor))
+        #endif
+


Some debugging functionality so I can test while compiling/uncomplying all the engines.

cuba · 2023-10-06T05:41:31Z

Sources/Brave/Frontend/Settings/Features/ShieldsPrivacy/FilterLists/FilterListsView.swift

+      Task {
+        await AdBlockStats.shared.removeDisabledEngines()
+      }
+    }


Remove some engines after changing the settings to help free up memory

cuba · 2023-10-06T05:41:43Z

Sources/Brave/Frontend/Settings/SettingsViewController.swift

+              // Fallback on earlier versions
+
+            }
+          }, accessory: .disclosureIndicator, cellClass: MultilineSubtitleCell.self),


Access to a debug tool

cuba · 2023-10-06T05:42:32Z

...erContent/UserScripts/Scripts_Dynamic/ScriptHandlers/Paged/ContentBlockerScriptHandler.swift

@@ -90,7 +90,8 @@ extension ContentBlockerHelper: TabContentScript {
          sourceURL: sourceURL,
          enabledRuleTypes: genericTypes,
          resourceType: dto.data.resourceType,
-          isAggressiveMode: domain.blockAdsAndTrackingLevel.isAggressive
+          isAggressiveMode: domain.blockAdsAndTrackingLevel.isAggressive,
+          domain: domain
        )


I noticed the domain wasn't being checked. It's not a big deal because this responds to scripts that wouldn't be injected anyway, but figured should fix it.

cuba · 2023-10-06T05:43:31Z

Sources/Brave/WebFilters/ContentBlocker/ContentBlockerHelper.swift

+      removedIds.sorted(by: { $0 < $1 }).map({ " - \($0)" }).joined(separator: "\n")
+    ]
+
+    ContentBlockerManager.log.debug("Set rule lists:\n\(parts.joined(separator: "\n"))")


Just some more logs for debugging reasons

cuba · 2023-10-06T05:44:03Z

Sources/Brave/WebFilters/ContentBlocker/ContentBlockerManager.swift

+      ContentBlockerManager.log.error(
+        "Failed to compile rule lists for `\(type.identifier)` v\(version):\n\(error)"
+      )
+      foundError = error


Some debugging changes

cuba · 2023-10-06T05:44:26Z

Sources/Brave/WebFilters/ContentBlocker/ContentBlockerManager.swift

+          self.cachedRuleLists[type.makeIdentifier(for: mode)] = .emptyRules
+        }
+        return
+      }


Here I handle empty lists or wkwebview spits out an error

cuba · 2023-10-06T05:44:47Z

Sources/Brave/WebFilters/ContentBlocker/ContentBlockerManager.swift

+    case .emptyRules:
+      return nil
+    case .none:
+      return try await loadRuleList(for: type, mode: mode)
    }


Gracefully handling the empty lists

cuba · 2023-10-06T05:45:10Z

Sources/Brave/WebFilters/ContentBlocker/ContentBlockerManager.swift

-    ContentBlockerManager.log.debug("Filtered out \(count) rules")
+    if count > 0 {
+      ContentBlockerManager.log.debug("Filtered out \(count) rules for `\(type.identifier)`")
+    }


Just some debugging changes

cuba · 2023-10-06T05:46:04Z

Sources/Brave/WebFilters/Debug/TestAdblockEnginesView.swift

+#Preview {
+  TestAdblockEnginesView()
+}
+#endif


Debugging tool to brute force large engine compilations under different conditions

cuba · 2023-10-06T05:47:00Z

Sources/Brave/WebFilters/FilterListCustomURLDownloader.swift

-    } else {
-      await AdBlockStats.shared.removeEngine(for: .filterListURL(uuid: uuid))
+    } catch {
+      ContentBlockerManager.log.error("Failed to compile engine for custom filter list `\(uuid)` v\(version): \(String(describing: error))")
    }


Responding to changes to AdBlockStats

cuba · 2023-10-06T05:47:26Z

Sources/Brave/WebFilters/FilterListResourceDownloader.swift

@@ -280,7 +274,7 @@ public actor FilterListResourceDownloader {
        let result = try AdblockEngine.contentBlockerRules(fromFilterSet: filterSet)

        try await ContentBlockerManager.shared.compile(
-          encodedContentRuleList: result.rulesJSON, for: blocklistType,
+          encodedContentRuleList: result.rulesJSON, for: blocklistType, version: version,
          options: .all, modes: modes


Responding to changes to AdBlockStats

cuba · 2023-10-06T05:48:10Z

Sources/Brave/WebFilters/ShieldStats/Adblock/AdBlockEngine+Extensions.swift

@@ -62,8 +50,7 @@ extension AdblockEngine {

  private func useResources(fromFileURL fileURL: URL) throws {
    // Add scriplets if available
-    if let data = FileManager.default.contents(atPath: fileURL.path),
-       let json = try Self.validateJSON(data) {
+    if let json = try Self.validateJSON(Data(contentsOf: fileURL)) {
      useResources(json)


Memory usage fixups. This alone would solve a large amount of crashes.

cuba · 2023-10-06T05:48:55Z

Sources/Brave/WebFilters/ShieldStats/Adblock/CachedAdBlockEngine.swift

+      return CachedAdBlockEngine(
+        engine: engine, filterListInfo: filterListInfo, resourcesInfo: resourcesInfo,
+        isAlwaysAggressive: isAlwaysAggressive
+      )
    }


Removed all serialQueues, None of these are async anymore.

cuba · 2023-10-06T06:04:49Z

Sources/Brave/WebFilters/ShieldStats/Adblock/AdBlockStats.swift

@@ -12,66 +12,228 @@ import os.log
 /// for injected scripts needed during web navigation and cosmetic filters models needed by the `SelectorsPollerScript.js` script.
 public actor AdBlockStats {


Here the following changes have been made:

resourcesInfo has been moved to here instead of FilterListResourcesDownloader. This is needed for lazy loading.

Non-enabled filter lists will be lazily loaded when they are enabled by the user next time the user reloads the page. We store the necessary information that is needed to lazy load in availableFilterLists

Serial queue is added to synchronously compile engines (one by one) so memory does not climb during critical compilation stages such as launch. Normally only 3 engines are compiled but if the user enables a large amount of filter lists or custom filter lists this, would cause a crash without this serial queue.

Handling of memory warnings by deleting non-critical engines. Later perhaps we should disable some filer lists and show a warning to the user.

iccub

tests fail

cuba · 2023-10-11T15:34:55Z

@iccub Sorry you shouldn't look at this PR.

cuba · 2023-10-11T17:06:31Z

Moved here: #8224

cuba force-pushed the js/engine-memory-fixes branch 4 times, most recently from a55cfc2 to 01a0062 Compare October 6, 2023 05:37

cuba added 4 commits October 5, 2023 23:38

Add a test engines tool

8e7ecc5

Fix issues with engine memory usage

dbb86c6

Remove serial queue from CachedAdBlockEngine

60fe1b1

Fix unwanted removal of content blockers upon launch

bb5b5c2

cuba force-pushed the js/engine-memory-fixes branch from 01a0062 to 66c39f2 Compare October 6, 2023 05:39

cuba commented Oct 6, 2023

View reviewed changes

Add memory usage improvements

7ddf697

cuba force-pushed the js/engine-memory-fixes branch from 66c39f2 to 7ddf697 Compare October 6, 2023 06:09

iccub suggested changes Oct 11, 2023

View reviewed changes

iccub marked this pull request as ready for review October 11, 2023 13:53

iccub requested a review from a team as a code owner October 11, 2023 13:53

cuba closed this Oct 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Js/engine memory fixes #8200

Js/engine memory fixes #8200

cuba commented Oct 5, 2023

cuba Oct 6, 2023

cuba Oct 6, 2023

cuba Oct 6, 2023

cuba Oct 6, 2023

cuba Oct 6, 2023

cuba Oct 6, 2023

cuba Oct 6, 2023

cuba Oct 6, 2023

cuba Oct 6, 2023

cuba Oct 6, 2023

cuba Oct 6, 2023

cuba Oct 6, 2023

cuba Oct 6, 2023

cuba Oct 6, 2023

cuba Oct 6, 2023

cuba Oct 6, 2023

cuba Oct 6, 2023

iccub left a comment

cuba commented Oct 11, 2023

cuba commented Oct 11, 2023

		@@ -12,66 +12,228 @@ import os.log
		/// for injected scripts needed during web navigation and cosmetic filters models needed by the `SelectorsPollerScript.js` script.
		public actor AdBlockStats {

Js/engine memory fixes #8200

Js/engine memory fixes #8200

Conversation

cuba commented Oct 5, 2023

Summary of Changes

Submitter Checklist:

Test Plan:

Screenshots:

Reviewer Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iccub left a comment

Choose a reason for hiding this comment

cuba commented Oct 11, 2023

cuba commented Oct 11, 2023