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

johanrd · 2022-11-25T12:04:42Z

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?

glimmer-vm/packages/@glimmer/util/lib/dom.ts

Lines 4 to 12 in 4f1bef0

    
           export function clearElement(parent: SimpleElement) { 
        
             let current: Option<SimpleNode> = parent.firstChild; 
        
             while (current) { 
        
               let next = current.nextSibling; 
        
               parent.removeChild(current); 
        
               current = next; 
        
             } 
        
           }

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

The text was updated successfully, but these errors were encountered:

sebamza17 · 2023-06-29T16:10:20Z

Any news about this one?

NullVoxPopuli · 2023-06-29T16:36:06Z

@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 · 2023-06-29T16:54:55Z

@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 html-next/vertical-collection#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 · 2023-07-19T14:27:45Z

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 · 2023-08-18T19:48:22Z

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 (#1410). Would be lovely if there was some common underlying bug with glimmer & conditionals, but I just mention this as a moonshot.

shama · 2023-08-23T21:30:25Z

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 · 2023-08-23T21:49:37Z

This hack ~~fixes it~~ works around the issue.
#1398 (comment)

shama · 2023-08-24T05:22:37Z

@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 · 2023-08-24T18:56:38Z

@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.

Fixes glimmerjsGH-1401 If `current` isn't a child of `parent` calling `parent.removeChild(current)` will result in the error: `Failed to execute 'removeChild' on 'Node': The node to be removed is not a child of this node`. This commonly occurs when the DOM is externally modified by a browser, such as when translating a page. The `nextSibling` may not be a child node of `parent`. `current.parentNode?.removeChild(current)` ensures we are only removing a child node of a parent node.

chancancode · 2023-08-25T02:03:06Z

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

Fixes glimmerjsGH-1401 If `current` isn't a child of `parent` calling `parent.removeChild(current)` will result in the error: `Failed to execute 'removeChild' on 'Node': The node to be removed is not a child of this node`. This commonly occurs when the DOM is externally modified by a browser, such as when translating a page. The `nextSibling` may not be a child node of `parent`. `current.parentNode?.removeChild(current)` ensures we are only removing a child node of a parent node.

Fixes glimmerjsGH-1401 If `current` isn't a child of `parent` calling `parent.removeChild(current)` will result in the error: `Failed to execute 'removeChild' on 'Node': The node to be removed is not a child of this node`. This commonly occurs when the DOM is externally modified by a browser, such as when translating a page. The `nextSibling` may not be a child node of `parent`.

apellerano-pw · 2023-09-20T14:34:49Z

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 · 2023-10-26T02:40:40Z

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 · 2023-10-26T03:05:03Z

also cross-linking #1438 (comment) for visibility and context

chancancode · 2023-10-26T03:18:25Z

@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 · 2023-10-26T03:22:28Z

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 · 2023-10-29T23:08:49Z

Tracked by Chromium issue #872770

It's 5 years old... 💀

johanrd mentioned this issue Dec 6, 2022

Regression after upgrading vertical-collection to v.3.1.0: Failed to execute 'removeChild' on 'Node' html-next/vertical-collection#397

Open

shama mentioned this issue Aug 24, 2023

fix: ensure child node is a child of parent before removing shama/glimmer-vm#1

Closed

shama mentioned this issue Aug 24, 2023

fix: ensure child node is a child of parent before removing #1438

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

johanrd commented Nov 25, 2022 •

edited

sebamza17 commented Jun 29, 2023

NullVoxPopuli commented Jun 29, 2023

johanrd commented Jun 29, 2023 •

edited

gvdp commented Jul 19, 2023

apellerano-pw commented Aug 18, 2023

shama commented Aug 23, 2023

golampo commented Aug 23, 2023 •

edited

shama commented Aug 24, 2023

golampo commented Aug 24, 2023 •

edited

chancancode commented Aug 25, 2023

apellerano-pw commented Sep 20, 2023 •

edited

gilest commented Oct 26, 2023

chancancode commented Oct 26, 2023

chancancode commented Oct 26, 2023

gilest commented Oct 26, 2023 •

edited

gilest commented Oct 29, 2023

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

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

Comments

johanrd commented Nov 25, 2022 • edited

sebamza17 commented Jun 29, 2023

NullVoxPopuli commented Jun 29, 2023

johanrd commented Jun 29, 2023 • edited

gvdp commented Jul 19, 2023

apellerano-pw commented Aug 18, 2023

shama commented Aug 23, 2023

golampo commented Aug 23, 2023 • edited

shama commented Aug 24, 2023

golampo commented Aug 24, 2023 • edited

chancancode commented Aug 25, 2023

apellerano-pw commented Sep 20, 2023 • edited

gilest commented Oct 26, 2023

chancancode commented Oct 26, 2023

chancancode commented Oct 26, 2023

gilest commented Oct 26, 2023 • edited

gilest commented Oct 29, 2023

johanrd commented Nov 25, 2022 •

edited

johanrd commented Jun 29, 2023 •

edited

golampo commented Aug 23, 2023 •

edited

golampo commented Aug 24, 2023 •

edited

apellerano-pw commented Sep 20, 2023 •

edited

gilest commented Oct 26, 2023 •

edited