-
Notifications
You must be signed in to change notification settings - Fork 415
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Reduce VPN manager instances #3097
Conversation
if manager.connection.status == .invalid { | ||
self.clearInternalManager() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the key to making sure that the shared manager is always valid. This publisher will clear it out, and the next time it's accessed then a new one will be created (assuming we have a configuration at all).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good - exactly as in macOS so I'm hoping that means it won't cause trouble.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code changes look good, but I'm getting a crash on launch.
The issue is that AppDependencyProvider
tries to instance ConnectionStatusObserverThroughSession
which tries to access AppDependencyProvider.shared
.
I have to say I don't love this AppDependencyProvider
approach we're taking as it makes circular dependencies between singletons really hard to spot and likely to fail at runtime, like in this case.
I'm attaching the stack trace.
if manager.connection.status == .invalid { | ||
self.clearInternalManager() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good - exactly as in macOS so I'm hoping that means it won't cause trouble.
# By Mariusz Śpiewak (2) and others # Via GitHub * main: Remove print (#3101) Update Package.resolved file (#3102) New Tab Page favorites section (#3083) Properly compare actual value of the entitlement check (#3100) fix ui tests broken by new onboarding and use shared setup flow (#3081) # Conflicts: # DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved
…nel controller." This reverts commit 427948a.
# By Diego Rey Mendez (1) and dependabot[bot] (1) # Via GitHub * main: Bump submodules/privacy-reference-tests from `a242bf0` to `afb4f61` (#3096) Updates BSK to 171.2.3 # Conflicts: # DuckDuckGo.xcodeproj/project.pbxproj # DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved
@@ -46,6 +46,7 @@ class MockDependencyProvider: DependencyProvider { | |||
var networkProtectionKeychainTokenStore: NetworkProtectionKeychainTokenStore | |||
var networkProtectionTunnelController: NetworkProtectionTunnelController | |||
var connectionObserver: NetworkProtection.ConnectionStatusObserver | |||
var serverInfoObserver: NetworkProtection.ConnectionServerInfoObserver |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to keep an instance of this alive from the very beginning so that it can listen for connection changes. Otherwise, when we enter the VPN UI, we won't have any server info to display.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works great!
Task/Issue URL: https://app.asana.com/0/72649045549333/1207151621945908/f iOS PR: duckduckgo/iOS#3097 macOS PR: duckduckgo/macos-browser#2989 What kind of version bump will this require?: Major Description: This PR removes connection utilities that haven't been used on macOS for a while, but were still used on iOS. These are now unused on iOS as well, so they're being removed.
# By Daniel Bernal (2) and others # Via GitHub * main: Update BSK for Mac RMF changes (#3107) [DuckPlayer] 7- Open Settings (#3110) [DuckPlayer] 6 - Init updates and Watch on YouTube (#3066) New Tab Page Shortcuts section (#3104) Fix VPN configuration removal to stop the tunnel (#3099) avoid resizing webview when keyboard shows/hides (#3094) Add support for skipping sending usage pixels for remote messages (#3106) # Conflicts: # DuckDuckGo.xcodeproj/project.pbxproj # DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved
# By Christopher Brind (9) and others # Via Chris Brind (1) and GitHub (1) * main: (48 commits) Reduce VPN manager instances (#3097) Update BSK for Mac RMF changes (#3107) [DuckPlayer] 7- Open Settings (#3110) [DuckPlayer] 6 - Init updates and Watch on YouTube (#3066) New Tab Page Shortcuts section (#3104) Fix VPN configuration removal to stop the tunnel (#3099) avoid resizing webview when keyboard shows/hides (#3094) Add support for skipping sending usage pixels for remote messages (#3106) Bump submodules/privacy-reference-tests from `a242bf0` to `afb4f61` (#3096) Updates BSK to 171.2.3 Remove print (#3101) Update Package.resolved file (#3102) New Tab Page favorites section (#3083) Properly compare actual value of the entitlement check (#3100) fix ui tests broken by new onboarding and use shared setup flow (#3081) AdHoc lane: Make proper assignment to variable (#3095) Expand AdHoc build workflow, add debug bookmarks screen (#3086) Update BSK to latest - for macOS fix to AdAttribution (#3084) Revert "Fix tests" Fix tests ... # Conflicts: # DuckDuckGo.xcodeproj/project.pbxproj # DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved # DuckDuckGo/NetworkProtectionStatusViewModel.swift
# By Daniel Bernal (6) and others # Via Chris Brind (3) and others * main: (24 commits) [DuckPlayer] 10 . Move DuckPlayer init to TabManager (#3124) Fix threading issue with Mesage broker (#3132) [Duckplayer] - 9. Edge Cases (#3115) Release 7.130.0-2 (#3134) Internal update for: fix index out-of-bounds in startAttachingCrashLogMessages (#3123) (#3128) Attach params to PPro pixels (#3092) Don't show key icon for empty passwords (#3070) Release 7.129.1-0 (#3127) fix index out-of-bounds in startAttachingCrashLogMessages (#3123) Release 7.130.0-1 (#3122) Revert old Dax icon for old onboarding (#3085) Scroll to Internal User setting (#3114) Release 7.130.0-0 (#3118) Allow activating subscription for internal users via debug menu (#3117) Update breakage report locale to JSON format (#3112) [Duckplayer] 8. Age restricted videos (#3111) Reduce VPN manager instances (#3097) Update BSK for Mac RMF changes (#3107) [DuckPlayer] 7- Open Settings (#3110) [DuckPlayer] 6 - Init updates and Watch on YouTube (#3066) ... # Conflicts: # DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift
Task/Issue URL: https://app.asana.com/0/72649045549333/1207151621945908/f
Tech Design URL:
CC:
Description:
This PR reduces the number of VPN manager instances that we create through the lifetime of the app.
Steps to test this PR:
Definition of Done (Internal Only):
Device Testing:
OS Testing:
Internal references:
Software Engineering Expectations
Technical Design Template