From: Tim Starling <tstarling@wikimedia.org>
Date: Fri, 17 Nov 2017 11:15:59 +0000 (+1100)
Subject: Fix RemexCompatMunger infinite recursion
X-Git-Tag: 1.31.0-rc.0~1470^2
X-Git-Url: http://git.cyclocoop.org/%22%20.%20generer_url_ecrire%28%22articles%22%2C%22id_article=%24id_article%22%29%20.%20%22?a=commitdiff_plain;h=324e4bca4f540bca6526b7d0fe88f2ef18807872;p=lhc%2Fweb%2Fwiklou.git

Fix RemexCompatMunger infinite recursion

When TreeBuilder requests reparenting of all child nodes of a given
element, we do this by removing the existing child nodes, and then
inserting the proposed new parent under the old parent. However, when a
p-wrap diversion is in place, the insertion of the new parent is
diverted into the p-wrap, and the p-wrap then becomes a child of the new
parent, causing a reference loop, and ultimately infinite recursion in
Serializer.

Instead, divert the entire reparent request to the p-wrap, so that the
new parent is a child of the p-wrap. This makes sense since the new
parent is always a formatting element. The only caller of
reparentChildren(), apart from proxies, is AAA step 17, which reparents
children under the formatting element cloned from the AFE list.

Left in some debug code for next time.

Bug: T178632
Change-Id: Id77d21d99748e94c064ef24c43ee0033de627b8e
---

diff --git a/includes/tidy/RemexCompatMunger.php b/includes/tidy/RemexCompatMunger.php
index 73bc5f8493..c06eea01b1 100644
--- a/includes/tidy/RemexCompatMunger.php
+++ b/includes/tidy/RemexCompatMunger.php
@@ -174,6 +174,10 @@ class RemexCompatMunger implements TreeHandler {
 			$length, $sourceStart, $sourceLength );
 	}
 
