Page MenuHomePhabricator

avoid Parser::parse recursion
Closed, DeclinedPublic

Description

This task is motivated by T75073. Recursive calls to Parser::parse are easy to unwittingly cause and can be hard to debug. Debug output should be improved (see recursion detection code in T75073) or even better Parser::parse should detect a recursive call and convert it into a recursion safe call (e.g. Parser::recursiveTagParse).

Event Timeline

Welterkj raised the priority of this task from to Needs Triage.
Welterkj updated the task description. (Show Details)
Welterkj added a project: MediaWiki-Core-Team.
Welterkj changed Security from none to None.
Welterkj subscribed.

recursiveTagsParse is not magically "safe". Calling it means that all kinds of metadata generated during parsing will be associated with the currently ongoing parse (link tables, RL modules, categories, interwikis, you name it) – and that parse might be unrelated to what you're doing, causing weird database inconsistencies.

@matmarex, thanks for the info about recursiveTagParse. The situation I'm grappling with is shown in the backtrace in T75073#763545. WikitextContent::getParserOutput called parser::parse in the midst of an ongoing parse due to the interoperation of two extensions (GraphViz and SMW). I'm fishing for a core improvement, if one can be found, because I don't love the solution I've written for GraphViz as described in T75073#798676 (I added you as a reviewer of that change in gerrit BTW).

Here are a couple other wild and probably dangerous ideas from someone naive about the Parser:

  1. Have Parser::parse detect if it is mid-parse then clone itself and call parse on the clone instead.
  2. The same as 1 but have WikitextContent detect if $wgParser is mid-parse and use a clone if so.

I'm happy to have these ideas shot down if they are bad.

Perhaps the best that can be done is for Parser::parse to detect recursion and throw an exception rather than waiting for the problem to manifest itself in a less predictable way as in T75073.

Welterkj renamed this task from Make Parser::parse tolerate recursion to Make Parser::parse handle recursion better (throw exception or tolerate it).Dec 9 2014, 10:16 PM
Welterkj updated the task description. (Show Details)

I vaguely recall that cloning $wgParser at the wrong time used to cause PHP to explode spectacularly, Brian might remember the details.

You can always create your own Parser instance – then you yourself control what calls parse() and when. MediaWiki itself does that for parsing all of the i18n messages that are shown on pages. (Check out the MessageCache class and its parse() method, and in particular getParser() – it actually does clone $wgParser sometimes.)

A trivial test using WikitextContent::getParser copied from MessageCache::getParser worked (replaced all instances of $wgParser in WikitextContent with getParser calls). It resolved the problem in T75073 and did not have any side effects I could see.

make phpunit FLAGS="--group Editing" passed all tests

Should I submit the code for review or is there some other due diligence I should do first?

I noticed that Parser::lock() now throws an exception in the case of Parser::parse recursion. So now my only interest is eliminating Parser::parse recursion that arises from WikitTextContent usage of wgParser.

I asked wikitech-l:
"Can anyone think of a reason *not* to eliminate wgParser usage from WikitextContent?"

@tstarling responded:
"Parser::firstCallInit() is quite slow, so it makes sense to cache Parser objects persistently. I haven't seen the bug, and phabricator is down right now, but if we really need to recreate parsers, I think we would have to split out parser::firstCallInit() to a separate, more persistent object."

Welterkj renamed this task from Make Parser::parse handle recursion better (throw exception or tolerate it) to avoid Parser::parse recursion.Dec 18 2014, 4:51 PM

@tstarling, could you review this bug when you have a moment and share any more thoughts about refactoring the Parser to avoid the recursion seen in T75073#763545?

The correct way to avoid this problem these days is to use MediaWikiServices::getInstance()->getParserFactory()->getInstance() (added in MW 1.39), which will always give you a Parser instance on which you can safely use parse().