Page MenuHomePhabricator

PHP Fatal error: Uncaught Error: Call to a member function canExist() on null
Closed, ResolvedPublic

Description

MediaWiki	1.29.0-rc.0 (4e4d48d)
PHP	7.1.1 (apache2handler)
MariaDB	10.1.21-MariaDB
ICU	57.1
PHP Fatal error:  Uncaught Error: Call to a member function canExist() on null in ...\includes\skins\Skin.php:223
Stack trace:
#0 ...\includes\skins\Skin.php(145): Skin->preloadExistence()
#1 ...\skins\Vector\SkinVector.php(47): Skin->initPage(Object(OutputPage))
#2 ...\includes\skins\SkinTemplate.php(248): SkinVector->initPage(Object(OutputPage))
#3 ...\includes\OutputPage.php(2441): SkinTemplate->outputPage()
#4 ...\includes\exception\MWExceptionRenderer.php(181): OutputPage->output()
#5 ...\includes\exception\MWExceptionRenderer.php(55): MWExceptionRenderer::reportHTML(Object(Error))
#6 ...\includes\exception\MWExceptionHandler.php(75): MWExceptionRenderer::output(Object(Error), 2)
#7 ...\includes\exception\MWExceptionHandler.php(141): MWExceptionHandler::report(Object(Error))
#8 [internal function]: MWExceptionHandler::handleException(Ob in ...\includes\skins\Skin.php on line 223

Checking the $title should be enough to prevent the fatal.

@@ -218,11 +218,11 @@ abstract class Skin extends ContextSource {
 			$titles[] = $user->getTalkPage();
 		}
 
 		// Check, if the page can hold some kind of content, otherwise do nothing
 		$title = $this->getRelevantTitle();