+	private function trace( $msg ) {
+		// echo "[RCM] $msg\n";
+	}
+
 	/**
 	 * Insert or reparent an element. Create p-wrappers or split the tag stack
 	 * as necessary.
@@ -242,6 +246,7 @@ class RemexCompatMunger implements TreeHandler {
 		if ( $under && $parentData->isPWrapper && !$inline ) {
 			// [B/b] The element is non-inline and the parent is a p-wrapper,
 			// close the parent and insert into its parent instead
+			$this->trace( 'insert B/b' );
 			$newParent = $this->serializer->getParentNode( $parent );
 			$parent = $newParent;
 			$parentData = $parent->snData;
@@ -255,12 +260,14 @@ class RemexCompatMunger implements TreeHandler {
 			// [CS/b, DS/i] The parent is splittable and the current element is
 			// inline in block context, or if the current element is a block
 			// under a p-wrapper, split the tag stack.
+			$this->trace( $inline ? 'insert DS/i' : 'insert CS/b' );
 			$newRef = $this->splitTagStack( $newRef, $inline, $sourceStart );
 			$parent = $newRef;
 			$parentData = $parent->snData;
 		} elseif ( $under && $parentData->needsPWrapping && $inline ) {
 			// [A/i] If the element is inline and we are in body/blockquote,
 			// we need to create a p-wrapper
+			$this->trace( 'insert A/i' );
 			$newRef = $this->insertPWrapper( $newRef, $sourceStart );
 			$parent = $newRef;
 			$parentData = $parent->snData;
@@ -268,9 +275,12 @@ class RemexCompatMunger implements TreeHandler {
 			// [CU/b] If the element is non-inline and (despite attempting to
 			// split above) there is still an ancestor p-wrap, disable that
 			// p-wrap
+			$this->trace( 'insert CU/b' );
 			$this->disablePWrapper( $parent, $sourceStart );
+		} else {
+			// [A/b, B/i, C/i, D/b, DU/i] insert as normal
+			$this->trace( 'insert normal' );
 		}
-		// else [A/b, B/i, C/i, D/b, DU/i] insert as normal
 
 		// An element with element children is a non-blank element
 		$parentData->nonblankNodeCount++;
@@ -457,6 +467,20 @@ class RemexCompatMunger implements TreeHandler {
 
 	public function reparentChildren( Element $element, Element $newParent, $sourceStart ) {
 		$self = $element->userData;
+		if ( $self->snData->childPElement ) {
+			// Reparent under the p-wrapper instead, so that e.g.
+			//   <blockquote><mw:p-wrap>...</mw:p-wrap></blockquote>
+			// becomes
+			//   <blockquote><mw:p-wrap><i>...</i></mw:p-wrap></blockquote>
+
+			// The formatting element should not be the parent of the p-wrap.
+			// Without this special case, the insertElement() of the <i> below
+			// would be diverted into the p-wrapper, causing infinite recursion
+			// (T178632)
+			$this->reparentChildren( $self->snData->childPElement, $newParent, $sourceStart );
+			return;
+		}
+
 		$children = $self->children;
 		$self->children = [];
 		$this->insertElement( TreeBuilder::UNDER, $element, $newParent, false, $sourceStart, 0 );
@@ -464,6 +488,7 @@ class RemexCompatMunger implements TreeHandler {
 		$newParentId = $newParentNode->id;
 		foreach ( $children as $child ) {
 			if ( is_object( $child ) ) {
+				$this->trace( "reparent <{$child->name}>" );
 				$child->parentId = $newParentId;
 			}
 		}
diff --git a/includes/tidy/RemexMungerData.php b/includes/tidy/RemexMungerData.php
index d614a38183..08d148f682 100644
--- a/includes/tidy/RemexMungerData.php
+++ b/includes/tidy/RemexMungerData.php
@@ -75,4 +75,43 @@ class RemexMungerData {
 	public function __set( $name, $value ) {
 		throw new \Exception( "Cannot set property \"$name\"" );
 	}
+
+	/**
+	 * Get a text representation of the current state of the serializer, for
+	 * debugging.
+	 *
+	 * @return string
+	 */
+	public function dump() {
+		if ( $this->childPElement ) {
+			$parts[] = 'childPElement=' . $this->childPElement->getDebugTag();
+		}
+		if ( $this->ancestorPNode ) {
+			$parts[] = "ancestorPNode=<{$this->ancestorPNode->name}>";
+		}
+		if ( $this->wrapBaseNode ) {
+			$parts[] = "wrapBaseNode=<{$this->wrapBaseNode->name}>";
+		}
+		if ( $this->currentCloneElement ) {
+			$parts[] = "currentCloneElement=" . $this->currentCloneElement->getDebugTag();
+		}
+		if ( $this->isPWrapper ) {
+			$parts[] = 'isPWrapper';
+		}
+		if ( $this->isSplittable ) {
+			$parts[] = 'isSplittable';
+		}
+		if ( $this->needsPWrapping ) {
+			$parts[] = 'needsPWrapping';
+		}
+		if ( $this->nonblankNodeCount ) {
+			$parts[] = "nonblankNodeCount={$this->nonblankNodeCount}";
+		}
+		$s = "RemexMungerData {\n";
+		foreach ( $parts as $part ) {
+			$s .= "  $part\n";
+		}
+		$s .= "}\n";
+		return $s;
+	}
 }
diff --git a/tests/phpunit/includes/tidy/RemexDriverTest.php b/tests/phpunit/includes/tidy/RemexDriverTest.php
index 6b16cbf695..f980af0090 100644
--- a/tests/phpunit/includes/tidy/RemexDriverTest.php
+++ b/tests/phpunit/includes/tidy/RemexDriverTest.php
@@ -252,6 +252,11 @@ class RemexDriverTest extends MediaWikiTestCase {
 			'<table><b>1<p>2</b>3</p>',
 			'<b>1</b><p><b>2</b>3</p><table></table>'
 		],
+		[
+			'AAA causes reparent of p-wrapped text node (T178632)',
+			'<i><blockquote>x</i></blockquote>',
+			'<i></i><blockquote><p><i>x</i></p></blockquote>',
+		],
 	];
 
 	public function provider() {