Page MenuHomePhabricator

Math extension causes timeout if ParserAfterTidy hook is called often before injecting math tags
Open, MediumPublicBUG REPORT

Description

Steps to replicate the issue (include links if applicable):

  • We use the "BiblioPlus" extension, which admittedly is a bit old. However, problem can also exist in other situations.
  • Make a page of the following general format:
<cite>[ref1, ref11, ref12, ref2, ref21, ref22, ref3, ref31, ref32, ref4, ref41, ref42, ref5, ref51, ref52, ref6, ref61, ref62, ref7, ref71, ref72, ref8, ref81, ref82, ref9, ref91, ref92, ref10, ref101, ref102]</cite>
<math>p_1</math>
<math>p_2</math>
<math>p_3</math>
<math>p_4</math>
<math>p_5</math>
<math>p_6</math>
<math>q_1</math>
<math>q_2</math>
<math>q_3</math>
<math>q_4</math>
<math>q_5</math>
<math>q_6</math>
<math>p_11</math>
<math>p_21</math>
<math>p_31</math>
<math>p_41</math>
<math>p_51</math>
<math>p_61</math>
<math>q_11</math>
<math>q_21</math>
<math>q_31</math>
<math>q_41</math>
<math>q_51</math>
<math>q_61</math>
Some text
<biblio>
#ref1

#ref2

#ref3

#ref4

#ref5

#ref6

#ref7

#ref8

#ref9

#ref101

#ref11

#ref21

#ref31

#ref41

#ref51

#ref61

#ref71

#ref81

#ref91

#ref11

#ref12

#ref22

#ref32

#ref42

#ref52

#ref62

#ref72

#ref82

#ref92

#ref102
</biblio>
  • Save the page
  • Open the page

What happens?:
Depending on platform specs, we get Fatal error: Maximum execution time of 30 seconds exceeded in /var/www/public_html/includes/libs/http/MultiHttpClient.php on line 274 or everything works just fine.

I did some digging and the problem is this:

  1. Every math tag content/string marker gets appended to ParserHooksHandler::$mathLazyRenderBatch
  2. Then the parsing encounters the biblio tag. This extension parses every refX, thus automatically calling the ParserAfterTidy hook. For every reference!
  3. The ParserHooksHandler::onParserAfterTidy then does a batchEvaluate on all stored renderers, and then a mathPostTagHook, on all stored math tag thingies. For every call to ParserAfterTidy!

What should have happened instead?:
Math code should be rendered at most once per each inclusion in a page.

Software version (on Special:Version page; skip for WMF-hosted wikis like Wikipedia):
Math: REL1_39, ab63662f
BiblioPlus: REL1_39, 2d14903
Mediawiki: MediaWiki 1.39.7 (f4addc1)

Other information (browser name/version, screenshots, etc.):

Event Timeline

Physikerwelt subscribed.

Math code should be rendered at most once per each inclusion in a page.

What happens is as close as one can get to rendering only once. The code unsets already processed tags.

https://github.com/wikimedia/mediawiki-extensions-Math/blame/8e70be8f3d74f49a4c785d9fe3dba7c281cb5170/src/HookHandlers/ParserHooksHandler.php#L171

I didn't test Math it with BiblioPlus; maybe the state is not properly passed between the calls to the ParserAfterTidy hook.

The most pragmatic solution would be to migrate to native MathML rendering. This is pure PHP-based math rendering without any calls to external web services and does not use the hook at all. It comes at the cost of updating to a more recent version of MW. You could check if the problem goes away when you set the rendering mode to tex, if so, it will also go away in native mode. If updating to a more recent MW version is not an option we could discuss backporting the new rendering to 1_39

What happens is as close as one can get to rendering only once. The code unsets already processed tags.

I saw that in the code yes. The problem is that it unsets _fully-processed_ tags, and in our case, the tags are not fully processed, because they are not replaced yet.

Consider the following code:

<?php

class Parser {
  private $mathTag;
  private $mathTidy;

