(Edge case) Elements are not being removed when promise resolves in onbeforeremove

[porsager]

I can't really explain the series of events causing this, but here is a reproducible case which will leave elements in the dom if the button is doubleclicked while the red box is visible.

(Double click the toggle button)
Repro setup

Maybe someone else with a clear set of eyes can spot / describe it better?

The real use case was a loading icon being animated inside a parent component and at certain timings would result in the parent component not being removed.

[pygy]

Thanks for the report @porsager

Here's an instrumented repro

The element that remains is an extra child that is added before the previous one has been removed... Not sure there's a lot we can do about it. Thinking needed.

Edit: bad link

[RodericDay]

Here's the bug reproduced in such a way that requires no clicks

Bug

[dead-claudia]

Here's what I suspect: it's a race condition between the promise resolution and the subsequent render adding the item, confusing the diff algorithm with inconsistent state. Here's a more stripped-down, synchronous repro using m.render with next.

Here's what I am observing:

1. The initial remove doesn't unlink the fragment from the real DOM nor the virtual tree, thanks to its onbeforeremove.
2. The new fragment is being immediately added to the DOM, appended past the old fragment.
3. The parent fragment, when removed removes the new fragment, but not the old one.
4. The parent fragment is restored after the old fragment.
5. When the continuation is resolved, the old fragment is never removed.

[dead-claudia]

Short-listing this for a v2 fix. The repro still works

[dead-claudia]

Found the issue, and it's two-part: Mithril's throwing an exception because of a race condition, and Mithril incorrectly relies on its domSize.

It boils down to this:

1. First, the first vnode's onbeforeremove is called as expected.
2. Then, a new vnode is added, "replacing" the existing one. Mithril loses track of the first vnode here.
3. Then, the vnodes' parent is removed. This results in clearing the first vnode.domSize vnodes from the parent starting from vnode.dom, but because it's unaware the first vnode is still there, it clears it, but not the child vnode.
4. Then, when the first vnode's onbeforeremove resolves, the callback assumes the element has a parent (it doesn't), and you'll end up with an Uncaught TypeError: Cannot read property 'removeChild' of null.

What this means is that we need to remove vnode.domSize + elementsAwaitingOnbeforeremove elements rather than just vnode.domSize elements whenever we're removing a fragment, and we need to not assume the parent still exists when we remove it. This means we'll need a parent reference in every awaited onbeforeremove resolver and a new vnode field to track this delta.

[dead-claudia]

I'm kind of leaning towards doing this to fix it:

1. When removing a vnode:

   1. I'll compute the list of vnodes to remove.

      • This will recurse through components and subtrees, but it will not recurse through element boundaries.
      • This list will include every vnode with a vnode.dom property, which conveniently includes pretty much every element type.
      • This list will include the contents of all such lists in child vnodes.
      • This will be stored on the parent vnode as a single list for all children with such hooks.
      • I'll have to benchmark this heavily.

   2. I'll call onbeforeremove on the parent vnode and await its result.

      • When I await this, I'll store the parent vnode in the array along with its children.
      • When the parent vnode itself is being removed as a child, it's just relocated.
      • I'll fix it to correctly await and unwrap its result if it has a .then method, since that needs fixed, too.

   3. I'll go through the array and invoke each found onremove hook on it. I'll remove the component or fragment vnodes in this pass as I encounter them.

   4. I'll go through the array and remove each child in it.

      • These might not necessarily be contiguous.

2. v2-specific break: I'll move the vnode.domSize count for trusted vnodes to vnode.children and kill the field entirely, now that we don't need it. Edit: I'll just leave it. It'll make certain API consumers a bit more practical. I'll still do the stuff I'll need to do to remove it, but I'll just leave the property in case it's useful to others.

   • I'll need to fix all uses of toFragment to recursively add the vnodes, but it would work similarly, so it's pretty straightforward. It'd also fix a similar bug.
   • While I'm going through, I'll also drop this function, since it's equivalent to parent.insertBefore(dom, nextSibling).
   • In general, most uses of vnode.domSize would make better sense just using a wrapper element's raw children. The property mainly exists for our own bookkeeping, so it doesn't make nearly as much sense to keep as, say, vnode.dom.

All this would be limited to the removal logic, but it'd mean I'd have to pretty much rewrite the whole thing.

This should help avoid a few potential issues with a naïve solution:

1. Since I'm going based on what the framework is aware of, what Mithril's current state forgets doesn't affect what its previous state does later.

2. I'm creating an intermediate array to avoid having to do a tree traversal three times in a row. It's a little extra memory, but it's generally not retained and it saves a lot of computation. The array is also usually pretty small, so it's not really a major allocation concern.

3. Since I'm going based on what the framework is aware of at the time I start when I remove a fragment, I don't need to account for potential new elements. This removes the issue of race conditions, since I know what elements I'm removing.

[dead-claudia]

Does this continue to repro with v2?

[porsager]

Unfortunately yes.

[porsager]

Wow.. Good job @isiahmeadows !

[dead-claudia]

@porsager It was a bit involved trying to figure out why the fuzzer suddenly started failing. Tried to do it in a way that didn't create a bunch of perf issues, too.

Sadly, the bundle is now about 9.7kB. Stupid bug fixes... 😉😄

[dead-claudia]

Reopening because my fix for this caused other breakage and I need to figure out how to fix this without breaking m.trust. Here's my plan:

1. Revert my fix for this and add tests for those two bugs.
2. Release a new hotfix with this broken again and those fixed. (This can at least be reasonably worked around.)
3. Attempt to fix this without breaking m.trust.
4. Release a new hotfix with this fixed and hopefully those not broken.

[dead-claudia]

Re-closing. Issue appears unrelated.