Surface specific XPC & login item errors #819
Conversation
Sources/NetworkProtection/Diagnostics/NetworkProtectionError.swift
Outdated
Show resolved
Hide resolved
| import Foundation | ||
|
|
||
| @objc | ||
| final public class KnownFailure: NSObject, Codable { |
There was a problem hiding this comment.
This forces all errors that are "known" to be defined in BSK which seems unnecessarily restrictive.
Why instead not just make the error adhere to Codable and store that in NetworkProtectionKnownFailureStore.
Then the VPN app can decide what errors are worth passing over IPC, but at least we don't have to add errors that are macOS specific to BSK.
There was a problem hiding this comment.
Why instead not just make the error adhere to Codable and store that in NetworkProtectionKnownFailureStore.
It's tricky as we need a concrete type to decode the error; and that type needs to be passed around in different ways (e.g. as part of XPCClientInterface, distributed notification object).
To keep it simple, I've moved the custom handling code for TunnelError outside of the initializer and instead made it so that an error conforming to InternalErrorWrapping can expose its internal error. That way we can keep specific errors outside of BSK.
There was a problem hiding this comment.
Understood.
That said, I do think that instead of encoding domain, code and localizedDescription, we should turn this into a Codable enum. See this comment for context.
# Conflicts: # Sources/NetworkProtection/ExtensionMessage/ExtensionRequest.swift
Sources/NetworkProtection/ExtensionMessage/ExtensionRequest.swift
Outdated
Show resolved
Hide resolved
| import Foundation | ||
|
|
||
| @objc | ||
| final public class KnownFailure: NSObject, Codable { |
There was a problem hiding this comment.
Understood.
That said, I do think that instead of encoding domain, code and localizedDescription, we should turn this into a Codable enum. See this comment for context.
Task/Issue URL: https://app.asana.com/0/72649045549333/1207013105069620/f Tech Design URL: CC: BSK PR: duckduckgo/BrowserServicesKit#819 **Description**: **Steps to test this PR**: 1. For the UI, go to Debug > VPN > Simulate known failure and check the warning banner in the VPN popover 2. To check if XPC errors are surfaced, go to `NetworkProtectionIPCTunnelController.start()`, and add a simulated error like `handleFailure(NSError(domain: "SMAppServiceErrorDomain", code: 1))` inside `handleFailure(_:)` 3. To check if login item version mismatch is handled, go to `TunnelControllerIPCService.register(version:bundlePath:completion)` and change the `DefaultIPCMetadataCollector.version != version` to `DefaultIPCMetadataCollector.version == version` 4. To check if ClientError code 5 is handled, block access to NetP endpoint (at router level or using Proxyman), then try to start the VPN. 5. Use Debug > VPN > Log metadata to console to check if lastKnownFailureDescription is recorded
Required:
Task/Issue URL: https://app.asana.com/0/72649045549333/1207013105069620/f
iOS PR:duckduckgo/iOS#2872
macOS PR: duckduckgo/macos-browser#2773
What kind of version bump will this require?: Minor
Optional:
Tech Design URL:
CC:
Description:
Steps to test this PR:
OS Testing:
Internal references:
Software Engineering Expectations
Technical Design Template