Snabbdom tries to remove a node that is already detached, throws exception

[cjohansen]

Changing the content of an element from an array of children to some innerHTML content currently produces the following exception:

Uncaught DOMException: Node.removeChild: The node to be removed is not a child of this node

This bug can be reproduced as such:

var patch = snabbdom.init([
  snabbdom.propsModule
]);

var el = document.createElement('div');
document.body.appendChild(el);

var vdom = patch(el, snabbdom.h('p', {}, ["Text"]));
patch(vdom, snabbdom.h('p', {props: {innerHTML: "Eh?"}}, []));

In other words:

• Snabbdom renders an element with TextNode children
• Snabbdom reuses the element for a new vnode that has no children, but instead sets innerHTML
• Snabbdom calls on the properties module to update the node first, causing it to set innerHTML
• Snabbdom attempts to remove the old TextNode from the DOM
• The TextNode is no longer attached to the DOM; Snabbdom encounters an exception

It can happen when trying to remove elements as well:

var vdom = patch(el, snabbdom.h('p', {}, [snabbdom.h('span', {}, ['LUL'])]));
patch(vdom, snabbdom.h('p', {props: {innerHTML: "Eh?"}}, []));

This time it fails in a different place.

---

I could provide a PR, but I'm not sure what the preferred fix is. It seems to me that the way Snabbdom allows its modules to "do anything" to a node is somewhat at odds with its assumptions that all DOM operations are safe. My suggestion for a fix is to ensure that the node being removed in DOMAPI.removeChild is still attached to the DOM (in the position expected by Snabbdom), before attempting to remove it:

function removeChild(node: Node, child: Node): void {
  if (child.parentNode === node) {
    node.removeChild(child);
  }
}

I'm not intimately familiar with the implementation or guiding principles in it, so this may not be your preferred solution.

[cjohansen]

Any chance of getting some feedback on this? I'd be happy to do a PR, but need some input as requested above.

[jvanbruegge]

Honestly, I am not sure what the best thing to do here is. There are basically two options:

• Usage of innerHTML and snabbdom components is documented as wrong. This would mean if you plan to use innerHTML you have to not create a text node and use innerHTML for that text too.
• The check you describe. I see two potential issues here: The first is performance and the second is that other modules might expect nodes to be part of the DOM so this could be a breaking change.

Other opinions @mreinstein @paldepind ?

[cjohansen]

This would mean if you plan to use innerHTML you have to not create a text node and use innerHTML for that text too.

This is just one of many ways the innerHTML module can break Snabbdom. Basically, when you set innerHTML, Snabbdom's assumptions about the world go out the window.

I don't think it's possible to implement innerHTML as a module the way Snabbdom currently does. It probably must be a core feature to work correctly, as it can make all kinds of (structural) changes to the tree structure that Snabbdom expects to be in full control of.

The first is performance

How much of a performance hit will this check bring with it? It will only be performed before removing a node.

[mreinstein]

Honestly, I am not sure what the best thing to do here is.

Same. I looked at this issue a few weeks ago to see if I could chime in with helpful feedback, but I wasn't really sure what to say that would be helpful.

I think the reason why is I'm unclear on "real" use cases where this would happen. It feels more like exploratory notes from someone doing work on vdom and analyzing the snabbdom codebase as a learning exercise.

I don't mean this to sound dismissive, there are some interesting bits in the description. There may be a genuine use case that is failing and I'm just not understanding it.

[cjohansen]

I think the reason why is I'm unclear on "real" use cases where this would happen. It feels more like exploratory notes from someone doing work on vdom and analyzing the snabbdom codebase as a learning exercise.

It's quite easy to trigger. The only reason I discovered this was because we ported our codebase from React (through https://github.com/levand/quiescent/) to Snabbdom (through https://github.com/cjohansen/dumdom/) and this happened in a few places. When it does happen, you're left with some pretty wild behavior.

I don't mean this to sound dismissive, there are some interesting bits in the description. There may be a genuine use case that is failing and I'm just not understanding it.

The guine use-case is this: I used the innerHTML module. I breaks the DOM in ways the core Snabbdom does not account for.

[mreinstein]

I get that it's easy to trigger, but I'm struggling to understand at the application level why someone would invoke this usage pattern. Admittedly, I have a very limited application of the innerHTML prop in my own code:

import sanitize from './html-sanitizer.js'


// render the modal window header
h('h2', {
        props: {
            innerHTML: sanitize(someRawHTML)
        }
    }
)

This is an escape hatch, a special condition where a header in one of my UI components has html formatting that gets passed in from the content management system that authored the original content. It's pretty much always some light html that doesn't change much, and doesn't change the structure of it's content much/at all.

