-
Notifications
You must be signed in to change notification settings - Fork 600
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
Fix binding observer memory leak #7023
Fix binding observer memory leak #7023
Conversation
# 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.
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.
Is this ready to be merged? |
@janechu Those two slider tests seem suspect. They fail for me locally with slightly different numbers (both with and without my change):
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. |
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! |
@janechu pushed change to skip those two tests. Looks like the build tests need approval from a maintainer to run. |
packages/web-components/fast-foundation/src/slider/slider.spec.ts
Outdated
Show resolved
Hide resolved
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 should remove the only, we're only skipping the 2 tests I believe.
Co-authored-by: Jane Chu <[email protected]>
fca56ad
into
microsoft:archives/fast-element-1
@janechu It looks like there are still a few open PRs into the |
Pull Request
π Description
Fixes #7022
π« Issues
#7022
π©βπ» Reviewer Notes
π Test Plan
No new tests, since branch is about to be frozen anyway.
β Checklist
General
$ npm run change
β Next Steps
None. Does not apply to mainline.