Merge branch 'main' into sam/include-dns-range-when-including-local-n…

…etworks * main: Update RMF version matching to ignore build number (#1118) Fix threading issue between using a Semaphore and async/await (#1115) add experiment test fake feature (#1119) Bump Tests/BrowserServicesKitTests/Resources/privacy-reference-tests from `a603ff9` to `6133e7d` (#1109)
duckduckgo · Dec 6, 2024 · 89d10b1 · 89d10b1
2 parents 9ec2896 + ad28b07
commit 89d10b1
Show file tree

Hide file tree

Showing 8 changed files with 129 additions and 70 deletions.
diff --git 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/.../BrowserServicesKit/ContentBlocking/ContentBlockingRulesLastCompiledRulesLookupTask.swift b/.../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
@@ -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
+        }
     }
 }
diff --git a/Sources/BrowserServicesKit/PrivacyConfig/Features/PrivacyFeature.swift b/Sources/BrowserServicesKit/PrivacyConfig/Features/PrivacyFeature.swift
@@ -64,6 +64,7 @@ public enum PrivacyFeature: String {
     case contextualOnboarding
     case textZoom
     case adAttributionReporting
+    case experimentTest
 }
 
 /// An abstraction to be implemented by any "subfeature" of a given `PrivacyConfiguration` feature.
@@ -192,3 +193,8 @@ public enum SyncPromotionSubfeature: String, PrivacySubfeature {
     case bookmarks
     case passwords
 }
+
+public enum ExperimentTestSubfeatures: String, PrivacySubfeature {
+    public var parent: PrivacyFeature { .experimentTest }
+    case experimentTestAA
+}
diff --git a/Sources/RemoteMessaging/Matchers/AppAttributeMatcher.swift b/Sources/RemoteMessaging/Matchers/AppAttributeMatcher.swift
@@ -81,7 +81,7 @@ public struct CommonAppAttributeMatcher: AttributeMatching {
             assertionFailure("BundleIdentifier should not be empty")
         }
         self.init(bundleId: AppVersion.shared.identifier,
-                  appVersion: AppVersion.shared.versionAndBuildNumber,
+                  appVersion: AppVersion.shared.versionNumber,
                   isInternalUser: isInternalUser,
                   statisticsStore: statisticsStore,
                   variantManager: variantManager)

diff --git a/Sources/RemoteMessaging/Model/MatchingAttributes.swift b/Sources/RemoteMessaging/Model/MatchingAttributes.swift
@@ -49,12 +49,27 @@ struct AppIdMatchingAttribute: SingleValueMatching {
 }
 
 struct AppVersionMatchingAttribute: StringRangeMatching {
-    static let defaultMaxValue: String = AppVersion.shared.versionAndBuildNumber
 
-    var min: String = MatchingAttributeDefaults.stringDefaultValue
-    var max: String = AppVersion.shared.versionAndBuildNumber
-    var value: String = MatchingAttributeDefaults.stringDefaultValue
+    static let defaultMaxValue: String = AppVersion.shared.versionNumber
+
+    var min: String
+    var max: String
+    var value: String
     var fallback: Bool?
+
+    // Legacy versions of the app require a build number in the version string in order to match correctly.
+    // To allow message authors to include a build number for backwards compatibility, while also allowing new clients to use the simpler version
+    // string, this initializer trims the build number before storing it.
+    init(min: String = MatchingAttributeDefaults.stringDefaultValue,
+         max: String  = AppVersion.shared.versionNumber,
+         value: String = MatchingAttributeDefaults.stringDefaultValue,
+         fallback: Bool?) {
+        self.min = min.trimmingBuildNumber
+        self.max = max.trimmingBuildNumber
+        self.value = value.trimmingBuildNumber
+        self.fallback = fallback
+    }
+
 }
 
 struct AtbMatchingAttribute: SingleValueMatching {
@@ -306,3 +321,17 @@ struct RangeStringNumericMatchingAttribute: Equatable {
         return version + String(repeating: ".0", count: matchComponents.count - versionComponents.count)
     }
 }
