Conversation
|
@spuppo-mux is attempting to deploy a commit to the Mux Team on Vercel. A member of the Team first needs to authorize it. |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1261 +/- ##
==========================================
- Coverage 78.55% 73.72% -4.84%
==========================================
Files 59 56 -3
Lines 11080 13821 +2741
Branches 0 784 +784
==========================================
+ Hits 8704 10189 +1485
- Misses 2376 3598 +1222
- Partials 0 34 +34 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
luwes
left a comment
There was a problem hiding this comment.
Thanks for all the fixes.
Are you sure these are all necessary?
Could you try to make the least amount of changes that are needed to solve the problem, could be that it's needed. Just double checking.
This PR addresses some memory leak issues across many of media-chrome components. Relates to muxinc/elements#1235
The main idea in these changes is to clean references on component disconnect. It includes changes like removing added event listeners, disconnecting MutationObservers, and making sure we keep reference to the functions passed as callbacks to these types of objects.
It also includes some minor fixes like: adding typing; moving some logic from the components constructor to connectedCallback, for example adding event listeners or reinitializing stuff that is removed on disconnect; adding guards so that listeners are not added more than once.
Testing
To test this I added the
examples/vanilla/memory-leak-tester.htmlfile which allows mounting and unmounting a<media-controller>element.The process consisted on mounting and unmounting the component and taking a heap snapshot to verify no leaks are present. To track memory leaks you can check the element's retainer tree to try and find who is holding the reference. It's highly recommended to use a non minified version of the code when testing.
Some extra details
<video>tagWhen debugging media chrome, even if it's not specifically a component being leaked, we may find a Detached video tag
It should be an empty
<video>tag, which relates totestMediaElused insrc/js/utils/platform-tests.tsas we can see from it's retainers. It is purposefully maintained in memory to not create several ones when checking some browser compatibilities.Meaning that a "clean" heap snapshot looks like this:

To remove this I added the
inertattribute onmedia-container.ts:414, but this makes the element non clickable, which may not be desired. Alternatively, we can remove the aria-hidden attribute, but I understand this was added to address accessibility issues in a previous PR. muxinc/elements#1040On media-controller:510 (mediaSetCallback), we set media tabIndex to -1, meaning it's not keyboard accessible.
<symbol Window#DocumentCachedAccessor>inWindow (global*) /appears as one of the first retainers, it means that this is happening.