Page MenuHomePhabricator

Fatal LogicException: Role mediainfo is already defined (DiscussionTools conflicts with WikibaseMediaInfo)
Closed, ResolvedPublicPRODUCTION ERROR

Description

Error message
LogicException: Role mediainfo is already defined
Stack Trace
#0 /srv/mediawiki/php-1.35.0-wmf.37/includes/Revision/SlotRoleRegistry.php(115): MediaWiki\Revision\SlotRoleRegistry->defineRole(string, Closure)
#1 /srv/mediawiki/php-1.35.0-wmf.37/extensions/WikibaseMediaInfo/src/WikibaseMediaInfoHooks.php(68): MediaWiki\Revision\SlotRoleRegistry->defineRoleWithModel(string, string)
#2 [internal function]: Wikibase\MediaInfo\WikibaseMediaInfoHooks::Wikibase\MediaInfo\{closure}(MediaWiki\Revision\SlotRoleRegistry, MediaWiki\MediaWikiServices)
#3 /srv/mediawiki/php-1.35.0-wmf.37/includes/libs/services/ServiceContainer.php(458): call_user_func_array(Closure, array)
#4 /srv/mediawiki/php-1.35.0-wmf.37/includes/libs/services/ServiceContainer.php(419): Wikimedia\Services\ServiceContainer->createService(string)
#5 /srv/mediawiki/php-1.35.0-wmf.37/includes/MediaWikiServices.php(1184): Wikimedia\Services\ServiceContainer->getService(string)
#6 /srv/mediawiki/php-1.35.0-wmf.37/includes/ServiceWiring.php(1031): MediaWiki\MediaWikiServices->getSlotRoleRegistry()
#7 /srv/mediawiki/php-1.35.0-wmf.37/includes/libs/services/ServiceContainer.php(451): Wikimedia\Services\ServiceContainer->{closure}(MediaWiki\MediaWikiServices)
#8 /srv/mediawiki/php-1.35.0-wmf.37/includes/libs/services/ServiceContainer.php(419): Wikimedia\Services\ServiceContainer->createService(string)
#9 /srv/mediawiki/php-1.35.0-wmf.37/includes/MediaWikiServices.php(1119): Wikimedia\Services\ServiceContainer->getService(string)
#10 /srv/mediawiki/php-1.35.0-wmf.37/includes/ServiceWiring.php(1013): MediaWiki\MediaWikiServices->getRevisionStoreFactory()
#11 /srv/mediawiki/php-1.35.0-wmf.37/includes/libs/services/ServiceContainer.php(451): Wikimedia\Services\ServiceContainer->{closure}(MediaWiki\MediaWikiServices)
#12 /srv/mediawiki/php-1.35.0-wmf.37/includes/libs/services/ServiceContainer.php(419): Wikimedia\Services\ServiceContainer->createService(string)
#13 /srv/mediawiki/php-1.35.0-wmf.37/includes/MediaWikiServices.php(1111): Wikimedia\Services\ServiceContainer->getService(string)
#14 /srv/mediawiki/php-1.35.0-wmf.37/includes/ServiceWiring.php(999): MediaWiki\MediaWikiServices->getRevisionStore()
#15 /srv/mediawiki/php-1.35.0-wmf.37/includes/libs/services/ServiceContainer.php(451): Wikimedia\Services\ServiceContainer->{closure}(MediaWiki\MediaWikiServices)
#16 /srv/mediawiki/php-1.35.0-wmf.37/includes/libs/services/ServiceContainer.php(419): Wikimedia\Services\ServiceContainer->createService(string)
#17 /srv/mediawiki/php-1.35.0-wmf.37/includes/MediaWikiServices.php(1095): Wikimedia\Services\ServiceContainer->getService(string)
#18 /srv/mediawiki/php-1.35.0-wmf.37/includes/ServiceWiring.php(885): MediaWiki\MediaWikiServices->getRevisionLookup()
#19 /srv/mediawiki/php-1.35.0-wmf.37/includes/libs/services/ServiceContainer.php(451): Wikimedia\Services\ServiceContainer->{closure}(MediaWiki\MediaWikiServices)
#20 /srv/mediawiki/php-1.35.0-wmf.37/includes/libs/services/ServiceContainer.php(419): Wikimedia\Services\ServiceContainer->createService(string)
#21 /srv/mediawiki/php-1.35.0-wmf.37/includes/MediaWikiServices.php(1039): Wikimedia\Services\ServiceContainer->getService(string)
#22 /srv/mediawiki/php-1.35.0-wmf.37/includes/ServiceWiring.php(130): MediaWiki\MediaWikiServices->getPermissionManager()
#23 /srv/mediawiki/php-1.35.0-wmf.37/includes/libs/services/ServiceContainer.php(451): Wikimedia\Services\ServiceContainer->{closure}(MediaWiki\MediaWikiServices)
#24 /srv/mediawiki/php-1.35.0-wmf.37/includes/libs/services/ServiceContainer.php(419): Wikimedia\Services\ServiceContainer->createService(string)
#25 /srv/mediawiki/php-1.35.0-wmf.37/includes/MediaWikiServices.php(493): Wikimedia\Services\ServiceContainer->getService(string)
#26 /srv/mediawiki/php-1.35.0-wmf.37/extensions/GlobalPreferences/includes/Hooks.php(198): MediaWiki\MediaWikiServices->getAuthManager()
#27 /srv/mediawiki/php-1.35.0-wmf.37/includes/libs/services/ServiceContainer.php(451): GlobalPreferences\Hooks::GlobalPreferences\{closure}(MediaWiki\MediaWikiServices)
#28 /srv/mediawiki/php-1.35.0-wmf.37/includes/libs/services/ServiceContainer.php(419): Wikimedia\Services\ServiceContainer->createService(string)
#29 /srv/mediawiki/php-1.35.0-wmf.37/includes/MediaWikiServices.php(1047): Wikimedia\Services\ServiceContainer->getService(string)
#30 /srv/mediawiki/php-1.35.0-wmf.37/extensions/GlobalPreferences/includes/Hooks.php(294): MediaWiki\MediaWikiServices->getPreferencesFactory()
#31 /srv/mediawiki/php-1.35.0-wmf.37/extensions/GlobalPreferences/includes/Hooks.php(43): GlobalPreferences\Hooks::getPreferencesFactory()
#32 /srv/mediawiki/php-1.35.0-wmf.37/includes/HookContainer/HookContainer.php(319): GlobalPreferences\Hooks::onUserLoadOptions(User, array)
#33 /srv/mediawiki/php-1.35.0-wmf.37/includes/HookContainer/HookContainer.php(131): MediaWiki\HookContainer\HookContainer->callLegacyHook(string, array, array, array)
#34 /srv/mediawiki/php-1.35.0-wmf.37/includes/HookContainer/HookRunner.php(4358): MediaWiki\HookContainer\HookContainer->run(string, array)
#35 /srv/mediawiki/php-1.35.0-wmf.37/includes/user/UserOptionsManager.php(557): MediaWiki\HookContainer\HookRunner->onUserLoadOptions(User, array)
#36 /srv/mediawiki/php-1.35.0-wmf.37/includes/user/UserOptionsManager.php(135): MediaWiki\User\UserOptionsManager->loadUserOptions(User, integer)
#37 /srv/mediawiki/php-1.35.0-wmf.37/includes/user/UserOptionsLookup.php(123): MediaWiki\User\UserOptionsManager->getOption(User, string, integer, boolean, integer)
#38 /srv/mediawiki/php-1.35.0-wmf.37/includes/user/User.php(2740): MediaWiki\User\UserOptionsLookup->getIntOption(User, string, integer)
#39 /srv/mediawiki/php-1.35.0-wmf.37/includes/user/User.php(2930): User->getIntOption(string)
#40 /srv/mediawiki/php-1.35.0-wmf.37/includes/linker/LinkRendererFactory.php(99): User->getStubThreshold()
#41 /srv/mediawiki/php-1.35.0-wmf.37/includes/ServiceWiring.php(521): MediaWiki\Linker\LinkRendererFactory->createForUser(User)
#42 /srv/mediawiki/php-1.35.0-wmf.37/includes/libs/services/ServiceContainer.php(451): Wikimedia\Services\ServiceContainer->{closure}(MediaWiki\MediaWikiServices)
#43 /srv/mediawiki/php-1.35.0-wmf.37/includes/libs/services/ServiceContainer.php(419): Wikimedia\Services\ServiceContainer->createService(string)
#44 /srv/mediawiki/php-1.35.0-wmf.37/includes/MediaWikiServices.php(825): Wikimedia\Services\ServiceContainer->getService(string)
#45 /srv/mediawiki/php-1.35.0-wmf.37/includes/page/Article.php(165): MediaWiki\MediaWikiServices->getLinkRenderer()
#46 /srv/mediawiki/php-1.35.0-wmf.37/includes/page/Article.php(212): Article->__construct(Title)
#47 /srv/mediawiki/php-1.35.0-wmf.37/includes/page/Article.php(228): Article::newFromTitle(Title, RequestContext)
#48 /srv/mediawiki/php-1.35.0-wmf.37/includes/actions/Action.php(186): Article::newFromWikiPage(WikiPage, RequestContext)
#49 /srv/mediawiki/php-1.35.0-wmf.37/includes/MediaWiki.php(166): Action::getActionName(RequestContext)
#50 /srv/mediawiki/php-1.35.0-wmf.37/includes/MediaWiki.php(903): MediaWiki->getAction()
#51 /srv/mediawiki/php-1.35.0-wmf.37/includes/MediaWiki.php(543): MediaWiki->main()
#52 /srv/mediawiki/php-1.35.0-wmf.37/index.php(47): MediaWiki->run()
#53 /srv/mediawiki/w/index.php(3): require(string)
#54 {main}
Impact