I'm not saying that this is the only use case for innerHTML, but it's how I have been using it.

Managing the internal HTML content of a DOM element inside the vnode tree is going to be tricky in some cases, as you mentioned due to the way snabbdom allows it's plugins to do anything to a node.

Without understanding the patterns at the app level it's hard to say, but my gut feeling is that there are other workarounds that don't involve completely re-architecting snabbdom:

• if the use cases of this in your application are limited, perhaps you can avoid directly setting innerHTML
• you could declare your own v1 custom element(s), and that might obviate some of the need for innerHTML

If you share what patterns are popping up in your app where you're setting innerHTML maybe we can work through some of these alternatives.

[cjohansen]

I get that it's easy to trigger, but I'm struggling to understand at the application level why someone would invoke this usage pattern.

Because it's so easy to trigger, there are literally countless ways to do it. Basically, any pattern that puts an element with innerHTML in the same relative position as another element without innerHTML. I discovered it because we have some code that sometimes returns null and sometimes an element. This caused Snabbdom to reuse a DOM element that logically came from one function in my codebase to render a DOM element from a completely different function.

I through the best approach was to boil the problem down as much as possible, hence my initial example.

If you want another, slightly more involved example, I'll provide one. It illustrates how insidious the problem is:

• One page renders a div with an h1 and an a
• Another page renders the same structure - a div with a h1 and an a. Only this time, the h1 uses innerHTML to render some pre-formatted text (from a CMS or whatever)
• Snabbdom goes boom

var patch = snabbdom.init([
  snabbdom.propsModule,
  snabbdom.styleModule,
  snabbdom.eventListenersModule
]);

var el = document.createElement('div');
document.body.appendChild(el);

var currentPage = 'page1';
var currentRoot = el;

function render() {
  var pageFn = currentPage === 'page1' ? page1 : page2;
  currentRoot = patch(currentRoot, pageFn());
}

function navigate(page) {
  currentPage = page;
  render();
}

function page1() {
  return snabbdom.h('div', {}, [
    snabbdom.h('h1', {}, ["Title"]),
    snabbdom.h('a', {
      props: {
        href: "#/page2"
      },
      on: {
        click: e => {
          e.preventDefault();
          navigate('page2');
        }
      }
    }, ["Page 2"])
  ]);
}

function page2() {
  return snabbdom.h('div', {}, [
    snabbdom.h('h1', {props: {innerHTML: '<strong>Fancy title</strong>'}}, []),
    snabbdom.h('a', {
      props: {
        href: "#/page1"
      },
      on: {
        click: e => {
          e.preventDefault();
          navigate('page1');
        }
      }
    }, ["Page 1"])
  ]);
}

render();

In any case I think the whole discussion about whether the use cases are "real enough" is a bit of a detour. Snabbdom makes assumptions in its core that are broken by one of its core modules (the props module with the innerHTML prop). This is an exception-producing problem that's hard for end-users to guard against (or even track down, trust me 😅), even if it hasn't bitten you often. I'd much rather be discussing what's a good fix.

The way I see it, either Snabbdom does not assume that the DOM has been untouched, and guards some of its exception-prone DOM API usage, or innerHTML is handled specifically. For instance, by removing innerHTML from the props module and specifically handling innerHTML at the end of the patch operation, I think this problem could be avoided. Another approach could be to not reuse DOM elements that have had, or is in the process of having its innerHTML tampered with.

The last suggestion is what we're effectively doing while we're awaiting a resolution here - any element that sets innerHTML gets a hash of the innerHTML as its key if it doesn't already specify a key. This thwarts the reuse of the element and avoids the problem.

---

As a side-note - after the port we're also seeing some stray exceptions in production from insertBefore being used incorrectly. I haven't had time to hunt this one down yet, but Snabbdom´s insertBefore is written similarly to removeChild, and I suspect it could be a related problem of assumptions not holding up. I'll get back when I know more.

[mreinstein]

the whole discussion about whether the use cases are "real enough" is a bit of a detour

For people that have to maintain snabbdom, we need understanding on the application level patterns that appear in people's use cases. I am genuinely sorry if this feels like a waste of your time. I'm coming from the standpoint of needing to not break the world for everyone relying on snabbdom, today.

Again, I am not discounting that there may be some kernels of goodness in this issue. Maybe these things could provide future refactoring ideas for a next-gen snabbdom. I really don't know.

If you want another, slightly more involved example, I'll provide one

Thank you for an application example. If the goal is to get your application code working on snabbdom today, you could refactor it, something like this:

