Page MenuHomePhabricator

"User::loadFromSession called before the end of Setup.php" (violation by Wikibase/ULS) [Story Points 5]
Closed, ResolvedPublic

Description

Error message
User::loadFromSession called before the end of Setup.php
exception.trace
#0 /srv/mediawiki/php-1.35.0-wmf.8/includes/user/User.php(2264): User->load()
#1 /srv/mediawiki/php-1.35.0-wmf.8/includes/user/User.php(3569): User->getId()
#2 /srv/mediawiki/php-1.35.0-wmf.8/includes/user/User.php(3585): User->isRegistered()
#3 /srv/mediawiki/php-1.35.0-wmf.8/extensions/UniversalLanguageSelector/includes/UniversalLanguageSelectorHooks.php(247): User->isAnon()
#4 /srv/mediawiki/php-1.35.0-wmf.8/includes/Hooks.php(174): UniversalLanguageSelectorHooks::getLanguage(User, string, RequestContext)
#5 /srv/mediawiki/php-1.35.0-wmf.8/includes/Hooks.php(202): Hooks::callHook(string, array, array, NULL)
#6 /srv/mediawiki/php-1.35.0-wmf.8/includes/context/RequestContext.php(361): Hooks::run(string, array)
#7 /srv/mediawiki/php-1.35.0-wmf.8/includes/StubUserLang.php(52): RequestContext->getLanguage()
#8 /srv/mediawiki/php-1.35.0-wmf.8/includes/StubObject.php(168): StubUserLang->_newObject()
#9 /srv/mediawiki/php-1.35.0-wmf.8/includes/StubObject.php(95): StubObject->_unstub(string, integer)
#10 /srv/mediawiki/php-1.35.0-wmf.8/includes/content/ContentHandler.php(697): StubObject::unstub(StubUserLang)
#11 /srv/mediawiki/php-1.35.0-wmf.8/includes/Title.php(4603): ContentHandler->getPageLanguage(Title)
#12 /srv/mediawiki/php-1.35.0-wmf.8/includes/Title.php(3550): Title->getPageLanguage()
#13 /srv/mediawiki/php-1.35.0-wmf.8/includes/Title.php(3576): Title->getCdnUrls()
#14 /srv/mediawiki/php-1.35.0-wmf.8/includes/user/User.php(4057): Title->purgeSquid()
#15 /srv/mediawiki/php-1.35.0-wmf.8/extensions/Wikibase/client/includes/Hooks/EchoNotificationsHandlers.php(124): User->saveSettings()
#16 /srv/mediawiki/php-1.35.0-wmf.8/extensions/Wikibase/client/includes/Hooks/EchoNotificationsHandlers.php(114): Wikibase\Client\Hooks\EchoNotificationsHandlers->doLocalUserCreated(User, boolean)
#17 /srv/mediawiki/php-1.35.0-wmf.8/includes/Hooks.php(174): Wikibase\Client\Hooks\EchoNotificationsHandlers::onLocalUserCreated(User, boolean)
#18 /srv/mediawiki/php-1.35.0-wmf.8/includes/Hooks.php(202): Hooks::callHook(string, array, array, NULL)
#19 /srv/mediawiki/php-1.35.0-wmf.8/includes/auth/AuthManager.php(1744): Hooks::run(string, array)
#20 /srv/mediawiki/php-1.35.0-wmf.8/includes/Setup.php(894): MediaWiki\Auth\AuthManager->autoCreateUser(User, string, boolean)
#21 /srv/mediawiki/php-1.35.0-wmf.8/includes/WebStart.php(81): require_once(string)
#22 /srv/mediawiki/php-1.35.0-wmf.8/api.php(36): require(string)
#23 /srv/mediawiki/w/api.php(3): require(string)
Impact

(To be determined.)

Notes

I don't yet know what caused this, whether it's urgent, or how to fix it.

But, below is an initial scan of the stack trace and its surrounding code that might help.

  • User::saveSettings is calling Title::purgeSquid. It is undocumented why changing preferences requires purging the user page, unconditionally, synchronously.
