Page MenuHomePhabricator

incubatorwiki: PHP Fatal Error from line 143 of /srv/mediawiki/php-1.33.0-wmf.6/extensions/GeoCrumbs/GeoCrumbs.class.php: Argument 1 passed to GeoCrumbs::getParentRegion() must be an instance of ParserOutput, bool given
Closed, ResolvedPublic

Description

Error

Request ID: W--yowpAAEIAAJfXIUYAAABH

message
PHP Fatal Error: Argument 1 passed to GeoCrumbs::getParentRegion() must be an instance of ParserOutput, bool given
trace
#0 /srv/mediawiki/php-1.33.0-wmf.6/extensions/GeoCrumbs/GeoCrumbs.class.php(143): NO_FUNCTION_GIVEN()
#1 /srv/mediawiki/php-1.33.0-wmf.6/extensions/GeoCrumbs/GeoCrumbs.class.php(79): GeoCrumbs::makeTrail(Title, boolean)
#2 /srv/mediawiki/php-1.33.0-wmf.6/includes/Hooks.php(174): GeoCrumbs::onOutputPageParserOutput(OutputPage, ParserOutput)
#3 /srv/mediawiki/php-1.33.0-wmf.6/includes/Hooks.php(234): Hooks::callHook(string, array, array, NULL, string)
#4 /srv/mediawiki/php-1.33.0-wmf.6/includes/OutputPage.php(2018): Hooks::runWithoutAbort(string, array)
#5 /srv/mediawiki/php-1.33.0-wmf.6/includes/OutputPage.php(2069): OutputPage->addParserOutputMetadata(ParserOutput)
#6 /srv/mediawiki/php-1.33.0-wmf.6/includes/page/Article.php(698): OutputPage->addParserOutput(ParserOutput, array)
#7 /srv/mediawiki/php-1.33.0-wmf.6/includes/actions/ViewAction.php(68): Article->view()
#8 /srv/mediawiki/php-1.33.0-wmf.6/includes/MediaWiki.php(501): ViewAction->show()
#9 /srv/mediawiki/php-1.33.0-wmf.6/includes/MediaWiki.php(294): MediaWiki->performAction(Article, Title)
#10 /srv/mediawiki/php-1.33.0-wmf.6/includes/MediaWiki.php(862): MediaWiki->performRequest()
#11 /srv/mediawiki/php-1.33.0-wmf.6/includes/MediaWiki.php(517): MediaWiki->main()
#12 /srv/mediawiki/php-1.33.0-wmf.6/index.php(42): MediaWiki->run()
#13 /srv/mediawiki/w/index.php(3): include(string)
#14 {main}

Impact

Notes

Apparently it happened solely on incubatorwiki

Details

Related Gerrit Patches:
mediawiki/extensions/GeoCrumbs : masterAvoid fatals when PoolCounterArticleView fails

Event Timeline

hashar triaged this task as Unbreak Now! priority.Nov 29 2018, 2:53 PM
hashar created this task.
Restricted Application added subscribers: Liuxinyu970226, TerraCodes, Aklapper. · View Herald TranscriptNov 29 2018, 2:53 PM
greg added a subscriber: greg.

Adding the Editing-team per https://www.mediawiki.org/wiki/Developers/Maintainers.

Please investigate ASAP (or de-prioritize if not impactful).

While officially the stewards of that extension, in reality we have never touched it.

The code has not been touched in more than a year. It seems that the only commits are localisation updates and version bumps for linting. This issue is almost certainly not new in 1.33.0-wmf.6.

GeoCrumbs is probably not the culprit here. Likely the OutputPageParserOutput hook handler before it is messing with the $parserOutput variable.

The code has not been touched in more than a year. It seems that the only commits are localisation updates and version bumps for linting. This issue is almost certainly not new in 1.33.0-wmf.6.

I looked in logstash for message:"GeoCrumbs::getParentRegion" over 30 days and there is no occurrences before 1.33.0-wmf.6 deployment. Which seems to indicate we have a regression somewhere in MediaWiki core?

I have looked a bit at the code and trace, seems the ParserOutput is being passed to methods until it becomes a boolean somehow:

#0 /srv/mediawiki/php-1.33.0-wmf.6/extensions/GeoCrumbs/GeoCrumbs.class.php(143): NO_FUNCTION_GIVEN()
#1 /srv/mediawiki/php-1.33.0-wmf.6/extensions/GeoCrumbs/GeoCrumbs.class.php(79): GeoCrumbs::makeTrail(Title, boolean)
                                                                                                             ^^^^^^^
