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 73bc5f8..c06eea0 100644 (file)
--- 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 d614a38..08d148f 100644 (file)
--- 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 6b16cbf..f980af0 100644 (file)
--- 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() {