Page MenuHomePhabricator

Parser::braceSubstitution() recreates new RequestContext and calls RequestContextCreateSkin twice
Open, MediumPublic

Description

On the MainPage RequestContextCreateSkin hook is called twice. It's because Parser::braceSubstitution() creates a new RequestContext instance (https://github.com/wikimedia/mediawiki/blob/master/includes/parser/Parser.php#L3202). Minerva hook onSpecialPageBeforeExecute try to get a skin which is not created as it operates on newly created RequestContext. It doesn't have side effects, my concerns are around performance as execute lots of uncesseary code.

Trace:

First Call

#0 /vagrant/mediawiki/includes/Hooks.php(186): MobileFrontendHooks::onRequestContextCreateSkin()
#1 /vagrant/mediawiki/includes/context/RequestContext.php(406): Hooks::run()
#2 /vagrant/mediawiki/includes/context/ContextSource.php(154): RequestContext->getSkin()
#3 /vagrant/mediawiki/extensions/MobileFrontend/includes/Minerva.hooks.php(30): ContextSource->getSkin()
#4 /vagrant/mediawiki/includes/Hooks.php(186): MinervaHooks::onSpecialPageBeforeExecute()
#5 /vagrant/mediawiki/includes/specialpage/SpecialPage.php(515): Hooks::run()
#6 /vagrant/mediawiki/includes/specialpage/SpecialPageFactory.php(578): SpecialPage->run()
#7 /vagrant/mediawiki/includes/specialpage/SpecialPageFactory.php(633): SpecialPageFactory::executePath()
#8 /vagrant/mediawiki/includes/parser/Parser.php(3213): SpecialPageFactory::capturePath()
#9 /vagrant/mediawiki/includes/parser/Preprocessor_Hash.php(1039): Parser->braceSubstitution()
#10 /vagrant/mediawiki/includes/parser/Preprocessor_Hash.php(1472): PPFrame_Hash->expand()
#11 /vagrant/mediawiki/includes/parser/Parser.php(3287): PPTemplateFrame_Hash->cachedExpand()
#12 /vagrant/mediawiki/includes/parser/Preprocessor_Hash.php(1039): Parser->braceSubstitution()
#13 /vagrant/mediawiki/includes/parser/Parser.php(2950): PPFrame_Hash->expand()
#14 /vagrant/mediawiki/includes/parser/Parser.php(1305): Parser->replaceVariables()
#15 /vagrant/mediawiki/includes/parser/Parser.php(451): Parser->internalParse()
#16 /vagrant/mediawiki/includes/content/WikitextContent.php(330): Parser->parse()
#17 /vagrant/mediawiki/includes/content/AbstractContent.php(497): WikitextContent->fillParserOutput()
#18 /vagrant/mediawiki/includes/poolcounter/PoolWorkArticleView.php(140): AbstractContent->getParserOutput()
#19 /vagrant/mediawiki/includes/poolcounter/PoolCounterWork.php(123): PoolWorkArticleView->doWork()
#20 /vagrant/mediawiki/includes/page/Article.php(586): PoolCounterWork->execute()
#21 /vagrant/mediawiki/includes/actions/ViewAction.php(68): Article->view()
#22 /vagrant/mediawiki/includes/MediaWiki.php(499): ViewAction->show()
#23 /vagrant/mediawiki/includes/MediaWiki.php(293): MediaWiki->performAction()
#24 /vagrant/mediawiki/includes/MediaWiki.php(862): MediaWiki->performRequest()
#25 /vagrant/mediawiki/includes/MediaWiki.php(523): MediaWiki->main()
#26 /vagrant/mediawiki/index.php(43): MediaWiki->run()
#27 /var/www/w/index.php(5): include()
#28 {main}

Second call

