From e3b2f37267b43ca5ea17352891a511201a82f038 Mon Sep 17 00:00:00 2001 From: Jacob Sikorski Date: Mon, 13 Feb 2023 20:31:15 +0100 Subject: [PATCH 1/6] Fix #6254: Fix engine crashes by using serial queue for cosmetic filters --- ...rViewController+WKNavigationDelegate.swift | 9 +-- .../Browser/LinkPreviewViewController.swift | 4 +- Sources/Brave/Frontend/Browser/PageData.swift | 22 ++++-- .../PlaylistCacheLoader.swift | 11 +-- .../Paged/ContentBlockerScriptHandler.swift | 2 +- .../Paged/CosmeticFiltersScriptHandler.swift | 2 +- .../ContentBlocker/ContentBlockerHelper.swift | 77 ++++++------------- .../ContentBlockerManager.swift | 69 ++++------------- .../ShieldStats/Adblock/AdBlockStats.swift | 6 +- .../Adblock/CachedAdBlockEngine.swift | 61 +++++++++------ .../ContentBlockerManagerTests.swift | 18 +++-- Tests/ClientTests/PageDataTests.swift | 10 +-- 12 files changed, 125 insertions(+), 166 deletions(-) diff --git a/Sources/Brave/Frontend/Browser/BrowserViewController/BrowserViewController+WKNavigationDelegate.swift b/Sources/Brave/Frontend/Browser/BrowserViewController/BrowserViewController+WKNavigationDelegate.swift index 963bcf82bd1..f0eae312d35 100644 --- a/Sources/Brave/Frontend/Browser/BrowserViewController/BrowserViewController+WKNavigationDelegate.swift +++ b/Sources/Brave/Frontend/Browser/BrowserViewController/BrowserViewController+WKNavigationDelegate.swift @@ -279,7 +279,7 @@ extension BrowserViewController: WKNavigationDelegate { // Check if custom user scripts must be added to or removed from the web view. if let targetFrame = navigationAction.targetFrame { tab?.currentPageData?.addSubframeURL(forRequestURL: url, isForMainFrame: targetFrame.isMainFrame) - let scriptTypes = tab?.currentPageData?.makeUserScriptTypes(domain: domainForMainFrame) ?? [] + let scriptTypes = await tab?.currentPageData?.makeUserScriptTypes(domain: domainForMainFrame) ?? [] tab?.setCustomUserScript(scripts: scriptTypes) } } @@ -350,9 +350,8 @@ extension BrowserViewController: WKNavigationDelegate { let domainForShields = Domain.getOrCreate(forUrl: mainDocumentURL, persistent: !isPrivateBrowsing) // Load rule lists - tab?.contentBlocker.ruleListTypes = ContentBlockerManager.shared.compiledRuleTypes( - for: domainForShields - ) + let ruleLists = await ContentBlockerManager.shared.ruleLists(for: domainForShields) + tab?.contentBlocker.set(ruleLists: ruleLists) let isScriptsEnabled = !domainForShields.isShieldExpected(.NoScript, considerAllShieldsOption: true) preferences.allowsContentJavaScript = isScriptsEnabled @@ -402,7 +401,7 @@ extension BrowserViewController: WKNavigationDelegate { if let responseURL = responseURL, let domain = tab?.currentPageData?.domain(persistent: !isPrivateBrowsing), tab?.currentPageData?.upgradeFrameURL(forResponseURL: responseURL, isForMainFrame: navigationResponse.isForMainFrame) == true { - let scriptTypes = tab?.currentPageData?.makeUserScriptTypes(domain: domain) ?? [] + let scriptTypes = await tab?.currentPageData?.makeUserScriptTypes(domain: domain) ?? [] tab?.setCustomUserScript(scripts: scriptTypes) } diff --git a/Sources/Brave/Frontend/Browser/LinkPreviewViewController.swift b/Sources/Brave/Frontend/Browser/LinkPreviewViewController.swift index 4bb311deb99..9324ef2644f 100644 --- a/Sources/Brave/Frontend/Browser/LinkPreviewViewController.swift +++ b/Sources/Brave/Frontend/Browser/LinkPreviewViewController.swift @@ -45,10 +45,10 @@ class LinkPreviewViewController: UIViewController { } let domain = Domain.getOrCreate(forUrl: url, persistent: !PrivateBrowsingManager.shared.isPrivateBrowsing) - let compiledRuleTypes = ContentBlockerManager.shared.compiledRuleTypes(for: domain) Task(priority: .userInitiated) { - let ruleLists = await ContentBlockerManager.shared.loadRuleLists(for: Set(compiledRuleTypes.map({ $0.ruleType }))) + let ruleLists = await ContentBlockerManager.shared.ruleLists(for: domain) + for ruleList in ruleLists { webView.configuration.userContentController.add(ruleList) } diff --git a/Sources/Brave/Frontend/Browser/PageData.swift b/Sources/Brave/Frontend/Browser/PageData.swift index 322e7d8b72b..7a2aeefc3ca 100644 --- a/Sources/Brave/Frontend/Browser/PageData.swift +++ b/Sources/Brave/Frontend/Browser/PageData.swift @@ -70,7 +70,7 @@ struct PageData { } /// Return all the user script types for this page. The number of script types grows as more frames are loaded. - @MainActor func makeUserScriptTypes(domain: Domain) -> Set { + @MainActor func makeUserScriptTypes(domain: Domain) async -> Set { var userScriptTypes: Set = [.siteStateListener] // Handle dynamic domain level scripts on the main document. @@ -98,18 +98,26 @@ struct PageData { } } + let allEngineScriptTypes = await makeAllEngineScripts(for: domain) + return userScriptTypes.union(allEngineScriptTypes) + } + + func makeMainFrameEngineScriptTypes(domain: Domain) async -> Set { + return await adBlockStats.makeEngineScriptTypes(frameURL: mainFrameURL, isMainFrame: true, domain: domain) + } + + func makeAllEngineScripts(for domain: Domain) async -> Set { // Add engine scripts for the main frame - userScriptTypes = userScriptTypes.union( - adBlockStats.makeEngineScriptTypes(frameURL: mainFrameURL, isMainFrame: true, domain: domain) - ) + async let engineScripts = adBlockStats.makeEngineScriptTypes(frameURL: mainFrameURL, isMainFrame: true, domain: domain) // Add engine scripts for all of the known sub-frames - let additionalScriptTypes = allSubframeURLs.map({ frameURL in - return self.adBlockStats.makeEngineScriptTypes(frameURL: frameURL, isMainFrame: false, domain: domain) + async let additionalScriptTypes = allSubframeURLs.asyncConcurrentCompactMap({ frameURL in + return await self.adBlockStats.makeEngineScriptTypes(frameURL: frameURL, isMainFrame: false, domain: domain) }).reduce(Set(), { partialResult, scriptTypes in return partialResult.union(scriptTypes) }) - return userScriptTypes.union(additionalScriptTypes) + let allEngineScripts = await (mainFrame: engineScripts, subFrames: additionalScriptTypes) + return allEngineScripts.mainFrame.union(allEngineScripts.subFrames) } } diff --git a/Sources/Brave/Frontend/Browser/Playlist/Managers & Cache/PlaylistCacheLoader.swift b/Sources/Brave/Frontend/Browser/Playlist/Managers & Cache/PlaylistCacheLoader.swift index db64121c0ee..bf6a458f0e6 100644 --- a/Sources/Brave/Frontend/Browser/Playlist/Managers & Cache/PlaylistCacheLoader.swift +++ b/Sources/Brave/Frontend/Browser/Playlist/Managers & Cache/PlaylistCacheLoader.swift @@ -557,7 +557,7 @@ extension PlaylistWebLoader: WKNavigationDelegate { if let requestURL = navigationAction.request.url, let targetFrame = navigationAction.targetFrame { tab.currentPageData?.addSubframeURL(forRequestURL: requestURL, isForMainFrame: targetFrame.isMainFrame) - let scriptTypes = tab.currentPageData?.makeUserScriptTypes(domain: domainForMainFrame) ?? [] + let scriptTypes = await tab.currentPageData?.makeUserScriptTypes(domain: domainForMainFrame) ?? [] tab.setCustomUserScript(scripts: scriptTypes) } @@ -584,16 +584,13 @@ extension PlaylistWebLoader: WKNavigationDelegate { domainForShields.shield_adblockAndTp = true // Load block lists - let enabledRuleTypes = ContentBlockerManager.shared.compiledRuleTypes(for: domainForShields) - tab.contentBlocker.ruleListTypes = enabledRuleTypes + let ruleLists = await ContentBlockerManager.shared.ruleLists(for: domainForShields) + tab.contentBlocker.set(ruleLists: ruleLists) let isScriptsEnabled = !domainForShields.isShieldExpected(.NoScript, considerAllShieldsOption: true) preferences.allowsContentJavaScript = isScriptsEnabled } - // Cookie Blocking code below - tab.setScript(script: .cookieBlocking, enabled: Preferences.Privacy.blockAllCookies.value) - return (.allow, preferences) } @@ -614,7 +611,7 @@ extension PlaylistWebLoader: WKNavigationDelegate { if let responseURL = responseURL, tab.currentPageData?.upgradeFrameURL(forResponseURL: responseURL, isForMainFrame: navigationResponse.isForMainFrame) == true, let domain = tab.currentPageData?.domain(persistent: false) { - let scriptTypes = tab.currentPageData?.makeUserScriptTypes(domain: domain) ?? [] + let scriptTypes = await tab.currentPageData?.makeUserScriptTypes(domain: domain) ?? [] tab.setCustomUserScript(scripts: scriptTypes) } diff --git a/Sources/Brave/Frontend/UserContent/UserScripts/Scripts_Dynamic/ScriptHandlers/Paged/ContentBlockerScriptHandler.swift b/Sources/Brave/Frontend/UserContent/UserScripts/Scripts_Dynamic/ScriptHandlers/Paged/ContentBlockerScriptHandler.swift index 5626fd13368..be6c090f7d1 100644 --- a/Sources/Brave/Frontend/UserContent/UserScripts/Scripts_Dynamic/ScriptHandlers/Paged/ContentBlockerScriptHandler.swift +++ b/Sources/Brave/Frontend/UserContent/UserScripts/Scripts_Dynamic/ScriptHandlers/Paged/ContentBlockerScriptHandler.swift @@ -82,7 +82,7 @@ extension ContentBlockerHelper: TabContentScript { guard let requestURL = NSURL(idnString: dto.data.resourceURL) as URL? else { return } guard let sourceURL = NSURL(idnString: dto.data.sourceURL) as URL? else { return } guard let domainURLString = domain.url else { return } - let loadedRuleTypes = Set(self.loadedRuleTypeWithSourceTypes.map({ $0.ruleType })) + let loadedRuleTypes = ContentBlockerManager.shared.enabledRuleTypes(for: domain) let blockedType = await TPStatsBlocklistChecker.shared.blockedTypes( requestURL: requestURL, diff --git a/Sources/Brave/Frontend/UserContent/UserScripts/Scripts_Dynamic/ScriptHandlers/Paged/CosmeticFiltersScriptHandler.swift b/Sources/Brave/Frontend/UserContent/UserScripts/Scripts_Dynamic/ScriptHandlers/Paged/CosmeticFiltersScriptHandler.swift index bb6b9ab5e38..a16fc754980 100644 --- a/Sources/Brave/Frontend/UserContent/UserScripts/Scripts_Dynamic/ScriptHandlers/Paged/CosmeticFiltersScriptHandler.swift +++ b/Sources/Brave/Frontend/UserContent/UserScripts/Scripts_Dynamic/ScriptHandlers/Paged/CosmeticFiltersScriptHandler.swift @@ -58,7 +58,7 @@ class CosmeticFiltersScriptHandler: TabContentScript { let selectorArrays = await cachedEngines.asyncConcurrentMap { cachedEngine -> [String] in do { - return try cachedEngine.selectorsForCosmeticRules( + return try await cachedEngine.selectorsForCosmeticRules( frameURL: frameURL, ids: dto.data.ids, classes: dto.data.classes diff --git a/Sources/Brave/WebFilters/ContentBlocker/ContentBlockerHelper.swift b/Sources/Brave/WebFilters/ContentBlocker/ContentBlockerHelper.swift index 42f32d2cd70..48e4e943378 100644 --- a/Sources/Brave/WebFilters/ContentBlocker/ContentBlockerHelper.swift +++ b/Sources/Brave/WebFilters/ContentBlocker/ContentBlockerHelper.swift @@ -45,12 +45,8 @@ enum BlockingStrength: String { class ContentBlockerHelper { private(set) weak var tab: Tab? - /// The rule types and source types that are currently loaded in this tab - private(set) var loadedRuleTypeWithSourceTypes: Set = [] - /// The rule types with their source types that should be loaded in this tab - var ruleListTypes: Set = [] { - didSet { reloadNeededRuleLists() } - } + /// The rule lists that are loaded into the current tab + private var setRuleLists: Set = [] var stats: TPPageStats = TPPageStats() { didSet { @@ -69,58 +65,31 @@ class ContentBlockerHelper { self.tab = tab } - private func reloadNeededRuleLists() { - // Remove old values that were previously added - for loadedRuleTypeWithSourceType in loadedRuleTypeWithSourceTypes { - // Check if should be removed or if the source type doesn't match, otherwise don't do anything - guard !ruleListTypes.contains(loadedRuleTypeWithSourceType) else { - continue - } - - guard let ruleList = ContentBlockerManager.shared.cachedRuleList(for: loadedRuleTypeWithSourceType.ruleType) else { - // For some reason the rule is not cached. Shouldn't happen. - // But if it does we have to remove all the rule lists - // We will add back all the necessary ones below - tab?.webView?.configuration.userContentController.removeAllContentRuleLists() - loadedRuleTypeWithSourceTypes = [] - assertionFailure("This shouldn't happen!") - break - } - - // Since either it shouldn't be included or the source type doesn't match, we remove it + @MainActor func set(ruleLists: Set) { + guard ruleLists != setRuleLists else { return } + #if DEBUG + ContentBlockerManager.log.debug("Set rule lists:") + #endif + + // Remove unwanted rule lists + for ruleList in setRuleLists.subtracting(ruleLists) { + // It's added but we don't want it. So we remove it. tab?.webView?.configuration.userContentController.remove(ruleList) - loadedRuleTypeWithSourceTypes.remove(loadedRuleTypeWithSourceType) + setRuleLists.remove(ruleList) + + #if DEBUG + ContentBlockerManager.log.debug(" - \(ruleList.identifier)") + #endif } - // Add new values that are not yet added (or were removed above because the source type didn't match) - for ruleTypeWithSourceType in ruleListTypes { - // Only add rule lists that are missing - guard !loadedRuleTypeWithSourceTypes.contains(ruleTypeWithSourceType) else { continue } - guard let ruleList = ContentBlockerManager.shared.cachedRuleList(for: ruleTypeWithSourceType.ruleType) else { continue } + // Add missing rule lists + for ruleList in ruleLists.subtracting(setRuleLists) { tab?.webView?.configuration.userContentController.add(ruleList) - loadedRuleTypeWithSourceTypes.insert(ruleTypeWithSourceType) - } - - #if DEBUG - let rulesString = loadedRuleTypeWithSourceTypes.map { ruleTypeWithSourceType -> String in - let ruleTypeString: String - - switch ruleTypeWithSourceType.ruleType { - case .general(let type): - ruleTypeString = type.rawValue - case .filterList(let uuid): - ruleTypeString = "filterList(\(uuid))" - } + setRuleLists.insert(ruleList) - let rulesDebugString = [ - "ruleType: \(ruleTypeString)", - "sourceType: \(ruleTypeWithSourceType.sourceType)" - ].joined(separator: ", ") - - return ["{", rulesDebugString, "}"].joined() - }.joined(separator: ", ") - - log.debug("Loaded \(self.loadedRuleTypeWithSourceTypes.count, privacy: .public) tab rules: \(rulesString, privacy: .public)") - #endif + #if DEBUG + ContentBlockerManager.log.debug(" + \(ruleList.identifier)") + #endif + } } } diff --git a/Sources/Brave/WebFilters/ContentBlocker/ContentBlockerManager.swift b/Sources/Brave/WebFilters/ContentBlocker/ContentBlockerManager.swift index 27f80ab5a86..b7c1fb89e3b 100644 --- a/Sources/Brave/WebFilters/ContentBlocker/ContentBlockerManager.swift +++ b/Sources/Brave/WebFilters/ContentBlocker/ContentBlockerManager.swift @@ -307,53 +307,44 @@ final public class ContentBlockerManager: Sendable { return ruleList } - /// Return the enabled rule types for this domain and the enabled settings - @MainActor public func compiledRuleTypes(for domain: Domain) -> Set { + @MainActor public func enabledRuleTypes(for domain: Domain) -> Set { let filterLists = FilterListResourceDownloader.shared.filterLists if domain.shield_allOff == 1 { return [] } - var results = Set() + var results = Set() // Get domain specific rule types if domain.isShieldExpected(.AdblockAndTp, considerAllShieldsOption: true) { - if let sourceType = self.sourceType(for: .general(.blockAds)) { - results.insert(RuleTypeWithSourceType(ruleType: .general(.blockAds), sourceType: sourceType)) - } - - if let sourceType = self.sourceType(for: .general(.blockTrackers)) { - results.insert(RuleTypeWithSourceType(ruleType: .general(.blockTrackers), sourceType: sourceType)) - } + results.insert(.general(.blockAds)) + results.insert(.general(.blockTrackers)) } // Get filter list specific rule types filterLists.forEach { filterList in guard filterList.isEnabled else { return } - let ruleType = filterList.makeRuleType() - - guard let sourceType = self.sourceType(for: ruleType) else { - return - } - - guard cachedRuleList(for: ruleType) != nil else { - return - } - - results.insert(RuleTypeWithSourceType(ruleType: ruleType, sourceType: sourceType)) + results.insert(filterList.makeRuleType()) } // Get global rule types if Preferences.Privacy.blockAllCookies.value { - if let sourceType = sourceType(for: .general(.blockCookies)) { - results.insert(RuleTypeWithSourceType(ruleType: .general(.blockCookies), sourceType: sourceType)) - } + results.insert(.general(.blockCookies)) } return results } + /// Return the enabled rule types for this domain and the enabled settings + @MainActor public func ruleLists(for domain: Domain) async -> Set { + let ruleTypes = enabledRuleTypes(for: domain) + + return await Set(ruleTypes.asyncConcurrentCompactMap { ruleType in + return self.cachedRuleList(for: ruleType) + }) + } + /// Return a source type for the given rule type public func sourceType(for ruleType: BlocklistRuleType) -> BlocklistSourceType? { guard let compileResult = cachedCompileResults[ruleType.identifier] else { return nil } @@ -367,7 +358,7 @@ final public class ContentBlockerManager: Sendable { } /// Get the cached rule list for this rule type. Returns nil if it's either not compiled or there were errors during compilation - public func cachedRuleList(for ruleType: BlocklistRuleType) -> WKContentRuleList? { + private func cachedRuleList(for ruleType: BlocklistRuleType) -> WKContentRuleList? { guard let compileResult = cachedCompileResults[ruleType.identifier] else { return nil } switch compileResult.result { @@ -375,34 +366,6 @@ final public class ContentBlockerManager: Sendable { case .failure: return nil } } - - /// Load the rule lists for the given rule types - public func loadRuleLists(for ruleTypes: Set) async -> [WKContentRuleList] { - return await withTaskGroup(of: WKContentRuleList?.self) { group in - for ruleType in ruleTypes { - group.addTask { - do { - return try await self.loadRuleList(for: ruleType) - } catch { - Self.log.error("\(error.localizedDescription)") - return nil - } - } - } - - var results: [WKContentRuleList] = [] - for await ruleList in group { - guard let ruleList = ruleList else { continue } - results.append(ruleList) - } - return results - } - } - - /// Load the rule list for the given rule type - public func loadRuleList(for ruleType: BlocklistRuleType) async throws -> WKContentRuleList? { - return try await ruleStore.contentRuleList(forIdentifier: ruleType.identifier) - } /// Compile the data and set it for the given general type. private func getBundledResource(for type: GeneralBlocklistTypes) async -> Resource? { diff --git a/Sources/Brave/WebFilters/ShieldStats/Adblock/AdBlockStats.swift b/Sources/Brave/WebFilters/ShieldStats/Adblock/AdBlockStats.swift index fc892af3e3b..4464a55faec 100644 --- a/Sources/Brave/WebFilters/ShieldStats/Adblock/AdBlockStats.swift +++ b/Sources/Brave/WebFilters/ShieldStats/Adblock/AdBlockStats.swift @@ -43,13 +43,13 @@ public class AdBlockStats { } /// This returns all the user script types for the given frame - @MainActor func makeEngineScriptTypes(frameURL: URL, isMainFrame: Bool, domain: Domain) -> Set { + @MainActor func makeEngineScriptTypes(frameURL: URL, isMainFrame: Bool, domain: Domain) async -> Set { // Add any engine scripts for this frame - return cachedEngines.enumerated().map({ (index, cachedEngine) -> Set in + return await cachedEngines.enumerated().asyncMap({ (index, cachedEngine) -> Set in guard cachedEngine.isEnabled(for: domain) else { return [] } do { - return try cachedEngine.makeEngineScriptTypes( + return try await cachedEngine.makeEngineScriptTypes( frameURL: frameURL, isMainFrame: isMainFrame, domain: domain, index: index ) } catch { diff --git a/Sources/Brave/WebFilters/ShieldStats/Adblock/CachedAdBlockEngine.swift b/Sources/Brave/WebFilters/ShieldStats/Adblock/CachedAdBlockEngine.swift index d4c79676d7e..7976006f720 100644 --- a/Sources/Brave/WebFilters/ShieldStats/Adblock/CachedAdBlockEngine.swift +++ b/Sources/Brave/WebFilters/ShieldStats/Adblock/CachedAdBlockEngine.swift @@ -40,31 +40,48 @@ public class CachedAdBlockEngine { /// Returns all the models for this frame URL /// The results are cached per url, so you may call this method as many times for the same url without any performance implications. - @MainActor func cosmeticFilterModel(forFrameURL frameURL: URL) throws -> CosmeticFilterModel? { - if let model = self.cachedCosmeticFilterModels.getElement(frameURL) { - return model + func cosmeticFilterModel(forFrameURL frameURL: URL) async throws -> CosmeticFilterModel? { + return try await withCheckedThrowingContinuation { continuation in + serialQueue.async { [weak self] in + if let model = self?.cachedCosmeticFilterModels.getElement(frameURL) { + continuation.resume(returning: model) + return + } + + do { + let model = try self?.engine.cosmeticFilterModel(forFrameURL: frameURL) + self?.cachedCosmeticFilterModels.addElement(model, forKey: frameURL) + continuation.resume(returning: model) + } catch { + continuation.resume(throwing: error) + } + } } - - let model = try self.engine.cosmeticFilterModel(forFrameURL: frameURL) - self.cachedCosmeticFilterModels.addElement(model, forKey: frameURL) - return model } /// Return the selectors that need to be hidden given the frameURL, ids and classes - @MainActor func selectorsForCosmeticRules(frameURL: URL, ids: [String], classes: [String]) throws -> [String] { - let model = try cosmeticFilterModel(forFrameURL: frameURL) - let selectorsJSON = self.engine.stylesheetForCosmeticRulesIncluding( - classes: classes, - ids: ids, - exceptions: model?.exceptions ?? [] - ) - - guard let data = selectorsJSON.data(using: .utf8) else { - return [] + func selectorsForCosmeticRules(frameURL: URL, ids: [String], classes: [String]) async throws -> [String] { + let exceptions = try await cosmeticFilterModel(forFrameURL: frameURL)?.exceptions ?? [] + + return try await withCheckedThrowingContinuation { continuation in + serialQueue.async { [weak self] in + let selectorsJSON = self?.engine.stylesheetForCosmeticRulesIncluding(classes: classes, ids: ids, exceptions: exceptions) + + guard let data = selectorsJSON?.data(using: .utf8) else { + continuation.resume(returning: []) + return + } + + let decoder = JSONDecoder() + + do { + let result = try decoder.decode([String].self, from: data) + continuation.resume(returning: result) + } catch { + continuation.resume(throwing: error) + } + } } - - let decoder = JSONDecoder() - return try decoder.decode([String].self, from: data) } /// Checks the general and regional engines to see if the request should be blocked @@ -86,7 +103,7 @@ public class CachedAdBlockEngine { } /// This returns all the user script types for the given frame - @MainActor func makeEngineScriptTypes(frameURL: URL, isMainFrame: Bool, domain: Domain, index: Int) throws -> Set { + @MainActor func makeEngineScriptTypes(frameURL: URL, isMainFrame: Bool, domain: Domain, index: Int) async throws -> Set { if let userScriptTypes = cachedFrameScriptTypes.getElement(frameURL) { return userScriptTypes } @@ -94,7 +111,7 @@ public class CachedAdBlockEngine { // Add the selectors poller scripts for this frame var userScriptTypes: Set = [] - if let source = try cosmeticFilterModel(forFrameURL: frameURL)?.injectedScript, !source.isEmpty { + if let source = try await cosmeticFilterModel(forFrameURL: frameURL)?.injectedScript, !source.isEmpty { let configuration = UserScriptType.EngineScriptConfiguration( frameURL: frameURL, isMainFrame: isMainFrame, source: source, order: index, isDeAMPEnabled: Preferences.Shields.autoRedirectAMPPages.value diff --git a/Tests/ClientTests/ContentBlockerManagerTests.swift b/Tests/ClientTests/ContentBlockerManagerTests.swift index aa131c9ac8b..fdfb16af5af 100644 --- a/Tests/ClientTests/ContentBlockerManagerTests.swift +++ b/Tests/ClientTests/ContentBlockerManagerTests.swift @@ -5,6 +5,7 @@ import XCTest import WebKit +import Data @testable import Brave class ContentBlockerManagerTests: XCTestCase { @@ -22,6 +23,7 @@ class ContentBlockerManagerTests: XCTestCase { let downloadedSourceType = ContentBlockerManager.BlocklistSourceType.downloaded(version: "123") let expectation = XCTestExpectation(description: "Test loading resources") let manager = ContentBlockerManager(ruleStore: ruleStore) + let domain = Domain.getOrCreate(forUrl: URL(string: "https://example.com")!, persistent: false) Task.detached { // When @@ -32,19 +34,22 @@ class ContentBlockerManagerTests: XCTestCase { ), for: downloadedRuleType) await manager.compilePendingResources() + let returnedCachedRuleLists = await manager.ruleLists(for: domain) await MainActor.run { // Then // Check for the correct source and cached rule list for generalType in ContentBlockerManager.GeneralBlocklistTypes.allCases { let returnedSourceType = manager.sourceType(for: .general(generalType)) - let returnedCachedRuleList = manager.cachedRuleList(for: .general(generalType)) + switch generalType { case .blockAds: // Check for downloaded rule type XCTAssertEqual(returnedSourceType, downloadedSourceType) - XCTAssertNotNil(returnedCachedRuleList) - case .blockCookies, .blockTrackers: + XCTAssertTrue(returnedCachedRuleLists.contains(where: { + $0.identifier == ContentBlockerManager.BlocklistRuleType.general(generalType).identifier + })) + case .blockCookies: // Check for bundled rule type XCTAssertEqual(returnedSourceType, .bundled) XCTAssertNotNil(returnedCachedRuleList) @@ -61,9 +66,10 @@ class ContentBlockerManagerTests: XCTestCase { manager.sourceType(for: downloadedRuleType), .bundled ) - XCTAssertNotNil( - manager.cachedRuleList(for: .general(.blockAds)) - ) + let returnedCachedRuleLists = await manager.ruleLists(for: domain) + XCTAssertTrue(returnedCachedRuleLists.contains(where: { + $0.identifier == downloadedRuleType.identifier + })) expectation.fulfill() } diff --git a/Tests/ClientTests/PageDataTests.swift b/Tests/ClientTests/PageDataTests.swift index 87ad22a2b06..7ce2455e9bc 100644 --- a/Tests/ClientTests/PageDataTests.swift +++ b/Tests/ClientTests/PageDataTests.swift @@ -22,7 +22,7 @@ final class PageDataTests: XCTestCase { // When // We get the script types for the main frame let domain = pageData.domain(persistent: false) - let mainFrameRequestTypes = pageData.makeUserScriptTypes(domain: domain) + let mainFrameRequestTypes = await pageData.makeUserScriptTypes(domain: domain) // Then // We get only entries of the main frame @@ -34,7 +34,7 @@ final class PageDataTests: XCTestCase { // When // Nothing has changed - let unchangedScriptTypes = pageData.makeUserScriptTypes(domain: domain) + let unchangedScriptTypes = await pageData.makeUserScriptTypes(domain: domain) // Then // We get the same result as before @@ -48,7 +48,7 @@ final class PageDataTests: XCTestCase { // We get an aditional scripts for sub-frame // NOTE: This is because we have no engines on AdBlockStats. // If we were to add some engines we might see additional types - let addedSubFrameFrameRequestTypes = pageData.makeUserScriptTypes(domain: domain) + let addedSubFrameFrameRequestTypes = await pageData.makeUserScriptTypes(domain: domain) let expectedMainAndSubFrameTypes: Set = [ .siteStateListener, .nacl, .farblingProtection(etld: "example.com") ] @@ -61,7 +61,7 @@ final class PageDataTests: XCTestCase { // Then // We get the same result as before XCTAssertTrue(isUpgradedMainFrame) - let upgradedMainFrameRequestTypes = pageData.makeUserScriptTypes(domain: domain) + let upgradedMainFrameRequestTypes = await pageData.makeUserScriptTypes(domain: domain) XCTAssertEqual(upgradedMainFrameRequestTypes, unchangedScriptTypes) // When @@ -71,7 +71,7 @@ final class PageDataTests: XCTestCase { // Then // We get the same result as before XCTAssertTrue(isUpgradedSubFrame) - let upgradedSubFrameFrameRequestTypes = pageData.makeUserScriptTypes(domain: domain) + let upgradedSubFrameFrameRequestTypes = await pageData.makeUserScriptTypes(domain: domain) XCTAssertEqual(upgradedSubFrameFrameRequestTypes, unchangedScriptTypes) expectation.fulfill() From b5e204d6a837e6a2c8c7ef98843ded4d686646a6 Mon Sep 17 00:00:00 2001 From: Jacob Sikorski Date: Tue, 7 Feb 2023 18:00:39 +0100 Subject: [PATCH 2/6] Add debugging logs --- ...rViewController+WKNavigationDelegate.swift | 4 ++ .../Browser/Helpers/LaunchHelper.swift | 9 ++++ .../WebFilters/AdBlockEngineManager.swift | 32 ++++++------ .../ContentBlockerManager.swift | 49 +++++++++---------- 4 files changed, 52 insertions(+), 42 deletions(-) diff --git a/Sources/Brave/Frontend/Browser/BrowserViewController/BrowserViewController+WKNavigationDelegate.swift b/Sources/Brave/Frontend/Browser/BrowserViewController/BrowserViewController+WKNavigationDelegate.swift index f0eae312d35..39cdf5e7897 100644 --- a/Sources/Brave/Frontend/Browser/BrowserViewController/BrowserViewController+WKNavigationDelegate.swift +++ b/Sources/Brave/Frontend/Browser/BrowserViewController/BrowserViewController+WKNavigationDelegate.swift @@ -129,6 +129,10 @@ extension BrowserViewController: WKNavigationDelegate { guard let url = navigationAction.request.url else { return (.cancel, preferences) } + + #if DEBUG + ContentBlockerManager.log.debug("Nav: \(url.absoluteString)") + #endif if InternalURL.isValid(url: url) { if navigationAction.navigationType != .backForward, navigationAction.isInternalUnprivileged { diff --git a/Sources/Brave/Frontend/Browser/Helpers/LaunchHelper.swift b/Sources/Brave/Frontend/Browser/Helpers/LaunchHelper.swift index 987eb337eb6..02e4f6df029 100644 --- a/Sources/Brave/Frontend/Browser/Helpers/LaunchHelper.swift +++ b/Sources/Brave/Frontend/Browser/Helpers/LaunchHelper.swift @@ -27,6 +27,10 @@ public actor LaunchHelper { // Otherwise prepare the services and await the task let task = Task { + #if DEBUG + let startTime = CFAbsoluteTimeGetCurrent() + #endif + // Load cached data // This is done first because compileResources and loadCachedRuleLists need their results async let loadBundledResources: Void = ContentBlockerManager.shared.loadBundledResources() @@ -39,6 +43,11 @@ public actor LaunchHelper { async let cachedRuleListLoad: Void = ContentBlockerManager.shared.loadCachedRuleLists() _ = await (compiledResourcesCompile, cachedRuleListLoad) + #if DEBUG + let timeElapsed = CFAbsoluteTimeGetCurrent() - startTime + ContentBlockerManager.log.debug("Adblock loaded: \(timeElapsed)s") + #endif + // This one is non-blocking performPostLoadTasks(adBlockService: adBlockService) markAdBlockReady() diff --git a/Sources/Brave/WebFilters/AdBlockEngineManager.swift b/Sources/Brave/WebFilters/AdBlockEngineManager.swift index 002fd80cf20..975080878ca 100644 --- a/Sources/Brave/WebFilters/AdBlockEngineManager.swift +++ b/Sources/Brave/WebFilters/AdBlockEngineManager.swift @@ -187,38 +187,36 @@ public actor AdBlockEngineManager: Sendable { extension AdBlockEngineManager { /// A method that logs info on the given resources fileprivate func debug(compiledResults: [ResourceWithVersion: Result]) { - let resourcesString = compiledResults.sorted(by: { $0.key.order < $1.key.order }) - .map { (resourceWithVersion, compileResult) -> String in + log.debug("Loaded \(compiledResults.count, privacy: .public) (total) engine resources:") + + compiledResults.sorted(by: { $0.key.order < $1.key.order }) + .forEach { (resourceWithVersion, compileResult) in let resultString: String switch compileResult { case .success: - resultString = "success" + resultString = "✔︎" case .failure(let error): - resultString = error.localizedDescription + resultString = "\(error)" } let sourceDebugString = [ - resourceWithVersion.debugDescription, - "result: \(resultString)", - ].joined(separator: ", ") + "", resourceWithVersion.debugDescription, + "\(resultString)", + ].joined(separator: " ") - return ["{", sourceDebugString, "}"].joined() - }.joined(separator: ", ") - - log.debug("Loaded \(self.enabledResources.count, privacy: .public) (total) engine resources: \(resourcesString, privacy: .public)") + log.debug("\(sourceDebugString, privacy: .public)") + } } } extension AdBlockEngineManager.ResourceWithVersion: CustomDebugStringConvertible { public var debugDescription: String { return [ - "order: \(order)", - "fileName: \(fileURL.lastPathComponent)", - "source: \(resource.source.debugDescription)", - "version: \(version ?? "nil")", - "type: \(resource.type.debugDescription)" - ].joined(separator: ", ") + "#\(order)", + "\(resource.source.debugDescription).\(resource.type.debugDescription)", + "v\(version ?? "nil")", + ].joined(separator: " ") } } diff --git a/Sources/Brave/WebFilters/ContentBlocker/ContentBlockerManager.swift b/Sources/Brave/WebFilters/ContentBlocker/ContentBlockerManager.swift index b7c1fb89e3b..c2842022b67 100644 --- a/Sources/Brave/WebFilters/ContentBlocker/ContentBlockerManager.swift +++ b/Sources/Brave/WebFilters/ContentBlocker/ContentBlockerManager.swift @@ -388,45 +388,44 @@ final public class ContentBlockerManager: Sendable { #if DEBUG /// A method that logs info on the given resources private func debug(resources: [String: Resource]) { - let resourcesString = resources + Self.log.debug("Compiled \(resources.count, privacy: .public) block list resources:") + + resources .sorted(by: { lhs, rhs in lhs.value.url.absoluteString < rhs.value.url.absoluteString }) - .map { identifier, compiledResource -> String in - let sourceString: String - let versionString: String + .forEach { identifier, compiledResource in let resultString: String - switch compiledResource.sourceType { - case .bundled: - sourceString = "bundled" - versionString = "nil" - case .downloaded(let version): - sourceString = "downloaded" - versionString = version ?? "nil" - } - switch self.cachedCompileResults[identifier]?.result { case .failure(let error): resultString = error.localizedDescription case .success: - resultString = "success" + resultString = "✔︎" case .none: - resultString = "nil" + resultString = "?" } let resourcesDebugString = [ - "identifier: \(identifier)", - "fileName: \(compiledResource.url.lastPathComponent)", - "source: \(sourceString)", - "version: \(versionString)", - "result: \(resultString)" - ].joined(separator: ", ") + identifier, compiledResource.sourceType.debugDescription, + resultString + ].joined(separator: " ") - return ["{", resourcesDebugString, "}"].joined() - }.joined(separator: ", ") - - Self.log.debug("Compiled \(resources.count, privacy: .public) additional block list resources: \(resourcesString, privacy: .public))") + Self.log.debug(" \(resourcesDebugString)") + } } #endif } + +#if DEBUG +extension ContentBlockerManager.BlocklistSourceType: CustomDebugStringConvertible { + public var debugDescription: String { + switch self { + case .bundled: + return "bundled" + case .downloaded(let version): + return version ?? "nil" + } + } +} +#endif From 94791aade889034713f95379c242142395eb8682 Mon Sep 17 00:00:00 2001 From: Jacob Sikorski Date: Fri, 3 Mar 2023 18:36:56 +0100 Subject: [PATCH 3/6] Fix for review --- .../BrowserViewController+WKNavigationDelegate.swift | 2 +- Sources/Brave/WebFilters/AdBlockEngineManager.swift | 4 ++-- .../WebFilters/ContentBlocker/ContentBlockerManager.swift | 4 ++-- .../Brave/WebFilters/ShieldStats/Adblock/AdBlockStats.swift | 4 ++-- Tests/ClientTests/ContentBlockerManagerTests.swift | 6 +++--- 5 files changed, 10 insertions(+), 10 deletions(-) diff --git a/Sources/Brave/Frontend/Browser/BrowserViewController/BrowserViewController+WKNavigationDelegate.swift b/Sources/Brave/Frontend/Browser/BrowserViewController/BrowserViewController+WKNavigationDelegate.swift index 39cdf5e7897..3a4eea0ee1c 100644 --- a/Sources/Brave/Frontend/Browser/BrowserViewController/BrowserViewController+WKNavigationDelegate.swift +++ b/Sources/Brave/Frontend/Browser/BrowserViewController/BrowserViewController+WKNavigationDelegate.swift @@ -354,7 +354,7 @@ extension BrowserViewController: WKNavigationDelegate { let domainForShields = Domain.getOrCreate(forUrl: mainDocumentURL, persistent: !isPrivateBrowsing) // Load rule lists - let ruleLists = await ContentBlockerManager.shared.ruleLists(for: domainForShields) + let ruleLists = ContentBlockerManager.shared.ruleLists(for: domainForShields) tab?.contentBlocker.set(ruleLists: ruleLists) let isScriptsEnabled = !domainForShields.isShieldExpected(.NoScript, considerAllShieldsOption: true) diff --git a/Sources/Brave/WebFilters/AdBlockEngineManager.swift b/Sources/Brave/WebFilters/AdBlockEngineManager.swift index 975080878ca..30a12b23511 100644 --- a/Sources/Brave/WebFilters/AdBlockEngineManager.swift +++ b/Sources/Brave/WebFilters/AdBlockEngineManager.swift @@ -184,9 +184,9 @@ public actor AdBlockEngineManager: Sendable { } #if DEBUG -extension AdBlockEngineManager { +private extension AdBlockEngineManager { /// A method that logs info on the given resources - fileprivate func debug(compiledResults: [ResourceWithVersion: Result]) { + func debug(compiledResults: [ResourceWithVersion: Result]) { log.debug("Loaded \(compiledResults.count, privacy: .public) (total) engine resources:") compiledResults.sorted(by: { $0.key.order < $1.key.order }) diff --git a/Sources/Brave/WebFilters/ContentBlocker/ContentBlockerManager.swift b/Sources/Brave/WebFilters/ContentBlocker/ContentBlockerManager.swift index c2842022b67..a5c8ee943a2 100644 --- a/Sources/Brave/WebFilters/ContentBlocker/ContentBlockerManager.swift +++ b/Sources/Brave/WebFilters/ContentBlocker/ContentBlockerManager.swift @@ -337,10 +337,10 @@ final public class ContentBlockerManager: Sendable { } /// Return the enabled rule types for this domain and the enabled settings - @MainActor public func ruleLists(for domain: Domain) async -> Set { + @MainActor public func ruleLists(for domain: Domain) -> Set { let ruleTypes = enabledRuleTypes(for: domain) - return await Set(ruleTypes.asyncConcurrentCompactMap { ruleType in + return Set(ruleTypes.compactMap { ruleType in return self.cachedRuleList(for: ruleType) }) } diff --git a/Sources/Brave/WebFilters/ShieldStats/Adblock/AdBlockStats.swift b/Sources/Brave/WebFilters/ShieldStats/Adblock/AdBlockStats.swift index 4464a55faec..6e81d0d75e4 100644 --- a/Sources/Brave/WebFilters/ShieldStats/Adblock/AdBlockStats.swift +++ b/Sources/Brave/WebFilters/ShieldStats/Adblock/AdBlockStats.swift @@ -43,10 +43,10 @@ public class AdBlockStats { } /// This returns all the user script types for the given frame - @MainActor func makeEngineScriptTypes(frameURL: URL, isMainFrame: Bool, domain: Domain) async -> Set { + func makeEngineScriptTypes(frameURL: URL, isMainFrame: Bool, domain: Domain) async -> Set { // Add any engine scripts for this frame return await cachedEngines.enumerated().asyncMap({ (index, cachedEngine) -> Set in - guard cachedEngine.isEnabled(for: domain) else { return [] } + guard await cachedEngine.isEnabled(for: domain) else { return [] } do { return try await cachedEngine.makeEngineScriptTypes( diff --git a/Tests/ClientTests/ContentBlockerManagerTests.swift b/Tests/ClientTests/ContentBlockerManagerTests.swift index fdfb16af5af..65dbab2e721 100644 --- a/Tests/ClientTests/ContentBlockerManagerTests.swift +++ b/Tests/ClientTests/ContentBlockerManagerTests.swift @@ -49,10 +49,10 @@ class ContentBlockerManagerTests: XCTestCase { XCTAssertTrue(returnedCachedRuleLists.contains(where: { $0.identifier == ContentBlockerManager.BlocklistRuleType.general(generalType).identifier })) - case .blockCookies: + case .blockCookies, .blockTrackers: // Check for bundled rule type XCTAssertEqual(returnedSourceType, .bundled) - XCTAssertNotNil(returnedCachedRuleList) + XCTAssertNotNil(returnedSourceType) } } } @@ -66,7 +66,7 @@ class ContentBlockerManagerTests: XCTestCase { manager.sourceType(for: downloadedRuleType), .bundled ) - let returnedCachedRuleLists = await manager.ruleLists(for: domain) + let returnedCachedRuleLists = manager.ruleLists(for: domain) XCTAssertTrue(returnedCachedRuleLists.contains(where: { $0.identifier == downloadedRuleType.identifier })) From 003beeebcf3a785763db48adfbbbc1c3a0fb4c4a Mon Sep 17 00:00:00 2001 From: Jacob Sikorski Date: Tue, 7 Mar 2023 19:42:05 +0100 Subject: [PATCH 4/6] Fix for review --- .../Browser/Playlist/Managers & Cache/PlaylistCacheLoader.swift | 2 ++ Sources/Brave/WebFilters/AdBlockEngineManager.swift | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/Sources/Brave/Frontend/Browser/Playlist/Managers & Cache/PlaylistCacheLoader.swift b/Sources/Brave/Frontend/Browser/Playlist/Managers & Cache/PlaylistCacheLoader.swift index bf6a458f0e6..d881c938a46 100644 --- a/Sources/Brave/Frontend/Browser/Playlist/Managers & Cache/PlaylistCacheLoader.swift +++ b/Sources/Brave/Frontend/Browser/Playlist/Managers & Cache/PlaylistCacheLoader.swift @@ -591,6 +591,8 @@ extension PlaylistWebLoader: WKNavigationDelegate { preferences.allowsContentJavaScript = isScriptsEnabled } + // Cookie Blocking code below + tab.setScript(script: .cookieBlocking, enabled: Preferences.Privacy.blockAllCookies.value) return (.allow, preferences) } diff --git a/Sources/Brave/WebFilters/AdBlockEngineManager.swift b/Sources/Brave/WebFilters/AdBlockEngineManager.swift index 30a12b23511..763ca8b7d6c 100644 --- a/Sources/Brave/WebFilters/AdBlockEngineManager.swift +++ b/Sources/Brave/WebFilters/AdBlockEngineManager.swift @@ -205,7 +205,7 @@ private extension AdBlockEngineManager { "\(resultString)", ].joined(separator: " ") - log.debug("\(sourceDebugString, privacy: .public)") + log.debug("\(sourceDebugString, privacy: .private)") } } } From 2e7ebd6b641548facc1f87de8feca174c6e865d3 Mon Sep 17 00:00:00 2001 From: Jacob Sikorski Date: Fri, 10 Mar 2023 20:13:04 +0100 Subject: [PATCH 5/6] Fix for review --- Sources/Brave/WebFilters/AdBlockEngineManager.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Sources/Brave/WebFilters/AdBlockEngineManager.swift b/Sources/Brave/WebFilters/AdBlockEngineManager.swift index 763ca8b7d6c..c282d8737f2 100644 --- a/Sources/Brave/WebFilters/AdBlockEngineManager.swift +++ b/Sources/Brave/WebFilters/AdBlockEngineManager.swift @@ -187,7 +187,7 @@ public actor AdBlockEngineManager: Sendable { private extension AdBlockEngineManager { /// A method that logs info on the given resources func debug(compiledResults: [ResourceWithVersion: Result]) { - log.debug("Loaded \(compiledResults.count, privacy: .public) (total) engine resources:") + log.debug("Loaded \(compiledResults.count, privacy: .private) (total) engine resources:") compiledResults.sorted(by: { $0.key.order < $1.key.order }) .forEach { (resourceWithVersion, compileResult) in From 2bad56ee7159955c381a84f9eed5d94e0ae48a4b Mon Sep 17 00:00:00 2001 From: Jacob Sikorski Date: Tue, 14 Mar 2023 19:58:40 +0100 Subject: [PATCH 6/6] Fix for review --- Sources/Brave/WebFilters/AdBlockEngineManager.swift | 4 ++-- .../WebFilters/ContentBlocker/ContentBlockerManager.swift | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Sources/Brave/WebFilters/AdBlockEngineManager.swift b/Sources/Brave/WebFilters/AdBlockEngineManager.swift index c282d8737f2..268b20cbccf 100644 --- a/Sources/Brave/WebFilters/AdBlockEngineManager.swift +++ b/Sources/Brave/WebFilters/AdBlockEngineManager.swift @@ -187,7 +187,7 @@ public actor AdBlockEngineManager: Sendable { private extension AdBlockEngineManager { /// A method that logs info on the given resources func debug(compiledResults: [ResourceWithVersion: Result]) { - log.debug("Loaded \(compiledResults.count, privacy: .private) (total) engine resources:") + log.debug("Loaded \(compiledResults.count) (total) engine resources:") compiledResults.sorted(by: { $0.key.order < $1.key.order }) .forEach { (resourceWithVersion, compileResult) in @@ -205,7 +205,7 @@ private extension AdBlockEngineManager { "\(resultString)", ].joined(separator: " ") - log.debug("\(sourceDebugString, privacy: .private)") + log.debug("\(sourceDebugString)") } } } diff --git a/Sources/Brave/WebFilters/ContentBlocker/ContentBlockerManager.swift b/Sources/Brave/WebFilters/ContentBlocker/ContentBlockerManager.swift index a5c8ee943a2..f22b4561f1e 100644 --- a/Sources/Brave/WebFilters/ContentBlocker/ContentBlockerManager.swift +++ b/Sources/Brave/WebFilters/ContentBlocker/ContentBlockerManager.swift @@ -388,7 +388,7 @@ final public class ContentBlockerManager: Sendable { #if DEBUG /// A method that logs info on the given resources private func debug(resources: [String: Resource]) { - Self.log.debug("Compiled \(resources.count, privacy: .public) block list resources:") + Self.log.debug("Compiled \(resources.count) block list resources:") resources .sorted(by: { lhs, rhs in