Fix a crash when attempting to send a bug report with excessively large logs. (#4243)

* Alert the user when there are excessively large log files.
This commit is contained in:
Doug
2025-06-24 09:47:20 +01:00
committed by GitHub
parent 0bf6ed1c63
commit 0f9d700c99
10 changed files with 50 additions and 16 deletions
@@ -632,6 +632,7 @@
"screen_bug_report_error_description_too_short" = "The description is too short, please provide more details about what happened. Thanks!"; "screen_bug_report_error_description_too_short" = "The description is too short, please provide more details about what happened. Thanks!";
"screen_bug_report_include_crash_logs" = "Send crash logs"; "screen_bug_report_include_crash_logs" = "Send crash logs";
"screen_bug_report_include_logs" = "Allow logs"; "screen_bug_report_include_logs" = "Allow logs";
"screen_bug_report_include_logs_error" = "Your logs are excessively large so cannot be included in this report, please send them to us another way.";
"screen_bug_report_include_screenshot" = "Send screenshot"; "screen_bug_report_include_screenshot" = "Send screenshot";
"screen_bug_report_logs_description" = "Logs will be included with your message to make sure that everything is working properly. To send your message without logs, turn off this setting."; "screen_bug_report_logs_description" = "Logs will be included with your message to make sure that everything is working properly. To send your message without logs, turn off this setting.";
"screen_bug_report_view_logs" = "View logs"; "screen_bug_report_view_logs" = "View logs";
+2
View File
@@ -1224,6 +1224,8 @@ internal enum L10n {
internal static var screenBugReportIncludeCrashLogs: String { return L10n.tr("Localizable", "screen_bug_report_include_crash_logs") } internal static var screenBugReportIncludeCrashLogs: String { return L10n.tr("Localizable", "screen_bug_report_include_crash_logs") }
/// Allow logs /// Allow logs
internal static var screenBugReportIncludeLogs: String { return L10n.tr("Localizable", "screen_bug_report_include_logs") } internal static var screenBugReportIncludeLogs: String { return L10n.tr("Localizable", "screen_bug_report_include_logs") }
/// Your logs are excessively large so cannot be included in this report, please send them to us another way.
internal static var screenBugReportIncludeLogsError: String { return L10n.tr("Localizable", "screen_bug_report_include_logs_error") }
/// Send screenshot /// Send screenshot
internal static var screenBugReportIncludeScreenshot: String { return L10n.tr("Localizable", "screen_bug_report_include_screenshot") } internal static var screenBugReportIncludeScreenshot: String { return L10n.tr("Localizable", "screen_bug_report_include_screenshot") }
/// Logs will be included with your message to make sure that everything is working properly. To send your message without logs, turn off this setting. /// Logs will be included with your message to make sure that everything is working properly. To send your message without logs, turn off this setting.
@@ -17,6 +17,7 @@ enum BugReportScreenViewModelAction {
} }
struct BugReportScreenViewState: BindableState { struct BugReportScreenViewState: BindableState {
let canSendLogFiles: Bool
var screenshot: UIImage? var screenshot: UIImage?
var bindings: BugReportScreenViewStateBindings var bindings: BugReportScreenViewStateBindings
let isModallyPresented: Bool let isModallyPresented: Bool
@@ -14,6 +14,8 @@ class BugReportScreenViewModel: BugReportScreenViewModelType, BugReportScreenVie
private let bugReportService: BugReportServiceProtocol private let bugReportService: BugReportServiceProtocol
private let clientProxy: ClientProxyProtocol? private let clientProxy: ClientProxyProtocol?
private let logFiles = Tracing.logFiles
private let actionsSubject: PassthroughSubject<BugReportScreenViewModelAction, Never> = .init() private let actionsSubject: PassthroughSubject<BugReportScreenViewModelAction, Never> = .init()
// periphery:ignore - when set to nil this is automatically cancelled // periphery:ignore - when set to nil this is automatically cancelled
@CancellableTask private var uploadTask: Task<Void, Never>? @CancellableTask private var uploadTask: Task<Void, Never>?
@@ -29,8 +31,10 @@ class BugReportScreenViewModel: BugReportScreenViewModelType, BugReportScreenVie
self.bugReportService = bugReportService self.bugReportService = bugReportService
self.clientProxy = clientProxy self.clientProxy = clientProxy
let bindings = BugReportScreenViewStateBindings(reportText: "", sendingLogsEnabled: true, canContact: false) let canSendLogFiles = Self.validate(logFiles)
super.init(initialViewState: BugReportScreenViewState(screenshot: screenshot, let bindings = BugReportScreenViewStateBindings(reportText: "", sendingLogsEnabled: canSendLogFiles, canContact: false)
super.init(initialViewState: BugReportScreenViewState(canSendLogFiles: canSendLogFiles,
screenshot: screenshot,
bindings: bindings, bindings: bindings,
isModallyPresented: isModallyPresented)) isModallyPresented: isModallyPresented))
} }
@@ -56,6 +60,23 @@ class BugReportScreenViewModel: BugReportScreenViewModelType, BugReportScreenVie
// MARK: Private // MARK: Private
/// Kind of a hack - when a log file balloons in size (e.g. due to a tight loop) the app is terminated by iOS for using
/// too much memory. This is because we compare the file size against the upload limit **after** compressing it,
/// but in order to compress it we load the whole file into memory.
///
/// We could fix that, but then the problematic log file would be silently dropped and in reality it is much more valuable
/// to have the user contact us to say there's an issue with their logs so we can actually fix whatever is generating the
/// excessively large log files.
private static func validate(_ logFiles: [URL]) -> Bool {
for fileURL in logFiles {
if let size = try? FileManager.default.sizeForItem(at: fileURL),
size > Double(1024 * 1024 * 1024) { // Consider anything over 1GB to be excessive.
return false
}
}
return true
}
private func submitBugReport() async { private func submitBugReport() async {
let progressSubject = CurrentValueSubject<Double, Never>(0.0) let progressSubject = CurrentValueSubject<Double, Never>(0.0)
@@ -81,7 +102,7 @@ class BugReportScreenViewModel: BugReportScreenViewModelType, BugReportScreenVie
ed25519: ed25519, ed25519: ed25519,
curve25519: curve25519, curve25519: curve25519,
text: context.reportText, text: context.reportText,
includeLogs: context.sendingLogsEnabled, logFiles: context.sendingLogsEnabled ? logFiles : nil,
canContact: context.canContact, canContact: context.canContact,
githubLabels: [], githubLabels: [],
files: files) files: files)
@@ -14,6 +14,7 @@ struct BugReportScreen: View {
@Bindable var context: BugReportScreenViewModel.Context @Bindable var context: BugReportScreenViewModel.Context
var canSendLogFiles: Bool { context.viewState.canSendLogFiles }
var photosPickerTitle: String { context.viewState.screenshot == nil ? L10n.screenBugReportAttachScreenshot : L10n.screenBugReportEditScreenshot } var photosPickerTitle: String { context.viewState.screenshot == nil ? L10n.screenBugReportAttachScreenshot : L10n.screenBugReportEditScreenshot }
var body: some View { var body: some View {
@@ -56,15 +57,23 @@ struct BugReportScreen: View {
private var sendLogsSection: some View { private var sendLogsSection: some View {
Section { Section {
if canSendLogFiles {
ListRow(label: .plain(title: L10n.screenBugReportIncludeLogs), ListRow(label: .plain(title: L10n.screenBugReportIncludeLogs),
kind: .toggle($context.sendingLogsEnabled)) kind: .toggle($context.sendingLogsEnabled))
.accessibilityIdentifier(A11yIdentifiers.bugReportScreen.sendLogs) .accessibilityIdentifier(A11yIdentifiers.bugReportScreen.sendLogs)
}
ListRow(label: .plain(title: L10n.screenBugReportViewLogs), ListRow(label: .plain(title: L10n.screenBugReportViewLogs),
kind: .navigationLink { context.send(viewAction: .viewLogs) }) kind: .navigationLink { context.send(viewAction: .viewLogs) })
.accessibilityIdentifier(A11yIdentifiers.bugReportScreen.sendLogs) .accessibilityIdentifier(A11yIdentifiers.bugReportScreen.sendLogs)
} footer: { } footer: {
if canSendLogFiles {
Text(L10n.screenBugReportLogsDescription) Text(L10n.screenBugReportLogsDescription)
.compoundListSectionFooter() .compoundListSectionFooter()
} else {
Label(L10n.screenBugReportIncludeLogsError, icon: \.errorSolid, iconSize: .xSmall, relativeTo: .compound.bodySM)
.foregroundStyle(.compound.textCriticalPrimary)
.compoundListSectionFooter()
}
} }
} }
@@ -80,8 +80,8 @@ class BugReportService: NSObject, BugReportServiceProtocol {
params.append(MultipartFormData(key: "label", type: .text(value: label))) params.append(MultipartFormData(key: "label", type: .text(value: label)))
} }
if bugReport.includeLogs { if let logFiles = bugReport.logFiles {
let logAttachments = await zipFiles(Tracing.logFiles) let logAttachments = await zipFiles(logFiles)
for url in logAttachments.files { for url in logAttachments.files {
params.append(MultipartFormData(key: "compressed-log", type: .file(url: url))) params.append(MultipartFormData(key: "compressed-log", type: .file(url: url)))
} }
@@ -15,7 +15,7 @@ struct BugReport: Equatable {
let ed25519: String? let ed25519: String?
let curve25519: String? let curve25519: String?
let text: String let text: String
let includeLogs: Bool let logFiles: [URL]?
let canContact: Bool let canContact: Bool
var githubLabels: [String] var githubLabels: [String]
let files: [URL] let files: [URL]
@@ -86,7 +86,7 @@ class BugReportScreenViewModelTests: XCTestCase {
XCTAssertEqual(mockService.submitBugReportProgressListenerReceivedArguments?.bugReport.curve25519, "THECURVEKEYKEY") XCTAssertEqual(mockService.submitBugReportProgressListenerReceivedArguments?.bugReport.curve25519, "THECURVEKEYKEY")
XCTAssertEqual(mockService.submitBugReportProgressListenerReceivedArguments?.bugReport.ed25519, "THEEDKEYKEY") XCTAssertEqual(mockService.submitBugReportProgressListenerReceivedArguments?.bugReport.ed25519, "THEEDKEYKEY")
XCTAssertEqual(mockService.submitBugReportProgressListenerReceivedArguments?.bugReport.text, "This will succeed") XCTAssertEqual(mockService.submitBugReportProgressListenerReceivedArguments?.bugReport.text, "This will succeed")
XCTAssertEqual(mockService.submitBugReportProgressListenerReceivedArguments?.bugReport.includeLogs, true) XCTAssertEqual(mockService.submitBugReportProgressListenerReceivedArguments?.bugReport.logFiles?.isEmpty, false)
XCTAssertEqual(mockService.submitBugReportProgressListenerReceivedArguments?.bugReport.canContact, false) XCTAssertEqual(mockService.submitBugReportProgressListenerReceivedArguments?.bugReport.canContact, false)
XCTAssertEqual(mockService.submitBugReportProgressListenerReceivedArguments?.bugReport.githubLabels, []) XCTAssertEqual(mockService.submitBugReportProgressListenerReceivedArguments?.bugReport.githubLabels, [])
XCTAssertEqual(mockService.submitBugReportProgressListenerReceivedArguments?.bugReport.files, []) XCTAssertEqual(mockService.submitBugReportProgressListenerReceivedArguments?.bugReport.files, [])
@@ -31,7 +31,7 @@ class BugReportServiceTests: XCTestCase {
ed25519: nil, ed25519: nil,
curve25519: nil, curve25519: nil,
text: "i cannot send message", text: "i cannot send message",
includeLogs: true, logFiles: [URL(filePath: "/logs/1.log"), URL(filePath: "/logs/2.log")],
canContact: false, canContact: false,
githubLabels: [], githubLabels: [],
files: []) files: [])
@@ -76,7 +76,7 @@ class BugReportServiceTests: XCTestCase {
ed25519: nil, ed25519: nil,
curve25519: nil, curve25519: nil,
text: "i cannot send message", text: "i cannot send message",
includeLogs: true, logFiles: Tracing.logFiles,
canContact: false, canContact: false,
githubLabels: [], githubLabels: [],
files: []) files: [])