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.

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

Bug: Mux player duplicate render / frozen TemplateObject array not unique per call site #517

Closed
luwes opened this issue Dec 10, 2022 · 3 comments
Assignees
Labels
bug Something isn't working

Comments

@luwes
Copy link
Contributor

luwes commented Dec 10, 2022

Related tagged template spec change
tc39/ecma262#840

Which Mux Elements/Packages has a bug?

  • mux-player

Which browser(s) are you using?

Chrome 108.0.5359.98
didn't affect Firefox and Safari in my tests

Which operating system(s) are you using?

MacOS

How are you using Mux Elements?

npm


I finally reproduced this bug locally, one of the hardest bugs I've had the pleasure to diagnose 😅

It turns out it is unrelated to any JS framework, Nuxt nope, Mithril nope. It's reproducible with vanilla JS..
I think it's a Chrome only bug, at least I can only repro it there.

The bug is that the result of our html tagged template does not always return the same frozen TemplateObject array invalidating the WeakMap cache and causing a new DOM fragment to be appended in the shadow root of the player.

export const template = (props: MuxTemplateProps) => html`
<style>
${cssStr}
</style>
${content(props)}
`;

if (renderedTemplates.get(element) !== template) {
renderedTemplates.set(element, template);
const instance = new TemplateInstance(template, this.values, this.processor);
renderedTemplateInstances.set(element, instance);
if (element instanceof NodeTemplatePart) {
element.replace(...instance.children);
} else {
element.appendChild(instance);
}
return;
}

For some reason the frozen TemplateObject array is not unique per call site under a big amount of memory pressure in Chrome.

I could finally reproduce it by adding the app.js file of the ijf.org Judo website which is a huge JS file and adding logic to change the playback id on the fly with Mithril.js. I reproduced it twice by luck, after that nada couldn't reproduce it for another hour or something tried everything. Then thought maybe it's related to some memory increase thing and randomly added this rainbow spiral (https://luwes.github.io/sinuous-rainbow-spiral/) to the Mux player vanilla demo (https://elements-demo-vanilla-mux.vercel.app/mux-player.html) First setting the amount to 2000 for increased memory usage and voila could reproduce it like 50% of the time 🥹

@luwes luwes added bug Something isn't working enhancement New feature or request Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug labels Dec 10, 2022
@luwes luwes self-assigned this Dec 10, 2022
@luwes luwes removed enhancement New feature or request Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug labels Dec 10, 2022
@luwes
Copy link
Contributor Author

luwes commented Dec 10, 2022

I confirmed that the TemplateObject array actually changes by using a regular Map and see if there is a "duplicate" key entry, same array values and this was the case.

SCR-20221210-bv7

@luwes luwes changed the title Bug: Mux player duplicate render / TemplateObject array (incorrectly) garbage collected Bug: Mux player duplicate render / frozen TemplateObject array not unique per call site Dec 12, 2022
@luwes
Copy link
Contributor Author

luwes commented Dec 12, 2022

Update: seems garbage collection is not the culprit, since the old array is still in memory.
It just doesn't return the same frozen array per call site under some conditions.

luwes added a commit to luwes/elements that referenced this issue Dec 12, 2022
@dylanjha
Copy link
Contributor

dylanjha commented Dec 16, 2022

We were able to reproduce this in some isolated cases. We have a patch in 1.5 that we're hoping is a permanent fix.

Going to go ahead and close this and let's re-open if it comes up again

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants