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 binding observer memory leak #7023

Merged
merged 4 commits into from
Oct 2, 2024
Merged

Fix binding observer memory leak #7023

merged 4 commits into from
Oct 2, 2024

Conversation

m-akinc
Copy link
Contributor

@m-akinc m-akinc commented Sep 11, 2024

Pull Request

📖 Description

Fixes #7022

🎫 Issues

#7022

👩‍💻 Reviewer Notes

📑 Test Plan

No new tests, since branch is about to be frozen anyway.

✅ Checklist

General

  • I have included a change request file using $ npm run change
  • I have added tests for my changes.
  • I have tested my changes.
  • I have updated the project documentation to reflect my changes.
  • I have read the CONTRIBUTING documentation and followed the standards for this project.

⏭ Next Steps

None. Does not apply to mainline.

@m-akinc m-akinc changed the base branch from master to archives/fast-element-1 September 11, 2024 17:59
This was referenced Sep 11, 2024
Merged
Open
m-akinc added a commit to ni/nimble that referenced this pull request Sep 11, 2024
@m-akinc
Fix two sources of references to detached rows (#2392)
e14c27d 
# Pull Request

## 🤨 Rationale

While investigating a [memory leak
issue](https://ni.visualstudio.com/DevCentral/_workitems/edit/2843309)
reported in AzDO, we found a couple places in the Nimble table code that
wasn't cleaning up observers/notifiers, causing us to hold references to
disconnected row elements.

There is also a FAST bug that contributes to the leak (which is [getting
fixed](microsoft/fast#7023)), so the leak will
still not be fixed until this PR and the FAST one are both in.

## 👩‍💻 Implementation

1. Rows observe columns, which results in there being a reference chain
from each column to each row. When the row is detached, we must remove
the column observers.
2. The keyboard navigation manager has logic for storing the row
notifiers it creates, so that it can clean them up at the appropriate
time. However, by a simple oversight, we were never actually adding the
notifiers to the array.

## 🧪 Testing

Verified (in Chrome dev tools) that after these changes, detached rows
are no longer referenced via the columns or keyboard navigation manager.

## ✅ Checklist

<!--- Review the list and put an x in the boxes that apply or ~~strike
through~~ around items that don't (along with an explanation). -->

- [x] I have updated the project documentation to reflect my changes or
determined no changes are needed.
chrisdholt
chrisdholt approved these changes Sep 13, 2024
View reviewed changes
Copy link
Member

@chrisdholt chrisdholt left a comment

Choose a reason for hiding this comment

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

Would love to get @janechu's review on this as well. Thanks @m-akinc for this fix!

@m-akinc
Copy link
Contributor Author

m-akinc commented Sep 13, 2024

Would love to get @janechu's review on this as well. Thanks @m-akinc for this fix!

She actually approved it a few hours before you. 🙂

@m-akinc
Copy link
Contributor Author

m-akinc commented Sep 16, 2024

Is this ready to be merged?

@janechu janechu mentioned this pull request Sep 24, 2024
Closed
@janechu
Copy link
Collaborator

janechu commented Sep 25, 2024

@m-akinc for some strange reason the checks are not running on this PR, so I took your changes and created a PR myself to see what would happen #7026 and for whatever reason, we are consistently failing some tests on windows.

@m-akinc
Copy link
Contributor Author

m-akinc commented Sep 30, 2024

@m-akinc for some strange reason the checks are not running on this PR, so I took your changes and created a PR myself to see what would happen #7026 and for whatever reason, we are consistently failing some tests on windows.

@janechu Those two slider tests seem suspect. They fail for me locally with slightly different numbers (both with and without my change):

Chrome Headless 128.0.0.0 (Windows 10) Slider should constrain and normalize the value when the `step` attribute has been provided and is a float FAILED
        AssertionError: expected 0.4 to equal 0.6
            at dist/esm/__test__/webpack:/dist/esm/slider/slider.spec.js:134:15
            at Generator.next (<anonymous>)
            at fulfilled (dist/esm/__test__/webpack:/node_modules/tslib/tslib.es6.js:71:42)
Chrome Headless 128.0.0.0 (Windows 10) Slider should update the `stepMultiplier` when the `step` attribute has been updated FAILED                                                                                                                         
        AssertionError: expected 4 to equal 6
            at dist/esm/__test__/webpack:/dist/esm/slider/slider.spec.js:142:15
            at Generator.next (<anonymous>)
            at fulfilled (dist/esm/__test__/webpack:/node_modules/tslib/tslib.es6.js:71:42)

The tests seem to use pixel values with an assumption that the slider is a certain length in pixels. Maybe that is not reliable. Maybe the default control or browser window width has changed. Given that the fast-element-1 branch is about to be locked down, I'd imagine there's no real drawback to just disabling these two tests if you're convinced they're not failing due to my changes.

@janechu
Copy link
Collaborator

janechu commented Oct 1, 2024

Since they are flakey I think it's fine if you skip those tests in this PR so they're working locally, see if that also kicks off the build tests and we can just make a note of it. Thanks for your patience!

@m-akinc
Copy link
Contributor Author

m-akinc commented Oct 1, 2024

@janechu pushed change to skip those two tests. Looks like the build tests need approval from a maintainer to run.

janechu
janechu requested changes Oct 1, 2024
View reviewed changes
Copy link
Collaborator

@janechu janechu left a comment

Choose a reason for hiding this comment

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

We should remove the only, we're only skipping the 2 tests I believe.

@m-akinc m-akinc requested a review from janechu October 1, 2024 20:17
@janechu janechu merged commit fca56ad into microsoft:archives/fast-element-1 Oct 2, 2024
4 of 5 checks passed
@m-akinc
Copy link
Contributor Author

m-akinc commented Oct 15, 2024

@janechu It looks like there are still a few open PRs into the fast-element-1 branch, but can we get another release from that branch in the meantime?

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

@janechu janechu janechu approved these changes

@chrisdholt chrisdholt chrisdholt approved these changes

@awentzel awentzel Awaiting requested review from awentzel

@Falkicon Falkicon Awaiting requested review from Falkicon

@radium-v radium-v Awaiting requested review from radium-v

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

Successfully merging this pull request may close these issues.

fix: memory leak due to design token bindings
3 participants
@m-akinc @janechu @chrisdholt