Fix #1881 + related ospec bug (#2492)

* Fix #1881 + related ospec bug

* Test duplicate resolves, update changelog

docs/change-log.md

@@ -16,6 +16,8 @@
 
 ### Upcoming...
 
+- Ensure vnodes are removed correctly in the face of `onbeforeremove` resolving after new nodes are added ([#2492](https://github.com/MithrilJS/mithril.js/pull/2492) [@isiahmeadows](https://github.com/isiahmeadows))
+
 -->
 
 ### v2.0.1

ospec/ospec.js

@@ -289,12 +289,11 @@ else window.o = m()
 		Assert.prototype[name] = function assert(value) {
 			var self = this
 			var message = serialize(self.value) + "\n  " + verb + "\n" + serialize(value)
-			if (compare(self.value, value)){
+			if (compare(self.value, value)) succeed(self, message)
-				succeed(self, message)
+			else fail(self, message)
-				return function(message) {
+			return function(message) {
-					if (!self.pass) self.message = message + "\n\n" + self.message
+				if (!self.pass) self.message = message + "\n\n" + self.message
-				}
+			}
-			}else fail(self, message)
 		}
 	}
 	function succeed(assertion, message) {

render/render.js

@@ -86,9 +86,12 @@ module.exports = function($window) {
 		}
 		vnode.dom = temp.firstChild
 		vnode.domSize = temp.childNodes.length
+		// Capture nodes to remove, so we don't confuse them.
+		vnode.instance = []
 		var fragment = $doc.createDocumentFragment()
 		var child
 		while (child = temp.firstChild) {
+			vnode.instance.push(child)
 			fragment.appendChild(child)
 		}
 		insertNode(parent, fragment, nextSibling)
@@ -272,7 +275,7 @@ module.exports = function($window) {
 	function updateNodes(parent, old, vnodes, hooks, nextSibling, ns) {
 		if (old === vnodes || old == null && vnodes == null) return
 		else if (old == null || old.length === 0) createNodes(parent, vnodes, 0, vnodes.length, hooks, nextSibling, ns)
-		else if (vnodes == null || vnodes.length === 0) removeNodes(old, 0, old.length)
+		else if (vnodes == null || vnodes.length === 0) removeNodes(parent, old, 0, old.length)
 		else {
 			var isOldKeyed = old[0] != null && old[0].key != null
 			var isKeyed = vnodes[0] != null && vnodes[0].key != null
@@ -281,7 +284,7 @@ module.exports = function($window) {
 			if (!isKeyed) while (start < vnodes.length && vnodes[start] == null) start++
 			if (isKeyed === null && isOldKeyed == null) return // both lists are full of nulls
 			if (isOldKeyed !== isKeyed) {
-				removeNodes(old, oldStart, old.length)
+				removeNodes(parent, old, oldStart, old.length)
 				createNodes(parent, vnodes, start, vnodes.length, hooks, nextSibling, ns)
 			} else if (!isKeyed) {
 				// Don't index past the end of either list (causes deopts).
@@ -295,10 +298,10 @@ module.exports = function($window) {
 					v = vnodes[start]
 					if (o === v || o == null && v == null) continue
 					else if (o == null) createNode(parent, v, hooks, ns, getNextSibling(old, start + 1, nextSibling))
-					else if (v == null) removeNode(o)
+					else if (v == null) removeNode(parent, o)
 					else updateNode(parent, o, v, hooks, getNextSibling(old, start + 1, nextSibling), ns)
 				}
-				if (old.length > commonLength) removeNodes(old, start, old.length)
+				if (old.length > commonLength) removeNodes(parent, old, start, old.length)
 				if (vnodes.length > commonLength) createNodes(parent, vnodes, start, vnodes.length, hooks, nextSibling, ns)
 			} else {
 				// keyed diff
@@ -326,9 +329,9 @@ module.exports = function($window) {
 					if (start === end) break
 					if (o.key !== ve.key || oe.key !== v.key) break
 					topSibling = getNextSibling(old, oldStart, nextSibling)
-					insertNode(parent, toFragment(oe), topSibling)
+					moveNodes(parent, oe, topSibling)
 					if (oe !== v) updateNode(parent, oe, v, hooks, topSibling, ns)
-					if (++start <= --end) insertNode(parent, toFragment(o), nextSibling)
+					if (++start <= --end) moveNodes(parent, o, nextSibling)
 					if (o !== ve) updateNode(parent, o, ve, hooks, nextSibling, ns)
 					if (ve.dom != null) nextSibling = ve.dom
 					oldStart++; oldEnd--
@@ -346,7 +349,7 @@ module.exports = function($window) {
 					oe = old[oldEnd]
 					ve = vnodes[end]
 				}
-				if (start > end) removeNodes(old, oldStart, oldEnd + 1)
+				if (start > end) removeNodes(parent, old, oldStart, oldEnd + 1)
 				else if (oldStart > oldEnd) createNodes(parent, vnodes, start, end + 1, hooks, nextSibling, ns)
 				else {
 					// inspired by ivi https://github.com/ivijs/ivi/ by Boris Kaul
@@ -367,7 +370,7 @@ module.exports = function($window) {
 						}
 					}
 					nextSibling = originalNextSibling
-					if (matched !== oldEnd - oldStart + 1) removeNodes(old, oldStart, oldEnd + 1)
+					if (matched !== oldEnd - oldStart + 1) removeNodes(parent, old, oldStart, oldEnd + 1)
 					if (matched === 0) createNodes(parent, vnodes, start, end + 1, hooks, nextSibling, ns)
 					else {
 						if (pos === -1) {
@@ -380,7 +383,7 @@ module.exports = function($window) {
 								if (oldIndices[i-start] === -1) createNode(parent, v, hooks, ns, nextSibling)
 								else {
 									if (lisIndices[li] === i - start) li--
-									else insertNode(parent, toFragment(v), nextSibling)
+									else moveNodes(parent, v, nextSibling)
 								}
 								if (v.dom != null) nextSibling = vnodes[i].dom
 							}
@@ -416,7 +419,7 @@ module.exports = function($window) {
 			else updateComponent(parent, old, vnode, hooks, nextSibling, ns)
 		}
 		else {
-			removeNode(old)
+			removeNode(parent, old)
 			createNode(parent, vnode, hooks, ns, nextSibling)
 		}
 	}
@@ -428,7 +431,7 @@ module.exports = function($window) {
 	}
 	function updateHTML(parent, old, vnode, ns, nextSibling) {
 		if (old.children !== vnode.children) {
-			toFragment(old)
+			removeHTML(parent, old)
 			createHTML(parent, vnode, ns, nextSibling)
 		}
 		else vnode.dom = old.dom, vnode.domSize = old.domSize
@@ -483,7 +486,7 @@ module.exports = function($window) {
 			vnode.domSize = vnode.instance.domSize
 		}
 		else if (old.instance != null) {
-			removeNode(old.instance)
+			removeNode(parent, old.instance)
 			vnode.dom = undefined
 			vnode.domSize = 0
 		}
@@ -550,19 +553,6 @@ module.exports = function($window) {
 		return result
 	}
 
-	function toFragment(vnode) {
-		var count = vnode.domSize
-		if (count != null || vnode.dom == null) {
-			var fragment = $doc.createDocumentFragment()
-			if (count > 0) {
-				var dom = vnode.dom
-				while (--count) fragment.appendChild(dom.nextSibling)
-				fragment.insertBefore(dom, fragment.firstChild)
-			}
-			return fragment
-		}
-		else return vnode.dom
-	}
 	function getNextSibling(vnodes, i, nextSibling) {
 		for (; i < vnodes.length; i++) {
 			if (vnodes[i] != null && vnodes[i].dom != null) return vnodes[i].dom
@@ -570,6 +560,45 @@ module.exports = function($window) {
 		return nextSibling
 	}
 
+	// This covers a really specific edge case:
+	// - Parent node is keyed and contains child
+	// - Child is removed, returns unresolved promise in `onbeforeremove`
+	// - Parent node is moved in keyed diff
+	// - Remaining children still need moved appropriately
+	//
+	// Ideally, I'd track removed nodes as well, but that introduces a lot more
+	// complexity and I'm not exactly interested in doing that.
+	function moveNodes(parent, vnode, nextSibling) {
+		var frag = $doc.createDocumentFragment()
+		moveChildToFrag(parent, frag, vnode)
+		insertNode(parent, frag, nextSibling)
+	}
+	function moveChildToFrag(parent, frag, vnode) {
+		// Dodge the recursion overhead in a few of the most common cases.
+		while (vnode.dom != null && vnode.dom.parentNode === parent) {
+			if (typeof vnode.tag !== "string") {
+				vnode = vnode.instance
+				if (vnode != null) continue
+			} else if (vnode.tag === "<") {
+				for (var i = 0; i < vnode.instance.length; i++) {
+					frag.appendChild(vnode.instance[i])
+				}
+			} else if (vnode.tag !== "[") {
+				// Don't recurse for text nodes *or* elements, just fragments
+				frag.appendChild(vnode.dom)
+			} else if (vnode.children.length === 1) {
+				vnode = vnode.children[0]
+				if (vnode != null) continue
+			} else {
+				for (var i = 0; i < vnode.children.length; i++) {
+					var child = vnode.children[i]
+					if (child != null) moveChildToFrag(parent, frag, child)
+				}
+			}
+			break
+		}
+	}
+
 	function insertNode(parent, dom, nextSibling) {
 		if (nextSibling != null) parent.insertBefore(dom, nextSibling)
 		else parent.appendChild(dom)
@@ -589,41 +618,89 @@ module.exports = function($window) {
 	}
 
 	//remove
-	function removeNodes(vnodes, start, end) {
+	function removeNodes(parent, vnodes, start, end) {
 		for (var i = start; i < end; i++) {
 			var vnode = vnodes[i]
-			if (vnode != null) removeNode(vnode)
+			if (vnode != null) removeNode(parent, vnode)
 		}
 	}
-	function removeNode(vnode) {
+	function removeNode(parent, vnode) {
-		var expected = 1, called = 0
+		var mask = 0
 		var original = vnode.state
+		var stateResult, attrsResult
 		if (typeof vnode.tag !== "string" && typeof vnode.state.onbeforeremove === "function") {
 			var result = callHook.call(vnode.state.onbeforeremove, vnode)
 			if (result != null && typeof result.then === "function") {
-				expected++
+				mask = 1
-				result.then(continuation, continuation)
+				stateResult = result
 			}
 		}
 		if (vnode.attrs && typeof vnode.attrs.onbeforeremove === "function") {
 			var result = callHook.call(vnode.attrs.onbeforeremove, vnode)
 			if (result != null && typeof result.then === "function") {
-				expected++
+				// eslint-disable-next-line no-bitwise
-				result.then(continuation, continuation)
+				mask |= 2
+				attrsResult = result
 			}
 		}
-		continuation()
+		checkState(vnode, original)
-		function continuation() {
+
-			if (++called === expected) {
+		// If we can, try to fast-path it and avoid all the overhead of awaiting
-				checkState(vnode, original)
+		if (!mask) {
-				onremove(vnode)
+			onremove(vnode)
-				if (vnode.dom) {
+			removeChild(parent, vnode)
-					var parent = vnode.dom.parentNode
+		} else {
-					var count = vnode.domSize || 1
+			if (stateResult != null) {
-					while (--count) parent.removeChild(vnode.dom.nextSibling)
+				var next = function () {
+					// eslint-disable-next-line no-bitwise
+					if (mask & 1) { mask &= 2; if (!mask) reallyRemove() }
+				}
+				stateResult.then(next, next)
+			}
+			if (attrsResult != null) {
+				var next = function () {
+					// eslint-disable-next-line no-bitwise
+					if (mask & 2) { mask &= 1; if (!mask) reallyRemove() }
+				}
+				attrsResult.then(next, next)
+			}
+		}
+
+		function reallyRemove() {
+			checkState(vnode, original)
+			onremove(vnode)
+			removeChild(parent, vnode)
+		}
+	}
+	function removeHTML(parent, vnode) {
+		for (var i = 0; i < vnode.instance.length; i++) {
+			parent.removeChild(vnode.instance[i])
+		}
+	}
+	function removeChild(parent, vnode) {
+		// Dodge the recursion overhead in a few of the most common cases.
+		while (vnode.dom != null && vnode.dom.parentNode === parent) {
+			if (typeof vnode.tag !== "string") {
+				vnode = vnode.instance
+				if (vnode != null) continue
+			} else if (vnode.tag === "<") {
+				removeHTML(parent, vnode)
+			} else {
+				if (vnode.tag !== "[") {
 					parent.removeChild(vnode.dom)
+					if (!Array.isArray(vnode.children)) break
+				}
+				if (vnode.children.length === 1) {
+					vnode = vnode.children[0]
+					if (vnode != null) continue
+				} else {
+					for (var i = 0; i < vnode.children.length; i++) {
+						var child = vnode.children[i]
+						if (child != null) removeChild(parent, child)
+					}
 				}
 			}
+			break
 		}
 	}
 	function onremove(vnode) {

render/tests/test-onremove.js

@@ -214,6 +214,224 @@ o.spec("onremove", function() {
 				o(vnode.dom).notEquals(updated.dom) // this used to be a recycling pool test
 				o(onremove.callCount).equals(1)
 			})
+			// Warning: this test is complicated because it's replicating a race condition.
+			o("removes correct nodes when child delays removal, parent removes, then child resolves", function () {
+				// Sugar over the complexity - I need to test the entire tree for consistency.
+				function expect(expectedPairs) {
+					var expected = []
+
+					for (var i = 0; i < expectedPairs.length; i++) {
+						var name = expectedPairs[i][0]
+						var text = expectedPairs[i][1]
+						expected.push({
+							name: name,
+							firstType: name === "#text" ? null : "#text",
+							text: text,
+						})
+					}
+
+					var actual = []
+					var list = root.firstChild.childNodes
+					for (var i = 0; i < list.length; i++) {
+						var current = list[i]
+						var textNode = current.childNodes.length === 1
+							? current.firstChild
+							: current
+						actual.push({
+							name: current.nodeName,
+							firstType: textNode === current ? null : textNode.nodeName,
+							text: textNode.nodeValue,
+						})
+					}
+
+					o(actual).deepEquals(expected)
+				}
+
+				var resolve
+
+				function update(id, showParent, showChild) {
+					render(root, [
+						{tag: "div", children: [
+							showParent ? {tag: "[", children: [
+								{tag: "#", children: ""}, // Required
+								showChild ? {tag: "[", attrs: {
+									onbeforeremove: function () {
+										return {then: function (r) { resolve = r }}
+									},
+								}, children: [
+									{tag: "div", text: id},
+								]} : undefined,
+							]} : undefined,
+						]}
+					])
+				}
+
+				update("1", true, true)
+				expect([
+					["#text", ""],
+					["DIV", "1"],
+				])
+				o(resolve).equals(undefined)
+
+				update("2", true, false)
+				expect([
+					["#text", ""],
+					["DIV", "1"],
+				])
+				o(typeof resolve).equals("function")
+				var original = resolve
+
+				update("3", true, true)
+				expect([
+					["#text", ""],
+					["DIV", "1"],
+					["DIV", "3"],
+				])
+				o(resolve).equals(original)
+
+				update("4", false, true)
+				expect([
+					["DIV", "1"],
+				])
+				o(resolve).equals(original)
+
+				update("5", true, true)
+				expect([
+					["DIV", "1"],
+					["#text", ""],
+					["DIV", "5"],
+				])
+				o(resolve).equals(original)
+
+				resolve()
+				expect([
+					["#text", ""],
+					["DIV", "5"],
+				])
+				o(resolve).equals(original)
+
+				update("6", true, true)
+				expect([
+					["#text", ""],
+					["DIV", "6"],
+				])
+				o(resolve).equals(original)
+			})
+			// Warning: this test is complicated because it's replicating a race condition.
+			o("removes correct nodes when child delays removal, parent removes, then child resolves + rejects both", function () {
+				// Sugar over the complexity - I need to test the entire tree for consistency.
+				function expect(expectedPairs) {
+					var expected = []
+
+					for (var i = 0; i < expectedPairs.length; i++) {
+						var name = expectedPairs[i][0]
+						var text = expectedPairs[i][1]
+						expected.push({
+							name: name,
+							firstType: name === "#text" ? null : "#text",
+							text: text,
+						})
+					}
+
+					var actual = []
+					var list = root.firstChild.childNodes
+					for (var i = 0; i < list.length; i++) {
+						var current = list[i]
+						var textNode = current.childNodes.length === 1
+							? current.firstChild
+							: current
+						actual.push({
+							name: current.nodeName,
+							firstType: textNode === current ? null : textNode.nodeName,
+							text: textNode.nodeValue,
+						})
+					}
+
+					o(actual).deepEquals(expected)
+				}
+
+				var resolve, reject
+
+				function update(id, showParent, showChild) {
+					render(root, [
+						{tag: "div", children: [
+							showParent ? {tag: "[", children: [
+								{tag: "#", children: ""}, // Required
+								showChild ? {tag: "[", attrs: {
+									onbeforeremove: function () {
+										return {then: function (res, rej) {
+											resolve = res
+											reject = rej
+										}}
+									},
+								}, children: [
+									{tag: "div", text: id},
+								]} : undefined,
+							]} : undefined,
+						]}
+					])
+				}
+
+				update("1", true, true)
+				expect([
+					["#text", ""],
+					["DIV", "1"],
+				])
+				o(resolve).equals(undefined)
+
+				update("2", true, false)
+				expect([
+					["#text", ""],
+					["DIV", "1"],
+				])
+				o(typeof resolve).equals("function")
+				var originalResolve = resolve
+				var originalReject = reject
+
+				update("3", true, true)
+				expect([
+					["#text", ""],
+					["DIV", "1"],
+					["DIV", "3"],
+				])
+				o(resolve).equals(originalResolve)
+				o(reject).equals(originalReject)
+
+				update("4", false, true)
+				expect([
+					["DIV", "1"],
+				])
+				o(resolve).equals(originalResolve)
+				o(reject).equals(originalReject)
+
+				update("5", true, true)
+				expect([
+					["DIV", "1"],
+					["#text", ""],
+					["DIV", "5"],
+				])
+				o(resolve).equals(originalResolve)
+				o(reject).equals(originalReject)
+
+				resolve()
+				reject()
+				reject()
+				resolve()
+				expect([
+					["#text", ""],
+					["DIV", "5"],
+				])
+				o(resolve).equals(originalResolve)
+				o(reject).equals(originalReject)
+
+				update("6", true, true)
+				expect([
+					["#text", ""],
+					["DIV", "6"],
+				])
+				o(resolve).equals(originalResolve)
+				o(reject).equals(originalReject)
+			})
 		})
 	})
 })

render/tests/test-updateNodesFuzzer.js

@@ -13,7 +13,7 @@ o.spec("updateNodes keyed list Fuzzer", function() {
 		render = vdom($window)
 	})
 
-	
+
 	void [
 		{delMax: 0, movMax: 50, insMax: 9},
 		{delMax: 3, movMax: 5, insMax: 5},
@@ -31,7 +31,7 @@ o.spec("updateNodes keyed list Fuzzer", function() {
 				render(root, test.updated.map(function(x){return {tag: x, key: x}}))
 
 				if (root.appendChild.callCount + root.insertBefore.callCount !== test.expected.creations + test.expected.moves) console.log(test, {aC: root.appendChild.callCount, iB: root.insertBefore.callCount}, [].map.call(root.childNodes, function(n){return n.nodeName.toLowerCase()}))
-				
+
 				o(root.appendChild.callCount + root.insertBefore.callCount).equals(test.expected.creations + test.expected.moves)("moves")
 				o(root.removeChild.callCount).equals(test.expected.deletions)("deletions")
 				o([].map.call(root.childNodes, function(n){return n.nodeName.toLowerCase()})).deepEquals(test.updated)
@@ -154,4 +154,3 @@ function addSpies(node) {
 	node.insertBefore = o.spy(node.insertBefore)
 	node.removeChild = o.spy(node.removeChild)
 }
-

test-utils/domMock.js

@@ -94,7 +94,7 @@ module.exports = function(options) {
 		}
 		else {
 			this.childNodes.push(child)
-			if (child.parentNode != null && child.parentNode !== this) child.parentNode.removeChild(child)
+			if (child.parentNode != null && child.parentNode !== this) removeChild.call(child.parentNode, child)
 			child.parentNode = this
 		}
 	}
@@ -124,14 +124,14 @@ module.exports = function(options) {
 				this.childNodes.splice.apply(this.childNodes, [refIndex, 0].concat(child.childNodes))
 				while (child.firstChild) {
 					var subchild = child.firstChild
-					child.removeChild(subchild)
+					removeChild.call(child, subchild)
 					subchild.parentNode = this
 				}
 				child.childNodes = []
 			}
 			else {
 				this.childNodes.splice(refIndex, 0, child)
-				if (child.parentNode != null && child.parentNode !== this) child.parentNode.removeChild(child)
+				if (child.parentNode != null && child.parentNode !== this) removeChild.call(child.parentNode, child)
 				child.parentNode = this
 			}
 		}
@@ -214,14 +214,14 @@ module.exports = function(options) {
 					if (ns != null) element.setAttributeNS(ns, name, value)
 					else element.setAttribute(name, value)
 				})
-				stack[depth].appendChild(element)
+				appendChild.call(stack[depth], element)
 				if (!selfClosed && voidElements.indexOf(startTag.toLowerCase()) < 0) stack[++depth] = element
 			}
 			else if (endTag) {
 				depth--
 			}
 			else if (text) {
-				stack[depth].appendChild($window.document.createTextNode(text)) // FIXME handle html entities
+				appendChild.call(stack[depth], $window.document.createTextNode(text)) // FIXME handle html entities
 			}
 		})
 	}
@@ -319,7 +319,7 @@ module.exports = function(options) {
 					},
 					set innerHTML(value) {
 						var voidElements = ["area", "base", "br", "col", "command", "embed", "hr", "img", "input", "keygen", "link", "meta", "param", "source", "track", "wbr"]
-						while (this.firstChild) this.removeChild(this.firstChild)
+						while (this.firstChild) removeChild.call(this, this.firstChild)
 						var match = value.match(/^<svg xmlns="http:\/\/www\.w3\.org\/2000\/svg">(.*)<\/svg>$/), root, ns
 						if (match) {
 							var value = match[1]
@@ -659,12 +659,20 @@ module.exports = function(options) {
 					nodeType: 3,
 					nodeName: "#text",
 					parentNode: null,
+					get childNodes() { return [] },
+					get firstChild() { return null },
 					get nodeValue() {return nodeValue},
 					set nodeValue(value) {
 						/*eslint-disable no-implicit-coercion*/
 						nodeValue = "" + value
 						/*eslint-enable no-implicit-coercion*/
 					},
+					get nextSibling() {
+						if (this.parentNode == null) return null
+						var index = this.parentNode.childNodes.indexOf(this)
+						if (index < 0) throw new TypeError("Parent's childNodes is out of sync")
+						return this.parentNode.childNodes[index + 1] || null
+					},
 				}
 			},
 			createDocumentFragment: function() {
@@ -691,9 +699,9 @@ module.exports = function(options) {
 		},
 	}
 	$window.document.documentElement = $window.document.createElement("html")
-	$window.document.documentElement.appendChild($window.document.createElement("head"))
+	appendChild.call($window.document.documentElement, $window.document.createElement("head"))
 	$window.document.body = $window.document.createElement("body")
-	$window.document.documentElement.appendChild($window.document.body)
+	appendChild.call($window.document.documentElement, $window.document.body)
 	activeElement = $window.document.body
 
 	if (options.spy) $window.__getSpies = getSpies