Fix log redaction loading issue

[mojganii]

this PR take log redaction out of main thread which caused blocking UI when working around it.

---

This change is 

[linear]

IOS-943 Show the spinner whilst the logs are being loaded and redacted

[rablador]

Reviewed 6 of 6 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @mojganii)

---

ios/MullvadVPN/View controllers/ProblemReport/ProblemReportInteractor.swift line 29 at r1 (raw file):

    func loadLogFiles(completion: @escaping () -> Void) {
        DispatchQueue

Is this queueu necessary when consolidatedLog.addLogFiles already runs in a concurrent queue? Same for the calls to consolidatedLog.string below.

---

ios/MullvadVPN/Classes/ConsolidatedApplicationLog.swift line 55 at r1 (raw file):

    func addLogFiles(fileURLs: [URL], completion: @escaping () -> Void = {}) {
        logQueue.async(flags: .barrier) {

Why do we use .barrier?

---

ios/MullvadVPN/Classes/ConsolidatedApplicationLog.swift line 113 at r1 (raw file):

        let redactedPath = redact(string: path)

        if let lossyString = readFileLossy(path: path, maxBytes: bufferSize) {

Why getting rid of the gobal var? .logMaximumFileSize ensures all log related activities use the same size.

[buggmagnet]

Reviewed 2 of 6 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @mojganii)

---

ios/MullvadVPNTests/MullvadVPN/Log/ConsolidatedApplicationLogTests.swift line 28 at r2 (raw file):

    override func tearDownWithError() throws {
        super.tearDown()

We should call the correct super function, tearDownWithError here

---

ios/MullvadVPNTests/MullvadVPN/Log/ConsolidatedApplicationLogTests.swift line 39 at r2 (raw file):

    func testAddLogFiles() {
        var string = ""
        let expectation = self.expectation(description: "Log files added")

nit
no need for self here and below

---

ios/MullvadVPNTests/MullvadVPN/Log/ConsolidatedApplicationLogTests.swift line 80 at r2 (raw file):

    }

    func testAddLogFilesRaceCondition() {

We discussed this offline, and decided to remove this test and testReportedStringRaceCondition as it doesn't really test against race conditions.

---

ios/MullvadVPN/Classes/ConsolidatedApplicationLog.swift line 133 at r2 (raw file):

            String(decoding: data, as: UTF8.self)
                .drop { ch in
                    // Drop leading replacement characters produced when decoding data

That comment was useful, we should leave it

---

ios/MullvadVPN/View controllers/ProblemReport/ProblemReportInteractor.swift line 30 at r2 (raw file):

    func loadLogFiles(completion: @escaping () -> Void) {
        DispatchQueue
            .global()

We don't need to dispatch on the global queue here since we're already doing this in a private concurrent queue.
The same applies to all uses of global in this file.

If we absolutely need to dispatch off the UI thread here, the ProblemReportInteractor class can use a private dispatch queue.

[mojganii]

Reviewable status: 2 of 7 files reviewed, 11 unresolved discussions (waiting on @buggmagnet and @rablador)

---

ios/MullvadVPN/Classes/ConsolidatedApplicationLog.swift line 55 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

Why do we use .barrier?

Since the queue in concurrent, we need to be sure manipulation is executing in thread safe manner.

---

ios/MullvadVPN/Classes/ConsolidatedApplicationLog.swift line 113 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

Why getting rid of the gobal var? .logMaximumFileSize ensures all log related activities use the same size.

I'll add it through dependency injection.

---

ios/MullvadVPN/Classes/ConsolidatedApplicationLog.swift line 133 at r2 (raw file):

Previously, buggmagnet wrote…

That comment was useful, we should leave it

It was unintentional.

---

ios/MullvadVPN/View controllers/ProblemReport/ProblemReportInteractor.swift line 30 at r2 (raw file):

Previously, buggmagnet wrote…

We don't need to dispatch on the global queue here since we're already doing this in a private concurrent queue.
The same applies to all uses of global in this file.

If we absolutely need to dispatch off the UI thread here, the ProblemReportInteractor class can use a private dispatch queue.

We don't need that here. I made mistake.

---

ios/MullvadVPNTests/MullvadVPN/Log/ConsolidatedApplicationLogTests.swift line 28 at r2 (raw file):

Previously, buggmagnet wrote…

We should call the correct super function, tearDownWithError here

good catch.

---

ios/MullvadVPNTests/MullvadVPN/Log/ConsolidatedApplicationLogTests.swift line 39 at r2 (raw file):

Previously, buggmagnet wrote…

nit
no need for self here and below

Nice.

---

ios/MullvadVPNTests/MullvadVPN/Log/ConsolidatedApplicationLogTests.swift line 80 at r2 (raw file):

Previously, buggmagnet wrote…

We discussed this offline, and decided to remove this test and testReportedStringRaceCondition as it doesn't really test against race conditions.

Done.

---

ios/MullvadVPN/Classes/ConsolidatedApplicationLog.swift line 36 at r3 (raw file):

    private let logQueue = DispatchQueue(
        label: "com.mullvad.consolidation.logs.queue",
        attributes: .concurrent

since there is I/O Operations and they can sometimes cause heavy process, I rather to user serial queue instead of concurrent one.

---

ios/MullvadVPN/Classes/ConsolidatedApplicationLog.swift line 68 at r4 (raw file):

        logQueue.async(flags: .barrier) {
            self.logs.append(LogAttachment(label: message, content: redactedError))
            DispatchQueue.main.async {

switch to main for calling the completion, since ConsolidatedApplicationLog belongs and interacts with the main app.

---

ios/MullvadVPN/Classes/ConsolidatedApplicationLog.swift line 76 at r4 (raw file):

    var string: String {
        var result = ""
        logQueue.sync {

Since It's a serial queue anymore then for avoiding deadlock, I removed invoking write function an the sync process.

[rablador]

Reviewed 5 of 5 files at r3, 3 of 3 files at r4, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @buggmagnet and @mojganii)

---

ios/MullvadVPNTests/MullvadVPN/Log/ConsolidatedApplicationLogTests.swift line 23 at r3 (raw file):

            redactCustomStrings: mockRedactStrings,
            redactContainerPathsForSecurityGroupIdentifiers: mockSecurityGroupIdentifiers,
            bufferSize: 65_536

How come we use a different buffer size than the default in prod code?

[mojganii]

Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @buggmagnet and @rablador)

---

ios/MullvadVPNTests/MullvadVPN/Log/ConsolidatedApplicationLogTests.swift line 23 at r3 (raw file):

Previously, rablador (Jon Petersson) wrote…

How come we use a different buffer size than the default in prod code?

since it's a test and the content is short, then I think it doesn't matter not to use that buffer size on prod

[rablador]

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @buggmagnet)

[buggmagnet]

Reviewed all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @mojganii)

---

ios/MullvadVPN/Classes/ConsolidatedApplicationLog.swift line 157 at r4 (raw file):

        }

        let data = fileHandle.readData(ofLength: Int(bufferSize))

This method seems deprecated, we should use its replacement

public func read(upToCount count: Int) throws -> Data?

---

ios/MullvadVPN/Classes/ConsolidatedApplicationLog.swift line 160 at r4 (raw file):

        let replacementCharacter = Character(UTF8.decode(UTF8.encodedReplacementCharacter))
        let lossyString = String(
            String(decoding: data, as: UTF8.self)

👮‍♀️ ⚠️ Optional Data -> String Conversion Violation: Prefer failable String(data:encoding:)initializer when convertingDatatoString (optional_data_string_conversion)

---

ios/MullvadVPNTests/MullvadVPN/Log/ConsolidatedApplicationLogTests.swift line 38 at r4 (raw file):

    }

    func testAddLogFiles() {

Somehow, this test is broken now

[rablador]

Reviewed 5 of 5 files at r5, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @mojganii)

---

ios/MullvadVPN/Classes/ConsolidatedApplicationLog.swift line 77 at r5 (raw file):

        var result = ""
        logQueue.sync {
            if !logs.isEmpty {

This seems to be almost identical to write() below. Can we create a new function for the constructiom of result/localOutput?

[mojganii]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @buggmagnet and @rablador)

---

ios/MullvadVPN/Classes/ConsolidatedApplicationLog.swift line 157 at r4 (raw file):

Previously, buggmagnet wrote…

This method seems deprecated, we should use its replacement

public func read(upToCount count: Int) throws -> Data?

Done.

---

ios/MullvadVPN/Classes/ConsolidatedApplicationLog.swift line 160 at r4 (raw file):

Previously, buggmagnet wrote…

👮‍♀️ ⚠️ Optional Data -> String Conversion Violation: Prefer failable String(data:encoding:)initializer when convertingDatatoString (optional_data_string_conversion)

Done.

---

ios/MullvadVPN/Classes/ConsolidatedApplicationLog.swift line 77 at r5 (raw file):

Previously, rablador (Jon Petersson) wrote…

This seems to be almost identical to write() below. Can we create a new function for the constructiom of result/localOutput?

No, that can cause deadlock.

---

ios/MullvadVPNTests/MullvadVPN/Log/ConsolidatedApplicationLogTests.swift line 38 at r4 (raw file):

Previously, buggmagnet wrote…

Somehow, this test is broken now

Fixed.

[buggmagnet]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @mojganii and @rablador)

---

ios/MullvadVPN/Classes/ConsolidatedApplicationLog.swift line 77 at r5 (raw file):

Previously, mojganii wrote…

No, that can cause deadlock.

I agree with @rablador, we can create such a function

Note that you can also use dispatchPrecondition(condition: .onQueue(logQueue)) (or the .notOnQueue variant) to make assertions about deadlocks and whatnot.

    func formatHeader() -> String {
        var result = ""
        result += "System information:\n"
        for (key, value) in metadata {
            result += "\(key.rawValue): \(value)\n"
        }
        result += "\n"
        for attachment in logs {
            result += "\(kLogDelimiter)\n"
            result += "\(attachment.label)\n"
            result += "\(kLogDelimiter)\n"
            result += "\(attachment.content)\n\n"
        }

        return result
    }

    var string: String {
        var result = ""
        logQueue.sync {
            if !logs.isEmpty {
                result = formatHeader()
            }
        }
        return result
    }

    func write(to stream: inout some TextOutputStream) {
        logQueue.sync {
            var localOutput = ""
            if !logs.isEmpty {
                localOutput = formatHeader()
                stream.write(localOutput)
            }
        }
    }

---

ios/MullvadSettings/SettingsManager.swift line 50 at r5 (raw file):

    public static func getLastUsedAccount() throws -> String {
        let data = try store.read(key: .lastUsedAccount)
        return String(bytes: data, encoding: .utf8) ?? ""

We should throw a StringDecodingError instead of silently failing and returning an empty string here.

---

ios/MullvadVPN/View controllers/ProblemReport/ProblemReportInteractor.swift line 24 at r5 (raw file):

        self.tunnelManager = tunnelManager
        self.consolidatedLog = ConsolidatedApplicationLog(
            redactCustomStrings: [tunnelManager.deviceState.accountData?.number].compactMap { $0 },

I realize that this redactCustomStrings is not a good design.
It only does something if the user is logged in when sending logs, otherwise this will be empty (because no accountData.number when logged out) and won't redact anything.

Thankfully, we have a redactAccountNumber function that uses a regex to redact account numbers, otherwise we'd have a pretty major problem.

We should delete this false sense of privacy, and make sure the regex we use (for now) is solid instead.

[mojganii]

Reviewable status: 7 of 9 files reviewed, 3 unresolved discussions (waiting on @buggmagnet and @rablador)

---

ios/MullvadSettings/SettingsManager.swift line 50 at r5 (raw file):

Previously, buggmagnet wrote…

We should throw a StringDecodingError instead of silently failing and returning an empty string here.

but the function String(bytes: data, encoding: .utf8) doesn't throw any thing and the new one is the same as what they were.

---

ios/MullvadVPN/Classes/ConsolidatedApplicationLog.swift line 77 at r5 (raw file):

Previously, buggmagnet wrote…

I agree with @rablador, we can create such a function

Note that you can also use dispatchPrecondition(condition: .onQueue(logQueue)) (or the .notOnQueue variant) to make assertions about deadlocks and whatnot.

    func formatHeader() -> String {
        var result = ""
        result += "System information:\n"
        for (key, value) in metadata {
            result += "\(key.rawValue): \(value)\n"
        }
        result += "\n"
        for attachment in logs {
            result += "\(kLogDelimiter)\n"
            result += "\(attachment.label)\n"
            result += "\(kLogDelimiter)\n"
            result += "\(attachment.content)\n\n"
        }

        return result
    }

    var string: String {
        var result = ""
        logQueue.sync {
            if !logs.isEmpty {
                result = formatHeader()
            }
        }
        return result
    }

    func write(to stream: inout some TextOutputStream) {
        logQueue.sync {
            var localOutput = ""
            if !logs.isEmpty {
                localOutput = formatHeader()
                stream.write(localOutput)
            }
        }
    }

dispatchPrecondition(condition: .onQueue(logQueue)) can't prevent from probably deadlock, it's just for checking, I found another way to do that.

---

ios/MullvadVPN/View controllers/ProblemReport/ProblemReportInteractor.swift line 24 at r5 (raw file):

Previously, buggmagnet wrote…

I realize that this redactCustomStrings is not a good design.
It only does something if the user is logged in when sending logs, otherwise this will be empty (because no accountData.number when logged out) and won't redact anything.

Thankfully, we have a redactAccountNumber function that uses a regex to redact account numbers, otherwise we'd have a pretty major problem.

We should delete this false sense of privacy, and make sure the regex we use (for now) is solid instead.

I know it's good to improve code when there is room to do that but It's beyond the current issue I opened PR for. since it's adding through DI then I changed that to accept nil and mange that in ConsolidatedApplicationLog

[rablador]

Reviewed 2 of 2 files at r6, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @buggmagnet)

[buggmagnet]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @mojganii)

---

ios/MullvadSettings/SettingsManager.swift line 50 at r5 (raw file):

Previously, mojganii wrote…

but the function String(bytes: data, encoding: .utf8) doesn't throw any thing and the new one is the same as what they were.

That init can return a nil value, and this API is already declared as throws so the call site expects to handle errors.
In general, it's bad practice to hide errors like this, because it makes it significantly harder to debug when things go wrong.

---

ios/MullvadVPN/Classes/ConsolidatedApplicationLog.swift line 77 at r5 (raw file):

Previously, mojganii wrote…

dispatchPrecondition(condition: .onQueue(logQueue)) can't prevent from probably deadlock, it's just for checking, I found another way to do that.

You're right that dispatchPrecondition does not prevent deadlocks, but that isn't the point of using that API.
The point is that whenever you cannot guarantee that a call happens on a specific dispatch queue, this will blow up and give you a stack trace, which will lead to the root cause.

[mojganii]

Reviewable status: 5 of 10 files reviewed, 2 unresolved discussions (waiting on @buggmagnet and @rablador)

---

ios/MullvadSettings/SettingsManager.swift line 50 at r5 (raw file):

Previously, buggmagnet wrote…

That init can return a nil value, and this API is already declared as throws so the call site expects to handle errors.
In general, it's bad practice to hide errors like this, because it makes it significantly harder to debug when things go wrong.

I agree to throw exception when it's not able to cast data to intended encoding but I'm just telling it was the thing we had before :)

---

ios/MullvadVPN/Classes/ConsolidatedApplicationLog.swift line 77 at r5 (raw file):

Previously, buggmagnet wrote…

You're right that dispatchPrecondition does not prevent deadlocks, but that isn't the point of using that API.
The point is that whenever you cannot guarantee that a call happens on a specific dispatch queue, this will blow up and give you a stack trace, which will lead to the root cause.

I agree but with the current implementation I suppose we don't need that anymore.

[rablador]

Reviewed 5 of 5 files at r7, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @buggmagnet)

[buggmagnet]

Reviewed 1 of 5 files at r3, 1 of 3 files at r4, 1 of 5 files at r5, 1 of 2 files at r6, 3 of 5 files at r7.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @mojganii)

---

ios/Shared/LaunchArguments.swift line 51 at r7 (raw file):

        let data = try encoder.encode(self)
        guard let result = String(bytes: data, encoding: .utf8) else {
            throw StringDecodingError(data: data)

That should be a StringEncodingError here

[buggmagnet]

Reviewable status: all files reviewed, 1 unresolved discussion

[github-actions]

🚨 End to end tests failed. Please check the failed workflow run.