Page MenuHomePhabricator

"User::loadFromSession called before the end of Setup.php" due to a combination of TwoColConflict and BetaFeatures
Closed, ResolvedPublic

Description

I don't know which extension to blame for this one, so I'll tag all of them.

During user auto-creation, AbuseFilter calls SpecialPage::getTitleFor. During the hook for that method to allow extensions to add special pages, TwoColConflictHooks::onSpecialPage_initList calls BetaFeatures::isFeatureEnabled which attempts to load the user from session before it's ready.

One of the following must be true:

  • SpecialPage::getTitleFor is not safe to call during auto-creation. Although that seems somewhat restrictive.
  • BetaFeatures::isFeatureEnabled is not safe to call during auto-creation. Extensions should register their special pages without that conditional.
  • BetaFeatures::isFeatureEnabled needs to somehow do its job without checking the session-user.
#0 /srv/mediawiki/php-1.31.0-wmf.2/includes/user/User.php(2233): User->load()
#1 /srv/mediawiki/php-1.31.0-wmf.2/includes/user/User.php(5277): User->getId()
#2 /srv/mediawiki/php-1.31.0-wmf.2/includes/user/User.php(2887): User->loadOptions()
#3 /srv/mediawiki/php-1.31.0-wmf.2/extensions/BetaFeatures/includes/BetaFeaturesUtil.php(43): User->getOption(string)
#4 /srv/mediawiki/php-1.31.0-wmf.2/extensions/TwoColConflict/includes/TwoColConflictHooks.php(124): BetaFeatures::isFeatureEnabled(User, string)
#5 /srv/mediawiki/php-1.31.0-wmf.2/includes/Hooks.php(177): TwoColConflictHooks::onSpecialPage_initList(array)
#6 /srv/mediawiki/php-1.31.0-wmf.2/includes/Hooks.php(205): Hooks::callHook(string, array, array, NULL)
#7 /srv/mediawiki/php-1.31.0-wmf.2/includes/specialpage/SpecialPageFactory.php(268): Hooks::run(string, array)
#8 /srv/mediawiki/php-1.31.0-wmf.2/includes/specialpage/SpecialPageFactory.php(285): SpecialPageFactory::getPageList()
#9 /srv/mediawiki/php-1.31.0-wmf.2/includes/specialpage/SpecialPageFactory.php(660): SpecialPageFactory::getAliasList()
#10 /srv/mediawiki/php-1.31.0-wmf.2/includes/specialpage/SpecialPage.php(98): SpecialPageFactory::getLocalNameFor(string, boolean)
#11 /srv/mediawiki/php-1.31.0-wmf.2/includes/specialpage/SpecialPage.php(85): SpecialPage::getTitleValueFor(string, boolean, string)
#12 /srv/mediawiki/php-1.31.0-wmf.2/extensions/AbuseFilter/includes/AbuseFilterPreAuthenticationProvider.php(41): SpecialPage::getTitleFor(string)
#13 /srv/mediawiki/php-1.31.0-wmf.2/extensions/AbuseFilter/includes/AbuseFilterPreAuthenticationProvider.php(13): AbuseFilterPreAuthenticationProvider->testUser(User, User, boolean)
#14 /srv/mediawiki/php-1.31.0-wmf.2/includes/auth/AuthManager.php(1649): AbuseFilterPreAuthenticationProvider->testUserForCreation(User, string, array)
#15 /srv/mediawiki/php-1.31.0-wmf.2/includes/Setup.php(869): MediaWiki\Auth\AuthManager->autoCreateUser(User, string, boolean)
#16 /srv/mediawiki/php-1.31.0-wmf.2/includes/WebStart.php(114): include(string)
#17 /srv/mediawiki/php-1.31.0-wmf.2/api.php(38): include(string)
#18 /srv/mediawiki/w/api.php(3): include(string)
#19 {main}

Alternative trace:

