From 55a08dfe6bb299ac74e69346c1ad6da51355c258 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jacek=20=C5=81yp?= Date: Thu, 19 Dec 2024 14:50:12 +0100 Subject: [PATCH 1/3] Add force old app delegate flag (#1141) Task/Issue URL: https://app.asana.com/0/414235014887631/1208832732122403/f iOS PR: [n/a](https://github.com/duckduckgo/iOS/pull/3727) macOS PR: n/a What kind of version bump will this require?: Major **Description**: Simple privacy config flag that will allow us to use old app delegate in case of any issues. --- .../PrivacyConfig/Features/PrivacyFeature.swift | 1 + 1 file changed, 1 insertion(+) diff --git a/Sources/BrowserServicesKit/PrivacyConfig/Features/PrivacyFeature.swift b/Sources/BrowserServicesKit/PrivacyConfig/Features/PrivacyFeature.swift index dc95e2ad6..4bb99735b 100644 --- a/Sources/BrowserServicesKit/PrivacyConfig/Features/PrivacyFeature.swift +++ b/Sources/BrowserServicesKit/PrivacyConfig/Features/PrivacyFeature.swift @@ -65,6 +65,7 @@ public enum PrivacyFeature: String { case textZoom case adAttributionReporting case experimentTest + case forceOldAppDelegate } /// An abstraction to be implemented by any "subfeature" of a given `PrivacyConfiguration` feature. From f287e30d8fc695cbfba5a2549f7dce9dbec8411e Mon Sep 17 00:00:00 2001 From: Sam Symons Date: Thu, 19 Dec 2024 12:52:18 -0800 Subject: [PATCH 2/3] Disable flaky tests (#1142) Please review the release process for BrowserServicesKit [here](https://app.asana.com/0/1200194497630846/1200837094583426). **Required**: Task/Issue URL: https://app.asana.com/0/1200194497630846/1209019292147975/f iOS PR: N/A macOS PR: N/A What kind of version bump will this require?: N/A **Description**: This PR disables a set of flaky navigation tests. **Steps to test this PR**: 1. Check that CI is green **OS Testing**: * [ ] iOS 14 * [ ] iOS 15 * [ ] iOS 16 * [ ] macOS 10.15 * [ ] macOS 11 * [ ] macOS 12 --- ###### Internal references: [Software Engineering Expectations](https://app.asana.com/0/59792373528535/199064865822552) [Technical Design Template](https://app.asana.com/0/59792373528535/184709971311943) --- .../NavigationTests/DistributedNavigationDelegateTests.swift | 4 ++++ Tests/NavigationTests/NavigationRedirectsTests.swift | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/Tests/NavigationTests/DistributedNavigationDelegateTests.swift b/Tests/NavigationTests/DistributedNavigationDelegateTests.swift index f93206b3b..b0aa80459 100644 --- a/Tests/NavigationTests/DistributedNavigationDelegateTests.swift +++ b/Tests/NavigationTests/DistributedNavigationDelegateTests.swift @@ -652,6 +652,8 @@ class DistributedNavigationDelegateTests: DistributedNavigationDelegateTestsBase } func testOpenAboutBlankInNewWindow() throws { + throw XCTSkip("Flaky, see https://app.asana.com/0/1200194497630846/1205018266972898/f") + navigationDelegate.setResponders(.strong(NavigationResponderMock(defaultHandler: { _ in }))) navigationDelegateProxy.finishEventsDispatchTime = .instant @@ -1633,6 +1635,8 @@ class DistributedNavigationDelegateTests: DistributedNavigationDelegateTestsBase } func testWhenWebContentProcessIsTerminated_webProcessDidTerminateAndNavigationDidFailReceived() throws { + throw XCTSkip("Flaky, see https://app.asana.com/0/1200194497630846/1205018266972898/f") + navigationDelegate.setResponders(.strong(NavigationResponderMock(defaultHandler: { _ in }))) responder(at: 0).onNavigationResponse = { [unowned webView=withWebView(do: { $0 })] _ in diff --git a/Tests/NavigationTests/NavigationRedirectsTests.swift b/Tests/NavigationTests/NavigationRedirectsTests.swift index 5ea195f30..8927e466d 100644 --- a/Tests/NavigationTests/NavigationRedirectsTests.swift +++ b/Tests/NavigationTests/NavigationRedirectsTests.swift @@ -1009,6 +1009,8 @@ class NavigationRedirectsTests: DistributedNavigationDelegateTestsBase { } func testWhenServerRedirectIsInterruptedThenDidFailProvisionalIsCalled() throws { + throw XCTSkip("Flaky, see https://app.asana.com/0/1200194497630846/1205018266972898/f") + navigationDelegate.setResponders(.strong(NavigationResponderMock(defaultHandler: { _ in }))) server.middleware = [{ [urls, data] request in @@ -1050,6 +1052,8 @@ class NavigationRedirectsTests: DistributedNavigationDelegateTestsBase { } func testWhenCustomSchemeNavigationIsInterruptedByNewRequestThenDidFailIsCalled() throws { + throw XCTSkip("Flaky, see https://app.asana.com/0/1200194497630846/1205018266972898/f") + navigationDelegate.setResponders(.strong(NavigationResponderMock(defaultHandler: { _ in }))) let lock = NSLock() From 5704d77e3b4c77c7387518d796d31a35f7a1ffcf Mon Sep 17 00:00:00 2001 From: Jonathan Jackson Date: Thu, 19 Dec 2024 16:11:10 -0500 Subject: [PATCH 3/3] Apple client send and receive support for crash report cohort IDs (CRCIDs) (#1116) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Required: Task/Issue URL: https://app.asana.com/0/1208592102886666/1208759541597499/f iOS & macOS PRs: I’m looking for feedback on these BSK changes before posting app-level PRs What kind of version bump will this require?: Minor (is this right? 😊). CC: @LarryTurtis Optional: Tech Design URL: https://app.asana.com/0/1208592102886666/1208660326715650/f Description: DO NOT MERGE - this is a draft for input, not ready to go live yet. Before this is ready to merge: I’ll have to coordinate iOS and macOS changes to match, as the API signatures for the CrashCollection class have changed I will be waiting past the December 9 code freeze, so these changes are not included in the last public release of 2024, and instead have an extended internal testing period --- Package.swift | 3 +- Sources/Crashes/CRCIDManager.swift | 62 +++++++ Sources/Crashes/CrashCollection.swift | 37 +++- Sources/Crashes/CrashReportSender.swift | 79 ++++++++- Tests/CrashesTests/CrashCollectionTests.swift | 159 ++++++++++++++++-- 5 files changed, 309 insertions(+), 31 deletions(-) create mode 100644 Sources/Crashes/CRCIDManager.swift diff --git a/Package.swift b/Package.swift index 21a162064..ab002bf4c 100644 --- a/Package.swift +++ b/Package.swift @@ -533,7 +533,8 @@ let package = Package( .testTarget( name: "CrashesTests", dependencies: [ - "Crashes" + "Crashes", + "TestUtils" ] ), .testTarget( diff --git a/Sources/Crashes/CRCIDManager.swift b/Sources/Crashes/CRCIDManager.swift new file mode 100644 index 000000000..899897b23 --- /dev/null +++ b/Sources/Crashes/CRCIDManager.swift @@ -0,0 +1,62 @@ +// +// CRCIDManager.swift +// +// Copyright © 2024 DuckDuckGo. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// + +import Foundation +import Persistence +import os.log + +/// Cohort identifier used exclusively to distinguish systemic crashes, only after the user opts in to send them. +/// Its purpose is strictly limited to improving the reliability of crash reporting and is never used elsewhere. +public class CRCIDManager { + static let crcidKey = "CRCIDManager.crcidKey" + var store: KeyValueStoring + + public init(store: KeyValueStoring = UserDefaults.standard) { + self.store = store + } + + public func handleCrashSenderResult(result: Result, response: HTTPURLResponse?) { + switch result { + case .success: + Logger.general.debug("Crash Collection - Sending Crash Report: succeeded") + if let receivedCRCID = response?.allHeaderFields[CrashReportSender.httpHeaderCRCID] as? String { + if crcid != receivedCRCID { + Logger.general.debug("Crash Collection - Received new value for CRCID: \(receivedCRCID), setting local crcid value") + crcid = receivedCRCID + } else { + Logger.general.debug("Crash Collection - Received matching value for CRCID: \(receivedCRCID), no update necessary") + } + } else { + Logger.general.debug("Crash Collection - No value for CRCID header: \(CRCIDManager.crcidKey), clearing local crcid value if present") + crcid = "" + } + case .failure(let failure): + Logger.general.debug("Crash Collection - Sending Crash Report: failed (\(failure))") + } + } + + public var crcid: String? { + get { + return self.store.object(forKey: CRCIDManager.crcidKey) as? String + } + + set { + store.set(newValue, forKey: CRCIDManager.crcidKey) + } + } +} diff --git a/Sources/Crashes/CrashCollection.swift b/Sources/Crashes/CrashCollection.swift index 0d555a208..3f1f71384 100644 --- a/Sources/Crashes/CrashCollection.swift +++ b/Sources/Crashes/CrashCollection.swift @@ -18,6 +18,9 @@ import Foundation import MetricKit +import Persistence +import os.log +import Common public enum CrashCollectionPlatform { case iOS, macOS, macOSAppStore @@ -38,9 +41,12 @@ public enum CrashCollectionPlatform { @available(iOS 13, macOS 12, *) public final class CrashCollection { - public init(platform: CrashCollectionPlatform) { - crashHandler = CrashHandler() - crashSender = CrashReportSender(platform: platform) + public init(crashReportSender: CrashReportSending, + crashCollectionStorage: KeyValueStoring = UserDefaults.standard) { + self.crashHandler = CrashHandler() + self.crashSender = crashReportSender + self.crashCollectionStorage = crashCollectionStorage + self.crcidManager = CRCIDManager(store: crashCollectionStorage) } public func start(didFindCrashReports: @escaping (_ pixelParameters: [[String: String]], _ payloads: [Data], _ uploadReports: @escaping () -> Void) -> Void) { @@ -55,7 +61,11 @@ public final class CrashCollection { /// - didFindCrashReports: callback called after payload preprocessing is finished. /// Provides processed JSON data to be presented to the user and Pixel parameters to fire a crash Pixel. /// `uploadReports` callback is used when the user accepts uploading the crash report and starts crash upload to the server. - public func start(process: @escaping ([MXDiagnosticPayload]) -> [Data], didFindCrashReports: @escaping (_ pixelParameters: [[String: String]], _ payloads: [Data], _ uploadReports: @escaping () -> Void) -> Void) { + public func start(process: @escaping ([MXDiagnosticPayload]) -> [Data], + didFindCrashReports: @escaping (_ pixelParameters: [[String: String]], + _ payloads: [Data], + _ uploadReports: @escaping () -> Void) -> Void, + didFinishHandlingResponse: @escaping (() -> Void) = {}) { let first = isFirstCrash isFirstCrash = false @@ -80,8 +90,13 @@ public final class CrashCollection { didFindCrashReports(pixelParameters, processedData) { Task { for payload in processedData { - await self.crashSender.send(payload) + // Note: It's important that we submit crashes to our service one by one. CRCIDs are assigned (or potentially expired + // and updated with a new one) in response to a call to crash.js, and making multiple calls in parallel would mean + // the server may assign several CRCIDs to a single client in rapid succession. + let result = await self.crashSender.send(payload, crcid: self.crcidManager.crcid) + self.crcidManager.handleCrashSenderResult(result: result.result, response: result.response) } + didFinishHandlingResponse() } } } @@ -171,18 +186,24 @@ public final class CrashCollection { }, didFindCrashReports: didFindCrashReports) } + public func clearCRCID() { + self.crcidManager.crcid = nil + } + var isFirstCrash: Bool { get { - UserDefaults().object(forKey: Const.firstCrashKey) as? Bool ?? true + crashCollectionStorage.object(forKey: Const.firstCrashKey) as? Bool ?? true } set { - UserDefaults().set(newValue, forKey: Const.firstCrashKey) + crashCollectionStorage.set(newValue, forKey: Const.firstCrashKey) } } let crashHandler: CrashHandler - let crashSender: CrashReportSender + let crashSender: CrashReportSending + let crashCollectionStorage: KeyValueStoring + let crcidManager: CRCIDManager enum Const { static let firstCrashKey = "CrashCollection.first" diff --git a/Sources/Crashes/CrashReportSender.swift b/Sources/Crashes/CrashReportSender.swift index 52779bbb7..5aea0b1b1 100644 --- a/Sources/Crashes/CrashReportSender.swift +++ b/Sources/Crashes/CrashReportSender.swift @@ -18,29 +18,92 @@ import Foundation import MetricKit +import Common +import os.log -public final class CrashReportSender { +public protocol CrashReportSending { + var pixelEvents: EventMapping? { get } + init(platform: CrashCollectionPlatform, pixelEvents: EventMapping?) + + func send(_ crashReportData: Data, crcid: String?) async -> (result: Result, response: HTTPURLResponse?) + func send(_ crashReportData: Data, crcid: String?, completion: @escaping (_ result: Result, _ response: HTTPURLResponse?) -> Void) +} + +public enum CrashReportSenderError: Error { + case crcidMissing + case submissionFailed(HTTPURLResponse?) +} + +public final class CrashReportSender: CrashReportSending { static let reportServiceUrl = URL(string: "https://duckduckgo.com/crash.js")! + + static let httpHeaderCRCID = "crcid" + public let platform: CrashCollectionPlatform + public var pixelEvents: EventMapping? - public init(platform: CrashCollectionPlatform) { + private let session = URLSession(configuration: .ephemeral) + + public init(platform: CrashCollectionPlatform, pixelEvents: EventMapping?) { self.platform = platform + self.pixelEvents = pixelEvents } - public func send(_ crashReportData: Data) async { + public func send(_ crashReportData: Data, crcid: String?, completion: @escaping (_ result: Result, _ response: HTTPURLResponse?) -> Void) { var request = URLRequest(url: Self.reportServiceUrl) request.setValue("text/plain", forHTTPHeaderField: "Content-Type") request.setValue(platform.userAgent, forHTTPHeaderField: "User-Agent") + + let crcidHeaderValue = crcid ?? "" + request.setValue(crcidHeaderValue, forHTTPHeaderField: CrashReportSender.httpHeaderCRCID) + Logger.general.debug("Configured crash report HTTP request with crcid: \(crcidHeaderValue)") + request.httpMethod = "POST" request.httpBody = crashReportData - do { - _ = try await session.data(for: request) - } catch { - assertionFailure("CrashReportSender: Failed to send the crash report") + Logger.general.debug("CrashReportSender: Awaiting session data") + let task = session.dataTask(with: request) { data, response, error in + if let response = response as? HTTPURLResponse { + Logger.general.debug("CrashReportSender: Received HTTP response code: \(response.statusCode)") + if response.statusCode == 200 { + response.allHeaderFields.forEach { headerField in + Logger.general.debug("CrashReportSender: \(String(describing: headerField.key)): \(String(describing: headerField.value))") + } + let receivedCRCID = response.allHeaderFields[CrashReportSender.httpHeaderCRCID] + if receivedCRCID == nil || receivedCRCID as? String == "" { + let crashReportError = CrashReportSenderError.crcidMissing + self.pixelEvents?.fire(crashReportError) + } + } else { + assertionFailure("CrashReportSender: Failed to send the crash report: \(response.statusCode)") + } + + if let data { + completion(.success(data), response) + } else if let error { + let crashReportError = CrashReportSenderError.submissionFailed(response) + self.pixelEvents?.fire(crashReportError) + completion(.failure(error), response) + } else { + let crashReportError = CrashReportSenderError.submissionFailed(response) + self.pixelEvents?.fire(crashReportError) + completion(.failure(crashReportError), response) + } + } else { + let crashReportError = CrashReportSenderError.submissionFailed(nil) + self.pixelEvents?.fire(crashReportError) + completion(.failure(crashReportError), nil) + } } + task.resume() } - private let session = URLSession(configuration: .ephemeral) + public func send(_ crashReportData: Data, crcid: String?) async -> (result: Result, response: HTTPURLResponse?) { + await withCheckedContinuation { continuation in + send(crashReportData, crcid: crcid) { result, response in + continuation.resume(returning: (result, response)) + } + } + } } diff --git a/Tests/CrashesTests/CrashCollectionTests.swift b/Tests/CrashesTests/CrashCollectionTests.swift index d65492f25..8c71605c9 100644 --- a/Tests/CrashesTests/CrashCollectionTests.swift +++ b/Tests/CrashesTests/CrashCollectionTests.swift @@ -19,21 +19,16 @@ @testable import Crashes import MetricKit import XCTest +import Persistence +import TestUtils +import Common class CrashCollectionTests: XCTestCase { - override func setUp() { - super.setUp() - clearUserDefaults() - } - - override func tearDown() { - super.tearDown() - clearUserDefaults() - } - func testFirstCrashFlagSent() { - let crashCollection = CrashCollection(platform: .iOS) + let crashReportSender = CrashReportSender(platform: .iOS, pixelEvents: nil) + let crashCollection = CrashCollection(crashReportSender: crashReportSender, + crashCollectionStorage: MockKeyValueStore()) // 2 pixels with first = true attached XCTAssertTrue(crashCollection.isFirstCrash) crashCollection.start { pixelParameters, _, _ in @@ -50,7 +45,9 @@ class CrashCollectionTests: XCTestCase { } func testSubsequentPixelsDontSendFirstFlag() { - let crashCollection = CrashCollection(platform: .iOS) + let crashReportSender = CrashReportSender(platform: .iOS, pixelEvents: nil) + let crashCollection = CrashCollection(crashReportSender: crashReportSender, + crashCollectionStorage: MockKeyValueStore()) // 2 pixels with no first parameter crashCollection.isFirstCrash = false crashCollection.start { pixelParameters, _, _ in @@ -66,8 +63,101 @@ class CrashCollectionTests: XCTestCase { XCTAssertFalse(crashCollection.isFirstCrash) } - private func clearUserDefaults() { - UserDefaults().removeObject(forKey: CrashCollection.Const.firstCrashKey) + func testCRCIDIsStoredWhenReceived() { + let responseCRCIDValue = "CRCID Value" + + let store = MockKeyValueStore() + let crashReportSender = MockCrashReportSender(platform: .iOS, pixelEvents: nil) + crashReportSender.responseCRCID = responseCRCIDValue + let crashCollection = CrashCollection(crashReportSender: crashReportSender, + crashCollectionStorage: store) + let expectation = self.expectation(description: "Crash collection response") + + // Set up closures on our CrashCollection object + crashCollection.start(process: {_ in + return ["fake-crash-data".data(using: .utf8)!] // Not relevant to this test + }) { pixelParameters, payloads, uploadReports in + uploadReports() + } didFinishHandlingResponse: { + expectation.fulfill() + } + + // Execute crash collection (which will call our mocked CrashReportSender as well) + XCTAssertNil(store.object(forKey: CRCIDManager.crcidKey), "CRCID should not be present in the store before crashHandler receives crashes") + crashCollection.crashHandler.didReceive([ + MockPayload(mockCrashes: [ + MXCrashDiagnostic(), + MXCrashDiagnostic() + ]) + ]) + + self.wait(for: [expectation], timeout: 3) + + XCTAssertEqual(store.object(forKey: CRCIDManager.crcidKey) as? String, responseCRCIDValue) + } + + func testCRCIDIsClearedWhenServerReturnsSuccessWithNoCRCID() + { + let store = MockKeyValueStore() + let crashReportSender = MockCrashReportSender(platform: .iOS, pixelEvents: nil) + let crashCollection = CrashCollection(crashReportSender: crashReportSender, + crashCollectionStorage: store) + let expectation = self.expectation(description: "Crash collection response") + + // Set up closures on our CrashCollection object + crashCollection.start(process: {_ in + return ["fake-crash-data".data(using: .utf8)!] // Not relevant to this test + }) { pixelParameters, payloads, uploadReports in + uploadReports() + } didFinishHandlingResponse: { + expectation.fulfill() + } + + // Execute crash collection (which will call our mocked CrashReportSender as well) + store.set("Initial CRCID Value", forKey: CRCIDManager.crcidKey) + XCTAssertNotNil(store.object(forKey: CRCIDManager.crcidKey)) + crashCollection.crashHandler.didReceive([ + MockPayload(mockCrashes: [ + MXCrashDiagnostic(), + MXCrashDiagnostic() + ]) + ]) + + self.wait(for: [expectation], timeout: 3) + + XCTAssertEqual(store.object(forKey: CRCIDManager.crcidKey) as! String, "", "CRCID should not be present in the store after receiving a successful response") + } + + func testCRCIDIsRetainedWhenServerErrorIsReceived() { + let store = MockKeyValueStore() + let crashReportSender = MockCrashReportSender(platform: .iOS, pixelEvents: nil) + let crashCollection = CrashCollection(crashReportSender: crashReportSender, + crashCollectionStorage: store) + let expectation = self.expectation(description: "Crash collection response") + + // Set up closures on our CrashCollection object + crashCollection.start(process: {_ in + return ["fake-crash-data".data(using: .utf8)!] // Not relevant to this test + }) { pixelParameters, payloads, uploadReports in + uploadReports() + } didFinishHandlingResponse: { + expectation.fulfill() + } + + // Execute crash collection (which will call our mocked CrashReportSender as well) + let crcid = "Initial CRCID Value" + store.set(crcid, forKey: CRCIDManager.crcidKey) + crashReportSender.responseStatusCode = 500 + crashCollection.crashHandler.didReceive([ + MockPayload(mockCrashes: [ + MXCrashDiagnostic(), + MXCrashDiagnostic() + ]) + ]) + + self.wait(for: [expectation], timeout: 3) + + XCTAssertEqual(store.object(forKey: CRCIDManager.crcidKey) as? String, crcid) } } @@ -87,5 +177,46 @@ class MockPayload: MXDiagnosticPayload { override var crashDiagnostics: [MXCrashDiagnostic]? { return mockCrashes } +} + +class MockCrashReportSender: CrashReportSending { + + let platform: CrashCollectionPlatform + var responseCRCID: String? + var responseStatusCode = 200 + + var pixelEvents: EventMapping? + + required init(platform: CrashCollectionPlatform, pixelEvents: EventMapping?) { + self.platform = platform + } + + func send(_ crashReportData: Data, crcid: String?, completion: @escaping (_ result: Result, _ response: HTTPURLResponse?) -> Void) { + var responseHeaderFields: [String: String] = [:] + if let responseCRCID { + responseHeaderFields[CrashReportSender.httpHeaderCRCID] = responseCRCID + } + + guard let response = HTTPURLResponse(url: URL(string: "fakeURL")!, + statusCode: responseStatusCode, + httpVersion: nil, + headerFields: responseHeaderFields) else { + XCTFail("Failed to create HTTPURLResponse") + return + } + + if responseStatusCode == 200 { + completion(.success(nil), response) // Success with nil data + } else { + completion(.failure(CrashReportSenderError.submissionFailed(response)), response) + } + } + func send(_ crashReportData: Data, crcid: String?) async -> (result: Result, response: HTTPURLResponse?) { + await withCheckedContinuation { continuation in + send(crashReportData, crcid: crcid) { result, response in + continuation.resume(returning: (result, response)) + } + } + } }