Page MenuHomePhabricator
Create Task
Maniphest T303236

GrowthExperiments\VariantHooks->onSpecialPage_initList uses unsafe global context
Closed, ResolvedPublic
Actions

Description

Observed in CI mw-debug-www.log, when investigating unrelated issues relating to T300069.

And

[GlobalTitleFail] RequestContext::getTitle called with no title set.
#0 /workspace/src/extensions/GrowthExperiments/includes/VariantHooks.php(205): RequestContext->getTitle()
#1 /workspace/src/extensions/GrowthExperiments/includes/VariantHooks.php(129): GrowthExperiments\VariantHooks::shouldCheckForCampaign(RequestContext)
#2 /workspace/src/extensions/GrowthExperiments/includes/VariantHooks.php(114): GrowthExperiments\VariantHooks::isDonorOrGlamCampaign(RequestContext, GrowthExperiments\NewcomerTasks\CampaignConfig)
#3 /workspace/src/includes/HookContainer/HookContainer.php(160): GrowthExperiments\VariantHooks->onSpecialPage_initList(array)
#4 /workspace/src/includes/HookContainer/HookRunner.php(3652): MediaWiki\HookContainer\HookContainer->run(string, array)
#5 /workspace/src/includes/specialpage/SpecialPageFactory.php(1116): MediaWiki\HookContainer\HookRunner->onSpecialPage_initList(array)
#6 /workspace/src/includes/specialpage/SpecialPageFactory.php(1131): MediaWiki\SpecialPage\SpecialPageFactory->getPageList()
#7 /workspace/src/includes/specialpage/SpecialPageFactory.php(1469): MediaWiki\SpecialPage\SpecialPageFactory->getAliasList()
#8 /workspace/src/includes/specialpage/SpecialPage.php(148): MediaWiki\SpecialPage\SpecialPageFactory->getLocalNameFor(string, boolean)
#9 /workspace/src/includes/specialpage/SpecialPage.php(133): SpecialPage::getTitleValueFor(string, boolean, string)
#10 /workspace/src/extensions/VisualEditor/includes/VisualEditorHooks.php(1087): SpecialPage::getTitleFor(string)
#11 /workspace/src/includes/HookContainer/HookContainer.php(338): VisualEditorHooks::onResourceLoaderGetConfigVars(array, string, GlobalVarConfig)
#12 /workspace/src/includes/HookContainer/HookContainer.php(137): MediaWiki\HookContainer\HookContainer->callLegacyHook(string, array, array, array)
#13 /workspace/src/includes/HookContainer/HookRunner.php(3160): MediaWiki\HookContainer\HookContainer->run(string, array, array)
#14 /workspace/src/includes/resourceloader/ResourceLoader.php(2019): MediaWiki\HookContainer\HookRunner->onResourceLoaderGetConfigVars(array, string, GlobalVarConfig)
#15 /workspace/src/includes/resourceloader/ResourceLoaderFileModule.php(1312): ResourceLoader::getSiteConfigSettings(ResourceLoaderContext, GlobalVarConfig, NULL)
#16 /workspace/src/includes/resourceloader/ResourceLoaderFileModule.php(635): ResourceLoaderFileModule->expandPackageFiles(ResourceLoaderContext)
#17 /workspace/src/includes/resourceloader/ResourceLoaderModule.php(908): ResourceLoaderFileModule->getDefinitionSummary(ResourceLoaderContext)
#18 /workspace/src/includes/resourceloader/ResourceLoaderStartUpModule.php(220): ResourceLoaderModule->getVersionHash(ResourceLoaderContext)
#19 /workspace/src/includes/resourceloader/ResourceLoaderStartUpModule.php(421): ResourceLoaderStartUpModule->getModuleRegistrations(ResourceLoaderContext)
#20 /workspace/src/includes/resourceloader/ResourceLoaderModule.php(796): ResourceLoaderStartUpModule->getScript(ResourceLoaderContext)
#21 /workspace/src/includes/resourceloader/ResourceLoaderModule.php(765): ResourceLoaderModule->buildContent(ResourceLoaderContext)
#22 /workspace/src/includes/resourceloader/ResourceLoaderModule.php(905): ResourceLoaderModule->getModuleContent(ResourceLoaderContext)
#23 /workspace/src/includes/resourceloader/ResourceLoader.php(718): ResourceLoaderModule->getVersionHash(ResourceLoaderContext)
#24 [internal function]: ResourceLoader->{closure}(string)
#25 /workspace/src/includes/resourceloader/ResourceLoader.php(732): array_map(Closure, array)
#26 /workspace/src/includes/resourceloader/ResourceLoader.php(821): ResourceLoader->getCombinedVersion(ResourceLoaderContext, array)
#27 /workspace/src/load.php(52): ResourceLoader->respond(ResourceLoaderContext)
#28 /workspace/src/load.php(38): wfLoadMain()
#29 {main}

https://integration.wikimedia.org/ci/job/wmf-quibble-selenium-php72-docker/139590/artifact/log/

Details

SubjectRepoBranchLines +/-
VariantHooks: Don't use unsafe global context for Titlemediawiki/extensions/GrowthExperimentsmaster+11 -24
Customize query in gerrit

Related Objects

Event Timeline

Krinkle created this task.Mar 8 2022, 1:08 AM
Restricted Application added a project: Growth-Team. · View Herald TranscriptMar 8 2022, 1:08 AM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
kostajh edited projects, added Growth-Team (Sprint 0 (Growth Team)); removed Growth-Team.Mar 8 2022, 12:16 PM
kostajh subscribed.

So is the problem specifically that RequestContext::getTitle() is called? Or the use of RequestContext::getMain() in general? I think we can do without the getTitle call.

gerritbot added a comment.Mar 8 2022, 12:18 PM

Change 769036 had a related patch set uploaded (by Kosta Harlan; author: Kosta Harlan):

[mediawiki/extensions/GrowthExperiments@master] VariantHooks: Don't use unsafe global context for Title

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

gerritbot added a project: Patch-For-Review.Mar 8 2022, 12:18 PM
kostajh claimed this task.Mar 8 2022, 12:18 PM
kostajh moved this task from Incoming to Code Review on the Growth-Team (Sprint 0 (Growth Team)) board.
gerritbot added a comment.Mar 8 2022, 5:09 PM

Change 769036 merged by jenkins-bot:

[mediawiki/extensions/GrowthExperiments@master] VariantHooks: Don't use unsafe global context for Title

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

Maintenance_bot removed a project: Patch-For-Review.Mar 8 2022, 5:10 PM
ReleaseTaggerBot added a project: MW-1.38-notes (1.38.0-wmf.26; 2022-03-14).Mar 8 2022, 6:00 PM
• mewoph moved this task from Code Review to QA on the Growth-Team (Sprint 0 (Growth Team)) board.Mar 8 2022, 6:24 PM
Krinkle added a comment.Mar 9 2022, 3:02 PM
In T303236#7759771, @kostajh wrote:

So is the problem specifically that RequestContext::getTitle() is called? Or the use of RequestContext::getMain() in general? I think we can do without the getTitle call.

Both, although getTitle is definitely the more problematic of the two, in the long run both will result in run-time warnings or throw exceptions as it has no "correct" thing to return outside index.php requests and some api.php requests (e.g. think load.php, rest.php, job runners, CLI batch scripts; whatever information the code is trying to glean from it, would have to come from elsewhere; in some cases a neutral negative value will do, in other cases that would do the wrong thing or hide a business logic conflict).

Thanks for the quick patch!

Krinkle closed this task as Resolved.Mar 9 2022, 3:13 PM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL · Credits