Don't fire fake onNodeClosed for /body or /html

jhy · jhy · commit df347747385e · 2025-04-03T10:49:55.000+11:00
These were fired to get those closing node source positions tracked, but the node isn't actually popped at that stage. So, it was causing the StreamParser to emit twice. Just call trackNodePosition directly. Fixes #2295
diff --git a/CHANGES.md b/CHANGES.md
@@ -1,6 +1,6 @@
 # jsoup Changelog
 
-## 1.19.2 (PENDING)
+## 1.20.1 (PENDING)
 
 ### Changes
 
@@ -22,6 +22,7 @@
 * When converting a Document to the W3C DOM in `W3CDom`, an element with an attribute in an undeclared namespace now
   gets a declaration of `xmlns:prefix="undefined"`. This allows subsequent serializations to XML via `W3CDom.asString()`
   to succeed. [#2087](https://github.com/jhy/jsoup/issues/2087).
+* The `StreamParser` could emit the final elements of a document twice, due to how `onNodeCompleted` was fired when closing out the stack. [#2295](https://github.com/jhy/jsoup/issues/2295).
 
 ## 1.19.1 (2025-03-04)
 
diff --git a/src/main/java/org/jsoup/parser/HtmlTreeBuilderState.java b/src/main/java/org/jsoup/parser/HtmlTreeBuilderState.java
@@ -687,7 +687,7 @@ private boolean inBodyEndTag(Token t, HtmlTreeBuilder tb) {
                     } else {
                         if (tb.onStackNot(InBodyEndOtherErrors))
                             tb.error(this);
-                        tb.onNodeClosed(tb.getFromStack("body")); // track source position of close; everything is still on stack in case of trailers
+                        tb.trackNodePosition(tb.getFromStack("body"), false); // track source position of close; body is left on stack, in case of trailers
                         tb.transition(AfterBody);
                     }
                     break;
@@ -1656,7 +1656,7 @@ else if (name.equals("col")) {
                     tb.error(this);
                     return false;
                 } else {
-                    if (html != null) tb.onNodeClosed(html); // track source position of close; everything is still on stack in case of trailers
+                    if (html != null) tb.trackNodePosition(html, false); // track source position of close; html is left on stack, in case of trailers
                     tb.transition(AfterAfterBody);
                 }
             } else if (t.isEOF()) {
diff --git a/src/main/java/org/jsoup/parser/TreeBuilder.java b/src/main/java/org/jsoup/parser/TreeBuilder.java
@@ -279,7 +279,7 @@ void onNodeClosed(Node node) {
             nodeListener.tail(node, stack.size());
     }
 
-    private void trackNodePosition(Node node, boolean isStart) {
+    void trackNodePosition(Node node, boolean isStart) {
         if (!trackSourceRange) return;
 
         final Token token = currentToken;
diff --git a/src/test/java/org/jsoup/parser/PositionTest.java b/src/test/java/org/jsoup/parser/PositionTest.java
@@ -220,6 +220,23 @@ private void printRange(Node node) {
         assertEquals("2,9:15-4,8:33", data.sourceRange().toString());
     }
 
+    @Test void tracksExplicitAndImplicitBodyHtml() {
+        // Tests that </body></html> tokens are tracked when present
+        String htmlSans = "<body><a>Link</a>";
+        String htmlWith = "<html><head></head><body><a>Link</a></body></html>";
+
+        Document docSans = Jsoup.parse(htmlSans, TrackingHtmlParser);
+        Document docWith = Jsoup.parse(htmlWith, TrackingHtmlParser);
+
+        StringBuilder trackSans = new StringBuilder();
+        StringBuilder trackWith = new StringBuilder();
+        docSans.forEachNode(node -> accumulatePositions(node, trackSans));
+        docWith.forEachNode(node -> accumulatePositions(node, trackWith));
+
+        assertEquals("#document:0-0~17-17; html:0-0~17-17; head:0-0~0-0; body:0-6~17-17; a:6-9~13-17; #text:9-13; ", trackSans.toString());
+        assertEquals("#document:0-0~50-50; html:0-6~43-50; head:6-12~12-19; body:19-25~36-43; a:25-28~32-36; #text:28-32; ", trackWith.toString());
+    }
+
     @Test void tracksXml() {
         String xml = "<?xml version=\"1.0\" encoding=\"utf-8\"?>\n<!doctype html>\n<rss url=foo>\nXML\n</rss>\n<!-- comment -->";
         Document doc = Jsoup.parse(xml, TrackingXmlParser);
diff --git a/src/test/java/org/jsoup/parser/StreamParserTest.java b/src/test/java/org/jsoup/parser/StreamParserTest.java
@@ -10,6 +10,8 @@
 import org.jsoup.nodes.Node;
 import org.jsoup.select.Elements;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
 
 import java.io.BufferedReader;
 import java.io.File;
@@ -463,4 +465,23 @@ void canStreamFragmentXml() throws IOException {
         }
     }
 
+    @ParameterizedTest
+    @ValueSource(strings = {
+        "<html><body><a>Link</a></body></html>",
+        "<html><body><a>Link</a>",
+        "<a>Link</a></body></html>",
+        "<a>Link</a>",
+        "<a>Link",
+        "<a>Link</body>",
+    })
+    void emitsOnlyOnce(String html) {
+        try (StreamParser parser = new StreamParser(Parser.htmlParser()).parse(html, "")) {
+            // https://github.com/jhy/jsoup/issues/2295
+            // When there was a /body or /html, those were being emitted twice, due to firing a fake onNodeClosed to track their source positions
+            StringBuilder seen = new StringBuilder();
+            parser.stream().forEach(el -> trackSeen(el, seen));
+            assertEquals("head+;a[Link];body;html;#root;", seen.toString());
+        }
+    }
+
 }

Original file line number	Diff line number	Diff line change
`@@ -279,7 +279,7 @@ void onNodeClosed(Node node) {`
`279`	`279`	`nodeListener.tail(node, stack.size());`
`280`	`280`	`}`
`281`	`281`
`282`		`- private void trackNodePosition(Node node, boolean isStart) {`
	`282`	`+ void trackNodePosition(Node node, boolean isStart) {`
`283`	`283`	`if (!trackSourceRange) return;`
`284`	`284`
`285`	`285`	`final Token token = currentToken;`