#0 /srv/mediawiki/php-1.31.0-wmf.2/includes/user/User.php(5269): User->load()
#1 /srv/mediawiki/php-1.31.0-wmf.2/includes/user/User.php(2887): User->loadOptions()
#2 /srv/mediawiki/php-1.31.0-wmf.2/extensions/BetaFeatures/includes/BetaFeaturesUtil.php(43): User->getOption(string)
#3 /srv/mediawiki/php-1.31.0-wmf.2/extensions/TwoColConflict/includes/TwoColConflictHooks.php(128): BetaFeatures::isFeatureEnabled(User, string)
#4 /srv/mediawiki/php-1.31.0-wmf.2/includes/Hooks.php(177): TwoColConflictHooks::onSpecialPage_initList(array)
#5 /srv/mediawiki/php-1.31.0-wmf.2/includes/Hooks.php(205): Hooks::callHook(string, array, array, NULL)
#6 /srv/mediawiki/php-1.31.0-wmf.2/includes/specialpage/SpecialPageFactory.php(268): Hooks::run(string, array)
#7 /srv/mediawiki/php-1.31.0-wmf.2/includes/specialpage/SpecialPageFactory.php(285): SpecialPageFactory::getPageList()
#8 /srv/mediawiki/php-1.31.0-wmf.2/includes/specialpage/SpecialPageFactory.php(660): SpecialPageFactory::getAliasList()
#9 /srv/mediawiki/php-1.31.0-wmf.2/includes/specialpage/SpecialPage.php(98): SpecialPageFactory::getLocalNameFor(string, string)
#10 /srv/mediawiki/php-1.31.0-wmf.2/includes/specialpage/SpecialPage.php(85): SpecialPage::getTitleValueFor(string, string, string)
#11 /srv/mediawiki/php-1.31.0-wmf.2/extensions/ShortUrl/ShortUrl.hooks.php(23): SpecialPage::getTitleFor(string, string)
#12 /srv/mediawiki/php-1.31.0-wmf.2/includes/Hooks.php(177): ShortUrlHooks::setupUrlRouting(PathRouter)
#13 /srv/mediawiki/php-1.31.0-wmf.2/includes/Hooks.php(205): Hooks::callHook(string, array, array, NULL)
#14 /srv/mediawiki/php-1.31.0-wmf.2/includes/WebRequest.php(171): Hooks::run(string, array)
#15 /srv/mediawiki/php-1.31.0-wmf.2/includes/WebRequest.php(316): WebRequest::getPathInfo(string)
#16 /srv/mediawiki/php-1.31.0-wmf.2/includes/Setup.php(721): WebRequest->interpolateTitle()
#17 /srv/mediawiki/php-1.31.0-wmf.2/includes/WebStart.php(114): include(string)
#18 /srv/mediawiki/php-1.31.0-wmf.2/index.php(40): include(string)
#19 /srv/mediawiki/w/index.php(3): include(string)
#20 {main}

Related Objects

Event Timeline

Anomie created this task.Oct 5 2017, 6:30 PM
Restricted Application added a project: TCB-Team. · View Herald TranscriptOct 5 2017, 6:30 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
Anomie added a comment.Oct 5 2017, 7:16 PM

I see the TwoColConflict/BetaFeatures combination is also hitting the "Sessions are disabled for this entry point" exception in load.php:

#0 /srv/mediawiki/php-1.31.0-wmf.2/includes/session/SessionManager.php(309): MediaWiki\Session\SessionManager->getSessionFromInfo(MediaWiki\Session\SessionInfo, WebRequest)
#1 /srv/mediawiki/php-1.31.0-wmf.2/includes/session/SessionManager.php(243): MediaWiki\Session\SessionManager->getEmptySessionInternal(WebRequest)
#2 /srv/mediawiki/php-1.31.0-wmf.2/includes/session/SessionManager.php(193): MediaWiki\Session\SessionManager->getEmptySession(WebRequest)
#3 /srv/mediawiki/php-1.31.0-wmf.2/includes/WebRequest.php(735): MediaWiki\Session\SessionManager->getSessionForRequest(WebRequest)
#4 /srv/mediawiki/php-1.31.0-wmf.2/includes/user/User.php(1231): WebRequest->getSession()
#5 /srv/mediawiki/php-1.31.0-wmf.2/includes/user/User.php(408): User->loadFromSession()
#6 /srv/mediawiki/php-1.31.0-wmf.2/includes/user/User.php(5269): User->load()
#7 /srv/mediawiki/php-1.31.0-wmf.2/includes/user/User.php(2887): User->loadOptions()
#8 /srv/mediawiki/php-1.31.0-wmf.2/extensions/BetaFeatures/includes/BetaFeaturesUtil.php(43): User->getOption(string)
#9 /srv/mediawiki/php-1.31.0-wmf.2/extensions/TwoColConflict/includes/TwoColConflictHooks.php(124): BetaFeatures::isFeatureEnabled(User, string)
#10 /srv/mediawiki/php-1.31.0-wmf.2/includes/Hooks.php(177): TwoColConflictHooks::onSpecialPage_initList(array)
#11 /srv/mediawiki/php-1.31.0-wmf.2/includes/Hooks.php(205): Hooks::callHook(string, array, array, NULL)
#12 /srv/mediawiki/php-1.31.0-wmf.2/includes/specialpage/SpecialPageFactory.php(268): Hooks::run(string, array)
#13 /srv/mediawiki/php-1.31.0-wmf.2/includes/specialpage/SpecialPageFactory.php(285): SpecialPageFactory::getPageList()
#14 /srv/mediawiki/php-1.31.0-wmf.2/includes/specialpage/SpecialPageFactory.php(660): SpecialPageFactory::getAliasList()
#15 /srv/mediawiki/php-1.31.0-wmf.2/includes/specialpage/SpecialPage.php(98): SpecialPageFactory::getLocalNameFor(string, boolean)
#16 /srv/mediawiki/php-1.31.0-wmf.2/includes/specialpage/SpecialPage.php(85): SpecialPage::getTitleValueFor(string, boolean, string)
#17 /srv/mediawiki/php-1.31.0-wmf.2/extensions/VisualEditor/VisualEditor.hooks.php(846): SpecialPage::getTitleFor(string)
#18 /srv/mediawiki/php-1.31.0-wmf.2/includes/Hooks.php(177): VisualEditorHooks::onResourceLoaderGetConfigVars(array)
#19 /srv/mediawiki/php-1.31.0-wmf.2/includes/Hooks.php(205): Hooks::callHook(string, array, array, NULL)
#20 /srv/mediawiki/php-1.31.0-wmf.2/includes/resourceloader/ResourceLoaderStartUpModule.php(118): Hooks::run(string, array)
#21 /srv/mediawiki/php-1.31.0-wmf.2/includes/resourceloader/ResourceLoaderStartUpModule.php(403): ResourceLoaderStartUpModule->getConfigSettings(DerivativeResourceLoaderContext)
#22 /srv/mediawiki/php-1.31.0-wmf.2/includes/resourceloader/ResourceLoaderModule.php(844): ResourceLoaderStartUpModule->getDefinitionSummary(DerivativeResourceLoaderContext)
#23 /srv/mediawiki/php-1.31.0-wmf.2/includes/resourceloader/ResourceLoader.php(667): ResourceLoaderModule->getVersionHash(DerivativeResourceLoaderContext)
#24 [internal function]: Closure$ResourceLoader::getCombinedVersion(string)
#25 /srv/mediawiki/php-1.31.0-wmf.2/includes/resourceloader/ResourceLoader.php(680): array_map(Closure$ResourceLoader::getCombinedVersion;1712, array)
#26 /srv/mediawiki/php-1.31.0-wmf.2/includes/resourceloader/ResourceLoader.php(760): ResourceLoader->getCombinedVersion(ResourceLoaderContext, array)
#27 /srv/mediawiki/php-1.31.0-wmf.2/load.php(53): ResourceLoader->respond(ResourceLoaderContext)
#28 /srv/mediawiki/w/load.php(3): include(string)
#29 {main}
Nikerabbit raised the priority of this task from Normal to Unbreak Now!.Oct 6 2017, 11:59 AM
Nikerabbit added a subscriber: Nikerabbit.

My guess is that this is causing user interface language selection to fail on hi.wikipedia.org and other sites, but for example on gu.wikipedia.org where I don't see this exception with mwdebug. End result seems to be the same as in T177478: Unable to change user settings except this is currently in production (and weekend is coming).

Restricted Application added subscribers: Liuxinyu970226, Jay8g, TerraCodes. · View Herald TranscriptOct 6 2017, 11:59 AM
Nikerabbit added a subscriber: greg.

I also see a trace with ShortUrl instead of AbuseFilter, hence removing it as probably unrelated.

Nikerabbit updated the task description. (Show Details)Oct 6 2017, 12:15 PM

It looks like b9146ba1d932dca00912a186191e3218d7aef734 didn't fully solve the issue, but I'm not sure what the original issue was that caused the first revert. Maybe reverting that again would solve this.

Instead of conditionally registering the special page, could it just be marked as unlisted or listed depending whether the beta feature is enabled, and show an error if it is not supposed to be accessed?

Nikerabbit renamed this task from "User::loadFromSession called before the end of Setup.php" due to a combination of AbuseFilter, TwoColConflict, and BetaFeatures to "User::loadFromSession called before the end of Setup.php" due to a combination of TwoColConflict and BetaFeatures.Oct 6 2017, 12:27 PM

I think this might be a dupe of T177468 or at least the issues described and covered in that?
This is all fixed up on master now.

