Page MenuHomePhabricator

FlaggedRevs causes unbounded growth of ParserCache objects
Closed, ResolvedPublic

Description

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.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptNov 15 2018, 1:19 AM

Change 473656 had a related patch set uploaded (by Mischanix; owner: Mischanix):
[mediawiki/extensions/FlaggedRevs@master] frontend: Save to the correct ParserCache

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

Change 473825 had a related patch set uploaded (by Umherirrender; owner: Mischanix):
[mediawiki/extensions/FlaggedRevs@REL1_31] frontend: Save to the correct ParserCache

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

Change 473826 had a related patch set uploaded (by Umherirrender; owner: Mischanix):
[mediawiki/extensions/FlaggedRevs@REL1_32] frontend: Save to the correct ParserCache

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

Umherirrender closed this task as Resolved.Nov 15 2018, 7:44 PM
Umherirrender assigned this task to Mischanix.
Umherirrender triaged this task as Normal priority.
Umherirrender added a subscriber: Umherirrender.

Nice catch. Thanks for the patch set

I have backported to REL1_31 and REL1_32, because the causing patch set is included there

Change 473656 merged by jenkins-bot:
[mediawiki/extensions/FlaggedRevs@master] frontend: Save to the correct ParserCache

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

Change 473825 merged by jenkins-bot:
[mediawiki/extensions/FlaggedRevs@REL1_31] frontend: Save to the correct ParserCache

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

Change 473826 merged by jenkins-bot:
[mediawiki/extensions/FlaggedRevs@REL1_32] frontend: Save to the correct ParserCache

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