Notes
  • Spike on commonswiki for render api events affecting all kinds of pages including wikitext and JS pages.
  • 8,000 exceptions all at once, then nothing...

Event Timeline

Krinkle renamed this task from Role mediainfo is already defined to Fatal LogicException: Role mediainfo is already defined.Jun 17 2020, 6:45 PM
Krinkle updated the task description. (Show Details)
Krinkle triaged this task as High priority.Jun 17 2020, 6:45 PM

This is relevant to https://sal.toolforge.org/log/SrGDw3IBLkHzneNN3dWT (reverted in https://sal.toolforge.org/log/ybGGw3IBLkHzneNNQNbu), in which I tried to deploy https://gerrit.wikimedia.org/r/c/operations/mediawiki-config/+/599307 at @matmarex's request. I don't think there's anything to look into (except why DiscussionTools dislikes MCR :)).

matmarex renamed this task from Fatal LogicException: Role mediainfo is already defined to Fatal LogicException: Role mediainfo is already defined (DiscussionTools conflicts with WikibaseMediaInfo).Jun 17 2020, 7:07 PM
matmarex added a project: DiscussionTools.

Change 606244 had a related patch set uploaded (by Krinkle; owner: Urbanecm):
[operations/mediawiki-config@master] Disable DiscussionTools at beta

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