#2 /srv/mediawiki/php-1.33.0-wmf.6/includes/Hooks.php(174): GeoCrumbs::onOutputPageParserOutput(OutputPage, ParserOutput)
legoktm@mwmaint1002:~$ mwscript eval.php --wiki=incubatorwiki
> var_dump($wgHooks['OutputPageParserOutput']);
array(5) {
  [0]=>
  string(68) "\Wikibase\Client\Hooks\SidebarHookHandlers::onOutputPageParserOutput"
  [1]=>
  string(31) "CategoryTreeHooks::parserOutput"
  [2]=>
  string(45) "MobileFrontendHooks::onOutputPageParserOutput"
  [3]=>
  string(39) "GeoData\Hooks::onOutputPageParserOutput"
  [4]=>
  string(35) "GeoCrumbs::onOutputPageParserOutput"
}

So one of those?

matmarex added a comment.EditedNov 29 2018, 8:34 PM

I think the backtrace is incorrect. In reality, a ParserOutput is still being passed to GeoCrumbs::makeTrail(), but that function sets this variable to false, which must be somehow propagating to the generated backtrace. And the "NO_FUNCTION_GIVEN" thing should be GeoCrumbs::getParentRegion(). Here's an abridged version of that function:

	public static function makeTrail( Title $title, ParserOutput $parserOutput ) {
		...

		for ( $cnt = 0; $title && $cnt < 20; $cnt++ ) {
			if ( $parserOutput ) {
				$parserCache = $parserOutput;
				$parserOutput = false;
			} else {
				$parserCache = self::getParserCache( $title->getArticleID() );
			}
			...

			$title = self::getParentRegion( $parserCache );
		}

		...
	}

We definitely can set $parserCache to false and then pass it to getParentRegion(), causing the fatal. The code is buggy. But is has been buggy for months. No idea why it started fatalling now.

Anomie added a subscriber: Anomie.Nov 29 2018, 9:11 PM

I think the backtrace is incorrect. In reality, a ParserOutput is still being passed to GeoCrumbs::makeTrail(), but that function sets this variable to false, which must be somehow propagating to the generated backtrace.

PHP 7 does that, see https://secure.php.net/manual/en/migration70.incompatible.php#migration70.incompatible.other.func-parameter-modified. HHVM seems to do it too.

That PHP7 change is rather awkward but I guess there was a good reason for it and at lest it is documented. That still puzzled me for a while :-/

@matmarex proposed to wrap the code with if($parserCache). Then I have no idea what the code is actually doing nor what would be the right way to fix it up :/

Change 476621 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/extensions/GeoCrumbs@master] Avoid fatals when parent page doesn't exist

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

matmarex added a comment.EditedNov 29 2018, 9:31 PM

I also have no idea what it is doing. I just heard of this extension for the first time two hours ago :)

That patch should stop the fatal, but there might be some other issue here. $parserCache should only be false when the "parent" page doesn't exist. But in the examples I found in logstash, the page *does* exist.

The extension is also enabled on wikivoyage but I haven't seen any log for them.

'wmgUseGeoCrumbs' => [
    'default' => false,
    'incubatorwiki' => true, // T46725
    'wikivoyage' => true,
],

Also… this extension is only deployed on Incubator and Wikivoyages, which are both already on wmf.6. And the issue is no longer happening (logstash has the latest entry mentioning "GeoCrumbs::getParentRegion" at 2018-11-29T14:08:43). What release is this blocking?

Tracing through the code, it must be that getParserCache() is returning a boolean. I can't see any code path where $pageId would be <= 0, so that leaves WikiPage::newFromID() not finding the page or $page->getParserOutput() returning false.

Spot checking a few requests that hit this exception, they all seem to also have a message like Pool key 'incubatorwiki:pcache:idhash:31865-0!canonical!responsiveimages=0:revid:562825' (ArticleView): Timeout waiting for the lock. That log message will indeed result in $page->getParserOutput() returning false.

@Anomie Hm, neat. Thanks for investigating. Feel free to fix my commit message to be less wrong :)

I also have no idea what it is doing. I just heard of this extension for the first time two hours ago :)

Was also wondering. en:wv:Wikivoyage:Breadcrumb_navigation is the best I could find. mw:Extension:GeoCrumbs is rather unhelpful.

Also… this extension is only deployed on Incubator and Wikivoyages, which are both already on wmf.6. And the issue is no longer happening (logstash has the latest entry mentioning "GeoCrumbs::getParentRegion" at 2018-11-29T14:08:43). What release is this blocking?

"incubatorwiki": "php-1.33.0-wmf.6",
"enwikivoyage": "php-1.33.0-wmf.6",

Ah I am so sorry :-( Looks like I am only good at making everyone loose their time bah. So indeed it is no more a blocker.

Thanks for the investigation!!!

Change 476621 merged by jenkins-bot:
[mediawiki/extensions/GeoCrumbs@master] Avoid fatals when PoolCounterArticleView fails

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

matmarex closed this task as Resolved.Nov 30 2018, 12:34 AM
matmarex claimed this task.
matmarex removed a project: Patch-For-Review.
mmodell changed the subtype of this task from "Task" to "Production Error".Aug 28 2019, 11:07 PM