From be7efc94a384dff54556e461710d27661219dd62 Mon Sep 17 00:00:00 2001
From: Kingsley Yung <kingsley@kkoyung.dev>
Date: Thu, 29 May 2025 01:58:33 +0800
Subject: [PATCH] Refactoring `HTMLOptionElement::Text` into iterative style
 (#37167)

The original implementation of `HTMLOptionElement::Text` is recursive,
and the program may run out of stack space for a sufficiently large
number of iterations. The patch switches to an iterative implementation,
with `TreeIterator`.

Note that, instead of the usual `while let Some(node) = iterator.next()`
approach, we use `while let Some(node) = iterator.peek()` with the newly
added `TreeIterator::peek` function. This is because the choice of the
next node depends on some checks performed inside the `while` block,
whereas the `next` function determines the next node before entering the
block.

Moreover, the `TreeIterator::peek` function is added, instead of
wrapping the iterator into `Peekable`. This is because we will lose
access to the `TreeIterator::next_skipping_children` function if we wrap
it into `Peekable`.

Testing: This refactoring has to pass the existing tests.
Fixes: #36959

Signed-off-by: Kingsley Yung <kingsley@kkoyung.dev>
---
 components/script/dom/htmloptionelement.rs | 43 +++++++++++-----------
 components/script/dom/node.rs              |  4 ++
 2 files changed, 26 insertions(+), 21 deletions(-)

diff --git a/components/script/dom/htmloptionelement.rs b/components/script/dom/htmloptionelement.rs
index 800e88f0758..a61564d3c5b 100644
--- a/components/script/dom/htmloptionelement.rs
+++ b/components/script/dom/htmloptionelement.rs
@@ -152,25 +152,6 @@ impl HTMLOptionElement {
     }
 }
 
-// FIXME(ajeffrey): Provide a way of buffering DOMStrings other than using Strings
-fn collect_text(element: &Element, value: &mut String) {
-    let svg_script =
-        *element.namespace() == ns!(svg) && element.local_name() == &local_name!("script");
-    let html_script = element.is::<HTMLScriptElement>();
-    if svg_script || html_script {
-        return;
-    }
-
-    for child in element.upcast::<Node>().children() {
-        if child.is::<Text>() {
-            let characterdata = child.downcast::<CharacterData>().unwrap();
-            value.push_str(&characterdata.Data());
-        } else if let Some(element_child) = child.downcast() {
-            collect_text(element_child, value);
-        }
-    }
-}
-
 impl HTMLOptionElementMethods<crate::DomTypeHolder> for HTMLOptionElement {
     /// <https://html.spec.whatwg.org/multipage/#dom-option>
     fn Option(
@@ -216,8 +197,28 @@ impl HTMLOptionElementMethods<crate::DomTypeHolder> for HTMLOptionElement {
 
     /// <https://html.spec.whatwg.org/multipage/#dom-option-text>
     fn Text(&self) -> DOMString {
-        let mut content = String::new();
-        collect_text(self.upcast(), &mut content);
+        let mut content = DOMString::new();
+
+        let mut iterator = self.upcast::<Node>().traverse_preorder(ShadowIncluding::No);
+        while let Some(node) = iterator.peek() {
+            if let Some(element) = node.downcast::<Element>() {
+                let html_script = element.is::<HTMLScriptElement>();
+                let svg_script = *element.namespace() == ns!(svg) &&
+                    element.local_name() == &local_name!("script");
+                if html_script || svg_script {
+                    iterator.next_skipping_children();
+                    continue;
+                }
+            }
+
+            if node.is::<Text>() {
+                let characterdata = node.downcast::<CharacterData>().unwrap();
+                content.push_str(&characterdata.Data());
+            }
+
+            iterator.next();
+        }
+
         DOMString::from(str_join(split_html_space_chars(&content), " "))
     }
 
diff --git a/components/script/dom/node.rs b/components/script/dom/node.rs
index 1ff2e25efae..691b3a38f19 100644
--- a/components/script/dom/node.rs
+++ b/components/script/dom/node.rs
@@ -2035,6 +2035,10 @@ impl TreeIterator {
         self.current = None;
         Some(current)
     }
+
+    pub(crate) fn peek(&self) -> Option<&DomRoot<Node>> {
+        self.current.as_ref()
+    }
 }
 
 impl Iterator for TreeIterator {