Failed to execute 'removeChild' on 'Node': The node to be removed is not a child of this node.

johanrd commented 1 year ago

I see the error Failed to execute 'removeChild' on 'Node': The node to be removed is not a child of this node popping up in sentry from time to time. I have yet to reproduce it when I try, but I know it freezes the whole UI when it happens.

It is not a very viable solution to tell users to uninstall various chrome extensions and/or not use google translate in order to not break a glimmer app. This has occurred both on Chrome and Edge.

Is it possible with a more resilient handling in clearElement?

https://github.com/glimmerjs/glimmer-vm/blob/4f1bef0d9a8a3c3ebd934c5b6e09de4c5f6e4468/packages/%40glimmer/util/lib/dom.ts#L4-L12

Perhaps a try-catch? Or will that disturb some other inner workings?

sebamza17 commented 1 year ago

Any news about this one?

NullVoxPopuli commented 1 year ago

@sebamza17 do you have any code that modifiers / remove DOM on its own, unknown to Glimmer? (same for @johanrd )

A reproduction repo would be most helpful.

johanrd commented 1 year ago

@NullVoxPopuli: For me, this error is logged in sentry from time to time from real world usage by customers (both from Chrome and Edge users)

No steps to reproduce yet (the closest I've got is https://github.com/html-next/vertical-collection/issues/397), but a hypothesis is that some users have browser extensions that manipulate the DOM.

Anyways, glimmer should be more resilient to such errors IMO.

gvdp commented 1 year ago

Also seeing this pop up from time to time in our bug tracker, but unable to recreate it myself. Maybe just a specific error message instead of just the default DOMException would already help in tracking down the real issue?

apellerano-pw commented 1 year ago

We get this one in our logs using latest ember-table 5.0.6 and ember 3.28. We conditionally insert a div into the occluded list managed by vertical-collection, and the conditional's Comment Node in the DOM is the node implicated when breaking on the error.

<et.body as |b|>
  {{#if (eq this.rowHighlightId b.rowValue.id)}}
    <div class="rowHighlight"></div>
  {{/if}}
  <b.row/>
</et.body>

Conditionals have been a meaningful contributor for another glimmer-vm bug (https://github.com/glimmerjs/glimmer-vm/issues/1410). Would be lovely if there was some common underlying bug with glimmer & conditionals, but I just mention this as a moonshot.

shama commented 1 year ago

I can repro this by changing the language of my app with <html lang="es"> then use a browser's "translate this page" feature:

Wait for translate to modify the DOM, then do anything that causes the modified DOM to be removed (such as clicking a link to another route):

I haven't been able to figure out a workaround or way to ignore this error as this error halts Glimmer from rendering afterwards.

golampo commented 1 year ago

This hack ~~fixes it~~ works around the issue. https://github.com/glimmerjs/glimmer-vm/issues/1398#issuecomment-1380940238

shama commented 1 year ago

@golampo Thanks but wrapping things that are affected by this issue in htmlSafe isn't safe nor feasible for most apps. I wouldn't say it "fixes it" but appreciate the idea.

I think this is a very common use case that warrants more resilient handling as OP suggests.

golampo commented 1 year ago

@shama The linked comment describes that the issue comes from the existing usages of htmlSafe. The workaround is modifying the output of htmlSafe. I didn't intend to suggest it as a permanent fix, and I mentioned in the comment that it's "not a reasonable solution in most cases". I would really love to see this fixed permanently, but it's still existing after several years despite multiple reports. While not ideal, the workaround has been the only option for allowing "translate" to function w/ out error that I'm aware of.

chancancode commented 1 year ago

Let’s say the template is

{{#if this.isExpanded}}
  the time is {{this.currentTime}}.
{{/if}}

We kept a reference to the TextNode with the current time. When the time changes we need to update it. If at that time we discovered the text node is gone because google translate decided it’s ok to merge the adjacent text nodes together during translation, what are we supposed to do?

When the condition changes to false we need to clear the DOM. What are we supposed to do if we realized the nodes are not there anymore because Google translate decided to remove ours and make new ones in their place? Sure we can fail silently but that wouldn’t be correct either, you’ll just leave behind the translated text when it should have been removed.

I understand why it feels like we should “just fix it” but we need to be more specific about how we are expected to interop with things that are allowed to arbitrarily change the DOM we manage

apellerano-pw commented 1 year ago

Does anyone have a good debugging play when they encounter these? Perhaps a way to break execution the moment the Ember-managed element leaves the DOM in an unsupported way? Currently we are only seeing downstream of the root issue. We don't find out the DOM was invaded until Ember tries to remove an already-removed managed element. But reactivity broke long before then. Since we can't fix this downstream, we need to see if there are fixes that can be done at the root cause.

I think there's 2 classes of this issue. One is triggered by external actors, like Google translate or browser extension content scripts. The other kind comes from inside the house, e.g. it's possible to trigger these with handlebars conditionals inside VerticalCollection.

We should be able to address the instances caused by the Ember ecosystem itself, even if there's no good solution to the external actors problem.

There's maybe a clever spot to set up a MutationObserver. Temporarily during local development? With flag support inside Glimmer? Does knowing the parent element give us a foothold?

Being able to catch these in the moment will lead to reproduceable bug reports that don't involve external actors.

If there were a performant detector, it could even be used in production to show a banner or toast to users when they break reactivity via external actors. If we can't fix the root issue, maybe we can improve the user experience when it happens.

gilest commented 1 year ago

In the past I have tried to discourage page translation with some HTML meta. Something like

<!DOCTYPE html>
<html lang="en-US" translate="no" class="notranslate">
<head>
  <meta name="google" content="notranslate">

Although not specific glimmer and targeted at only a single root cause (page translation)

Also for context: Historical react issue facebook/react/issues/11538

chancancode commented 1 year ago

also cross-linking https://github.com/glimmerjs/glimmer-vm/pull/1438#issuecomment-1780311949 for visibility and context

chancancode commented 1 year ago

@gilest Yeah, I think that sounds about right and to the extent you are able to discourage these unauthorized mutations from happening, that would be the most ideal.

Beyond that, I think trying to catalog and the type of behavior of these tools may also help you find targeted workarounds. For example, if my assumption is correct that (one of) the issue is Google translate sometimes merging/replacing adjacent text nodes while doing the translation in a part of the DOM we are tracking, say:

{{!-- [ ... ] denotes a single text node --}}
[Hello, welcome ]{{#if @user.isNew}}[aboard]{{else}}[back]{{/if}}[.]

{{!-- rendered --}}
[Hello, welcome ][aboard][.]

{{!-- translated --}}
[你好，歡迎光臨。]

Then, maybe you can prevent that somewhat by:

{{!-- [ ... ] denotes a single text node --}}
[Hello, welcome ]{{#if @user.isNew}}<!-- -->[aboard]<!-- -->{{else}}[back]{{/if}}[.]

{{!-- [ ... ] denotes a single text node --}}
[Hello, welcome ]<!-- Glimmer can now use this comment node for the bound start -->[aboard]<!-- bound end -->[.]

If Google translate is civilized enough to know that it can't just remove comments randomly, then as far as we are concerned we don't care if they replace the middle text node. If we have specific cases we can analyze we can try to encourage the other party to play nicely, but yes, I think the root cause is not really fixable on our end.

gilest commented 1 year ago

Interesting. It sort of looks like Fastboot output markers for rehydration.

I think I read somewhere that Google Translate was particularly bad at mangling the DOM, like maybe it adds <font> nodes also.

Would your suggestion survive something like that?

Also curious about the performance impact of managing said comments... 🤔

gilest commented 1 year ago

Tracked by Chromium issue #872770

It's 5 years old... 💀

mkszepp commented 3 weeks ago

We have found in last time also this error in our production error logs... We have already <html lang="en-US" translate="no" class="notranslate"> and <meta name="google" content="notranslate">, but Firefox doesn't respect this (at least since version 128).

To fix this you need to add translate="no" also on body-tag

<!DOCTYPE html>
<html lang="en-US" translate="no" class="notranslate">
  <head>
    <meta name="google" content="notranslate">
  </head>
  <body translate="no">
  </body>
</html>

glimmerjs / glimmer-vm

Failed to execute 'removeChild' on 'Node': The node to be removed is not a child of this node. #1401