const patch = snabbdom.init([
  snabbdom.propsModule,
  snabbdom.styleModule,
  snabbdom.eventListenersModule
]);

let currentRoot = document.createElement('div');
document.body.appendChild(currentRoot);

let currentPage = 'page1';

function render () {
  currentRoot = patch(currentRoot, pageFn());
}

function navigate (page) {
  currentPage = page;
  render();
}

function pageFn () {
  const content = (currentPage === 'page1') ? 'Title' : '<strong>Fancy title</strong>';
  const href = (currentPage === 'page1') ? 'page2' : 'page1';
  const pageTitle = (href === 'page1') ? 'page 1' : 'page 2';

  return snabbdom.h('div', {}, [
    snabbdom.h('h1', { props: { innerHTML: content } }, [ ]),
    snabbdom.h('a', {
      props: {
        href: `#/${href}`
      },
      on: {
        click: e => {
          e.preventDefault();
          navigate(href);
        }
      }
    }, [ pageTitle ])
  ]);
}

render();

[magnars]

In a single page application with lots of pages, lots of different states, and lots of developers, it's not possible to use the workaround suggested here ("just use innerHTML in both places").

Since snabbdom will eagerly re-use dom elements, using innerHTML any place has to be considered a bug. Unless this is fixed.

[cjohansen]

I'm coming from the standpoint of needing to not break the world for everyone relying on snabbdom, today.

I have the utmost respect for this mindset! Breaking changes helps noone. But none of my suggested approaches need to be breaking changes. In any case, Snabbdom is very likely to break the world for anyone who uses innerHTML, today. Seems worth fixing to me.

I'm not sure why you rewrote my example... I wrote it specifically to trigger the bug, to illustrate in the kinds of situations where the bug will appear. Like @magnars pointed out - working around the problem only works in very simple cases. Add a smidge more levels of variability and you're all out of luck with this naive approach. I know, because in the code-base where we discovered this it was quite a bit of work to analyze our way to the conclusion, and a similar work-around would not be feasible, if even possible.

[mreinstein]

Snabbdom is very likely to break the world for anyone who uses innerHTML, today. Seems worth fixing to me.

No one is arguing against fixing bugs.

I'm not sure why you rewrote my example

I did not re-write anything. I asked for a real world instance of the problem from an app's codebase to look at, and then did my best to give you a practical way to workaround the problem with today's snabbdom.

Is there a concrete, actionable proposal on what you would fix?

[cjohansen]

I did not re-write anything. I asked for a real world instance of the problem from an app's codebase to look at, and then did my best to give you a practical way to workaround the problem with today's snabbdom.

I see, I guess we're talking past each other then. I wrote the example to illustrate how one might trigger this bug, since I got the impression that you didn't see the practical use-case where it might pop up from the initial minimal example to reproduce it.

---

Is there a concrete, actionable proposal on what you would fix?

I have two suggestions. First, this function makes unsafe assumptions:

function removeChild(node: Node, child: Node): void {
  node.removeChild(child);
}

So one fix is to guard it:

function removeChild(node: Node, child: Node): void {
  if (child.parentNode) {
    child.parentNode.removeChild(child);
  }
}

If you go this route, then I suspect insertBefore needs the same type of guarding. There was some worry about performance with this fix, which I honestly don't think is much of a problem, but it could be measured. The more worrying aspect of this is that the same assumption may exist in other modules, so this really only addresses the symptoms, not the underlying problem.

---

An alternative, more pervasive change is to not handle innerHTML in the props module, but in Snabbdom's core. This seems like the right approach to me, as innerHTML may cause structural changes that Snabbdom probably wants to stay on top of. I'm not deeply familiar with the implementation however, so this may be an uninformed viewpoint.

The 100% backwards-compatible way to do this would be to add the feature in core, and leave the broken implementation in the props module, e.g.:

// Get current implementation
snabbdom.h('h1', { props: { innerHTML: content } }, [ ])

// Get new core implementation
snabbdom.h('h1', { innerHTML: content }, [ ])

A small concession could be made such that core "steals" the innerHTML property from the props object. This would fix the problem for everyone, but may not be to your taste.

---

A third approach could be to not reuse DOM elements "across the innerHTML barrier":

• If a DOM element has had its innerHTML tampered with, never reuse it for a vdom node without the innerHTML prop (or maybe "vdom node with non-text children")
• If a DOM element has not had its innerHTML tampered with, never reuse it for a vdom node with an innerHTML prop.

I worry that this approach might trigger mount and unmount hooks for elements when they're not really being mounted, but just updated.

