Skip to content

Commit

Permalink
Merge branch 'main' into bartek/data-debugging
Browse files Browse the repository at this point in the history 
* main:
  [DuckPlayer] - Create DuckPlayer target and move initially shared code (#921)
  Improve CrashLogMessageExtractor memory safety (#909)
  • Loading branch information
@samsymons
samsymons committed Aug 3, 2024
2 parents 7317455 + ebad3db commit fe39d1d
Show file tree
Hide file tree
Showing 16 changed files with 643 additions and 69 deletions.
24 changes: 24 additions & 0 deletions .swiftpm/xcode/xcshareddata/xcschemes/BrowserServicesKit-Package.xcscheme
View file
Original file line number Diff line number Diff line change
Expand Up @@ -427,6 +427,20 @@
ReferencedContainer = "container:">
</BuildableReference>
</BuildActionEntry>
<BuildActionEntry
buildForTesting = "YES"
buildForRunning = "YES"
buildForProfiling = "YES"
buildForArchiving = "YES"
buildForAnalyzing = "YES">
<BuildableReference
BuildableIdentifier = "primary"
BlueprintIdentifier = "DuckPlayer"
BuildableName = "DuckPlayer"
BlueprintName = "DuckPlayer"
ReferencedContainer = "container:">
</BuildableReference>
</BuildActionEntry>
</BuildActionEntries>
</BuildAction>
<TestAction
Expand Down Expand Up @@ -636,6 +650,16 @@
ReferencedContainer = "container:">
</BuildableReference>
</TestableReference>
<TestableReference
skipped = "NO">
<BuildableReference
BuildableIdentifier = "primary"
BlueprintIdentifier = "DuckPlayerTests"
BuildableName = "DuckPlayerTests"
BlueprintName = "DuckPlayerTests"
ReferencedContainer = "container:">
</BuildableReference>
</TestableReference>
</Testables>
</TestAction>
<LaunchAction
Expand Down
17 changes: 17 additions & 0 deletions Package.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ let package = Package(
.library(name: "Suggestions", targets: ["Suggestions"]),
.library(name: "PixelKit", targets: ["PixelKit"]),
.library(name: "PixelKitTestingUtilities", targets: ["PixelKitTestingUtilities"]),
.library(name: "DuckPlayer", targets: ["DuckPlayer"]),
],
dependencies: [
.package(url: "https://github.com/duckduckgo/duckduckgo-autofill.git", exact: "12.1.0"),
Expand Down Expand Up @@ -380,6 +381,15 @@ let package = Package(
"PixelKit"
]
),
.target(
name: "DuckPlayer",
dependencies: [
"Common"
],
swiftSettings: [
.define("DEBUG", .when(configuration: .debug))
]
),

// MARK: - Test Targets
.testTarget(
Expand Down Expand Up @@ -572,6 +582,13 @@ let package = Package(
"PixelKitTestingUtilities",
]
),

.testTarget(
name: "DuckPlayerTests",
dependencies: [
"DuckPlayer"
]
),
],
cxxLanguageStandard: .cxx11
)
22 changes: 16 additions & 6 deletions Sources/Common/Extensions/StringExtension.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ extension RegEx {
// from https://stackoverflow.com/a/25717506/73479
static let hostName = regex("^(((?!-)[A-Za-z0-9-]{1,63}(?<!-)\\.)*[A-Za-z0-9-]{2,63})$", .caseInsensitive)

static let email = regex(#"[a-zA-Z0-9]+(?:\.[a-zA-Z0-9]+)*@[a-zA-Z0-9]+(?:\.[a-zA-Z0-9]+)+"#)
static let email = regex(#"[^\s]+@[^\s]+\.[^\s]+"#)
}

// Use this instead of NSLocalizedString for strings that are not supposed to be translated
Expand Down Expand Up @@ -99,7 +99,8 @@ public extension String {

// clean-up file paths and email addresses
func sanitized() -> String {
var message = self
// clean-up emails
var message = self.replacing(RegEx.email, with: "<removed>")

// find all the substring ranges looking like a file path
let pathRanges = message.rangesOfFilePaths()
Expand Down Expand Up @@ -132,15 +133,17 @@ public extension String {
}
}

// clean-up emails
message = message.replacing(RegEx.email, with: "<removed>")

return message
}

private enum FileRegex {
// "(matching url/file/path/at/in..-like prefix)(not an end of expr)(=|:) (open quote/brace)
static let varStart = regex(#"(?:url\b|\bfile\b|path\b|\bin\b|\bfrom\b|\bat)[^.,;?!"'`\])}>]\s*[:= ]?\s*["'“`\[({<]?"#, .caseInsensitive)
static let varStart = regex(#"(?:"# +
#"ur[il]\b|"# +
#"\b(?:config|input|output|temp|log|backup|resource)?file(?:ur[il]|name)?\b|"# +
#"\b(?:absolute|relative|network|temp|url|uri|config|input|output|log|backup|resource)?(?:file|directory|dir)?path(?:ur[il])?\b|"# +
#"\bin\b|\bfrom\b|\bat"# +
#")[^.,;?!"'`\])}>]\s*[:= ]?\s*["'“`\[({<]?"#, .caseInsensitive)
static let closingQuotes = [
"\"": regex(#""[,.;:]?(?:\s|$)|$"#),
"'": regex(#"'[,.;:]?(?:\s|$)|$"#),
Expand All @@ -163,6 +166,9 @@ public extension String {

static let lineNumber = regex(#":\d+$"#)
static let trailingSpecialCharacters = regex(#"[\s\.,;:\])}>"”'`]+$"#)

static let moduleTypeName = regex(#"^\.*[A-Za-z_]*(?:DuckDuckGo|DataBroker|NetworkProtection|VPNProxy)[A-Za-z_]*\.(?:(?:[A-Z_]+[a-z_]+)+)$"#)
static let swiftTypeName = regex(#"^\.*[A-Za-z_]+\.Type$"#)
}

// MARK: File Paths
Expand Down Expand Up @@ -291,6 +297,10 @@ public extension String {
dropLineNumberAndTrimSpaces(&resultRange)

guard let pathRange = Range(NSRange(resultRange, in: self)), pathRange.count > 2 else { continue }
// don‘t remove type names like _NSViewAnimator_DuckDuckGo_Privacy_Browser.MouseOverButton or Any.Type
let fileName = String(self[resultRange])
guard FileRegex.moduleTypeName.matches(in: fileName, range: fileName.fullRange).isEmpty,
FileRegex.swiftTypeName.matches(in: fileName, range: fileName.fullRange).isEmpty else { continue }
// collect the result
result.insert(integersIn: pathRange)
}
Expand Down
4 changes: 2 additions & 2 deletions Sources/Common/Extensions/URLExtension.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ extension URL {
public static let data = NavigationalScheme(rawValue: "data")
public static let blob = NavigationalScheme(rawValue: "blob")
public static let about = NavigationalScheme(rawValue: "about")

public static let duck = NavigationalScheme(rawValue: "duck")
public static let mailto = NavigationalScheme(rawValue: "mailto")

public init(rawValue: String) {
Expand All @@ -106,7 +106,7 @@ extension URL {
}

public static var navigationalSchemes: [NavigationalScheme] {
return [.http, .https, .ftp, .file, .data, .blob, .about]
return [.http, .https, .ftp, .file, .data, .blob, .about, duck]
}

public static var schemesWithRemovableBasicAuth: [NavigationalScheme] {
Expand Down
88 changes: 64 additions & 24 deletions Sources/Crashes/CrashCollection.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,12 @@ public final class CrashCollection {
}, didFindCrashReports: didFindCrashReports)
}

/// Start `MXDiagnostic` (App Store) crash processing with callbacks called when crash data is found
/// - Parameters:
/// - process: callback preprocessing `MXDiagnosticPayload` Array and returning processed JSON Data Array to be uploaded
/// - 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) {
let first = isFirstCrash
isFirstCrash = false
Expand Down Expand Up @@ -89,6 +95,12 @@ public final class CrashCollection {
MXMetricManager.shared.add(crashHandler)
}

/// Start `MXDiagnostic` (App Store) crash processing with `didFindCrashReports` callback called when crash data is found and processed.
/// This method collects additional NSException/C++ exception data (message and stack trace) saved by `CrashLogMessageExtractor` and attaches it to payloads.
/// - Parameters:
/// - 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 startAttachingCrashLogMessages(didFindCrashReports: @escaping (_ pixelParameters: [[String: String]], _ payloads: [Data], _ uploadReports: @escaping () -> Void) -> Void) {
start(process: { payloads in
payloads.compactMap { payload in
Expand All @@ -98,34 +110,62 @@ public final class CrashCollection {
if #available(macOS 14.0, iOS 17.0, *) {
pid = payload.crashDiagnostics?.first?.metaData.pid
}
var crashDiagnostics = dict["crashDiagnostics"] as? [[AnyHashable: Any]] ?? []
var crashDiagnosticsDict = crashDiagnostics.first ?? [:]
var diagnosticMetaDataDict = crashDiagnosticsDict["diagnosticMetaData"] as? [AnyHashable: Any] ?? [:]
var objCexceptionReason = diagnosticMetaDataDict["objectiveCexceptionReason"] as? [AnyHashable: Any] ?? [:]

var exceptionMessage = (objCexceptionReason["composedMessage"] as? String)?.sanitized()
var stackTrace: [String]?

// append crash log message if loaded
if let diagnostic = try? CrashLogMessageExtractor().crashDiagnostic(for: payload.timeStampBegin, pid: pid)?.diagnosticData(), !diagnostic.isEmpty {
if let existingMessage = exceptionMessage, !existingMessage.isEmpty {
exceptionMessage = existingMessage + "\n\n---\n\n" + diagnostic.message
} else {
exceptionMessage = diagnostic.message
// The `MXDiagnostic` payload may not include `crashDiagnostics`, but may instead contain `cpuExceptionDiagnostics`,
// `diskWriteExceptionDiagnostics`, or `hangDiagnostics`.
// For now, we are ignoring these.
if var crashDiagnostics = dict["crashDiagnostics"] as? [[AnyHashable: Any]], !crashDiagnostics.isEmpty {
var crashDiagnosticsDict = crashDiagnostics[0]
var diagnosticMetaDataDict = crashDiagnosticsDict["diagnosticMetaData"] as? [AnyHashable: Any] ?? [:]
var objCexceptionReason = diagnosticMetaDataDict["objectiveCexceptionReason"] as? [AnyHashable: Any] ?? [:]

var exceptionMessage = (objCexceptionReason["composedMessage"] as? String)?.sanitized()
var stackTrace: [String]?

// Look up for NSException/C++ exception diagnostics data (name, reason, `userInfo` dictionary, stack trace)
// by searching for the related crash diagnostics file using the provided crash event timestamp and/or the crashed process PID.
// The stored data was sanitized to remove any potential filename or email occurrences.
if let diagnostic = try? CrashLogMessageExtractor().crashDiagnostic(for: payload.timeStampBegin, pid: pid)?.diagnosticData(), !diagnostic.isEmpty {
// append the loaded crash diagnostics message if the `MXDiagnostic` already contains one
if let existingMessage = exceptionMessage, !existingMessage.isEmpty {
exceptionMessage = existingMessage + "\n\n---\n\n" + diagnostic.message
} else {
// set the loaded diagnostics message in place of the original `MXDiagnostic` exceptionMessage if none exists
exceptionMessage = diagnostic.message
}
stackTrace = diagnostic.stackTrace
}
stackTrace = diagnostic.stackTrace
}

objCexceptionReason["composedMessage"] = exceptionMessage
objCexceptionReason["stackTrace"] = stackTrace
diagnosticMetaDataDict["objectiveCexceptionReason"] = objCexceptionReason
crashDiagnosticsDict["diagnosticMetaData"] = diagnosticMetaDataDict
if crashDiagnostics.isEmpty {
crashDiagnostics = [crashDiagnosticsDict]
} else {
/** Rebuild the original `MXDiagnostic` JSON with appended crash diagnostics:
```
{
"crashDiagnostics": [
{
"callStackTree": { ... },
"diagnosticMetaData": {
"appVersion": "1.95.0",
"objectiveCexceptionReason": {
"composedMessage": "NSTableViewException: Row index 9223372036854775807 out of row range (numberOfRows: 0)",
"stackTrace": [
"0 CoreFoundation 0x00000001930072ec __exceptionPreprocess + 176",
"1 libobjc.A.dylib 0x0000000192aee788 objc_exception_throw + 60",
"2 AppKit 0x00000001968dc20c -[NSTableRowData _availableRowViewWhileUpdatingAtRow:] + 0,",
...
]
},
...
}
}
],
"timeStampBegin": "2024-07-05 14:10:00",
}
``` */
objCexceptionReason["composedMessage"] = exceptionMessage
objCexceptionReason["stackTrace"] = stackTrace
diagnosticMetaDataDict["objectiveCexceptionReason"] = objCexceptionReason
crashDiagnosticsDict["diagnosticMetaData"] = diagnosticMetaDataDict
crashDiagnostics[0] = crashDiagnosticsDict
dict["crashDiagnostics"] = crashDiagnostics
}
dict["crashDiagnostics"] = crashDiagnostics

guard JSONSerialization.isValidJSONObject(dict) else {
assertionFailure("Invalid JSON object: \(dict)")
Expand Down
31 changes: 29 additions & 2 deletions Sources/Crashes/CrashLogMessageExtractor.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ import Foundation
/// `applicationSupportDir/Diagnostics/2024-05-20T12:11:33Z-%pid%.log`
public struct CrashLogMessageExtractor {

/// Structure representing an NSException/C++ diagnostic data file stored in the `Application Support/Diagnostics` directory.
public struct CrashDiagnostic {

// ""2024-05-22T08:17:23Z59070.log"
Expand All @@ -57,6 +58,8 @@ public struct CrashLogMessageExtractor {
public let timestamp: Date
public let pid: pid_t

/// Failable constructor parsing the provided file name to extract crash timestamp and pid from the provided NSException/C++ diagnostic data file URL.
/// - Returns: `nil` if the provided file name doesn’t match the `2024-05-20T12:11:33Z-%pid%.log` format.
init?(url: URL) {
let fileName = url.lastPathComponent

Expand All @@ -77,6 +80,7 @@ public struct CrashLogMessageExtractor {
self.pid = pid
}

/// JSON-codable NSException/C++ diagnostic data container.
public struct DiagnosticData: Codable {
public let message: String
public let stackTrace: [String]
Expand All @@ -91,6 +95,7 @@ public struct CrashLogMessageExtractor {
}
}

/// Try reading diagnostic data from the diagnostics file URL.
public func diagnosticData() throws -> DiagnosticData {
do {
let data = try Data(contentsOf: url)
Expand All @@ -107,7 +112,24 @@ public struct CrashLogMessageExtractor {
fileprivate static var nextCppTerminateHandler: (() -> Void)!
fileprivate static var diagnosticsDirectory: URL!

public static func setUp() {
/// Install uncaught NSException and C++ exception handlers.
/// - Parameters:
/// - swapCxaThrow: whether the `__cxa_throw` hook should be installed.
/// - Note:
/// `__cxa_throw` is a method called under the hood when `throw MyCppException();` is executed.
/// It unwinds the stack to look for a `catch` handler to handle the thrown exception. If the handler can’t
/// be found, `std::terminate` is called, which crashes the app.
///
/// We use the `__cxa_throw` hook (or call it _swizzle_) to record the stack trace of the original place
/// where the exception was thrown and write it to the current NSThread dictionary.
///
/// If the exception causes app termination, we check the NSThread dictionary for the recorded stack trace
/// in our custom `std::terminate` handler that we install using `std::set_terminate` and save
/// the exception message and the stack trace to the _Diagnostics_ directory.
///
/// After the custom `std::terminate` or `NSUncaughtExceptionHandler` is done, we call the
/// original exception handler, which causes the app termination.
public static func setUp(swapCxaThrow: Bool = true) {
prepareDiagnosticsDirectory()

// Set unhandled NSException handler
Expand All @@ -116,7 +138,9 @@ public struct CrashLogMessageExtractor {
// Set unhandled C++ exception handler
nextCppTerminateHandler = SetCxxExceptionTerminateHandler(handleTerminateOnCxxException)
// Swap C++ `throw` to collect stack trace when throw happens
CxaThrowSwapper.swapCxaThrow(with: captureStackTrace)
if swapCxaThrow {
CxaThrowSwapper.swapCxaThrow(with: captureStackTrace)
}
}

/// create App Support/Diagnostics folder
Expand Down Expand Up @@ -146,6 +170,9 @@ public struct CrashLogMessageExtractor {
self.diagnosticsDirectory = diagnosticsDirectory ?? Self.diagnosticsDirectory ?? self.fileManager.diagnosticsDirectory
}

/// Write exception diagnostics as JSON data, including the exception name, reason, and `userInfo` dictionary,
/// to `applicationSupportDir/Diagnostics/2024-05-20T12:11:33Z-%pid%.log`.
/// - Note: Data sanitization is applied to clean up any potential filename or email occurrences.
func writeDiagnostic(for exception: NSException) throws {
// collect exception diagnostics data
let message = (
Expand Down
Loading

0 comments on commit fe39d1d

Please sign in to comment.