+
+private extension String {
+
+    var trimmingBuildNumber: String {
+        let components = self.split(separator: ".")
+
+        if components.count == 4 {
+            return components.dropLast().joined(separator: ".")
+        } else {
+            return self
+        }
+    }
+
+}
diff --git a/Tests/BrowserServicesKitTests/Resources/privacy-reference-tests b/Tests/BrowserServicesKitTests/Resources/privacy-reference-tests
diff --git a/Tests/RemoteMessagingTests/Matchers/CommonAppAttributeMatcherTests.swift b/Tests/RemoteMessagingTests/Matchers/CommonAppAttributeMatcherTests.swift
@@ -26,6 +26,7 @@ import XCTest
 class CommonAppAttributeMatcherTests: XCTestCase {
 
     private var matcher: CommonAppAttributeMatcher!
+    private let versionNumber = "3.2.1"
 
     override func setUpWithError() throws {
         try super.setUpWithError()
@@ -36,7 +37,13 @@ class CommonAppAttributeMatcherTests: XCTestCase {
         mockStatisticsStore.searchRetentionAtb = "v105-88"
 
         let manager = MockVariantManager(isSupportedReturns: true, currentVariant: MockVariant(name: "zo", weight: 44, isIncluded: { return true }, features: [.dummy]))
-        matcher = CommonAppAttributeMatcher(statisticsStore: mockStatisticsStore, variantManager: manager)
+        matcher = CommonAppAttributeMatcher(
+            bundleId: AppVersion.shared.identifier,
+            appVersion: versionNumber,
+            isInternalUser: true,
+            statisticsStore: mockStatisticsStore,
+            variantManager: manager
+        )
     }
 
     override func tearDownWithError() throws {
@@ -66,7 +73,7 @@ class CommonAppAttributeMatcherTests: XCTestCase {
     }
 
     func testWhenAppVersionEqualOrLowerThanMaxThenReturnMatch() throws {
-        let appVersionComponents = AppVersion.shared.versionAndBuildNumber.components(separatedBy: ".").map { $0 }
+        let appVersionComponents = versionNumber.components(separatedBy: ".").map { $0 }
         let appMajorVersion = appVersionComponents[0]
         let appMinorVersion = appVersionComponents.suffix(from: 1).joined(separator: ".")
 
@@ -82,7 +89,7 @@ class CommonAppAttributeMatcherTests: XCTestCase {
     }
 
     func testWhenAppVersionGreaterThanMaxThenReturnFail() throws {
-        let appVersionComponents = AppVersion.shared.versionAndBuildNumber.components(separatedBy: ".").map { $0 }
+        let appVersionComponents = versionNumber.components(separatedBy: ".").map { $0 }
         let appMajorVersion = appVersionComponents[0]
         let lessThanMax = String(Int(appMajorVersion)! - 1)
 
@@ -91,12 +98,12 @@ class CommonAppAttributeMatcherTests: XCTestCase {
     }
 
     func testWhenAppVersionEqualOrGreaterThanMinThenReturnMatch() throws {
-        XCTAssertEqual(matcher.evaluate(matchingAttribute: AppVersionMatchingAttribute(min: AppVersion.shared.versionAndBuildNumber, fallback: nil)),
+        XCTAssertEqual(matcher.evaluate(matchingAttribute: AppVersionMatchingAttribute(min: versionNumber, fallback: nil)),
                        .match)
     }
 
     func testWhenAppVersionLowerThanMinThenReturnFail() throws {
-        let appVersionComponents = AppVersion.shared.versionAndBuildNumber.components(separatedBy: ".").map { $0 }
+        let appVersionComponents = versionNumber.components(separatedBy: ".").map { $0 }
         let (major, minor, patch) = (appVersionComponents[0], appVersionComponents[1], appVersionComponents[2])
         let majorBumped = String(Int(major)! + 1)
         let patchBumped = [major, minor, String(Int(patch)! + 1)].joined(separator: ".")
@@ -105,7 +112,7 @@ class CommonAppAttributeMatcherTests: XCTestCase {
     }
 
     func testWhenAppVersionInRangeThenReturnMatch() throws {
-        let appVersionComponents = AppVersion.shared.versionAndBuildNumber.components(separatedBy: ".").map { $0 }
+        let appVersionComponents = versionNumber.components(separatedBy: ".").map { $0 }
         let (major, minor, patch) = (appVersionComponents[0], appVersionComponents[1], appVersionComponents[2])
         let majorBumped = String(Int(major)! + 1)
         let patchDecremented = [major, minor, String(Int(patch)! - 1)].joined(separator: ".")
@@ -115,7 +122,7 @@ class CommonAppAttributeMatcherTests: XCTestCase {
     }
 
     func testWhenAppVersionNotInRangeThenReturnFail() throws {
-        let appVersionComponents = AppVersion.shared.versionAndBuildNumber.components(separatedBy: ".").map { $0 }
+        let appVersionComponents = versionNumber.components(separatedBy: ".").map { $0 }
         let appMajorVersion = appVersionComponents[0]
         let greaterThanMax = String(Int(appMajorVersion)! + 1)
 
@@ -124,12 +131,12 @@ class CommonAppAttributeMatcherTests: XCTestCase {
     }
 
     func testWhenAppVersionSameAsDeviceThenReturnMatch() throws {
-        XCTAssertEqual(matcher.evaluate(matchingAttribute: AppVersionMatchingAttribute(min: AppVersion.shared.versionAndBuildNumber, max: AppVersion.shared.versionAndBuildNumber, fallback: nil)),
+        XCTAssertEqual(matcher.evaluate(matchingAttribute: AppVersionMatchingAttribute(min: versionNumber, max: versionNumber, fallback: nil)),
                        .match)
     }
 
     func testWhenAppVersionDifferentToDeviceThenReturnFail() throws {
-        let appVersionComponents = AppVersion.shared.versionAndBuildNumber.components(separatedBy: ".").map { $0 }
+        let appVersionComponents = versionNumber.components(separatedBy: ".").map { $0 }
         let (major, minor, patch) = (appVersionComponents[0], appVersionComponents[1], appVersionComponents[2])
         let patchDecremented = [major, minor, String(Int(patch)! - 1)].joined(separator: ".")
+18 −16		.github/workflows/test.yml
+4 −0		broken-site-reporting/README.md
+38 −0		broken-site-reporting/tests.json
+3 −2		fingerprinting-protections/README.md
+6 −4		fingerprinting-protections/init.js