Page MenuHomePhabricator

Some ApiQueryImageInfo queries consistently fail with a fatal BadMethodCallException from LocalFile.php
Closed, ResolvedPublicPRODUCTION ERROR

Description

There are 278 BadMethodCallExceptions reported in our Logstash. They take the following form:

Example
t  exception.class	       	BadMethodCallException
t  exception.code	       	0
t  exception.file	       	/srv/mediawiki/php-1.34.0-wmf.1/includes/filerepo/file/LocalFile.php:890
t  exception.message	       	Call to a member function getName() on a non-object (null)
t  exception.trace	       	#0 /srv/mediawiki/php-1.34.0-wmf.1/includes/api/ApiQueryImageInfo.php(416): LocalFile->getUser()
#1 /srv/mediawiki/php-1.34.0-wmf.1/includes/api/ApiQueryImageInfo.php(183): ApiQueryImageInfo::getInfo(LocalFile, array, ApiResult, NULL, array)
#2 /srv/mediawiki/php-1.34.0-wmf.1/includes/api/ApiQuery.php(249): ApiQueryImageInfo->execute()
#3 /srv/mediawiki/php-1.34.0-wmf.1/includes/api/ApiMain.php(1593): ApiQuery->execute()
#4 /srv/mediawiki/php-1.34.0-wmf.1/includes/api/ApiMain.php(531): ApiMain->executeAction()
#5 /srv/mediawiki/php-1.34.0-wmf.1/includes/api/ApiMain.php(502): ApiMain->executeActionWithErrorHandling()
#6 /srv/mediawiki/php-1.34.0-wmf.1/api.php(87): ApiMain->execute()
#7 /srv/mediawiki/w/api.php(3): include(string)
#8 {main}
t  exception_id	       	XL8QbgpAIDsAACePKCgAAACV
t  exception_url	       	/w/api.php?action=query&format=json&prop=imageinfo|pageimages&piprop=name|thumbnail&iiprop=timestamp|user|url|userid|canonicaltitle|size|dimensions|mime|thumbmime|extmetadata|archivename|uploadwarning|badfile&titles=Image:%E4%B8%AD%E5%9B%BD&pilicense=free&pithumbsize=600
t  message	       	[XL8QbgpAIDsAACePKCgAAACV] /w/api.php?action=query&format=json&prop=imageinfo|pageimages&piprop=name|thumbnail&iiprop=timestamp|user|url|userid|canonicaltitle|size|dimensions|mime|thumbmime|extmetadata|archivename|uploadwarning|badfile&titles=Image:%E4%B8%AD%E5%9B%BD&pilicense=free&pithumbsize=600   BadMethodCallException from line 890 of /srv/mediawiki/php-1.34.0-wmf.1/includes/filerepo/file/LocalFile.php: Call to a member function getName() on a non-object (null)

The source appears to be incoherent $type and $user state in LocalFile.php:

LocalFile.php:890
	/**
	 * Returns user who uploaded the file
	 *
	 * @param string $type 'text', 'id', or 'object'
	 * @return int|string|User
	 * @since 1.31 Added 'object'
	 */
	function getUser( $type = 'text' ) {
		$this->load();

		if ( $type === 'object' ) {
			return $this->user;
		} elseif ( $type === 'text' ) {
			return $this->user->getName();
		} elseif ( $type === 'id' ) {
			return $this->user->getId();
		}

		throw new MWException( "Unknown type '$type'." );
	}

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptApr 24 2019, 11:33 PM

This seems to be about the MediaWiki core codebase, hence adding project tag.

Restricted Application added projects: Commons, Multimedia. · View Herald TranscriptApr 25 2019, 9:15 AM
Tgr added a subscriber: Tgr.Apr 25 2019, 5:07 PM

LocalFile::getUser() does not handle the case when the file doesn't exist (which is the only way for LocalFile::$user to be unset after calling load()). uploadwarning and/or badfile will force the API to return information about non-existent files as well. So either getUser() should handle this case (I'd expect it's not the only method that doesn't though; there's also some semantic drift since LocalFile can represent non-existing files for the purpose of uploading and handling file pages for missing files, while the other File classes always represent an existing file) or it should document not being able to and ApiQueryImageInfo should take that into account.

