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

[3.17.0-beta] Destroying -in-element: Failed to execute 'removeChild' on 'Node' #18696

Closed
simonihmig opened this issue Jan 24, 2020 · 8 comments · Fixed by glimmerjs/glimmer-vm#1023 or #18728

Comments

@simonihmig
Copy link
Contributor

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

This error is thrown since 3.17.0-beta (IIRC also in canary versions before), apparently when destroying an -in-element. I did not dig into this, but all tests of ember-bootstrap that involve -in-element (modals, dropdowns, tooltips, popovers) are failing with that error.

To confirm I just triggered a rebuild of ember-popper - which also uses -in-element - and the same failing tests there.

Btw, both addons use ember-maybe-in-element, but I believe this should not matter as it is a build-time AST transform.

Likely related to #18621

/cc @pzuraq @cibernox

@pzuraq
Copy link
Contributor

pzuraq commented Jan 24, 2020

There have been changes to the {{-in-element}} helper in preparation for making it a public API, so it could be that the usage of it needs to change

@simonihmig
Copy link
Contributor Author

@pzuraq I have created a reproduction here: https://github.com/simonihmig/in-element-reproduction

This fails even when taking the new semantics (insertBefore) into account, so I don't think this particular error is related to wrong usage!

To reproduce it seems these preconditions need to come together:

  • using a rendering test. I was not able to reproduce in normal app code, neither was I able to create a failing test in the Ember codebase. Maybe it is somehow related to the way the test infrastructure does the teardown of DOM? 🤔
  • the destination element is part of the test element (#ember-testing). It did work when rendering to a detached DOM element.
  • and - and this is the most interesting part - the template needs to generate some empty text nodes. The error thrown seems to be about removing those text nodes (after the test finished!).

The latter point is easy to see in the failing test here, as the (passing) first test is identical except for the white space!

Note: both tests pass for Ember 3.16.0 (when removing the unsupported insertBefore=null)

@chancancode
Copy link
Member

I ran into this issue (Skylight tests currently failing on beta) with a similar setup.

I think the problem is that we are using -in-element to render into an element that was torn down before the -in-element block itself got torn down.

I think it boils down to something like this:

{{!-- application.hbs --}}
<header>First</header>

{{!-- in practice this happens somewhere deeper in the app, but shouldn't matter --}}
{{#-in-element ($ ".ember-application") insertBefore=null}}
  <div>Middle</div>
{{/-in-element}}

<footer>Last</footer>

Which results in this DOM output:

<header>First</header>
<div>Middle</div>
<footer>Last</footer>

The application.hbs has the bounds of (<header>First</header> to <footer>Last</footer> inclusive), and the remote element block has the bounds of (<div>Middle</div> to <div>Middle</div> inclusive). When tearing down the application, the new VM seems to run the teardown for the outer application.hbs block before its children, so by the time the remote element block's destructor runs, its DOM elements have already been removed, since the outer block ran clear(parent, <header>...</footer>).

Based on my initial reading of the code, I'm a bit worried that this is a bigger problem that affects more than just -in-element.

@rwjblue
Copy link
Member

rwjblue commented Jan 31, 2020

Chatted about this in today's framework meeting, @pzuraq mentioned he would try to dig into this next week...

@chancancode
Copy link
Member

chancancode commented Feb 6, 2020

@simonihmig can you check if #18727 fixed the issue for you by any chance? I didn't think it was related, but I can't get the Skylight tests to fail the same way anymore 😓

never mind, I was just testing it wrong!

@simonihmig
Copy link
Contributor Author

I tested it with my reproduction repo to make sure, but indeed it did not fix the issue!

wycats added a commit to glimmerjs/glimmer-vm that referenced this issue Feb 7, 2020
This usually happens when the app uses `{{in-element}}` to append
to `Ember.Application.rootElement` during initial render, which
causes Glimmer to (incorrectly) include the "remoted content" in
the root render result. When the render result is torn down, the
"remoted content" gets removed as well, and so by the time its
destructor runs it will be trying to clear its content again,
which causes the double-clearing error ("The node to be removed
is not a child of this node." in "removeChild").

This is probably a better fix to how Glimmer views the boundaries
it manages, but this should be sufficient to supress the error for
now without too much undesirable side-effects.

Fixes emberjs/ember.js#18696

Co-authored-by: Yehuda Katz <[email protected]>
wycats added a commit to glimmerjs/glimmer-vm that referenced this issue Feb 7, 2020
This usually happens when the app uses `{{in-element}}` to append
to `Ember.Application.rootElement` during initial render, which
causes Glimmer to (incorrectly) include the "remoted content" in
the root render result. When the render result is torn down, the
"remoted content" gets removed as well, and so by the time its
destructor runs it will be trying to clear its content again,
which causes the double-clearing error ("The node to be removed
is not a child of this node." in "removeChild").

This is probably a better fix to how Glimmer views the boundaries
it manages, but this should be sufficient to supress the error for
now without too much undesirable side-effects.

Fixes emberjs/ember.js#18696

Co-authored-by: Yehuda Katz <[email protected]>
@chancancode chancancode reopened this Feb 7, 2020
chancancode added a commit that referenced this issue Feb 7, 2020
@chancancode
Copy link
Member

@simonihmig can you test this again on beta or canary?

@simonihmig
Copy link
Contributor Author

@chancancode Did so, and can confirm the exception does not occur anymore, both in the reproduction and in the ember-bootstrap test suite! 🎉

I do see another issue for beta/canary, but this seems to be a different thing I need to investigate...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants