Throw an error if calling parser recursively

People accidentally (or sometimes intentionally) calling the
parser recursively has been a major source of bugs over the
years. I think its much better to fail suddenly, instead
of having unclear signs like UNIQ's all over the place.

Change-Id: I0e42aa69835c15a5df7aecb0dc5c3dec946bdf6a

diff --git a/RELEASE-NOTES-1.24 b/RELEASE-NOTES-1.24
index 189c5f1..09aeb89 100644 (file)
--- a/RELEASE-NOTES-1.24
+++ b/RELEASE-NOTES-1.24
@@ -30,6 +30,7 @@ production.
   which used to collapse some sidebar elements by default.
 * (bug 890) Links in Special:RecentChanges and Special:Watchlist no longer
   follow redirects to their target pages.
+* Parser now dies early if called recursively, instead of producing subtle bugs.
 
 === Web API changes in 1.24 ===
 * action=parse API now supports prop=modules, which provides the list of

diff --git a/includes/parser/Parser.php b/includes/parser/Parser.php
index 7423006..35b1b17 100644 (file)
--- a/includes/parser/Parser.php
+++ b/includes/parser/Parser.php
@@ -211,6 +211,12 @@ class Parser {
         */
        var $mLangLinkLanguages;
 
+       /**
+        * @var boolean Recursive call protection.
+        * This variable should be treated as if it were private.
+        */
+       public $mInParse = false;
+
        /**
         * Constructor
         *
@@ -254,6 +260,7 @@ class Parser {
         * Allow extensions to clean up when the parser is cloned
         */
        function __clone() {
+               $this->mInParse = false;
                wfRunHooks( 'ParserCloned', array( $this ) );
        }
 
@@ -364,6 +371,10 @@ class Parser {
                wfProfileIn( __METHOD__ );
                wfProfileIn( $fname );
 
+               if ( $clearState ) {
+                       $magicScopeVariable = $this->lock();
+               }
+
                $this->startParse( $title, $options, self::OT_HTML, $clearState );
 
                $this->mInputSize = strlen( $text );
@@ -615,6 +626,7 @@ class Parser {
         */
        function preprocess( $text, Title $title = null, ParserOptions $options, $revid = null ) {
                wfProfileIn( __METHOD__ );
+               $magicScopeVariable = $this->lock();
                $this->startParse( $title, $options, self::OT_PREPROCESS, true );
                if ( $revid !== null ) {
                        $this->mRevisionId = $revid;
@@ -662,6 +674,7 @@ class Parser {
                $text = $msg->params( $params )->plain();
 
                # Parser (re)initialisation
+               $magicScopeVariable = $this->lock();
                $this->startParse( $title, $options, self::OT_PLAIN, true );
 
                $flags = PPFrame::NO_ARGS | PPFrame::NO_TEMPLATES;
@@ -4593,6 +4606,9 @@ class Parser {
         * @return string The altered wiki markup
         */
        public function preSaveTransform( $text, Title $title, User $user, ParserOptions $options, $clearState = true ) {
+               if ( $clearState ) {
+                       $magicScopeVariable = $this->lock();
+               }
                $this->startParse( $title, $options, self::OT_WIKI, $clearState );
                $this->setUser( $user );
 
@@ -4767,6 +4783,7 @@ class Parser {
        public function cleanSig( $text, $parsing = false ) {
                if ( !$parsing ) {
                        global $wgTitle;
+                       $magicScopeVariable = $this->lock();
                        $this->startParse( $wgTitle, new ParserOptions, self::OT_PREPROCESS, true );
                }
 
@@ -5627,6 +5644,8 @@ class Parser {
         */
        private function extractSections( $text, $section, $mode, $newText = '' ) {
                global $wgTitle; # not generally used but removes an ugly failure mode
+
+               $magicScopeVariable = $this->lock();
                $this->startParse( $wgTitle, new ParserOptions, self::OT_PLAIN, true );
                $outText = '';
                $frame = $this->getPreprocessor()->newFrame();
@@ -5972,6 +5991,7 @@ class Parser {
         * @return string
         */
        function testSrvus( $text, Title $title, ParserOptions $options, $outputType = self::OT_HTML ) {
+               $magicScopeVariable = $this->lock();
                $this->startParse( $title, $options, $outputType, true );
 
                $text = $this->replaceVariables( $text );
@@ -6149,4 +6169,27 @@ class Parser {
                }
                return $parsedWidthParam;
        }
+
+       /**
+        * Lock the current instance of the parser.
+        *
+        * This is meant to stop someone from calling the parser
+        * recursively and messing up all the strip state.
+        *
+        * @throws MWException If parser is in a parse
+        * @return ScopedCallback The lock will be released once the return value goes out of scope.
+        */
+       protected function lock() {
+               if ( $this->mInParse ) {
+                       throw new MWException( "Parser state cleared while parsing. Did you call Parser::parse recursively?" );
+               }
+               $this->mInParse = true;
+
+               $that = $this;
+               $recursiveCheck = new ScopedCallback( function() use ( $that ) {
+                       $that->mInParse = false;
+               } );
+
+               return $recursiveCheck;
+       }
 }

diff --git a/tests/phpunit/includes/parser/ParserMethodsTest.php b/tests/phpunit/includes/parser/ParserMethodsTest.php
index 229eeb4..4ea8fc6 100644 (file)
--- a/tests/phpunit/includes/parser/ParserMethodsTest.php
+++ b/tests/phpunit/includes/parser/ParserMethodsTest.php
@@ -29,6 +29,26 @@ class ParserMethodsTest extends MediaWikiLangTestCase {
                $this->assertEquals( $expected, $text );
        }
 
+       /**
+        * @expectedException MWException
+        * @expectedExceptionMessage Parser state cleared while parsing. Did you call Parser::parse recursively?
+        * @covers Parser::lock
+        */
+       public function testRecursiveParse() {
+               global $wgParser;
+               $title = Title::newFromText( 'foo' );
+               $po = new ParserOptions;
+               $wgParser->setHook( 'recursivecallparser', array( $this, 'helperParserFunc' ) );
+               $wgParser->parse( '<recursivecallparser>baz</recursivecallparser>', $title, $po );
+       }
+
+       public function helperParserFunc( $input, $args, $parser) {
+               $title = Title::newFromText( 'foo' );
+               $po = new ParserOptions;
+               $parser->parse( $input, $title, $po );
+               return 'bar';
+       }
+
        /**
         * @covers Parser::callParserFunction
         */