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.

In T210734#4786809, @matmarex wrote:

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?

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.

In T210734#4786952, @matmarex wrote:

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

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 :)

In T210734#4787221, @matmarex wrote:

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.

In T210734#4787225, @matmarex wrote:

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