Relevant code in WikibaseMediaInfo:

	public static function onMediaWikiServices( MediaWikiServices $services ) {
		$services->addServiceManipulator( 'SlotRoleRegistry', function ( SlotRoleRegistry $registry ) {
			$registry->defineRoleWithModel(
				/* role */ 'mediainfo',
				/* content handler */ MediaInfoContent::CONTENT_MODEL_ID
				/*, layout – we want to set "prepend" in future, once MediaWiki supports that */
			);
		} );
	}

There is nothing else that tried to define the 'mediainfo' role. So as far as I can see, the only way for this to occur is for a hook handler for onMediaWikiServices to call the hook itself again (probably indirectly via 20 layers of services).

This is relevant to https://sal.toolforge.org/log/SrGDw3IBLkHzneNN3dWT (reverted in https://sal.toolforge.org/log/ybGGw3IBLkHzneNNQNbu), in which I tried to deploy https://gerrit.wikimedia.org/r/c/operations/mediawiki-config/+/599307 at @matmarex's request. I don't think there's anything to look into (except why DiscussionTools dislikes MCR :)).

For the record, the revert is: https://gerrit.wikimedia.org/r/#/c/operations/mediawiki-config/+/606235/

Change 606244 merged by jenkins-bot:
[operations/mediawiki-config@master] Disable DiscussionTools at beta commonswiki

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

It seems the DiscussionTools onRegistration hook (which runs much earlier that other hooks) is triggering this problem. It accesses the core config to check if Localtimezone is set, and accessing the core config that early is confusing WikibaseMediaInfo.

Unless we are explicitly not allowed to access the config during onRegistration the problem is still with WikibaseMediaInfo, but we can at least work around it in DiscussionTools by using the $wg global instead.

Change 606264 had a related patch set uploaded (by Esanders; owner: Esanders):
[mediawiki/extensions/DiscussionTools@master] Use $wgLocaltimezone global instead of request context

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

I think this is a bug in MediaWikiServices. When resetGlobalInstance() is called, it runs the 'MediaWikiServices' hook (which adds the manipulator from WikibaseMediaInfo), and then calls importWiring() (which copies all of the existing manipulators – including the one from WikibaseMediaInfo). This is how we end up with two copies of it, and when the second copy is called, it throws this exception.

I don't know why DiscussionTools triggers this, but I don't think it's the cause of the problem. I guess @Esanders explained it, I didn't see his comment before posting mine.

I'm not sure how this should be fixed in MediaWikiServices. It worries me a bit that a method which is supposed to reset the services actually doesn't.