  public function __construct() {
    $this->mathTag = [MathHooks::getInstance(), 'parseTag'];
    $this->mathTidy = [MathHooks::getInstance(), 'afterTidy'];
  }

  public function parse( $input ) {
    $text = $this->internalParse( $input );
    return $this->afterTidy( $text );
  }

  protected function internalParse( $input ) {
    $parsedText = '';
    foreach ( $input as $singleLine ) {
      $value = $singleLine['text'];
      $key = $singleLine['tag'] ?? 'plain';
      switch ($key) {
        case 'math':
          $parsedText .= call_user_func_array( $this->mathTag, [ $this, $value ] );
          break;

        case 'biblio':
          $value = [ $value ];
          $parsedText .= (new Parser)->parse($value);
          break;

        default:
          if ( !is_string( $value ) ) {
            $value = json_encode( $value );
          }
          $parsedText .= htmlspecialchars( $value ) . "\n";
      }
    }

    return $parsedText;
  }

  public function afterTidy( $parsedText ) {
    return call_user_func_array( $this->mathTidy, [ $this, $parsedText ] );
  }

  public function getStripMarker( $uniq ) {
    static $i = 0;
    $i++;
    return "STRIP-$uniq-$i-MARKER";
  }
}

class MathHooks {
  private static ?self $instance = null;
  private array $unparsedMath = [];

  public static function getInstance(): static
  {
    if ( !self::$instance ) {
      self::$instance = new static;
    }

    return self::$instance;
  }

  public function parseTag( $parser, $math ) {
    $key = $parser->getStripMarker( 'math' );
    $this->unparsedMath[ $key ] = $math;

    return $key;
  }

  public function afterTidy( $parser, $parserOut ) {
    foreach ( $this->unparsedMath as $key => $math ) {
      $mathResult = $this->expensiveParse( $math );
      $count = 0;
      $parserOut = str_replace( $key, $mathResult, $parserOut, $count );
      if ( $count !== 0 ) {
        unset( $this->unparsedMath[ $key ] );
      }
    }

    return $parserOut;
  }

  protected function expensiveParse( $math ) {
    echo "I am being called for $math!\n";
    return '<pre>'.json_encode($math,JSON_PRETTY_PRINT)."</pre>\n";
  }
}

echo (new Parser)->parse([
  ['text' => 'formula1', 'tag' => 'math' ],
  ['text' => 'formula2', 'tag' => 'math' ],
  ['text' => [ 'text' => 'some author' ], 'tag' => 'biblio' ],
  ['text' => [ 'text' => 'another author' ], 'tag' => 'biblio' ],
]);

It outputs:

I am being called for formula1!
I am being called for formula2!
I am being called for formula1!
I am being called for formula2!
I am being called for formula1!
I am being called for formula2!
<pre>"formula1"</pre>
<pre>"formula2"</pre>
some author
another author

This is similar to what I see in the logs.

Now consider this code:

<?php

class Parser {
  /* No changes */
}

class MathHooks {
  private static ?self $instance = null;
  private array $unparsedMath = [];
  private array $parsedMath = []; /* This here is new! */

  public static function getInstance(): static
  {
    /* No changes */
  }

  public function parseTag( $parser, $math ) {
    /* No changes */
  }

  public function afterTidy( $parser, $parserOut ) { /* Changes here! */
    foreach ( $this->unparsedMath as $key => $math ) {
      $mathResult = $this->expensiveParse( $math );
      $this->parsedMath[$key] = $mathResult;
      unset( $this->unparsedMath[ $key ] );
    }

    foreach ( $this->parsedMath as $key => $mathResult ) {
      $count = 0;
      $parserOut = str_replace( $key, $mathResult, $parserOut, $count );
      if ( $count !== 0 ) {
        unset( $this->parsedMath[ $key ] );
      }
    }

    return $parserOut;
  }

  protected function expensiveParse( $math ) {
    /* No changes */
  }
}

echo (new Parser)->parse([
  ['text' => 'formula1', 'tag' => 'math' ],
  ['text' => 'formula2', 'tag' => 'math' ],
  ['text' => [ 'text' => 'some author' ], 'tag' => 'biblio' ],
  ['text' => [ 'text' => 'another author' ], 'tag' => 'biblio' ],
]);

