From b955af7320f995bf2f25ae851f49ca3d5617ad4b Mon Sep 17 00:00:00 2001 From: Jacob Sikorski Date: Mon, 13 Feb 2023 20:31:15 +0100 Subject: [PATCH] Fix #6254: Fix engine crashes by using serial queue for cosmetic filters --- ...rViewController+WKNavigationDelegate.swift | 13 ++-- .../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 | 40 ++++------ .../ShieldStats/Adblock/AdBlockStats.swift | 6 +- .../Adblock/CachedAdBlockEngine.swift | 31 +++++--- 10 files changed, 93 insertions(+), 115 deletions(-) diff --git a/Sources/Brave/Frontend/Browser/BrowserViewController/BrowserViewController+WKNavigationDelegate.swift b/Sources/Brave/Frontend/Browser/BrowserViewController/BrowserViewController+WKNavigationDelegate.swift index 3157dcf4ea7..93157b5a06f 100644 --- a/Sources/Brave/Frontend/Browser/BrowserViewController/BrowserViewController+WKNavigationDelegate.swift +++ b/Sources/Brave/Frontend/Browser/BrowserViewController/BrowserViewController+WKNavigationDelegate.swift @@ -128,6 +128,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 { @@ -278,7 +282,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) } } @@ -364,9 +368,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 @@ -416,7 +419,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 b561b6380ad..f03f88629d2 100644 --- a/Sources/Brave/Frontend/Browser/Playlist/Managers & Cache/PlaylistCacheLoader.swift +++ b/Sources/Brave/Frontend/Browser/Playlist/Managers & Cache/PlaylistCacheLoader.swift @@ -591,7 +591,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) } @@ -633,16 +633,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) } @@ -663,7 +660,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 8eadd3b4cb0..93286ac68fc 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 290a862dee9..115a0d64b0d 100644 --- a/Sources/Brave/WebFilters/ContentBlocker/ContentBlockerManager.swift +++ b/Sources/Brave/WebFilters/ContentBlocker/ContentBlockerManager.swift @@ -313,53 +313,45 @@ 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 filterLists = FilterListResourceDownloader.shared.filterLists + 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 } 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..51595e12dcf 100644 --- a/Sources/Brave/WebFilters/ShieldStats/Adblock/CachedAdBlockEngine.swift +++ b/Sources/Brave/WebFilters/ShieldStats/Adblock/CachedAdBlockEngine.swift @@ -40,19 +40,28 @@ 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) + @MainActor func selectorsForCosmeticRules(frameURL: URL, ids: [String], classes: [String]) async throws -> [String] { + let model = try await cosmeticFilterModel(forFrameURL: frameURL) let selectorsJSON = self.engine.stylesheetForCosmeticRulesIncluding( classes: classes, ids: ids, @@ -86,7 +95,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 +103,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