Page MenuHomePhabricator

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


Observed on
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}

Event Timeline

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

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 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 < might be related. It's possible that it is triggered by a change in core or Flow or elsewhere.

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

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

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 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.

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

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

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

mmodell changed the subtype of this task from "Task" to "Production Error".Aug 28 2019, 11:08 PM