User.php
	public function saveSettings() {
		// …
		$this->getUserPage()->purgeSquid();
	}
  • Because MediaWiki does not have fixed slugs or urls for Titles, they have to be computed at run-time, and these are indeed dynamic, include possible variance on user gender and page language. Page language in turn is an customisable property of the page that extensions can hook into. One of the hook parameters, in addition to the title and the "normal" page language, is the user language. I don't know what the use case for this is or was.
  • While this "interesting" hook wasn't meaningfully involved right now, the preparation code for it does run. That preparation code involves "un-stubbing" the user language object, which in turn involves another hook (UserGetLanguageObject) at RequestContext::getLanguage, which is meaningfully involved. The UniversalLanguageSelector uses it. One of the things the ULS hook varies its behaviour by is whether the user is logged-in. Determining that requires loading the User object itself first, which brings us back to where we started and violates the "User object not safe for use during Setup.php execution".
  • The "just in case" mentioned in this code is in fact this case.
ContentHandler.php
	public function getPageLanguage( Title $title, Content $content = null ) {
		global $wgLang;
		$pageLang = MediaWikiServices::getInstance()->getContentLanguage();

		// …

		// Simplify hook handlers by only passing objects of one type, in case nothing
		// else has unstubbed the StubUserLang object by now.
		StubObject::unstub( $wgLang );

		Hooks::run( 'PageContentLanguage', [ $title, &$pageLang, $wgLang ] );

		// …
	}

Details

Request ID
XewEUgpAAEkAAHRt3FMAAABI
Request URL
/w/api.php?action=mobileview&…

Event Timeline

Krinkle created this task.Dec 7 2019, 8:34 PM
Restricted Application added a project: Wikidata. · View Herald TranscriptDec 7 2019, 8:34 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript

@Nikerabbit @Addshore When your respective teams are investigating this and if you need help with how the constraint works and how it is typically dealt with, you'll probably want to tag Core Platform Team as they know this code fairly well (and better than me).

I do remember adding the StubObject::unstub( $wgLang ); for T214358: Fatal error: Uncaught TypeError: Argument 2 passed to TranslateHooks::onPageContentLanguage() must be an instance of Language, string given. A few extensions are using it when setting the value in the hook while avoiding using request globals directly.

What for does purging need to know the page language? For language variants.

Since this is a newly created user, it doesn't seem necessary to purge the user pages, or maybe the user pages do not need to be purged for every option change. Just for a gender change?

Addshore renamed this task from "User::loadFromSession called before the end of Setup.php" (violation by Wikibase/ULS) to "User::loadFromSession called before the end of Setup.php" (violation by Wikibase/ULS) [Points 5].Dec 10 2019, 1:14 PM
Addshore renamed this task from "User::loadFromSession called before the end of Setup.php" (violation by Wikibase/ULS) [Points 5] to "User::loadFromSession called before the end of Setup.php" (violation by Wikibase/ULS) [Story Points 5].Dec 10 2019, 2:12 PM

I might be very wrong here but can it be that it's caused by I7ca40f8eda?

Ladsgroup added a subscriber: daniel.

I highly doubt this is something Wikibase team would be able to fix. The usage of the LocalUserCreated hook in Wikibase code seems to be straightforward and simple, something in core or Translate extension needs to change. It might be caused by splitting Language object. I add Daniel and CPT to the ticket.

The stack trace reads like a fun chain of lazy initialization logic causing a User object to be loaded prematurely. There are several places where this chain could potentially be cut, but I didn't find a trivial once at a first glance. I'm wondering though why the User object has to be loaded when a new user has just been created. Also, why is autoCreateUser() triggered before setup is complete? Lots of potential problems & solutions.

Anomie added a subscriber: Anomie.Dec 11 2019, 3:22 PM

User::saveSettings is calling Title::purgeSquid. It is undocumented why changing preferences requires purging the user page, unconditionally, synchronously.

It was added in r42179 with reference to T3306: Suppress the "email this user" link in the toolbox if said user has opted not to/can't receive emails. It seems the purge is intended to update the state of the "Email user" link when the associated preference is changed.

But the speculation about purging for gender changes might now be another use case requiring such a purge.

