-
Notifications
You must be signed in to change notification settings - Fork 58
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
SafeRenderManager retention workaround #733
Conversation
Security InsightsNo security relevant content was detected by automated scans. Action Items
Questions or Comments? Reach out on Slack: #support-infosec. |
|
||
// AF-3369 This will be removed once the transition to Dart 2 is complete. | ||
// ignore: mixin_of_non_class, undefined_class | ||
class SafeRenderManagerHelperProps extends _$SafeRenderManagerHelperProps with _$SafeRenderManagerHelperPropsAccessorsMixin { |
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.
I upgraded the boilerplate while I was in here, but this should be fine this component is completely internal
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.
I've confirmed this resolves the specific leak I was investigating for spreadsheets, and it appears that several other tests were also impacted by this issue and are resolved by this fix (still waiting on a final test run).
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.
+1 / QA +1 (CI passing)
FYI Luke is QAing in DPC before we merge. |
Yep waiting on https://ci.webfilings.com/build/3406425 |
@Workiva/release-management-p |
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.
+1 from RM
Motivation
SafeRenderManager gets retained by the rendered SafeRenderManagerHelper, which in some cases was being retained by React, seemingly because of the following behavior:
From facebook/react#16087:
These retentions eventually resolve themselves, so they're not true leaks, but they can show up as leaks in memory leak tests that look for disposed-but-retained objects (via Disposable's LeakFlag mechanism).
There are cases where it's very difficult to avoid these kinds of React retention paths. However, this case happens to be easier to avoid, and doing so would reduce noise and some false positives in these memory leak tests, so we want to update SafeRenderManager to not be retained by the tree it renders.
This won't prevent React from retaining the React tree it renders, which may retain other Disposable instances and still be flagged as leaks; only the SafeRenderManager object itself will no longer be retained. However, it's possible that the way content is "safely" unmounted (by rerendering SafeRenderManagerHelper with empty children before unmounting) helps prevent consumer-provided content being retained, but we're not sure at this time.
Changes
_contentCallbackRef
- fixed by using a ref object instead_helperRef
- fixed by instead retaining a_selfRef
ref object which holds a SafeRenderManager, and gets cleared out on disposalNote that callback refs aren't inherently problematic, nor was how SafeRenderManager was being retained by the tree it rendered. The root issue here is React retaining things longer than we'd like/expect it to, and this issue helps work around that to reduce noise in tests.
Release Notes
SafeRenderManager
to not be retained by the React tree it renders, to aid in memory leak testing. Outside of that context, this change has negligible effects.Review
See CONTRIBUTING.md for more details on review types (+1 / QA +1 / +10) and code review process.
Please review:
QA Checklist
Merge Checklist
While we perform many automated checks before auto-merging, some manual checks are needed: