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

Fix a memory leak when opening the Reader #541

Merged
merged 1 commit into from
Mar 30, 2023
Merged

Fix a memory leak when opening the Reader #541

merged 1 commit into from
Mar 30, 2023

Conversation

Gio2018
Copy link
Collaborator

@Gio2018 Gio2018 commented Mar 30, 2023 •
edited
Loading

Summary

  • This PR fixes a leak happening when opening an item in the reader

For reference, here's what was happening before

Implementation Details

  • Update ReadableViewModel, add an unowned reference to self in the closure that defines the layout property to avoid the cycle

Test Steps

  • If you're curious about testing, you can open an item in the reader a few times and check the memory graph making sure there's no more than one instance of ReadableViewModel (zero if you're not in the reader anymore)
  • If you wanna see the behavior in the picture, just remove unowned and repeat the test.

PR Checklist:

  • Added Unit / UI tests
  • Self Review (review, clean up, documentation, run tests)
  • Basic Self QA

Screenshots

@Gio2018 Gio2018 added the bug Something isn't working label Mar 30, 2023
@Gio2018 Gio2018 requested a review from a team as a code owner March 30, 2023 22:52
@Gio2018 Gio2018 self-assigned this Mar 30, 2023
@Gio2018 Gio2018 requested review from dskuza and nzeltzer and removed request for a team March 30, 2023 22:52
@pocket-ci
Copy link
Contributor

Messages
📖 No SwiftLint violations! 🎉
📖 Project coverage: 80.59%
📖 Edited 1 files
📖 Created 0 files

PocketKit: Coverage: 79.94

File Coverage
ReadableViewController.swift 77.01% ✅

Generated by 🚫 Danger Swift against bde5a41

@Gio2018 Gio2018 merged commit f2deccc into develop Mar 30, 2023
@Gio2018 Gio2018 deleted the fix/reader-leaks branch March 30, 2023 23:59
nzeltzer
nzeltzer reviewed Mar 31, 2023
View reviewed changes
Copy link
Collaborator

@nzeltzer nzeltzer left a comment

Choose a reason for hiding this comment

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

lgtm (amaze)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nzeltzer nzeltzer nzeltzer left review comments

@bassrock bassrock bassrock approved these changes

@timc-mozilla timc-mozilla timc-mozilla approved these changes

@dskuza dskuza Awaiting requested review from dskuza dskuza was automatically assigned from Pocket/ios

Assignees

@Gio2018 Gio2018

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Gio2018 @pocket-ci @bassrock @nzeltzer @timc-mozilla