Node.before(Node) / after(Node) throw NPE and detach the node when given the node itself #2628

Version

jsoup 1.23.2

Test

@Test
public void test_before_self() {
    Document doc = Jsoup.parse("<ul><li id=a>A</li><li id=b>B</li><li id=c>C</li></ul>");
    Element ul = doc.expectFirst("ul");
    Element b = doc.expectFirst("#b");

    try {
        b.before(b);
    } catch (ValidationException expected) {
        // Rejecting a self-insert may also be reasonable.
    }

    assertSame(ul, b.parent());
    assertEquals(3, ul.childrenSize());
}

Description

Calling before(Node) or after(Node) with the node itself throws a NullPointerException, and the node has already been removed from the document when the exception is thrown:


java.lang.NullPointerException: Cannot invoke "org.jsoup.nodes.Element.addChildren(int, org.jsoup.nodes.Node[])" because "this.parentNode" is null
 at org.jsoup.nodes.Node.before(Node.java:410)
 at org.jsoup.nodes.Element.before(Element.java:1032)


After this, b.parent() is null and the list only contains A and C.

This looks like a regression from 62de0a1, first released in 1.16.1. The test above passes on 1.15.4, where b.before(b) and b.after(b) leave the document unchanged.

Since that commit, when node == this, the check node.parentNode == parentNode is always true, so node.remove() removes this node and clears this.parentNode before parentNode.addChildren(...) runs. The validateChild check from #2558 runs inside addChildren, so it isn't reached.

This can happen when the node being moved and the reference node come from separate lookups and turn out to be the same element.