When developing the macOS app, we always build the exact same version
and build code for each build. ~~This _may_ be one reason why we
constantly have to deactivate the extension before the new one will
launch.~~ Edit: Just tested, and I can verify that this does fix the
issue on dev builds, so no more having to uninstall the sysex between
builds.
Even if that's not the reason, this is a cleaner approach than building
it in our prod-only scripts.
---------
Signed-off-by: Jamil <jamilbk@users.noreply.github.com>
When updating the provisioning profiles (i.e. when changing anything the
Apple Developer Portal), we needed to manually update these build
scripts to point to the new UUIDs.
This can be made simpler to automatically pull it out of the profiles in
CI.
Somewhere between Xcode 16.0 and Xcode 16.3, the API for the libresolv
functions we call changed slightly, and we can now pass the return value
of `__res_9_state()` directly to the `res_9_ninit`, `res_9_ndestroy` and
`res_9_getservers` functions.
This isn't plugged into anything yet on the Swift side but lays the
foundation for changing the log-level at runtime without having to sign
the user out.
If another VPN has been activated on the system while Firezone is
active, Apple OSes will deactivate our configuration, and never
reactivate it.
We knew this already, and always activated the configuration when
starting during the sign in flow, but failed to also do this when
autoStarting on launch.
This PR updates ensures that during autoStart, we re-enable the
configuration as well.
Fixes#8813
Within the event-loop, we already react to the channel being closed
which happens when the `Sender` within the `Session` gets dropped. As
such, there is no need to send an explicit `Stop` command, dropping the
`Session` is equivalent.
As it turns out, `swift-bridge` already calls `Drop` for us when the
last pointer is set to `nil`:
280a9dd999/swift/apple/FirezoneNetworkExtension/Connlib/Generated/connlib-client-apple/connlib-client-apple.swift (L24-L28)
Thus, we can also remove the explicit `disconnect` call to
`WrappedSession` entirely.
This is a regression introduced in c9f085c102. The `status` at this
point is still `nil` because we have not yet fully subscribed to VPN
status change updates from the system.
That actually shouldn't prevent us from trying to start the tunnel
anyway. If the `token` is missing from the Keychain, the tunnel process
will no-op. So we simply try to start a session on launch always.
Fixes#8456
In order to have the system expand search domains for us, we need to set
a very peculiar combination of configuration options in the
`NEDNSSettings` of the VPN configuration:
- We need to include our search domains in the list of `matchDomains`
- We need to set `matchDomainsNoSearch = false`
- We need to set the `searchDomains` field
Technically, we don't even need to set `searchDomains` by itself.
Reading the docs in more detail for the `matchDomainsNoSearch` flag
explains why:
> A Boolean that specifies if the domains in the matchDomains list
should not be appended to the resolver’s list of search domains.
The double-negative here is confusing but essentially, what this says
is:
> If false, append the list of match domains to the resolver's search
domains.
That is exactly what we want. We want a search domain of e.g.
`example.com` to append to the list of search domains for the primary
resolver of non-scoped DNS queries.
I tested without setting `searchDomains` and it does still work: The
system will still expand the domain for us und send us a FQDN query of
e.g. `foo.example.com`. However, I figured not setting `searchDomains`
at all is quite confusing so I left it in there.
Related: #8410 (Fixes it for MacOS)
---------
Signed-off-by: Thomas Eizinger <thomas@eizinger.io>
Co-authored-by: thomas <firezone@firezones-MacBook-Air.fritz.box>
Co-authored-by: Jamil <jamilbk@users.noreply.github.com>
Before, we would receive an `NSError` object and the type-matching
wouldn't take effect at all, causing the default alert to show every
time. This solves that by introducing a `UserFriendlyError` protocol
which is more robust against the two main `Error` and `NSError`
variants.
The current `VPNConfigurationManager` class is too large and handles 3
separate things:
- Encoding IPC messages
- Sending IPC messages
- Loading/storing the VPN configuration
With this PR, these are split out into:
- ProviderMessage class
- IPCClient class
- VPNConfigurationManager class
These are then use directly from our `Store` in order to load their
state upon `init()`, set the relevant properties, and thus views are
updated accordingly.
A couple minor bugs are fixed as well as part of the refactor.
### Tested: macOS
- [x] Sign in
- [x] Sign out
- [x] Yanking the VPN configuration while signed in / out
- [x] Yanking the system extension while signed in / out
- [x] Denying the VPN configuration
- [x] Denying Notifications
- [x] Denying System Extension
### Tested: iOS
- [x] Sign in
- [x] Sign out
- [x] Yanking the VPN configuration while signed in / out
- [x] Yanking the system extension while signed in / out
- [x] Denying the VPN configuration
- [x] Denying Notifications
---------
Signed-off-by: Jamil <jamilbk@users.noreply.github.com>
`@Published` properties that views subscribe to for UI updates need to
be updated from the main thread only. This PR annotates the relevant
variable and function from the original author's implementation with
`@MainActor` so that Swift will properly warn us when modifying these in
the future.
A regression was introduced in #8218 that removed the `menuBar` as an
environment object for `AppView`.
Unfortunately this compiles just fine, as EnvironmentObjects are loaded
at runtime, causing the "Open Menu" button to crash since it's looking
for a non-existent EnvironmentObject.
The original MenuBar developer set a few anti-patterns that were
somewhat followed by subsequent developers. As of now, the entire file
is too large and woefully cluttered.
This PR takes a big step towards #7771 by first organizing the current
thing into a more comprehensible file:
- `private` is removed. These are not needed in Swift unless you
actually need to make something private. The default `internal` level is
appropriate for most cases.
- state change handlers are consistently named `handleX`
- functions are reorganized, and `MARK` comments used to group similar
functions together
Our application state is incredibly simple, only consisting of a handful
of properties.
Throughout our codebase, we use a singular global state store called
`Store`. We then inject this as the singular piece of state into each
view's model.
This is unnecessary boilerplate and leads to lots of duplicated logic.
Instead we refactor away (nearly) all of the application's view models
and instead use an `@EnvironmentObject` to inject the store into each
view.
This convention drastically simplifies state tracking logic and
boilerplate in the views.
Loading images async isn't fixing the App Hanging reports we continue to
receive, so it's something else. Rather than trying to load them
asynchronously, we revert that change.
We instead eager-load all images needed by the MenuBar at init instead
of lazy-loading them, which in rare cases could cause apparent UI hangs.
If we can't load them we log an error but continue to try an operate, as
the icons are not strictly needed for Firezone operation.
Reverts firezone/firezone#8090
For some reason, this was being initialized twice, when it doesn't need
to be.
The whole reason Favorites is initialized in the FirezoneApp module is
so we can have one instance of it passed down to children.
Now that we have error reporting via Sentry in Swift-land as well, we
can handle errors in the FFI layer more gracefully and return them to
Swift.
---------
Signed-off-by: Jamil <jamilbk@users.noreply.github.com>
Co-authored-by: Jamil <jamilbk@users.noreply.github.com>
We can elegantly handle nil items in places where we currently don't.
This PR updates all cases in MenuBar.swift to gracefully handle nil
items like the menubar icons which can, in rare circumstances, be `nil`
if they haven't yet loaded.
Notifications on Apple platforms are delivered with best-effort
reliability and are not guaranteed.
They can also be queued up by the system so that, for example, it's
possible to issue a notification, quit the app, and then upon the next
launch of the app, receive the notification.
In this second case, if the user dismissed the notification, we will
crash. This is because we only track the `lastNotifiedVersion` in the
`NotificationAdapter` instance object and don't persist it to disk, then
we assert the value not to be nil when saving the user's `dismiss`
action.
To fix this, we persist the `lastNotifiedVersion` to the `UserDefaults`
store and attempt to read this when the user is dismissing the
notification. If we can't read it for some reason, we still dismiss the
notification but won't prevent showing it again on the next update
check.
A minor bug is also fixed where the original author didn't correctly
call the function's `completionHandler`. Also, unused instance vars
`lastDismissedVersion` left over from the original author are removed as
well.
In the window of time between we check `AdapterState == .tunnelStarted`
and we call `setDns` in the Apple `pathUpdateHandler`, it's possible
that connlib disconnected. This window of time could potentially be
non-trivial since we read system resolvers in there, which hits the
disk.
As such, we should always check the `session` pointer is valid just
before use.
The `AdapterState` enum tracks two states: `tunnelStopped` and
`tunnelStarted`. In the `tunnelStarted` state, we populate a
`WrappedSession` object. This is redundant - connlib is either
`connected` and we have a `WrappedSession`, or it is not. Therefore we
can remove the `AdapterState` abstraction completely (which was leftover
from a previous developer) and directly use a `WrappedSession?` object
to issue calls to connlib with.
We set this to a valid `WrappedSession` upon connecting, and back to
`nil` as soon as connlib either `onDisconnect`s us, or the user
disconnects the tunnel.
Lastly, we avoid early-returning from queued workItems because we now
call connlib with `session?` which will no-op if there is no session,
allowing whatever IPC call running at the time (such as fetchResources)
to complete successfully, even though they'll see a "snapshotted" state
of the Adapter/PacketTunnelProvider. In other words, we no longer
enforce the session pointer to be valid for things that don't depend on
its state.
Fixes#7882
After further investigation, it appears that the `NSImage` initializer
loads and decodes images *synchronously* from the disk. In the MenuBar,
we are "lazy-loading" these images, but since the menu is constructed as
part of app initialization, we are effectively loading these when the
app boots, in `FirezoneApp`.
After loading, these are cached, but the initial can hang the UI thread
on app launch for slow systems.
Unfortunately, `NSImage` does not _formally_ conform to `@Sendable`.
However, this may be a nuance that isn't true in most cases, such as
when treating `NSImage` instances as read-only from only a single
thread.
As such, we wrap `NSImage` with our own struct, and mark it `@unchecked
Sendable`. This allows us to load the images on a background thread and
assign them to their UI thread counterparts in an async manner.
See further discussion:
-
https://forums.swift.org/t/why-cant-i-send-an-nsimage-across-actor-boundaries/76199
-
https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/Multithreading/ThreadSafetySummary/ThreadSafetySummary.html#//apple_ref/doc/uid/10000057i-CH12-126728
Related: #7771
Whether we execute a task on the main thread or a background thread
doesn't affect whether the thread is "hung" as reported by Sentry.
Instead, our options for fixing these are:
- Try to use an async version of the underlying API (the [async
version](https://developer.apple.com/documentation/appkit/nsworkspace/open(_:configuration:completionhandler:))
of `open` for example)
- If there is none, and the call could potentially block (most likely to
do disk IO contention), at least schedule this on a new thread using
`Task.detached` but with `.background` priority so that it will avoid
blocking any other execution.
The main takeaway here is that unfortunately, under some conditions,
Sentry will _always_ report an "App Hanging" alert since it's constantly
monitoring all threads for paused execution longer than 2000ms.
We'll probably end up letting some of these slide (pausing a background
or worker thread isn't necessarily a UX issue), but pausing the UI
thread is.
Luckily, we're able to use async APIs for most things. The remaining
things (like working with log files over IPC) we use a `Task.detached`
for.
`fetchResources` is an IPC call, and we can use
`withCheckedThrowingContinuation` like the others to yield while we wait
for the provider to respond.
The particular sentry issue related to this isn't because we are
necessarily blocking the task thread, rather, I suspect it's when
applying the fetched Resources to the UI that we're slow. There isn't
much we can do about this, but this PR will only help.
Because we're using a timer that fires off a closure to do this, we
still use a `callback` inside the timer to actually set the Resources on
the main `Store`, which updates the UI.
Unfortunately refactoring these IPC calls lead to somewhat of a ball of
yarn, so the best way to summarize the spirit of this PR is:
- Ensure IPC calls use `withCheckedThrowingContinuation` where possible
- Thusly, marking these functions `async throws`
- Bubble these errors up the view where we can ultimately decide what to
do with them
- Keep VPN state management and conditional logic based on `NEVPNStatus`
in the vpnConfigurationManager
Followup to the discussion on
https://github.com/firezone/firezone/pull/8064. By annotating the
callback that updates our Resources `@MainActor`, the compiler will
correctly warn us when we call it from a non-isolated context.
If we receive two `.connected` status change updates from the system in
a row, we'll incorrectly schedule an additional timer, and the handle to
the first one will be lost.
This causes a memory leak because we'll then never call the first
timer's `invalidate()` function in the `endUpdatingResources` call.
This fixes a bug introduced in db655dd171 that could lead to a crash or
undefined behavior because it potentially updates resources (a
`@Published` object) from a background thread.
UI updates must occur on the main thread only.
