From e027724bcdac65fe2df0c175ac5f5b70c2057de0 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Fri, 6 Dec 2024 11:16:35 +0100 Subject: [PATCH 1/4] Bump Tests/BrowserServicesKitTests/Resources/privacy-reference-tests from `a603ff9` to `6133e7d` (#1109) Bumps [Tests/BrowserServicesKitTests/Resources/privacy-reference-tests](https://github.com/duckduckgo/privacy-reference-tests) from `a603ff9` to `6133e7d`.
Commits

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) ---
Dependabot commands and options
You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot show ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- Tests/BrowserServicesKitTests/Resources/privacy-reference-tests | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tests/BrowserServicesKitTests/Resources/privacy-reference-tests b/Tests/BrowserServicesKitTests/Resources/privacy-reference-tests index a603ff9af..6133e7d9d 160000 --- a/Tests/BrowserServicesKitTests/Resources/privacy-reference-tests +++ b/Tests/BrowserServicesKitTests/Resources/privacy-reference-tests @@ -1 +1 @@ -Subproject commit a603ff9af22ca3ff7ce2e7ffbfe18c447d9f23e8 +Subproject commit 6133e7d9d9cd5f1b925cab1971b4d785dc639df7 From bbcb41c87c5788718a43883b5b10eb3b4f54aff3 Mon Sep 17 00:00:00 2001 From: Sabrina Tardio <44158575+SabrinaTardio@users.noreply.github.com> Date: Fri, 6 Dec 2024 17:53:18 +0100 Subject: [PATCH 2/4] add experiment test fake feature (#1119) **Required**: Task/Issue URL: https://app.asana.com/0/72649045549333/1208905776162821/f iOS PR: https://github.com/duckduckgo/iOS/pull/3688 macOS PR: https://github.com/duckduckgo/macos-browser/pull/3637 What kind of version bump will this require?: Patch **Description**: Adds Fake Test feature to do and A/A test run of the New experiment framework. We want to make sure cohort is assigned and pixels are fired as expected. --- .../PrivacyConfig/Features/PrivacyFeature.swift | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/Sources/BrowserServicesKit/PrivacyConfig/Features/PrivacyFeature.swift b/Sources/BrowserServicesKit/PrivacyConfig/Features/PrivacyFeature.swift index c8a7ea892..3c6ccc61b 100644 --- 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 +} From ff606d7b640ae79257fddadad1ec93259b2dc803 Mon Sep 17 00:00:00 2001 From: bwaresiak Date: Fri, 6 Dec 2024 21:51:39 +0100 Subject: [PATCH 3/4] 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 + } } } From ad28b07d2df9f2ec45ab0306053126e2c8b237ca Mon Sep 17 00:00:00 2001 From: Sam Symons Date: Fri, 6 Dec 2024 12:52:01 -0800 Subject: [PATCH 4/4] Update RMF version matching to ignore build number (#1118) Required: Task/Issue URL: https://app.asana.com/0/1207619243206445/1208908529122506/f iOS PR: duckduckgo/iOS#3686 macOS PR: duckduckgo/macos-browser#3635 What kind of version bump will this require?: Patch Description: This PR updates the RMF version matching logic to ignore build number. Previously, when you tried to match a version like 1.110.0, it would fail because you needed to know the build number. --- .../Matchers/AppAttributeMatcher.swift | 2 +- .../Model/MatchingAttributes.swift | 37 +++++++++++++++++-- .../CommonAppAttributeMatcherTests.swift | 25 ++++++++----- 3 files changed, 50 insertions(+), 14 deletions(-) diff --git a/Sources/RemoteMessaging/Matchers/AppAttributeMatcher.swift b/Sources/RemoteMessaging/Matchers/AppAttributeMatcher.swift index 1805ebbd0..677ca9f3a 100644 --- 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 index becc8280e..4e2cf04a3 100644 --- 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/RemoteMessagingTests/Matchers/CommonAppAttributeMatcherTests.swift b/Tests/RemoteMessagingTests/Matchers/CommonAppAttributeMatcherTests.swift index 7c9d87672..b792cae83 100644 --- 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: ".")