As part of Gamepedia's upgrade from 1.29 to 1.31, we found a weird behavior where a new <!-- Saved in parser cache with key ... text was being appended to the cached parser output on every fresh page load by a logged-out user for pages on lol.gamepedia.com.
To track down the cause of this, I added a hacky check to verify my idea of the precondition in ParserCache::save that $parserOutput->mText not have that particular comment in it:
diff --git a/includes/parser/ParserCache.php b/includes/parser/ParserCache.php index 8a7fca6c29..eafc7f4576 100644 --- a/includes/parser/ParserCache.php +++ b/includes/parser/ParserCache.php @@ -323,6 +323,10 @@ class ParserCache { " and revision id $revId" . "\n"; + if ( strpos( $parserOutput->mText, "<!-- Saved in parser cache with key" ) !== false ) { + throw new Exception( 'ParserCache::save() was called with output that included a debug comment' ); + } + $parserOutput->mText .= "\n<!-- $msg -->\n"; wfDebug( $msg );
This resulted in a stack trace which pointed to extensions/FlaggedRevs/frontend/FlaggablePageView.php:763, where $parserCache->save( $parserOut, $this->article, $pOpts ); is called.
Looking at blame, I believe this $parserCache was intended to be the FRParserCacheStable instance created above the if block, not the main ParserCache instance, as the diff clearly redefines $parserCache without any explanation.
I supposed something like the following patch should suffice to fix the issue:
diff --git a/extensions/FlaggedRevs/frontend/FlaggablePageView.php b/extensions/FlaggedRevs/frontend/FlaggablePageView.php index a6d7904588..5d1d952392 100755 --- a/extensions/FlaggedRevs/frontend/FlaggablePageView.php +++ b/extensions/FlaggedRevs/frontend/FlaggablePageView.php @@ -739,9 +739,9 @@ class FlaggablePageView extends ContextSource { $parserOut = false; # Get the new stable parser output... if ( FlaggedRevs::inclusionSetting() == FR_INCLUDES_CURRENT && $synced ) { - $parserCache = MediaWikiServices::getInstance()->getParserCache(); + $mainParserCache = MediaWikiServices::getInstance()->getParserCache(); # We can try the current version cache, since they are the same revision - $parserOut = $parserCache->get( $this->article, $pOpts ); + $parserOut = $mainParserCache->get( $this->article, $pOpts ); } if ( !$parserOut ) {
It would also perhaps be really nice if ParserCache did some verification that it wasn't causing this sort of unbounded growth due to the save API being misused, but that's definitely a tougher ask.
As a fun statistic, the largest page served by this bug was inflated from 9.6KB to 6.2MB and that page alone wasted about a day of users' browser load time in the brief time it was inflated.