#0 /vagrant/mediawiki/includes/Hooks.php(186): MobileFrontendHooks::onRequestContextCreateSkin()
#1 /vagrant/mediawiki/includes/context/RequestContext.php(406): Hooks::run()
#2 /vagrant/mediawiki/includes/context/ContextSource.php(154): RequestContext->getSkin()
#3 /vagrant/mediawiki/includes/parser/ParserOutput.php(267): ContextSource->getSkin()
#4 /vagrant/mediawiki/includes/parser/ParserOutput.php(275): Closure$ParserOutput::getText()
#5 /vagrant/mediawiki/includes/OutputPage.php(1879): ParserOutput->getText()
#6 /vagrant/mediawiki/includes/OutputPage.php(1900): OutputPage->addParserOutputText()
#7 /vagrant/mediawiki/includes/page/Article.php(601): OutputPage->addParserOutput()
#8 /vagrant/mediawiki/includes/actions/ViewAction.php(68): Article->view()
#9 /vagrant/mediawiki/includes/MediaWiki.php(499): ViewAction->show()
#10 /vagrant/mediawiki/includes/MediaWiki.php(293): MediaWiki->performAction()
#11 /vagrant/mediawiki/includes/MediaWiki.php(862): MediaWiki->performRequest()
#12 /vagrant/mediawiki/includes/MediaWiki.php(523): MediaWiki->main()
#13 /vagrant/mediawiki/index.php(43): MediaWiki->run()
#14 /var/www/w/index.php(5): include()
#15 {main}

Event Timeline

Jdlrobson moved this task from Incoming to Needs Prioritization on the Web-Team-Backlog board.
Jdlrobson added a subscriber: Jdlrobson.

Is this just the main page or all pages?
Do you have a proposal for how to avoid this?

@Jdlrobson it's happening on every Special Page which uses brace substitution. Every substitution ({{PAGE_NAME}}) creates a new context, which creates new skin. Pages in the MAIN namespace are not affected.

Right now I have no idea how to fix it, the only thing that comes to my mind is to initialize Skin earlier, and pass it set it it newly created RequestContext inside Parser::braceSubstitution() method.

Jdlrobson triaged this task as Medium priority.May 16 2017, 10:26 AM
Jdlrobson changed the task status from Open to Stalled.Jun 20 2017, 10:40 PM

@pmiazga do you feel this is an upstream issue or one in MobileFrontend. Any idea how to make this actionable?

TL;DR; Root cause: SpecialPageBeforeExecute hook uses Skin object. This hook should be stateless - no skin/no request checks as it might be fired during brace substitution.
IMHO it's upstream as it is not possible to fix that in MobileFrontend/Minerva

  • Parser is trying to create new Context for every brace substitution and then SpecialPageBeforeExecute is fired
  • Minerva gets information that something is trying to render a SpecialPage - it triggers SpecialPage rendering logic. In this scenario, Minerva doesn't know is it a regular page render or brace substitution, onSpecialPageBeforeExecute tries to get the skin to verify is mobile view enabled (Minerva is a current skin)

If there is a way to detect mobile mode without using Skin object we can quickly fix it.

@ssastry, @Legoktm - could you shed some light, why does Parser create a new Context when it renders special page inside the article (brace substitution call)?

Jdlrobson moved this task from Needs Prioritization to 2014-15 Q4 on the Web-Team-Backlog board.

Maybe this is something the platform team have some views on?

  • Parser is trying to create new Context for every brace substitution and then SpecialPageBeforeExecute is fired

It's not "every brace substitution", only transclusion of special pages.

In this scenario, Minerva doesn't know is it a regular page render or brace substitution,

It could check $special->including() to determine that.

why does Parser create a new Context when it renders special page inside the article (brace substitution call)?

Special pages were designed to be called to generate web UI. The most straightforward way to get special page transclusion to work was presumably to fake up the request with an overridden title, request, and so on so the existing special pages didn't have to change much.

I don't know if there's a reason it creates a new RequestContext instead of making a DerivativeContext, although it might be that some special pages mess with RequestContext state somehow in a way that shouldn't be messed with during transclusion, similar to the old bug where transcluding the wrong special page would override the page title with the special page's title.

Jdlrobson renamed this task from Parser::braceSubstitution() recreates the Minerva skin to Parser::braceSubstitution() recreates whatever is the mobile skin (eg. Minerva skin).Jul 13 2017, 5:50 PM
Jdlrobson renamed this task from Parser::braceSubstitution() recreates whatever is the mobile skin (eg. Minerva skin) to Parser::braceSubstitution() recreates new RequestContext and calls RequestContextCreateSkin twice.
Jdlrobson removed a project: MobileFrontend.
Aklapper changed the task status from Stalled to Open.May 21 2020, 12:40 PM

Question answered hence resetting stalled status.