[magnars]

I'm not convinced that adding the guards will be much of a fix. Sure, the exceptions might go away, but Snabbdom will still unsafely reuse DOM elements.

Maybe marking DOM elements as "do not reuse" after they are tampered with outside the assumptions of Snabbdom core is a solution. This could then be used by other DOM-manipulating modules as well.

[cjohansen]

One last point re: guarding against reuse of elements. If done too conservatively, components like this:

function niceTitle({titleMarkup, title}) {
  return snabbdom.h('h1', {
    props: titleMarkup ? {innerHTML: titleMarkup} : {}
  }, titleMarkup ? [] : [title]);
}

Will get and unmount and a mount when it really should have gotten an update.

[jvanbruegge]

Yes the guards won't do much good. IMO, innerHTML is an escape hatch and that if you decide to use it for some reason you need to make sure it does not interfere with snabbdom yourself.

Supporting it by default is not something we should try to do. I think we should document this more prominently and point out the alternatives if you have to use it:

• Change the structure of your vdom, so the innerHTML does not interfere with diffing
• Use a html-to-snabbdom parser (I found snabbdom-virtualize but that seems outdated)

[cjohansen]

IMO, innerHTML is an escape hatch and that if you decide to use it for some reason you need to make sure it does not interfere with snabbdom yourself.

We obviously disagree on both the utility of innerHTML and the gravity of this problem. I don't think "the user must make sure to not interfere with the library while using the library's features" is a particularly productive or user-friendly stance.

If you don't intend to fix this problem, innerHTML should be prominently documented as broken and very likely to cause problems. Maybe suggest that users set the vdom element's key to the same string as innerHTML, as that will prevent Snabbdom from reusing the element in unsafe ways. This is the best you can do to guard against the problem.

Use a html-to-snabbdom parser (I found snabbdom-virtualize but that seems outdated)

Something like that could power the implementation of a Snabbdom innerHTML module, perhaps? With a dedicated module that did something like that, there would be a proper solution for a useful feature, and you don't need to put it in Snabbdom core.

[mreinstein]

We obviously disagree on both the utility of innerHTML and the gravity of this problem.

Well I just lost the first 2 hours of what should have been my sleep, tossing and turning over this particular issue and how to resolve it amicably, so let's cool it with the "you don't care" claims. :)

If you don't intend to fix this problem,

I'm not opposed to fixing problems, but I'm still having trouble understanding why a large app was built this way. It seems to me like an anti-pattern. At least in snabbdom, one shouldn't be injecting large chunks of raw HTML into a virtual DOM tree because it wasn't designed to work that way and will probably muck things up.

Part of why I lost sleep is one side is saying a) "this is definitely broken", and those people are trying to migrate a large app from one view engine to a totally different one. And the other group of people are saying b) "this isn't how snabbdom is supposed to work, you're leaning too heavily on a bad app design pattern (in snabbdom land at least." and we might just be stuck in our ways because it's always worked this way.

I don't know that one side is right or wrong. I can see it from both sides a little. Obviously I am more in camp b)

so I don't really know how to resolve this. I think if you're willing to put together a PR, at least then we could have arguments over a potential fix, instead of long winded theory back and forth. :) Assuming the PR is not crazy labor intensive to put together I think that's constructive. The only risk is there's no guarantee we will say "yes this is definitely a PR that will get merged"

I also think this is a good opportunity for us to learn from a team that is trying to migrate a large app away from react to snabbdom. I don't know if anyone has tried to do this for a sizable app before. Surely there are things to learn. Curious, what drove you folks to move away react to snabbdom in the first place?

[magnars]

Curious, what drove you folks to move away react to snabbdom in the first place?

To get off the churn train. Which is why your point about avoiding breaking changes is so sweet to my ears. 😊

As for innerHTML, I would say it is certainly a bad practice. If it were not for CMS-es with rich text editors, it would be pretty straightforward to migrate away from it.

[cjohansen]

At least in snabbdom, one shouldn't be injecting large chunks of raw HTML into a virtual DOM tree because it wasn't designed to work that way and will probably muck things up.

We're not. We're inserting some text with inline markup here and there.

I assumed that since Snabbdom had a way to set innerHTML, it was a feature along the lines of React's "dangerouslySetInnerHTML". But that assumption could be wrong, so I'll ask a question that I should have asked in the beginning: Snabbdom currently allows setting innerHTML via the props module. Is this an intended feature, or an unintended side-effect of mapping whatever is in props to the underlying DOM element?

[holyjak]