-		if ( $title->canExist() ) {
+		if ( $title !== null && $title->canExist() ) {

Event Timeline

mwjames created this task.Jun 19 2017, 8:30 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 19 2017, 8:30 PM

lol, I've encountered exactly the same issue and I was reporting it. I don't know how did you get this exception, but mine is as follows (may be worth a different report?) and probably needs a different fix:

Since mediawiki-1.28, when a fatal exception occurs running a maintenance script, another exception occurs that prevents the display of the original error:

PHP Fatal error: Call to a member function canExist() on null in /home/www/lib/mediawiki-1.29.0-rc.0/includes/skins/Skin.php on line 223

For example, trying to import images when the upload directory is not writeable by the user running the script:

/home/www/lib/mediawiki-1.29.0-rc.0> php maintenance/importImages.php --search-recursively /some/path
Import Images

Importing Testimage.png...PHP Fatal error:  Call to a member function canExist() on null in /home/www/lib/mediawiki-1.29.0-rc.0/includes/skins/Skin.php on line 223

In this case, the exception handler shouldn't call a skin at all if we're in a maintenance script!

Needs backports to 1.29 and 1.28 :)

@demon, @matmarex It doesn't fix it in MediaWiki 1.29 at least (for errors in maintenance scripts). MWExceptionRenderer isn't even called at all:

#0 /home/www/lib/mediawiki-1.29.0-rc.0/includes/skins/Skin.php(145): Skin->preloadExistence()
#1 /home/www/lib/mediawiki-1.29.0-rc.0/skins/Vector/SkinVector.php(47): Skin->initPage(Object(OutputPage))
#2 /home/www/lib/mediawiki-1.29.0-rc.0/includes/skins/SkinTemplate.php(248): SkinVector->initPage(Object(OutputPage))
#3 /home/www/lib/mediawiki-1.29.0-rc.0/includes/OutputPage.php(2441): SkinTemplate->outputPage()
#4 /home/www/lib/mediawiki-1.29.0-rc.0/includes/exception/ErrorPageError.php(67): OutputPage->output()
#5 /home/www/lib/mediawiki-1.29.0-rc.0/includes/filerepo/file/LocalFile.php(3172): ErrorPageError->report()
#6 /home/www/lib/mediawiki-1.29.0-rc.0/includes/exception/MWExceptionHandler.php(73): LocalFileLockError->report()
#7 /home/www/lib/mediawiki-1.29.0-rc.0/includes/exception/MWExceptionHandler.php(141): MWExceptionHandler::report(Object(LocalFileLockError))
#8 [internal function]: MWExceptionHandler::handleException(Object(LocalFileLockError))
#9 {main}
matmarex added a subscriber: bd808.Jun 21 2017, 8:26 PM
demon added a comment.Jun 22 2017, 7:42 PM

This should be fixed after f8b8d2689d57042401d7b25cac29a45ea77e58c4.

How is this patch supposed to do anything? RequestContext::getMain()->getTitle() will still return a title even in maintenance contexts. What we should probably do is avoid using OutputPage when we're in maintenance scripts.

RequestContext::getMain()->getTitle() clearly does not return a Title, otherwise we would not be getting the fatal in the title of this task?

demon added a comment.Jun 22 2017, 8:47 PM

I mean not always, but it does most of the time I believe. In any case, we should still stop using OutputPage in maintenance scripts anyway.

bd808 removed a subscriber: bd808.Jun 23 2017, 4:43 AM

I guess the problem is not on all maintenance scripts, but on specific errors thrown by maintenance scripts (and maybe other errors during web request depending on several factors)

My stack trace has the key:

class LocalFileLockError extends ErrorPageError

ErrorPageError has:

	public function report() {
		global $wgOut;

		$wgOut->showErrorPage( $this->title, $this->msg, $this->params );
		$wgOut->output();
	}

The weird thing is that ErrorPageError constructor has this parameters: $title, $msg, $params = [], but the LocalFileLockError exception is thrown as: throw new LocalFileLockError( $status );, which looks incompatible...

demon added a comment.EditedJun 30 2017, 8:59 PM

It's not incompatible, as the child constructor calls the parents with a title and msg (no params). That's fine. The problem is that ErrorPageError (and children) assume that they can use OutputPage, which is clearly not always the case. What we really want is the code path that's throwing the original exception so we can either clean up what's busted with OutputPage or change the exception type.

demon added a comment.EditedJun 30 2017, 9:00 PM

Tbh: I'm not sure why LocalFile (which is a deep internal class) would assume we can display an ErrorPageError. Seems like a bad superclass choice...

Tgr added a subscriber: Tgr.Jul 4 2017, 6:37 PM

Expose MWExceptionRenderer::useOutputPage and check it in MWExceptionHandler::report before passing control over rendering to the exception object?
(In the longer term, having the exception object do the rendering instead of just asking it for an error message, title etc is quite crazy. I think I have a half-finished patch somewhere for turning that around...)

demon added a comment.Jul 6 2017, 7:32 PM

But why is MWExceptionRenderer::output() not bailing earlier? MWExceptionRenderer::isCommandLine()should return true...we should never even be hitting the reportHTML() code path.

isCommandLine() is fairly naive. All it does is check that $wgCommandLineMode isn't empty. That seems fragile (I've always hated that global anyway).

Change 363660 had a related patch set uploaded (by Chad; owner: Chad):
[mediawiki/core@master] WIP: Fix ErrorPageError to work from non-web contexts

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

Change 363665 had a related patch set uploaded (by Chad; owner: Chad):
[mediawiki/core@REL1_29] Fix/hack ErrorPageError to work from non-UI contexts

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

Change 363660 merged by jenkins-bot:
[mediawiki/core@master] Fix/hack ErrorPageError to work from non-UI contexts

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

Change 363665 merged by jenkins-bot:
[mediawiki/core@REL1_29] Fix/hack ErrorPageError to work from non-UI contexts

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

demon closed this task as Resolved.Jul 6 2017, 8:35 PM
demon claimed this task.

Change 374038 had a related patch set uploaded (by MarkAHershberger; owner: Chad):
[mediawiki/core@REL1_28] Fix/hack ErrorPageError to work from non-UI contexts

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

Change 374038 merged by jenkins-bot:
[mediawiki/core@REL1_28] Fix/hack ErrorPageError to work from non-UI contexts

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