From ff606d7b640ae79257fddadad1ec93259b2dc803 Mon Sep 17 00:00:00 2001 From: bwaresiak Date: Fri, 6 Dec 2024 21:51:39 +0100 Subject: [PATCH] Fix threading issue between using a Semaphore and async/await (#1115) Required: Task/Issue URL: https://app.asana.com/0/856498667320406/1208887821349124/f iOS PR: tba macOS PR:tba What kind of version bump will this require?: Patch Description: Fix threading issue between using a Semaphore and async/await --- .../ContentBlockerRulesManager.swift | 38 +++++----------- ...kingRulesLastCompiledRulesLookupTask.swift | 45 +++++++++++++------ .../ContentBlockingRulesLookupTask.swift | 44 +++++++++++------- 3 files changed, 72 insertions(+), 55 deletions(-) diff --git a/Sources/BrowserServicesKit/ContentBlocking/ContentBlockerRulesManager.swift b/Sources/BrowserServicesKit/ContentBlocking/ContentBlockerRulesManager.swift index c2de45764..c7cb04bd9 100644 --- a/Sources/BrowserServicesKit/ContentBlocking/ContentBlockerRulesManager.swift +++ b/Sources/BrowserServicesKit/ContentBlocking/ContentBlockerRulesManager.swift @@ -242,26 +242,20 @@ public class ContentBlockerRulesManager: CompiledRuleListsSource { Logger.contentBlocking.debug("Lookup compiled rules") prepareSourceManagers() let initialCompilationTask = LookupRulesTask(sourceManagers: Array(sourceManagers.values)) - let mutex = DispatchSemaphore(value: 0) - Task { - do { - try await initialCompilationTask.lookupCachedRulesLists() - } catch { - Logger.contentBlocking.debug("❌ Lookup failed: \(error.localizedDescription, privacy: .public)") - } - mutex.signal() - } - // We want to confine Compilation work to WorkQueue, so we wait to come back from async Task - mutex.wait() + let result: [LookupRulesTask.LookupResult] + + do { + result = try initialCompilationTask.lookupCachedRulesLists() - if let result = initialCompilationTask.result { let rules = result.map(Rules.init(compilationResult:)) - Logger.contentBlocking.debug("🟩 Found \(rules.count, privacy: .public) rules") + Logger.contentBlocking.debug("🟩 Lookup Found \(rules.count, privacy: .public) rules") applyRules(rules) return true + } catch { + Logger.contentBlocking.debug("❌ Lookup failed: \(error.localizedDescription, privacy: .public)") + return false } - return false } /* @@ -273,18 +267,10 @@ public class ContentBlockerRulesManager: CompiledRuleListsSource { let initialCompilationTask = LastCompiledRulesLookupTask(sourceRules: rulesSource.contentBlockerRulesLists, lastCompiledRules: lastCompiledRules) - let mutex = DispatchSemaphore(value: 0) - Task { - try? await initialCompilationTask.fetchCachedRulesLists() - mutex.signal() - } - // We want to confine Compilation work to WorkQueue, so we wait to come back from async Task - mutex.wait() - - let rulesFound = initialCompilationTask.getFetchedRules() + let rules = initialCompilationTask.fetchCachedRulesLists() - if let rulesFound { - applyRules(rulesFound) + if let rules { + applyRules(rules) } else { lock.lock() state = .idle @@ -294,7 +280,7 @@ public class ContentBlockerRulesManager: CompiledRuleListsSource { // No matter if rules were found or not, we need to schedule recompilation, after all scheduleCompilation() - return rulesFound != nil + return rules != nil } private func prepareSourceManagers() { diff --git a/Sources/BrowserServicesKit/ContentBlocking/ContentBlockingRulesLastCompiledRulesLookupTask.swift b/Sources/BrowserServicesKit/ContentBlocking/ContentBlockingRulesLastCompiledRulesLookupTask.swift index 021e058ed..1399e6202 100644 --- a/Sources/BrowserServicesKit/ContentBlocking/ContentBlockingRulesLastCompiledRulesLookupTask.swift +++ b/Sources/BrowserServicesKit/ContentBlocking/ContentBlockingRulesLastCompiledRulesLookupTask.swift @@ -41,32 +41,49 @@ extension ContentBlockerRulesManager { self.lastCompiledRules = lastCompiledRules } - func fetchCachedRulesLists() async throws { + func fetchCachedRulesLists() -> [Rules]? { let sourceRulesNames = sourceRules.map { $0.name } let filteredBySourceLastCompiledRules = lastCompiledRules.filter { sourceRulesNames.contains($0.name) } guard filteredBySourceLastCompiledRules.count == sourceRules.count else { // We should only load rule lists from cache, in case we can match every one of these - throw WKError(.contentRuleListStoreLookUpFailed) + return nil } var result: [CachedRulesList] = [] + let group = DispatchGroup() + for rules in filteredBySourceLastCompiledRules { - guard let ruleList = try await Task(operation: { @MainActor in - try await WKContentRuleListStore.default().contentRuleList(forIdentifier: rules.identifier.stringValue) - }).value else { throw WKError(.contentRuleListStoreLookUpFailed) } - - result.append(CachedRulesList(name: rules.name, - rulesList: ruleList, - tds: rules.trackerData, - rulesIdentifier: rules.identifier)) + group.enter() + + DispatchQueue.main.async { + // This needs to be called from the main thread. + WKContentRuleListStore.default().lookUpContentRuleList(forIdentifier: rules.identifier.stringValue) { ruleList, error in + guard let ruleList, error == nil else { + group.leave() + return + } + + result.append(CachedRulesList(name: rules.name, + rulesList: ruleList, + tds: rules.trackerData, + rulesIdentifier: rules.identifier)) + group.leave() + } + } } - self.result = result + + let operationResult = group.wait(timeout: .now() + 6) + + guard operationResult == .success, result.count == filteredBySourceLastCompiledRules.count else { + return nil + } + + return getRules(from: result) } - public func getFetchedRules() -> [Rules]? { - guard let result else { return nil } - return result.map { + public func getRules(from cached: [CachedRulesList]) -> [Rules] { + return cached.map { let surrogateTDS = ContentBlockerRulesManager.extractSurrogates(from: $0.tds) let encodedData = try? JSONEncoder().encode(surrogateTDS) let encodedTrackerData = String(data: encodedData!, encoding: .utf8)! diff --git a/Sources/BrowserServicesKit/ContentBlocking/ContentBlockingRulesLookupTask.swift b/Sources/BrowserServicesKit/ContentBlocking/ContentBlockingRulesLookupTask.swift index 974c984ac..fadfc6fbf 100644 --- a/Sources/BrowserServicesKit/ContentBlocking/ContentBlockingRulesLookupTask.swift +++ b/Sources/BrowserServicesKit/ContentBlocking/ContentBlockingRulesLookupTask.swift @@ -28,31 +28,45 @@ extension ContentBlockerRulesManager { private let sourceManagers: [ContentBlockerRulesSourceManager] - public private(set) var result: [LookupResult]? - init(sourceManagers: [ContentBlockerRulesSourceManager]) { self.sourceManagers = sourceManagers } - func lookupCachedRulesLists() async throws { + func lookupCachedRulesLists() throws -> [LookupResult] { + + let models = sourceManagers.compactMap { $0.makeModel() } + if models.count != sourceManagers.count { + // We should only load rule lists, in case we can match every one of the expected ones + throw WKError(.contentRuleListStoreLookUpFailed) + } var result = [LookupResult]() - for sourceManager in sourceManagers { - guard let model = sourceManager.makeModel() else { - throw WKError(.contentRuleListStoreLookUpFailed) - } + let group = DispatchGroup() + + for model in models { + group.enter() - guard let ruleList = try await Task(operation: { @MainActor in - try await WKContentRuleListStore.default().contentRuleList(forIdentifier: model.rulesIdentifier.stringValue) - }).value else { - // All lists must be found for this to be considered successful - throw WKError(.contentRuleListStoreLookUpFailed) + DispatchQueue.main.async { + // This needs to be called from the main thread. + WKContentRuleListStore.default().lookUpContentRuleList(forIdentifier: model.rulesIdentifier.stringValue) { ruleList, error in + guard let ruleList, error == nil else { + group.leave() + return + } + + result.append((ruleList, model)) + group.leave() + } } + } + + let operationResult = group.wait(timeout: .now() + 6) - result.append((ruleList, model)) + guard operationResult == .success, result.count == models.count else { + throw WKError(.contentRuleListStoreLookUpFailed) } - self.result = result - } + return result + } } }