Page MenuHomePhabricator

Fatal when using FileImporter: "Call to a member function isOK() on boolean"
Open, HighPublic

Description

message
[XbTyEQpAIC4AACwueQ0AAABD] /w/index.php?title=File:Felicien_Rops,_Holocauste,_Naturalia_Non_Sunt_Turpia_(What_is_natural_is_not_dirty)_(1895)_heliogravure_retouched_with_drypoint_(21.6_x_15.2_cm)_Michael_C._Carlos_Museum,_Emory_University,_Atlanta.jpg&fileImporterSuccess=1   Error from line 51 of /srv/mediawiki/php-1.35.0-wmf.3/extensions/FileImporter/src/Html/ImportSuccessSnippet.php: Call to a member function isOK() on boolean
Impact

Users are unable to use the import files feature in some cases.

System responds with a generic error page (confusing to users).
Response code is HTTP 500, which cannot be cached.

Notes

https://logstash.wikimedia.org/goto/eedf69a62b595250251e90b51922a3de

  • The import appears to have been successful, but the error message was displayed instead of the file page with the success message.
  • The file was deleted on the source wiki.

Details

Request ID
XbTyEQpAIC4AACwueQ0AAABD
Request URL
https://commons.wikimedia.org/w/index.php?title=File:Felicien_Rops,_Holocauste,_Naturalia_Non_Sunt_Turpia_(What_is_natural_is_not_dirty)_(1895)_heliogravure_retouched_with_drypoint_(21.6_x_15.2_cm)_Michael_C._Carlos_Museum,_Emory_University,_Atlanta.jpg&fileImporterSuccess=1
Stack Trace
#0 /srv/mediawiki/php-1.35.0-wmf.3/extensions/FileImporter/src/FileImporterHooks.php(44): FileImporter\Html\ImportSuccessSnippet->getHtml(RequestContext, Title)
#1 /srv/mediawiki/php-1.35.0-wmf.3/includes/Hooks.php(174): FileImporter\FileImporterHooks::onBeforeInitialize(Title, NULL, OutputPage, User, WebRequest, MediaWiki)
#2 /srv/mediawiki/php-1.35.0-wmf.3/includes/Hooks.php(202): Hooks::callHook(string, array, array, NULL)
#3 /srv/mediawiki/php-1.35.0-wmf.3/includes/MediaWiki.php(188): Hooks::run(string, array)
#4 /srv/mediawiki/php-1.35.0-wmf.3/includes/MediaWiki.php(967): MediaWiki->performRequest()
#5 /srv/mediawiki/php-1.35.0-wmf.3/includes/MediaWiki.php(530): MediaWiki->main()
#6 /srv/mediawiki/php-1.35.0-wmf.3/index.php(44): MediaWiki->run()
#7 /srv/mediawiki/w/index.php(3): require(string)
#8 {main}
Related Gerrit Patches:
mediawiki/extensions/FileImporter : masterFix fatal and add logging to SuccessCache

Event Timeline

JJMC89 created this task.Oct 27 2019, 1:32 AM
Restricted Application added a project: TCB-Team. · View Herald TranscriptOct 27 2019, 1:32 AM
Daimona updated the task description. (Show Details)Oct 27 2019, 11:22 AM
Daimona edited Stack Trace. (Show Details)
Daimona set Phatality ID to 47c06ddb11f19a5b4df9bae25c4747dfe331e5ba3ff41e564c1a2090cd426c35.

Offending lines:

$importResult = $this->cache->fetchImportResult( $targetTitle );
if ( !$importResult->isOK() ) {

fetchImportResult is declared to @return StatusValue, but has no return type hint. In this case, it should have a typehint, although that would just shift the error to another place. Anyway, what the method does is:

return $this->cache->get( $this->makeCacheKey( $targetTitle ) );

So it can return false if no data is found in cache. I think this can happen for lots of reasons, mainly:

  1. No data was inserted or it has expired - The extension should guard against this. I don't know whether this can happen, so leaving up to maintainers;
  2. Wrong data was inserted (i.e. not a StatusValue) - This seems impossible: data is saved in stashImportResult, which has a StatusValue typehint.
  3. Different keys were used for set/get - If different $targetTitles are passed to the various methods in the SuccessCache class. Again, the extension should guard against this. If the SuccessCache class is only used for a Title at a time, it could be worth to make the Title a constructor argument to ensure consistency.
  4. Memcached/APC failure - There's not much you can do about this, although I think in this case it may error out as soon as set() fails.

Depending on whether the cached value is critical for the current operation, it could also have special cases for when get() doesn't return a StatusValue.

Krinkle renamed this task from FileImporter: Fatal exception after file import to Fatal when using FileImporter: "Call to a member function isOK() on boolean".Nov 7 2019, 8:16 PM
Krinkle updated the task description. (Show Details)Nov 7 2019, 8:19 PM
awight triaged this task as High priority.Nov 7 2019, 11:05 PM

I'm moving into the current workboard since this is a user-facing crash.

Change 549704 had a related patch set uploaded (by Thiemo Kreuz (WMDE); owner: Thiemo Kreuz (WMDE)):
[mediawiki/extensions/FileImporter@master] Fix fatal and add logging to SuccessCache

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

thiemowmde moved this task from Sprint Backlog to Review on the WMDE-QWERTY-Sprint-2019-11-06 board.

Change 549704 merged by jenkins-bot:
[mediawiki/extensions/FileImporter@master] Fix fatal and add logging to SuccessCache

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

@thiemowmde is this fixed and can the ticked be closed?

I would like to keep this open. Things to do:

  • Check the logs for the message "Failed to retrieve import result from".
  • Based on what we see we should decide on a new action, e.g. opening a new ticket.
  • Possibly remove the logging.