Tagging CPT to have a look, in the light of @matmarex analysis.

I'm not sure how this should be fixed in MediaWikiServices. It worries me a bit that a method which is supposed to reset the services actually doesn't.

Besides fixing a bug, there are two bigger issues to look at here:

  1. Ideally there should be no need to reset services. We should revisit the reasons (basically, because some services are needed before extensions have initialized, and others are impacted by extensions having been initialized)
  2. We should revisit the idea of ServiceManipulators. It was introduced to give extensions a way to inject SlotRoleHandlers. Perhaps that should be changed to use ObjectFactory and ExtensionRegistry, like we do for the new hook system. I think that would be much cleaner.

Change 606264 merged by jenkins-bot:
[mediawiki/extensions/DiscussionTools@master] Use $wgLocaltimezone global instead of request context

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

Change 606279 had a related patch set uploaded (by Bartosz Dziewoński; owner: Esanders):
[mediawiki/extensions/DiscussionTools@wmf/1.35.0-wmf.37] Use $wgLocaltimezone global instead of request context

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

Change 606279 merged by jenkins-bot:
[mediawiki/extensions/DiscussionTools@wmf/1.35.0-wmf.37] Use $wgLocaltimezone global instead of request context

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

Change 606292 had a related patch set uploaded (by Urbanecm; owner: Esanders):
[mediawiki/extensions/DiscussionTools@wmf/1.35.0-wmf.36] Use $wgLocaltimezone global instead of request context

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

Change 606293 had a related patch set uploaded (by Urbanecm; owner: Urbanecm):
[operations/mediawiki-config@master] Revert "Disable DiscussionTools at beta commonswiki"

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

Change 606293 merged by jenkins-bot:
[operations/mediawiki-config@master] Revert "Disable DiscussionTools at beta commonswiki"

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

Change 606292 merged by Urbanecm:
[mediawiki/extensions/DiscussionTools@wmf/1.35.0-wmf.36] Use $wgLocaltimezone global instead of request context

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

Mentioned in SAL (#wikimedia-operations) [2020-06-17T23:21:49Z] <urbanecm@deploy1001> Synchronized php-1.35.0-wmf.37/extensions/DiscussionTools/includes/Hooks.php: 4551d29: Use $wgLocaltimezone global instead of request context (T252264; T253943; T255704) (duration: 00m 58s)

Mentioned in SAL (#wikimedia-operations) [2020-06-17T23:23:20Z] <urbanecm@deploy1001> Synchronized php-1.35.0-wmf.36/extensions/DiscussionTools/includes/Hooks.php: ff01083: Use $wgLocaltimezone global instead of request context (T255704) (duration: 00m 57s)

The workaround fixes the exception, so from Editing's perspective I think this is done, but I'll leave it open for CPT to have a look at the underlying problem, per @daniel.

The onRegistration callback is imho for basic prepping of globals or constants for things that extension.json does (not) yet support.

The purpose for which it is used here by DT is imho inappropiocate and is part of a general pattern we've been moving away from in core. Pre-processing logic or validation of config should happen lazily from wherever something needs it. E.g. when interacting with a part of the system where DT is not involved, this logic does not need to happen, for example. If you find multiple parts of the code base and/or other extension need to access the configuration, use a public method as the stable interface to obtain and validate it as needed.

For the case of wgLocaltimezone checking, I suppose it depends on what part of DT is actually impacted by it. For example if there is a main service class where most of the server-side logic resides, it could be done by its constructor. Or if it (also) relates to client code, one could use any number of hooks (so long as it is post-setup), e.g. ResourceLoaderRegisterModules if need be.

@Krinkle I don't agree with that. Our check for $wgLocaltimezone seems similar to e.g. checking whether another extension is installed or checking for minimal MediaWiki version, both of which happen during extension registration.

@Krinkle I don't agree with that. Our check for $wgLocaltimezone seems similar to e.g. checking whether another extension is installed or checking for minimal MediaWiki version, both of which happen during extension registration.

During the extension registry phase, information about the environment (web server, OS, PHP) and MW core (MW version) are fixed.

Information about the site configuratation is not fixed and unsafe there because 1) Most of Setup.php hasn't happened yet, so various compat or aliases won't have expanded yet, 2) Other extension registration callbacks haven't run yet, which might declare their own magic defaults or compat aliases, and 3) $wgExtensionFunctions hasn't run yet.

What you are describing is an unconditional callback that can safely observe the real site config and then decide to abort. This is not currently supported and does not exist in MediaWiki today. This logic should imho be done in a service class where it is also testable and naturally deferred to relevant contexts only.