Krinkle renamed this task from [Bug] BadMethodCallException in LocalFile to Some ApiQueryImageInfo queries consistently fail with a fatal BadMethodCallException from LocalFile.php.May 6 2019, 6:17 PM

Change 509139 had a related patch set uploaded (by Umherirrender; owner: Umherirrender):
[mediawiki/core@master] Use a temp repo to handle api's imageinfo for uploadwarning or badfile

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

Change 509139 abandoned by Umherirrender:
Use a temp repo to handle api's imageinfo for uploadwarning or badfile

Reason:
I have no idea to solve the issue on another way

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

Krinkle updated the task description. (Show Details)Jul 1 2019, 4:35 PM
Tgr raised the priority of this task from Medium to Unbreak Now!.Jul 17 2019, 6:35 PM
Tgr added a subscriber: LarsWirzenius.
Restricted Application added a subscriber: Liuxinyu970226. · View Herald TranscriptJul 17 2019, 6:37 PM
WDoranWMF lowered the priority of this task from Unbreak Now! to Medium.Jul 17 2019, 8:19 PM
greg raised the priority of this task from Medium to Unbreak Now!.Jul 17 2019, 9:41 PM
greg added a subscriber: greg.

This was reported as a train blocker, setting to UBN!

Options include:

  • reasoning why it really shouldn't be a blocker
  • fixing it :)
Tgr added a comment.Jul 17 2019, 9:55 PM

It has been around for months so not much point in blocking the train on it right now. OTOH breaking imageinfo API queries any time a non-existent page is included in the query might merit an UBN, even if the parameter combination triggering the error (either uploadwarning or badfile needs to be included in iiprop) does not seem to be used much currently.

awight added a subscriber: awight.Jul 18 2019, 10:12 AM

+1 echoing tgr, this shouldn't be a blocker. Either way, it looks like second priority after the upload bug, T228292.

So, your bewildered train personnel here. If I understand the reasoning above correctly, this is sufficiently old that it's not blocking this week's train, right? So I'll drop it as a blocker? I'm going to do that when the deployment window starts, unless someone objects before that. Thank you!

@LarsWirzenius Hi, yes I think it's safe because an old bug, for example https://phabricator.wikimedia.org/phame/post/view/162/production_excellence_may_2019/ talks about this as an outstanding issue.

I should add, I'm assuming it's safe to continue with the train because this bug appeared in both wmf.13 and wmf.14. I don't see any comments suggesting there's a new interaction exacerbating the bug.

Thanks, removing as blocker for this week's train.

WDoranWMF lowered the priority of this task from Unbreak Now! to High.Jul 18 2019, 4:30 PM
mmodell changed the subtype of this task from "Task" to "Production Error".Aug 28 2019, 11:07 PM
daniel added a subscriber: daniel.Aug 29 2019, 4:54 PM

Hm, since this is now an "Error", I can't change the prio any more.

This doesn't seem to be "high", more like "normal". Does it still happen?

I'm unclear on the root cause. Could someone explain what exactly needs to happen to fix this?

daniel lowered the priority of this task from High to Medium.Aug 30 2019, 8:27 AM
daniel changed the subtype of this task from "Production Error" to "Task".
daniel changed the subtype of this task from "Task" to "Production Error".

Chaged prio to "normal". Should be fixed per Tgr's comment at T221812#5138224.

Change 533482 had a related patch set uploaded (by Daniel Kinzler; owner: Daniel Kinzler):
[mediawiki/core@master] LocalFile: avoid hard failures on non-existing files.

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

Anomie added a subscriber: Anomie.

Moving back to "Doing", patch has unaddressed review comments.

Jony added a subscriber: Jony.Sep 17 2019, 5:30 AM
mobrovac closed this task as Resolved.Sep 18 2019, 9:28 AM
mobrovac assigned this task to daniel.
mobrovac moved this task from Doing to Done on the Platform Team Workboards (Clinic Duty Team) board.

Change 533482 merged by jenkins-bot:
[mediawiki/core@master] LocalFile: avoid hard failures on non-existing files.

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

Aklapper removed a subscriber: Anomie.Oct 16 2020, 5:41 PM