Page MenuHomePhabricator

Certain Special:MobileDiff urls fatal with "Bad value for parameter $old: must be a TextContent"
Closed, ResolvedPublic

Description

Observed on https://m.mediawiki.org/wiki/Special:MobileDiff/2382486
Not sure if fix is needed in Flow or MobileFrontend.

This does not appear to be accessible via the mobile or desktop interface but if you pass a revision ID to Special:MobileDiff an exception is thrown.

#0 /srv/mediawiki/php-1.32.0-wmf.24/extensions/MobileFrontend/includes/diff/InlineDifferenceEngine.php(162): Wikimedia\Assert\Assert::parameterType(string, Flow\Content\BoardContent, string)
#1 /srv/mediawiki/php-1.32.0-wmf.24/includes/diff/DifferenceEngineSlotDiffRenderer.php(58): InlineDifferenceEngine->generateContentDiffBody(Flow\Content\BoardContent, Flow\Content\BoardContent)
#2 /srv/mediawiki/php-1.32.0-wmf.24/includes/diff/DifferenceEngine.php(1056): DifferenceEngineSlotDiffRenderer->getDiff(Flow\Content\BoardContent, Flow\Content\BoardContent)
#3 /srv/mediawiki/php-1.32.0-wmf.24/extensions/MobileFrontend/includes/diff/InlineDifferenceEngine.php(64): DifferenceEngine->getDiffBody()
#4 /srv/mediawiki/php-1.32.0-wmf.24/extensions/MobileFrontend/includes/specials/SpecialMobileDiff.php(160): InlineDifferenceEngine->showDiffPage()
#5 /srv/mediawiki/php-1.32.0-wmf.24/extensions/MobileFrontend/includes/specials/SpecialMobileDiff.php(132): SpecialMobileDiff->displayDiffPage()
#6 /srv/mediawiki/php-1.32.0-wmf.24/extensions/MobileFrontend/includes/specials/MobileSpecialPage.php(58): SpecialMobileDiff->executeWhenAvailable(string)
#7 /srv/mediawiki/php-1.32.0-wmf.24/includes/specialpage/SpecialPage.php(569): MobileSpecialPage->execute(string)
#8 /srv/mediawiki/php-1.32.0-wmf.24/includes/specialpage/SpecialPageFactory.php(568): SpecialPage->run(string)
#9 /srv/mediawiki/php-1.32.0-wmf.24/includes/MediaWiki.php(288): MediaWiki\Special\SpecialPageFactory->executePath(Title, RequestContext)
#10 /srv/mediawiki/php-1.32.0-wmf.24/includes/MediaWiki.php(868): MediaWiki->performRequest()
#11 /srv/mediawiki/php-1.32.0-wmf.24/includes/MediaWiki.php(525): MediaWiki->main()
#12 /srv/mediawiki/php-1.32.0-wmf.24/index.php(42): MediaWiki->run()
#13 /srv/mediawiki/w/index.php(3): include(string)
#14 {main}

Details

Related Gerrit Patches:
mediawiki/extensions/Flow : masterReturn false for Flow board diff

Event Timeline

Restricted Application added a project: Growth-Team. · View Herald TranscriptOct 2 2018, 11:40 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
Jdlrobson moved this task from Incoming to Needs Prioritization on the Readers-Web-Backlog board.

DifferenceEngine::getDiffBody should be returning a string or false so issue probably lies in Flow or includes/diff/DifferenceEngine.php

Jdlrobson added a subscriber: Tgr.Oct 2 2018, 11:47 PM

The error comes from

	public function generateContentDiffBody( Content $old, Content $new ) {
		Assert::parameterType( TextContent::class, $old, '$old' );
		Assert::parameterType( TextContent::class, $new, '$new' );

@Tgr added this in https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/MobileFrontend/+/452413/ as part of T201842. Can you provide some context?

Tentatively marking as blocking. It started we wmf.24 went to group0 with no instances of this error anywhere in the 30 days prior.

The code in MobileFrontend where the error first originated hasn't changed recently, although <https://gerrit.wikimedia.org/r/452413 might be related. It's possible that it is triggered by a change in core or Flow or elsewhere.

Tgr added a comment.Oct 3 2018, 12:09 AM

As the commit summary says, the patch just moved code around. The equivalent pre-MCR code was this.

Tgr added a comment.Oct 3 2018, 12:21 AM

Is this a frequent error or just no one bothered to manually construct a mobile diff of a Flow edit before? There are 4 separate URLs in Logstash, could just be a developer poking around after seeing the initial error. That or MobileFrontend started to expose links somehow. Either way, doesn't seem like a train blocker to me and I doubt such URLs would have worked before. (How does Flow handle diffs at all? It has a non-text content type but it does not seem to have its own diff engine implementation.)

Krinkle triaged this task as High priority.Oct 3 2018, 5:07 AM

I agree. Thanks for investigating. Based on what we now know about the code having moved (without significantly changed), it seems unlikely that this would've worked for users in the past.

Though it remains important to distinguish between "feature isn't offered to users" and "server emits HTTP 5xx with fatal spikes from a public GET url". Regardless of whether this is UBN, it's still High because these risks are making our logs unusable, cause false alarms for SRE, and can unexpectedly block a deployment when Scap sees such a spike (which is good on Scap, but bad on MW).

Given it hasn't shown up in logs for as long as we retain logs (30 days), new production fatals should block by default unless we can show it wasn't new. I noticed that the files in question did change recently, but even without that, the caller could've been changed elsewhere in core or in Flow.

I guess it's just a coincidence that it wasn't triggered for the 29 days before today. I'll lower it for now. Thanks for investigating.

Krinkle renamed this task from Bad value for parameter $old: must be a TextContent when viewing diff to Certain Special:MobileDiff urls fatal with "Bad value for parameter $old: must be a TextContent".Oct 3 2018, 5:07 AM
Tgr added a comment.Oct 3 2018, 6:42 AM

To be clear, the problem is not related to mobile diffs. Any extension that provides non-text content types (Content subclasses which are not TextContent subclasses) should also provide a custom DifferenceEngine or SlotDiffRenderer as the default one will throw. Flow somehow disables the normal diff URLs so something like https://www.mediawiki.org/w/index.php?diff=2382486 does not cause an error (although it doesn't do what the user would expect, either), but anything else that invokes the diff logic will throw, e.g. https://www.mediawiki.org/wiki/Special:ComparePages?rev1=2382480&rev2=2382486

kostajh added a subscriber: kostajh.Nov 1 2018, 3:27 PM

Growth-Team discussed in triage this week, we're going to look at this soon and we'll start with a ~30 minute investigation.

kostajh edited projects, added Growth-Team (Current Sprint); removed Growth-Team.
kostajh claimed this task.Nov 5 2018, 9:42 PM
kostajh moved this task from Incoming to In Progress on the Growth-Team (Current Sprint) board.

Change 471901 had a related patch set uploaded (by Kosta Harlan; owner: Kosta Harlan):
[mediawiki/extensions/Flow@master] WIP: Return empty string for flow board diff

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

Change 471901 merged by jenkins-bot:
[mediawiki/extensions/Flow@master] Return false for Flow board diff

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

Krinkle closed this task as Resolved.Nov 15 2018, 1:19 AM
Krinkle removed a project: Patch-For-Review.
mmodell changed the subtype of this task from "Task" to "Production Error".Aug 28 2019, 11:08 PM