Last modified: 2013-04-22 16:16:23 UTC

Wikimedia Bugzilla is closed!

Wikimedia migrated from Bugzilla to Phabricator. Bug reports are handled in Wikimedia Phabricator.
This static website is read-only and for historical purposes. It is not possible to log in and except for displaying bug reports and their history, links might be broken. See T34368, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 32368 - MessageCaches Parser clone is easily messing with original
MessageCaches Parser clone is easily messing with original
Status: RESOLVED FIXED
Product: MediaWiki
Classification: Unclassified
Parser (Other open bugs)
1.20.x
All All
: Normal normal (vote)
: ---
Assigned To: Brad Jorsch
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-11 21:56 UTC by Daniel A. R. Werner
Modified: 2013-04-22 16:16 UTC (History)
3 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments

Description Daniel A. R. Werner 2011-11-11 21:56:13 UTC
I just had the weirdest problems when fixing Extension:Variables where one variables store should exist per Parser object.
I did this by using a new property from the outside (which is not bad practice for this kind of thing) $parser->mExtVariables = new ExtVariables()

Certain hooks will affect all instances of that store, for example ParserClearState will reset it entirely, practically something like $parser->mExtVariables->mVariables = array()

I just wasn't aware of the fact that there is a cloned Parser instance for the message cache which might call ParserClearState before the original Parser objecdt where the clone comes from.
This means all Changes I do in ParserClearState or any other hook for the cloned parser in $parser->mExtVariables->mVariables (where mExtVariables is an object as you can see) will affect the original Parser where the clone came from as well, since the clone is not a deep clone where all internal objects will be cloned as well.
So by resetting the variables store for the cloned object, I unwillingly do the same for the original.

Conclusion: The clone should be a new instance of the parser with same parser options or a deep clone perhaps?
Or does the clone actually rely on keeping track on object member variables changed in the original parser?

The cloning is done in MessageCache::getParser() since 1.18
Comment 1 Brad Jorsch 2012-05-01 03:36:07 UTC
This problem also affects wfMsg, wfMsgForContent, wfMsgHtml, wfMsgWikiHtml, wfMsgReal and wfMsgGetKey when $transform is true, and wfMsgExt when parse or parsemag or parseinline is passed, when the value of the message contains a template. We recently had a problem on enwiki where someone added a template to [[MediaWiki:Pfunc time error]] leading to Cite.php losing references if there was an error in a #time parser function.

The basic problem seems to be that MessageCache grabs the global parser object and then calls functions like parse and transformMsg on it rather than recursiveTagParse and recursivePreprocess. This particular problem seems like it could be avoided by having MessageCache gets its own private parser object, or by deep-cloning the global parser as Daniel Werner mentioned, or by having MessageCache call recursive-safe functions on the parser object. But I don't know if any of that would break uses of MessageCache outside of a page parse.
Comment 2 Brad Jorsch 2012-11-15 00:54:51 UTC
Patch for core to add a ParserCloned hook is in
Gerrit change #33506.

Patch for Cite to make use of this is in
Gerrit change #33508.

Patch for Scribunto (which throws an exception if the clone gets its state cleared) is in
Gerrit change #33507.
Comment 3 Alex Monk 2013-02-24 15:06:33 UTC
All three of those are merged. Is this bug fixed now?
Comment 4 Brad Jorsch 2013-02-24 23:00:02 UTC
Should be, yes.

Maybe bug 34589 too, if someone can reproduce that and test it.

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links