Mithril.js: unmount fails due to null node when dom.nextSibling is null

[scktt]

sometimes when unmounting a view, removeNodeFromDOM fails when dom.nextSibling is null as it does not check that node is not null, causing the mount of the next view to also fail.

my changes to resolve the issue

--- a/root/js/mithril-1.1.1.js
+++ b/root/js/mithril-1.1.1.js
@@ -799,8 +799,10 @@ var coreRenderer = function($window) {
                }
        }
        function removeNodeFromDOM(node) {
-               var parent = node.parentNode
-               if (parent != null) parent.removeChild(node)
+               if(node != null) {
+                       var parent = node.parentNode;
+                       if (parent != null) parent.removeChild(node)
+               }
        }

[ArthurClemens]

I've come across that problem too.

[isiahmeadows]

Could you test it against Mithril's internal suite and file a PR?

[pygy]

I think the actual bug upstream, node should never be null in removeNodeFromDOM, by adding the proposed check we would be masking the actual bug...

Is vnode.domSize wrongly measured? Is it the bug in continuation? I'd love to have a repro for this.

[iainmcampbell]

Not sure if this is quite the same thing, but whatever it is I’ve managed to reproduce it: https://jsfiddle.net/qhprw0ku/7/. The exact error I’m getting is Uncaught DOMException: Failed to execute 'removeChild' on 'Node': The node to be removed is no longer a child of this node. Perhaps it was moved in a 'blur' event handler? I only get an error in Chrome 59, not Firefox 54 or Safari 10.

Call stack is removeNodeFromDOM → continuation → removeNode and so on.

To reproduce: create an <input>, add onfocus and onblur handlers, that add/remove a keydown event, that triggers a route change.

[isiahmeadows]

That is super odd, because the router is a very thin layer around mount, which simply throttles its calls to render.

[pygy]

@iainmcampbell I think it's another bug... Somehow redraw() ends up being called twice in the same tick, not sure why. Adding an async layer solves the issue.

[pygy]

Also props to @porsager for making it so quick and easy to test hypotheses about bugs. Flems.io rules!

[isiahmeadows]

@pygy That probably goes along with our redraw work, then. @iainmcampbell Can you file a bug with that repro?

[isiahmeadows]

(I'm working on a simplified version that doesn't require m.route.)

[pygy]

Actually, it may be due to the fact that m.route calls run directly, and then the onblur event fires and triggers a sync redraw, since redraw is unaware of the run() call...

[pygy]

That is fixed in #1592 since run() is only called at mount time. After the first call m.redraw() takes over.

https://github.com/MithrilJS/mithril.js/pull/1592/files#diff-63de04924f7fd5b53de072769e289dfbR17

[isiahmeadows]

@scktt Could you provide your own repro to your issue, or is this basically another variant of @iainmcampbell's repro?

[iainmcampbell]

@pygy @isiahmeadows should I still open a new bug if this has already been addressed in #1592?

[isiahmeadows]

@iainmcampbell Probably so, since we didn't realize until now it was creating hard-to-debug exceptions for people in cases that it obviously shouldn't be. Most of the discussion up to this point has been just questions of API ergonomics and a few specific edge cases, not indirectly causing DOM exceptions through a seemingly separate public API that visibly should be handling that case.

[scktt]

@isiahmeadows will do, i can preproduce fairly quickly.

[pygy]

@scktt ping?

[scktt]

sorry @pygy for the delay. i have pulled the erroneous part of our app: https://jsfiddle.net/scktt/1wonq37e/5/

@isiahmeadows looking at the error it might be along the same lines but appears different as it doesnt appear to know the node had a child

[isiahmeadows]

@scktt Any chance you could cut it down some and remove the Bootstrap dependency?

• Bootstrap's JS could be installing crap that interferes with Mithril.
• That's a lot of code to sift through - two mount points, one using the router, and the Bootstrap boilerplate doesn't make it any easier.

Also, BTW, @iainmcampbell's issue was that of unexpected, but technically correct behavior resulting in a recursive redraw call, which looks on its surface to be independent of yours (since you forgot to notify Bootstrap to install itself to its tabs).

[scktt]

@isiahmeadows

looks like the array inside the tabs view method is causing the problem. changing to

'''
view: function (vnode) {
return "tabs"
}
'''

seems okay

https://jsfiddle.net/1wonq37e/6/

[pygy]

@scktt Great, thanks, I'll look at your sample later today

[pygy]

@scktt The problem stems from the fact that you're calling m.mount() from oninit. In its current implementation,mount calls redraw() whereas m.route.set doesn't and only renders the routed tree... The implicit redraw is unaware of the route.set-triggered render(), and that causes a nested render() call on the same root, a recipe for crashes and odd behaviours. That behaviour will be fixed in Mithril v2 (through #1592).

In the mean time, I'd advise you to do something like this

There's an extra redraw scheduled on route change to update the emount mount point.

Alternatively, now that I think of it you could also asap(()=>m.mount(emount, linksN) in oninit, it will be less work on your end to refactor your app.

[scktt]

@pygy thanks for that, will do cheers