HTML API: Avoid rescanning unmatched character references

[sekalan]

When decode() finds an ampersand that does not start a character reference, it advances the old search offset by one byte. If the ampersand is farther ahead, the next iteration finds the same position and tries to decode it again. A long prefix makes this repeated scanning quadratic.

Advance past the ampersand that was actually found. The literal text is preserved, and subsequent references are still decoded.

The regression test counts token-map lookups rather than using a timing threshold. It covers text nodes, attributes, a miss after a successful decode, and an ambiguous ampersand in an attribute. All four cases fail before the fix and pass afterwards.

Testing: PHP 8.3.0 / PHPUnit 9.6.37, 129 decoder test cases and 274 assertions passed using a database-free bootstrap. The full WordPress test suite was not run. PHPCS and git diff --check passed.

For context, a separate PHP 8.3.35 benchmark of WP_HTML_Processor::normalize() on a synthetic 103 KB article with literal ampersands went from 39.194 ms to 2.591 ms (five paired trials, median). OPcache was enabled in both runs and the output was identical. An escaped-ampersand control showed no improvement. These are parser timings, not page-load timings.

Trac ticket: https://core.trac.wordpress.org/ticket/66241

AI assistance: OpenAI Codex assisted with investigation, the patch, tests, and this description.

---

This pull request is for code review only; discussion and the final commit belong in Trac.

[github-actions]

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props serhatsoylu06, westonruter, jonsurrell.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[github-actions]

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

• All changes will be lost when closing a tab with a Playground instance.
• All changes will be lost when refreshing the page.
• A fresh instance is created each time the link below is clicked.
• Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
  it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

[sirreal]

Thank you @sekalan, good find. The fix here seems correct. I'd prefer to add some test cases rather than creating new tests that are asserting on internal behaviors. For example, like this (from 245b0eb):

suggested change