but I'm still having trouble understanding why a large app was built this way. It seems to me like an anti-pattern. At least in snabbdom, one shouldn't be injecting large chunks of raw HTML into a virtual DOM tree

I am in the boat with @cjohansen here. Our React app also used dangerouslySetInnerHTML to include rich text from a CMS such as product descriptions (with links, bold and other formatting, paragraphs, sometimes images). That seems to me as a common need so it would be nice if there was a simple, safe way to do that in snabbddom. I guess using a html-to-snabbdom parser would be a solution, if there was such a tool, and good and reliable one (perhaps there is?). Just my 2 cents.

[aranchelk]

I've hit this issue as well.

I had previously implemented a dynamically expanding textarea, by having an outer div with overflow hidden, a textarea inside that div, and one additional hidden div as a sibling of the textarea with identical styling. I use an Elm-like application framework and it was very easy to have the same text from the textarea in the sibling div, positioned/styled such that it would push open the visible area of the outer div exposing more of the text area as text was added.

As the application grew both the view function and Snabbdom's diffing and patching started taking longer, and the user experience of typing text was getting laggy, so I modified the application framework. I implemented a batching mode that accumulated events, and batched user input so several events get applied to to the application state all at once, then the view function and patch run.

The delay between the users' text input and updates to the sibling div broke the layout, and I resorted to using an event outside of Snabbdom to immediately update the sibling div. The bugs were subtle, originally there was just occasional missing or incorrect content, but Snabbdom didn't throw any exceptions. Finally a change in layout caused Snabbdom to try to detach a nonexistent element and I got an exception. I then realized that my issue was this issue.

Rather than implementing some check on the existence of innerHTML, it might be nice if users could mark nodes as dirty (i.e. having been manipulated outside of Snabbdom and not reusable). Seems like that would be fast. It could be a configurable check that was off by default. This would cover more use cases, as I said: originally I didn't have any exceptions, just incorrect content.

[iambumblehead]

I've been using my down-time to make little Snabbdom PRs and resolve various issues. Because there are many opened issues and PRs, I did not read this great discussion until tonight. @cjohansen it is enjoyable to read your clear input and analysis in all issues and particularly this one.

It seems dumdom continues to be developed and to use Snabbdom despite annoying issues you found and carefully reported which were never resolved,

• https://github.com/snabbdom/snabbdom/issues/973
• https://github.com/snabbdom/snabbdom/issues/1011
• https://github.com/snabbdom/snabbdom/issues/970
• https://github.com/snabbdom/snabbdom/issues/972
• https://github.com/snabbdom/snabbdom/issues/971

Would you tell, from a high-level, what is your view of Snabbdom at this time? Has Snabbdom been a good vdom package for you, despite above listed issues? Have you encountered slowness issues like the ones described by @aranchelk? How do you think Snabbdom compares with Inferno and react-dom at this time? Are innerHTML issues the most serious short-coming with Snabbdom? (nice suggestion to support legacy and new core innerHTML implementations)

Also, sharing these recent PRs which might resolve some issues you reported,

• https://github.com/snabbdom/snabbdom/pull/1083
  • might resolve https://github.com/snabbdom/snabbdom/issues/970
• https://github.com/snabbdom/snabbdom/pull/1062
  • might resolve https://github.com/snabbdom/snabbdom/issues/971 and https://github.com/snabbdom/snabbdom/issues/972
• https://github.com/snabbdom/snabbdom/compare/master...iambumblehead:snabbdom:apply-remove-style-deeply
  • explores https://github.com/snabbdom/snabbdom/issues/973 and https://github.com/snabbdom/snabbdom/issues/1011

[aranchelk]

Despite my comment, I have a very positive view of Snabbdom, it's an integral part of an application I work on. In our case we use PureScript with a wrapper library [https://github.com/t-dash-do/snabbare].

Having the ability to mark nodes as altered outside of Snabbdom and not reusable would still be useful.

Our current workaround for that are too big to update promptly is to make all elements that would have been expanding textareas read-only with clickable links to a separate edit page -- not ideal but it works.

[iambumblehead]

thanks for the ping-back @aranchelk I haven't used Snabbdom for anything "heavy" myself yet and your previous comment about slowness did give pause. Its great to know things are basically good for you

[cjohansen]

@iambumblehead Thanks for your kind words 😊

I've been a happy Snabbdom user (through dumdom) for years. It was a bit of a bumpy ride to get on, as you've seen documented here, but we managed to work around them and things have been stable since.

This specific problem was solved by setting a key on any node that uses innerHTML. This way, innerHTML-nodes are never reused. Worked like a charm for us.

[iambumblehead]

that's great! thanks for sharing that :)