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)) + } + } + } }