It gives this output:

I am being called for formula1!
I am being called for formula2!
<pre>"formula1"</pre>
<pre>"formula2"</pre>
some author
another author

Which is what I want.

Change #1060756 had a related patch set uploaded (by WgevaertWikiBase; author: WgevaertWikiBase):

[mediawiki/extensions/Math@REL1_42] Cache math parse results, T371972

https://gerrit.wikimedia.org/r/1060756

Change #1060757 had a related patch set uploaded (by WgevaertWikiBase; author: WgevaertWikiBase):

[mediawiki/extensions/Math@master] Cache math parse results, T371972

https://gerrit.wikimedia.org/r/1060757

Change #1060758 had a related patch set uploaded (by WgevaertWikiBase; author: WgevaertWikiBase):

[mediawiki/extensions/Math@REL1_39] Cache math parse results, T371972

https://gerrit.wikimedia.org/r/1060758

I tested the 1_39 patch I uploaded in gerrit, it seems to fix the issue.

Ok, I see that this works. However, I think it would be better if the afterTidy callback was called only once. At least that was the intention by choosing that callback initially. A callback that is executed directly prior to printing out to HTML.
I am a bit worried that this might have unintended side effects in production, so I would like to have someone from the Wikimedia Foundation a review this. Git blame indicates that @cscott @tstarling and @matmarex might be good candidates.

However, I think it would be better if the afterTidy callback was called only once. At least that was the intention by choosing that callback initially. A callback that is executed directly prior to printing out to HTML.

I think afterTidy is the only reasonable hook to use if you want something "executed directly prior to printing out to HTML". If you hook in at a later time, you cannot guarantee that you are executed every time HTML is output. Because pages are not necessarily build up of a single parse (Usually, several parsed pieces of HTML are combined by a skin into one outputpage, even all i18n messages get parsed as well), there's no preventing you having to hook in after every HTML result.

Change #1060757 had a related patch set uploaded (by Pppery; author: WgevaertWikiBase):

[mediawiki/extensions/Math@master] Cache math parse results, prevent re-rendering the same formula

https://gerrit.wikimedia.org/r/1060757

Change #1060758 had a related patch set uploaded (by WgevaertWikiBase; author: WgevaertWikiBase):

[mediawiki/extensions/Math@REL1_39] Cache math parse results, prevent re-rendering the same formula

https://gerrit.wikimedia.org/r/1060758

Change #1060756 had a related patch set uploaded (by WgevaertWikiBase; author: WgevaertWikiBase):

[mediawiki/extensions/Math@REL1_42] Cache math parse results, prevent re-rendering the same formula

https://gerrit.wikimedia.org/r/1060756

Change #1060756 abandoned by Reedy:

[mediawiki/extensions/Math@REL1_42] Cache math parse results, prevent re-rendering the same formula

https://gerrit.wikimedia.org/r/1060756

Physikerwelt triaged this task as Medium priority.
Physikerwelt updated Other Assignee, added: Physikerwelt.

Change #1060757 merged by jenkins-bot:

[mediawiki/extensions/Math@master] Avoid redundant rendering under repeated ParserAfterTidy calls

https://gerrit.wikimedia.org/r/1060757

MeneerWout added a subscriber: Wgevaert.

I saw you assigned to Wgevaert, which is my old account from before I worked at Wikibase Solutions BV. Is there any action from me required on this?

@Wgevaert, yes please test if everything works as expected on the latest master. If everything works, follow https://www.mediawiki.org/wiki/Backporting_fixes if you need to have the fix in an old branch. I would only backport it to branches you specifically need.

PS: I feel sorry for accidentally messing up https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Math/+/1060758

Change #1060758 abandoned by Reedy:

[mediawiki/extensions/Math@REL1_39] Avoid redundant rendering under repeated ParserAfterTidy calls

Reason:

REL1_39 is EOL - T403199

https://gerrit.wikimedia.org/r/1060758