Race Condition with DOM Node Arrays

[danmckeon]

Hi,

I'm using hyperHTMLElement and ran into an issue that arises when I'm rendering multiple arrays of nodes and move a node from one array into another. I have seen these two DOMExceptions

Failed to execute 'removeChild' on 'Node'

Failed to execute 'insertBefore' on 'Node'

I did some debugging with domdiff, and it appears the problem arises in the order of operations (e.g. the insert operation is executed before the remove operation), reflected in the following partially abstracted example:

domdiff({
  parent: parent1,
  presentNodes: [nodeA, nodeB],
  futureNodes: [nodeA, nodeB, nodeC]
})

domdiff({
  parent: parent2,
  presentNodes: [nodeC, nodeD],
  futureNodes: [nodeD]
})

note: nodeC is the node being moved from one array to another

First, domdiff evaluates parent1 and inserts nodeC with something like parent1.appendChild(nodeC)

In so doing, there is a side effect: nodeC was removed from parent2.

Then, domdiff evaluates parent2. It attempts to execute parent2.removeChild(nodeC) but nodeC is no longer actually a child of that node because of the side effect described directly above.

Here is a jsfiddle showing the issue: https://jsfiddle.net/danielmckeon/7rq24yuj/30/

I have found a work-around in deep cloning the node prior to moving it to the other array, but it seems that domdiff should be able to handle a situation where a specific node is use in more than one diffing operation. Maybe there is an alternate way I can implement this? Any help would be much appreciated.

Thanks!

[WebReflection]

interesting finding, but I have a legit follow up question here: are you intentionally moving the same node from A to B ?

[WebReflection]

to expand a bit my question, checking node.parentNode before every remove would likely penalize performance a lot for all other 2+ years of operations where this issue is never intended

[WebReflection]

P.S. true to be told, I could use node.remove() instead which would do that for me, even if the node is not live, and fallback to the check if there's no remove method within nodes.

[WebReflection]

After further investigation, there is an issue here: nodes cannot be arbitrarily moved elsewhere, because while it's easy to solve a single node removal case, it's basically impossible to move a group of nodes, in case the first node of a range of nodes, or the last one, is moved elsewhere.

This also affects the exposed get logic, as it would be overly complicated to provide the right node to move, if these can be found elsewhere.

The main goal of this library is also to move the least amount of nodes, so that having the exact same node flying around different places, could lead to way many issues, that I'm not sure it should be encouraged at all.

Specially with many nodes, dropping one part would mean looping over all nodes and see if these can be removed from the initially meant place ...

TL;DR since this module never had such issue and it's used in production as it is, I am not sure I want to penalize all performance optimizations obtained by the common use cases, that would be erased by this fix if landed, as each node of a list might need to be checked a part, so that code like this wouldn't ever work again:

export const remove = (get, parent, children, start, end) => {
  if ((end - start) < 2)
    removeChild(get(children[start], -1), parent);
  else {
    // this won't be possible anymore at all, due same issue
    const range = parent.ownerDocument.createRange();
    range.setStartBefore(get(children[start], -1));
    range.setEndAfter(get(children[end - 1], -1));
    range.deleteContents();
  }
};

Accordingly, I'd like to properly understand the use case, before destroying node removal performance for something that maybe wasn't even meant.

Thanks for your patience and help in letting me figure out how to proceed.

[danmckeon]

From your description, I think you understand the use case - or set of use cases - and your point is valid: if there is a significant performance penalty in optimizing for this use case and it is not a very common use case, it would not be worthwhile. But perhaps discouraging this use case should be documented somewhere so a future user can hopefully avoid having to debug domdiff

[danmckeon]

And just to conceptualize my use case, I am working on developing a top navigation component and at narrow screen sizes, items move from the expanded navigation to a "More" menu. The navigation items themselves are child elements. So I am managing 2 arrays between expandedNavItems and collapsedNavItems and on resize, elements move from one array to the other.

[WebReflection]

I think it's important to remove possible footguns from the core, and while performance might be penalized in some case, specially because instead of removing N nodes at once, I now need to remove one node per time with all its DOM mutations implications while doing so (damn it browsers), I prefer dropping any sort of issue while working with different holes that might contain same node.

As summary, once CI passes, I'll land the change and update the libraries.

[WebReflection]

So, this landed everywhere (hyperHTML, lighterhtml, heresy)

Please do let me know if you have any issue, thanks.

[danmckeon]

many thanks for your quick response and hard work!