Skip to content
This repository was archived by the owner on Feb 24, 2025. It is now read-only.

Commit b1cb778

Browse files
authored
Remove NewTabPage retain cycles (#3532)
Task/Issue URL: https://app.asana.com/0/1206226850447395/1208686031091434/f Tech Design URL: CC: **Description**: Leak 1: `NewTabPageViewController` was not dismissed properly, this caused it to stay in memory as a child view controller of `MainViewController`. In effect it was possible to dismiss FaviconFetcherTutorial. Removing the leak required to do additional changes to make it work. I moved it to `MainViewController`, so that it's not dependent on NewTabPage. Leak 2: `NewTabPageSettingsModel` was leaking via strong reference present in `NTPSettingItem`. **Steps to test this PR**: 1. Set up Sync. 2. Add favorite. 3. On another synced device open New Tab Page 4. Favicon Tutorial should appear, see if buttons work, dismissing the tutorial 5. Open and close New Tab Page a few times. 6. Open Memory Graph Debugger, verify only single instance is present for `NewTabPageViewController` and `NewTabPageSettingsModel*` objects. **Definition of Done (Internal Only)**: * [ ] Does this PR satisfy our [Definition of Done](https://app.asana.com/0/1202500774821704/1207634633537039/f)? --- ###### Internal references: [Software Engineering Expectations](https://app.asana.com/0/59792373528535/199064865822552) [Technical Design Template](https://app.asana.com/0/59792373528535/184709971311943)
1 parent 2b49550 commit b1cb778

File tree

5 files changed

+20
-29
lines changed

5 files changed

+20
-29
lines changed

DuckDuckGo/MainViewController.swift

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,7 @@ class MainViewController: UIViewController {
128128

129129
private lazy var featureFlagger = AppDependencyProvider.shared.featureFlagger
130130
private lazy var faviconLoader: FavoritesFaviconLoading = FavoritesFaviconLoader()
131+
private lazy var faviconsFetcherOnboarding = FaviconsFetcherOnboarding(syncService: syncService, syncBookmarksAdapter: syncDataProviders.bookmarksAdapter)
131132

132133
lazy var menuBookmarksViewModel: MenuBookmarksInteracting = {
133134
let viewModel = MenuBookmarksViewModel(bookmarksDatabase: bookmarksDatabase, syncService: syncService)
@@ -795,8 +796,6 @@ class MainViewController: UIViewController {
795796
let controller = NewTabPageViewController(tab: tabModel,
796797
isNewTabPageCustomizationEnabled: homeTabManager.isNewTabPageSectionsEnabled,
797798
interactionModel: favoritesViewModel,
798-
syncService: syncService,
799-
syncBookmarksAdapter: syncDataProviders.bookmarksAdapter,
800799
homePageMessagesConfiguration: homePageConfiguration,
801800
privacyProDataReporting: privacyProDataReporter,
802801
variantManager: variantManager,
@@ -2172,6 +2171,10 @@ extension MainViewController: NewTabPageControllerDelegate {
21722171
func newTabPageDidDeleteFavorite(_ controller: NewTabPageViewController, favorite: BookmarkEntity) {
21732172
// no-op for now
21742173
}
2174+
2175+
func newTabPageDidRequestFaviconsFetcherOnboarding(_ controller: NewTabPageViewController) {
2176+
faviconsFetcherOnboarding.presentOnboardingIfNeeded(from: self)
2177+
}
21752178
}
21762179

21772180
extension MainViewController: NewTabPageControllerShortcutsDelegate {

DuckDuckGo/NewTabPageControllerDelegate.swift

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ protocol NewTabPageControllerDelegate: AnyObject {
2424
func newTabPageDidOpenFavoriteURL(_ controller: NewTabPageViewController, url: URL)
2525
func newTabPageDidDeleteFavorite(_ controller: NewTabPageViewController, favorite: BookmarkEntity)
2626
func newTabPageDidEditFavorite(_ controller: NewTabPageViewController, favorite: BookmarkEntity)
27+
func newTabPageDidRequestFaviconsFetcherOnboarding(_ controller: NewTabPageViewController)
2728
}
2829

2930
protocol NewTabPageControllerShortcutsDelegate: AnyObject {

DuckDuckGo/NewTabPageSettingsModel.swift

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -81,15 +81,15 @@ final class NewTabPageSettingsModel<SettingItem: NewTabPageSettingsStorageItem,
8181
private func populateSettings() {
8282
let allItemsOrder = settingsStorage.itemsOrder
8383
filteredItemsOrder = allItemsOrder.filter(visibilityFilter)
84-
84+
8585
itemsSettings = filteredItemsOrder.compactMap { item in
8686
return NTPSetting(item: item,
87-
isEnabled: Binding(get: {
88-
self.settingsStorage.isEnabled(item)
89-
}, set: { newValue in
90-
self.onItemEnabled?(item, newValue)
91-
self.settingsStorage.setItem(item, enabled: newValue)
92-
self.updatePublishedValues()
87+
isEnabled: Binding(get: { [weak self] in
88+
self?.settingsStorage.isEnabled(item) ?? false
89+
}, set: { [weak self] newValue in
90+
self?.onItemEnabled?(item, newValue)
91+
self?.settingsStorage.setItem(item, enabled: newValue)
92+
self?.updatePublishedValues()
9393
}))
9494
}
9595

DuckDuckGo/NewTabPageViewController.swift

Lines changed: 6 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -25,14 +25,10 @@ import Core
2525

2626
final class NewTabPageViewController: UIHostingController<AnyView>, NewTabPage {
2727

28-
private let syncService: DDGSyncing
29-
private let syncBookmarksAdapter: SyncBookmarksAdapter
3028
private let variantManager: VariantManager
3129
private let newTabDialogFactory: any NewTabDaxDialogProvider
3230
private let newTabDialogTypeProvider: NewTabDialogSpecProvider
3331

34-
private(set) lazy var faviconsFetcherOnboarding = FaviconsFetcherOnboarding(syncService: syncService, syncBookmarksAdapter: syncBookmarksAdapter)
35-
3632
private let newTabPageViewModel: NewTabPageViewModel
3733
private let messagesModel: NewTabPageMessagesModel
3834
private let favoritesModel: FavoritesViewModel
@@ -53,8 +49,6 @@ final class NewTabPageViewController: UIHostingController<AnyView>, NewTabPage {
5349
init(tab: Tab,
5450
isNewTabPageCustomizationEnabled: Bool,
5551
interactionModel: FavoritesListInteracting,
56-
syncService: DDGSyncing,
57-
syncBookmarksAdapter: SyncBookmarksAdapter,
5852
homePageMessagesConfiguration: HomePageMessagesConfiguration,
5953
privacyProDataReporting: PrivacyProDataReporting? = nil,
6054
variantManager: VariantManager,
@@ -63,8 +57,6 @@ final class NewTabPageViewController: UIHostingController<AnyView>, NewTabPage {
6357
faviconLoader: FavoritesFaviconLoading) {
6458

6559
self.associatedTab = tab
66-
self.syncService = syncService
67-
self.syncBookmarksAdapter = syncBookmarksAdapter
6860
self.variantManager = variantManager
6961
self.newTabDialogFactory = newTabDialogFactory
7062
self.newTabDialogTypeProvider = newTabDialogTypeProvider
@@ -145,7 +137,8 @@ final class NewTabPageViewController: UIHostingController<AnyView>, NewTabPage {
145137
private func assignFavoriteModelActions() {
146138
favoritesModel.onFaviconMissing = { [weak self] in
147139
guard let self else { return }
148-
self.faviconsFetcherOnboarding.presentOnboardingIfNeeded(from: self)
140+
141+
delegate?.newTabPageDidRequestFaviconsFetcherOnboarding(self)
149142
}
150143

151144
favoritesModel.onFavoriteURLSelected = { [weak self] url in
@@ -215,7 +208,10 @@ final class NewTabPageViewController: UIHostingController<AnyView>, NewTabPage {
215208
}
216209

217210
func dismiss() {
218-
211+
delegate = nil
212+
chromeDelegate = nil
213+
removeFromParent()
214+
view.removeFromSuperview()
219215
}
220216

221217
func showNextDaxDialog() {

DuckDuckGoTests/NewTabPageControllerDaxDialogTests.swift

Lines changed: 1 addition & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -38,14 +38,7 @@ final class NewTabPageControllerDaxDialogTests: XCTestCase {
3838
variantManager = CapturingVariantManager()
3939
dialogFactory = CapturingNewTabDaxDialogProvider()
4040
specProvider = MockNewTabDialogSpecProvider()
41-
let dataProviders = SyncDataProviders(
42-
bookmarksDatabase: db,
43-
secureVaultFactory: AutofillSecureVaultFactory,
44-
secureVaultErrorReporter: SecureVaultReporter(),
45-
settingHandlers: [],
46-
favoritesDisplayModeStorage: MockFavoritesDisplayModeStoring(),
47-
syncErrorHandler: SyncErrorHandler()
48-
)
41+
4942
let remoteMessagingClient = RemoteMessagingClient(
5043
bookmarksDatabase: db,
5144
appSettings: AppSettingsMock(),
@@ -60,8 +53,6 @@ final class NewTabPageControllerDaxDialogTests: XCTestCase {
6053
tab: Tab(),
6154
isNewTabPageCustomizationEnabled: false,
6255
interactionModel: MockFavoritesListInteracting(),
63-
syncService: MockDDGSyncing(authState: .active, isSyncInProgress: false),
64-
syncBookmarksAdapter: dataProviders.bookmarksAdapter,
6556
homePageMessagesConfiguration: homePageConfiguration,
6657
variantManager: variantManager,
6758
newTabDialogFactory: dialogFactory,

0 commit comments

Comments
 (0)