diff --git a/tests/phpunit/tests/html-api/wpHtmlDecoder.php b/tests/phpunit/tests/html-api/wpHtmlDecoder.php
index 81c047c437..701dd0ea2b 100644
--- a/tests/phpunit/tests/html-api/wpHtmlDecoder.php
+++ b/tests/phpunit/tests/html-api/wpHtmlDecoder.php
@@ -65,13 +65,14 @@ class Tests_HtmlApi_WpHtmlDecoder extends WP_UnitTestCase {
 	 * Ensures proper decoding of edge cases.
 	 *
 	 * @ticket 61072
+	 * @ticket 66241
 	 *
 	 * @dataProvider data_edge_cases
 	 *
-	 * @param $raw_text_node Raw input text.
-	 * @param $decoded_value The expected decoded text result.
+	 * @param string $raw_text_node Raw input text.
+	 * @param string $decoded_value The expected decoded text result.
 	 */
-	public function test_edge_cases( $raw_text_node, $decoded_value ) {
+	public function test_edge_cases( string $raw_text_node, string $decoded_value ): void {
 		$this->assertSame(
 			$decoded_value,
 			WP_HTML_Decoder::decode_text_node( $raw_text_node ),
@@ -79,77 +80,21 @@ class Tests_HtmlApi_WpHtmlDecoder extends WP_UnitTestCase {
 		);
 	}
 
-	public static function data_edge_cases() {
-		return array(
-			'Single ampersand' => array( '&', '&' ),
-		);
-	}
-
 	/**
-	 * Ensures unmatched character references are not examined repeatedly.
+	 * Data provider.
 	 *
-	 * @ticket 66241
-	 *
-	 * @dataProvider data_unmatched_character_references
-	 *
-	 * @param string $context  Decoder context.
-	 * @param string $text     Raw input text.
-	 * @param string $expected Expected decoded text.
+	 * @return array<string, array{string, string}>
 	 */
-	public function test_decode_does_not_repeat_unmatched_character_references( $context, $text, $expected ) {
-		global $html5_named_character_references;
+	public static function data_edge_cases(): array {
+		$long_text = str_repeat( 'a', 300000 );
 
-		$original_map = $html5_named_character_references;
-
-		// Note: setMethods() is deprecated in PHPUnit 9, but still supported.
-		$token_map = $this->getMockBuilder( WP_Token_Map::class )
-			->setMethods( array( 'read_token' ) )
-			->getMock();
-
-		// Each input contains two named references. Neither should be read twice.
-		$token_map->expects( $this->exactly( 2 ) )
-			->method( 'read_token' )
-			->willReturnCallback(
-				static function ( $text, $offset, &$matched_token_byte_length, $case_sensitivity = 'case-sensitive' ) use ( $original_map ) {
-					return $original_map->read_token( $text, $offset, $matched_token_byte_length, $case_sensitivity );
-				}
-			);
-
-		try {
-			$html5_named_character_references = $token_map;
-			$this->assertSame( $expected, WP_HTML_Decoder::decode( $context, $text ) );
-		} finally {
-			$html5_named_character_references = $original_map;
-		}
-	}
-
-	/**
-	 * Data provider for test_decode_does_not_repeat_unmatched_character_references().
-	 *
-	 * @return array[]
-	 */
-	public static function data_unmatched_character_references() {
 		return array(
-			'Unknown name in a text node'            => array(
-				'data',
-				'prefix &unknown; middle &amp; tail',
-				'prefix &unknown; middle & tail',
-			),
-			'Unknown name in an attribute'           => array(
-				'attribute',
-				'prefix &unknown; middle &amp; tail',
-				'prefix &unknown; middle & tail',
-			),
-			'Unknown name after a decoded reference' => array(
-				'data',
-				'prefix &amp; middle &unknown; tail',
-				'prefix & middle &unknown; tail',
-			),
-			'Ambiguous ampersand in an attribute'    => array(
-				'attribute',
-				'prefix &not=value middle &amp; tail',
-				'prefix &not=value middle & tail',
-			),
+			'Single ampersand'                    => array( '&', '&' ),
+			'Unmatched reference before a match'  => array( 'a &bogus; b &amp; c', 'a &bogus; b & c' ),
+			'Unmatched reference after a match'   => array( 'a &amp; b &bogus; c &lt; d', 'a & b &bogus; c < d' ),
+			'Unmatched numeric references'        => array( 'a &#; b &#x; c &amp;', 'a &#; b &#x; c &' ),
+			'Adjacent ampersands'                 => array( '&&&amp;', '&&&' ),
+			'Unmatched reference after long text' => array( "{$long_text}&bogus;&amp;", "{$long_text}&bogus;&" ),
 		);
 	}
 

It's very easy to observe the fix from there. If I reset src to the version on trunk, I run this:

WP_TESTS_SKIP_INSTALL=1 ./vendor/bin/phpunit --group=66241 --repeat 10

The 10× repeat exaggerates the difference, but there's no doubt there's a significant improvement:

-Time: 00:18.890, Memory: 224.50 MB
+Time: 00:00.794, Memory: 224.50 MB

[sekalan]

Thanks @sirreal and @westonruter . I replaced the mock-based test with sirreal's edge cases and carried the typing suggestions (non-falsy-string, native types) over to the new test and data provider.

Happy to also run the same cases through decode_attribute() if you think the attribute path is worth covering separately.

[sirreal]

This is an observation, not a request for changes.

An empty string test would probably be good here. For the values in a data provider, it may be best not to eagerly use non-falsy-string but keep wider string types:

-	 * @return array<non-falsy-string, array{ non-falsy-string, non-falsy-string }>
+	 * @return array<non-falsy-string, array{ string, string }>

[sirreal]

Very nice, thanks @sekalan!

[github-actions]

A commit was made that fixes the Trac ticket referenced in the description of this pull request.

SVN changeset: 64124
GitHub commit: 2bf0714

This PR will be closed, but please confirm the accuracy of this and reopen if there is more work to be done.