Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Remove NewTabPage retain cycles #3532

Merged
merged 3 commits into from
Nov 5, 2024
Merged

Remove NewTabPage retain cycles #3532

merged 3 commits into from
Nov 5, 2024
+20 −29

Conversation

dus7
Copy link
Contributor

@dus7 dus7 commented Nov 4, 2024 •
edited
Loading

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):

Internal references:

Software Engineering Expectations
Technical Design Template

@dus7 dus7 changed the base branch from main to release/7.144.0 November 4, 2024 20:33
@dus7 dus7 requested a review from samsymons November 4, 2024 21:02
samsymons
samsymons approved these changes Nov 4, 2024
View reviewed changes
Copy link
Contributor

@samsymons samsymons left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I've tested the favicon fetcher, which looks good, and confirmed there are no leaks when running through that flow. I've also smoke tested various NTP and settings features, no issues found there.

@dus7 dus7 merged commit b1cb778 into release/7.144.0 Nov 5, 2024
26 checks passed
@dus7 dus7 deleted the mariusz/ntp-leaking branch November 5, 2024 12:42
samsymons added a commit that referenced this pull request Nov 5, 2024
@samsymons
Merge branch 'main' into dominik/xcode-16
22a19c9 
# By Daniel Bernal (7) and others
# Via Daniel Bernal (3) and others
* main: (29 commits)
  Send pixel on sync secure storage failure (#3542)
  Onboarding Add to Dock Refactor for Intro scenario (#3538)
  Update C-S-S to 6.29.0 (#3541)
  Change save password Never for Site button to Not Now (#3471)
  Release 7.144.0-1 (#3540)
  UserDefaults misbehavior monitoring (#3510)
  Send pixel on sync secure storage read failure (#3530)
  Remove NewTabPage retain cycles (#3532)
  Update release notes (#3529)
  Release 7.144.0-0 (#3528)
  Add Privacy Config feature to control ad attribution reporting (#3506)
  Validate VPN errors before re-throwing them (#3513)
  Allowing users to delete suggestions on macOS (#3465)
  Update build number
  Update build number
  Bump rexml from 3.3.8 to 3.3.9 (#3495)
  Release 7.142.1-1 (#3525)
  Add a debouncer to NavBars animator (#3519)
  Release 7.143.0-1 (#3516)
  Update to subscription cookie (#3512)
  ...

# Conflicts:
#	DuckDuckGo.xcodeproj/project.pbxproj
#	DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved
samsymons added a commit that referenced this pull request Nov 6, 2024
@samsymons
Merge branch 'main' into sam/migrate-daily-and-count-type-to-legacy-s…
074f339 
…tatus

# By Graeme Arthur (4) and others
# Via GitHub (1) and Graeme Arthur (1)
* main:
  Adding app backgrounded result to rule compilation (#3533)
  Send pixel on sync secure storage failure (#3542)
  Onboarding Add to Dock Refactor for Intro scenario (#3538)
  Update C-S-S to 6.29.0 (#3541)
  Change save password Never for Site button to Not Now (#3471)
  Release 7.144.0-1 (#3540)
  UserDefaults misbehavior monitoring (#3510)
  Send pixel on sync secure storage read failure (#3530)
  Remove NewTabPage retain cycles (#3532)
  Update release notes (#3529)

# Conflicts:
#	DuckDuckGo.xcodeproj/project.pbxproj
#	DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved
samsymons added a commit that referenced this pull request Nov 6, 2024
@samsymons
Merge branch 'main' into sam/fix-memory-pressure-monitor
facee90 
* main:
  Update Ruby to 3.3.4 (#3547)
  Onboarding Add To Dock Pixels (#3543)
  Switch to free runners for tests that run on Maestro (#3546)
  Fix email protection test (#3539)
  Update BSK for PixelKit suffix change (#3534)
  Adding app backgrounded result to rule compilation (#3533)
  Send pixel on sync secure storage failure (#3542)
  Onboarding Add to Dock Refactor for Intro scenario (#3538)
  Update C-S-S to 6.29.0 (#3541)
  Change save password Never for Site button to Not Now (#3471)
  Release 7.144.0-1 (#3540)
  UserDefaults misbehavior monitoring (#3510)
  Send pixel on sync secure storage read failure (#3530)
  Remove NewTabPage retain cycles (#3532)
  Update release notes (#3529)
samsymons added a commit that referenced this pull request Nov 6, 2024
@samsymons
Merge branch 'main' into sam/vpn-clean-up
9b9be64 
# By Graeme Arthur (4) and others
# Via GitHub (1) and Graeme Arthur (1)
* main:
  Fix VPN memory pressure monitor (#3535)
  Update Ruby to 3.3.4 (#3547)
  Onboarding Add To Dock Pixels (#3543)
  Switch to free runners for tests that run on Maestro (#3546)
  Fix email protection test (#3539)
  Update BSK for PixelKit suffix change (#3534)
  Adding app backgrounded result to rule compilation (#3533)
  Send pixel on sync secure storage failure (#3542)
  Onboarding Add to Dock Refactor for Intro scenario (#3538)
  Update C-S-S to 6.29.0 (#3541)
  Change save password Never for Site button to Not Now (#3471)
  Release 7.144.0-1 (#3540)
  UserDefaults misbehavior monitoring (#3510)
  Send pixel on sync secure storage read failure (#3530)
  Remove NewTabPage retain cycles (#3532)
  Update release notes (#3529)
  Release 7.144.0-0 (#3528)
  Add Privacy Config feature to control ad attribution reporting (#3506)

# Conflicts:
#	DuckDuckGo.xcodeproj/project.pbxproj
#	DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved
samsymons added a commit that referenced this pull request Nov 6, 2024
@samsymons
Merge branch 'main' into sam/remove-test-rollout-pixel
6a62d10 
# By Graeme Arthur (4) and others
# Via GitHub (1) and Graeme Arthur (1)
* main:
  VPN clean-up (#3502)
  add daily pixel for deleting suggestions (#3531)
  Fix VPN memory pressure monitor (#3535)
  Update Ruby to 3.3.4 (#3547)
  Onboarding Add To Dock Pixels (#3543)
  Switch to free runners for tests that run on Maestro (#3546)
  Fix email protection test (#3539)
  Update BSK for PixelKit suffix change (#3534)
  Adding app backgrounded result to rule compilation (#3533)
  Send pixel on sync secure storage failure (#3542)
  Onboarding Add to Dock Refactor for Intro scenario (#3538)
  Update C-S-S to 6.29.0 (#3541)
  Change save password Never for Site button to Not Now (#3471)
  Release 7.144.0-1 (#3540)
  UserDefaults misbehavior monitoring (#3510)
  Send pixel on sync secure storage read failure (#3530)
  Remove NewTabPage retain cycles (#3532)
  Update release notes (#3529)

# Conflicts:
#	DuckDuckGo.xcodeproj/project.pbxproj
#	DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@samsymons samsymons samsymons approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dus7 @samsymons