Apple client send and receive support for crash report cohort IDs (CR…

…CIDs) (#1116)

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

@@ -533,7 +533,8 @@ let package = Package(
         .testTarget(
             name: "CrashesTests",
             dependencies: [
-                "Crashes"
+                "Crashes",
+                "TestUtils"
             ]
         ),
         .testTarget(

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<Data?, Error>, 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)
+        }
+    }
+}

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"

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<CrashReportSenderError>? { get }
 
+    init(platform: CrashCollectionPlatform, pixelEvents: EventMapping<CrashReportSenderError>?)
+
+    func send(_ crashReportData: Data, crcid: String?) async -> (result: Result<Data?, Error>, response: HTTPURLResponse?)
+    func send(_ crashReportData: Data, crcid: String?, completion: @escaping (_ result: Result<Data?, Error>, _ 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<CrashReportSenderError>?
 
-    public init(platform: CrashCollectionPlatform) {
+    private let session = URLSession(configuration: .ephemeral)
+
+    public init(platform: CrashCollectionPlatform, pixelEvents: EventMapping<CrashReportSenderError>?) {
         self.platform = platform
+        self.pixelEvents = pixelEvents
     }
 
-    public func send(_ crashReportData: Data) async {
+    public func send(_ crashReportData: Data, crcid: String?, completion: @escaping (_ result: Result<Data?, Error>, _ 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<Data?, Error>, response: HTTPURLResponse?) {
+        await withCheckedContinuation { continuation in
+            send(crashReportData, crcid: crcid) { result, response in
+                continuation.resume(returning: (result, response))
+            }
+        }
+    }
 }