There are several places where this chain could potentially be cut, but I didn't find a trivial once at a first glance.

ContentHandler::getPageLanguage() probably shouldn't depend on the request in the first place. If the content is really in multiple languages somehow such that it can be displayed in the user's UI language, that should probably be indicated directly for the caller to deal with (if it cares) instead of returning the current user's UI language. As it is, this particular use case (getCdnUrls) is broken because it won't be returning every possible URL, only those corresponding to the current user's UI language. But changing all that would probably be a large project.

Possibly the simplest place is here

Title.php
public function purgeSquid() {
    DeferredUpdates::addUpdate(
        new CdnCacheUpdate( $this->getCdnUrls() ),
        DeferredUpdates::PRESEND
    );
}

It's already deferring the actual update, you'd just have to somehow move the call to $this->getCdnUrls() to when the update runs instead of when it's constructed.

Also, why is autoCreateUser() triggered before setup is complete?

Part of setup is determining the User making the request. If that User doesn't exist locally yet, it has to be auto-created to have it available to finish the setup.

User::saveSettings is calling Title::purgeSquid. It is undocumented why changing preferences requires purging the user page, unconditionally, synchronously.

It was added in r42179 with reference to T3306: Suppress the "email this user" link in the toolbox if said user has opted not to/can't receive emails. It seems the purge is intended to update the state of the "Email user" link when the associated preference is changed.

"Email user" isn't shown for anonymous visitors, right? So it's not in the web cache, so purgeSquid() isn't needed. Never was, as far as I understand...

But the speculation about purging for gender changes might now be another use case requiring such a purge.

That may indeed be needed, but shouldn't have anything to do with purging web caches.

There are several places where this chain could potentially be cut, but I didn't find a trivial once at a first glance.

ContentHandler::getPageLanguage() probably shouldn't depend on the request in the first place. If the content is really in multiple languages somehow such that it can be displayed in the user's UI language, that should probably be indicated directly for the caller to deal with (if it cares) instead of returning the current user's UI language. As it is, this particular use case (getCdnUrls) is broken because it won't be returning every possible URL, only those corresponding to the current user's UI language. But changing all that would probably be a large project.

Yes, the concept of page languages as currently implemented is broken. When I created ContentHandler, I tried to keep the existing behavior, baking the existing confusion into the interface of ContentHandler. Would have been better to fix it.

It's already deferring the actual update, you'd just have to somehow move the call to $this->getCdnUrls() to when the update runs instead of when it's constructed.

Or make the list of URLs not depend on the user. As you point out, that doesn't make any sense, that list of URLs should depend solely on the title.

Also, why is autoCreateUser() triggered before setup is complete?

Part of setup is determining the User making the request. If that User doesn't exist locally yet, it has to be auto-created to have it available to finish the setup.

Could that be deferred until the point where the User object would usually be initialized from the session?

Could that be deferred until the point where the User object would usually be initialized from the session?

That seems like it would bring back the problems from T43201: UserLoadFromSession considered evil. We specifically wanted auto-creation to happen at a predictable place during setup rather that it happening unexpectedly at any time during the request.

Krinkle triaged this task as Medium priority.Dec 17 2019, 9:09 PM
Anomie claimed this task.Mar 17 2020, 9:22 PM
Anomie removed a project: Core Platform Team.

It's already deferring the actual update, you'd just have to somehow move the call to $this->getCdnUrls() to when the update runs instead of when it's constructed.

Or make the list of URLs not depend on the user. As you point out, that doesn't make any sense, that list of URLs should depend solely on the title.

I think for now lets do the simpler fix. Reworking how languages and variants work would be a much bigger project.

Change 580482 had a related patch set uploaded (by Anomie; owner: Anomie):
[mediawiki/core@master] CdnCacheUpdate: Accept Titles in addition to strings

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

Change 580482 merged by jenkins-bot:
[mediawiki/core@master] CdnCacheUpdate: Accept Titles in addition to strings

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

daniel closed this task as Resolved.Apr 8 2020, 11:15 AM

Note the the solution that was just merged is quite far from what is discussed in the task description. If further work is desired here, please re-open.