Anomie added a comment.EditedOct 6 2017, 2:27 PM

No, T177468 is about ->getUser() methods returning null. In this task, the ->getUser() methods are returning a valid User object, but it's attempting to load that user object while $user->isSafeToLoad() returns false.

The title and description of the task refer to situations where $user->isSafeToLoad() returns false because setup has not yet completed, while T177524#3662027 refers to situations where $user->isSafeToLoad() returns false because the endpoint does not have any session to load a user from.

It looks like your patch https://gerrit.wikimedia.org/r/#/c/382453/ fixed the no-session case, but the main case here is still occurring (see https://logstash.wikimedia.org/goto/51715162dbd687ade2f9a6f08824bcc9).

Nikerabbit's suggestion of always registering the special page and changing its listed status based on the beta feature toggle is probably the more reliable solution.

greg added a comment.Oct 6 2017, 5:51 PM

What is the user facing impact of this?

What is the user facing impact of this?

Users unable to change any preferences on some (?) wikis; made particularly acute by the roll-out from beta to default of the new RecentChanges tool, as complained about in T177590. (This is speculative causation.)

Anomie added a comment.Oct 6 2017, 6:11 PM

What is the user facing impact of this?

I personally don't know of any user-facing impact, but I can't say there isn't one. It doesn't help that TwoColConflict had a different bug in T177468 and the attempted removal of StubUserLang caused T177478 at the same time.

Change 382753 had a related patch set uploaded (by Catrope; owner: Catrope):
[mediawiki/extensions/TwoColConflict@master] Revert "Dont register simulate page when not enabled as a BF"

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

Catrope added a subscriber: Catrope.Oct 6 2017, 6:17 PM

Registering a special page conditionally based on a user preference seems shady to me anyway. I've put up a patch to revert this behavior from master.

What confuses me, though, is that the wmf.2 backport of the patch that introduced this behavior, has already been reverted in wmf.2. I'll keep digging.

Aha, it's because it was reapplied later. Let's revert that.

Change 382756 had a related patch set uploaded (by Catrope; owner: Catrope):
[mediawiki/extensions/TwoColConflict@wmf/1.31.0-wmf.2] Revert "Dont register simulate page when not enabled as a BF"

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

Change 382756 merged by jenkins-bot:
[mediawiki/extensions/TwoColConflict@wmf/1.31.0-wmf.2] Revert "Dont register simulate page when not enabled as a BF"

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

Restricted Application added a project: User-Addshore. · View Herald TranscriptOct 6 2017, 6:28 PM

Mentioned in SAL (#wikimedia-operations) [2017-10-06T18:38:58Z] <catrope@tin> Synchronized php-1.31.0-wmf.2/extensions/TwoColConflict/: Unbreak preference changes (T177524) (duration: 00m 47s)

My revert fixes this in production (wmf.2). We'll still have to merge my revert in master or do something else about this problem, otherwise it'll recur in wmf.3

mmodell lowered the priority of this task from Unbreak Now! to Normal.Oct 8 2017, 9:13 AM

Change 382753 merged by jenkins-bot:
[mediawiki/extensions/TwoColConflict@master] Revert "Dont register simulate page when not enabled as a BF"

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

Addshore closed this task as Resolved.Oct 9 2017, 3:47 PM

It looks like the hook that registered the special page could have been fixed by checking both $wgFullyInitialised and the existence of MW_NO_SESSION.
Although I havn't checked when wgFullyInitialised is actually set and if that is before of after the hook call on a regular index.php hit.

We are going to take a different approach and show a note on the special page when the BF is disabled rather than not register the page in the first place so we won't be reverting the revert and fixing. T177758

As this is reverted on branches & on master I'll mark as closed.

It looks like the hook that registered the special page could have been fixed by checking both $wgFullyInitialised and the existence of MW_NO_SESSION.

Doing that might have wound up with the special page never being registered on wikis with the ShortUrl extension enabled.

A more correct solution would have been to check ->isSafeToLoad() on the session User object ($wgUser or RequestContext::getMain()->getUser()).

Although I havn't checked when wgFullyInitialised is actually set and if that is before of after the hook call on a regular index.php hit.

It's set at the end of Setup.php.

Tobi_WMDE_SW moved this task from Proposed to Done on the WMDE-QWERTY-Team board.Oct 17 2017, 1:09 PM
Tobi_WMDE_SW moved this task from Done to Demoed on the WMDE-QWERTY-Team board.Oct 17 2017, 2:11 PM