Page MenuHomePhabricator

Call to a member function getRevisionRecord() on a non-object (boolean)
Closed, ResolvedPublic

Description

Started seeing this after rolling out wmf.18 to group0 wikis

#0 /srv/mediawiki/php-1.32.0-wmf.18/includes/diff/DifferenceEngine.php(224): DifferenceEngine->getSlotContents()
#1 /srv/mediawiki/php-1.32.0-wmf.18/includes/diff/DifferenceEngine.php(890): DifferenceEngine->getSlotDiffRenderers()
#2 /srv/mediawiki/php-1.32.0-wmf.18/extensions/Flow/includes/TemplateHelper.php(606): DifferenceEngine->showDiffStyle()
#3 /srv/mediawiki/php-1.32.0-wmf.18/vendor/zordius/lightncandy/src/lightncandy.php(2602): Flow\TemplateHelper::diffRevision(array, array)
#4 /srv/mediawiki/php-1.32.0-wmf.18/extensions/Flow/handlebars/compiled/flow_block_topic_diff_view.handlebars.php(31): LCRun3::ch(array, string, array, string)
#5 /srv/mediawiki/php-1.32.0-wmf.18/extensions/Flow/includes/TemplateHelper.php(113): Closure$(array, array)
#6 /srv/mediawiki/php-1.32.0-wmf.18/extensions/Flow/includes/TemplateHelper.php(188): Closure$Flow\TemplateHelper::getTemplate(array, array)
#7 /srv/mediawiki/php-1.32.0-wmf.18/extensions/Flow/includes/TemplateHelper.php(304): Flow\TemplateHelper::processTemplate(string, array)
#8 /srv/mediawiki/php-1.32.0-wmf.18/vendor/zordius/lightncandy/src/lightncandy.php(2602): Flow\TemplateHelper::block(array, array)
#9 /srv/mediawiki/php-1.32.0-wmf.18/extensions/Flow/handlebars/compiled/flow_block_loop.handlebars.php(25): LCRun3::ch(array, string, array, string)
#10 /srv/mediawiki/php-1.32.0-wmf.18/vendor/zordius/lightncandy/src/lightncandy.php(2485): Closure$(array, array)
#11 /srv/mediawiki/php-1.32.0-wmf.18/extensions/Flow/handlebars/compiled/flow_block_loop.handlebars.php(26): LCRun3::sec(array, array, array, boolean, Closure$;3160)
#12 /srv/mediawiki/php-1.32.0-wmf.18/extensions/Flow/includes/TemplateHelper.php(113): Closure$#2(array, array)
#13 /srv/mediawiki/php-1.32.0-wmf.18/extensions/Flow/includes/View.php(371): Closure$Flow\TemplateHelper::getTemplate(array)
#14 /srv/mediawiki/php-1.32.0-wmf.18/extensions/Flow/includes/View.php(83): Flow\View->renderApiResponse(array, array)
#15 /srv/mediawiki/php-1.32.0-wmf.18/extensions/Flow/includes/Actions/Action.php(112): Flow\View->show(Flow\WorkflowLoader, string)
#16 /srv/mediawiki/php-1.32.0-wmf.18/extensions/Flow/includes/Actions/Action.php(50): Flow\Actions\FlowAction->showForAction(string)
#17 /srv/mediawiki/php-1.32.0-wmf.18/includes/MediaWiki.php(501): Flow\Actions\FlowAction->show()
#18 /srv/mediawiki/php-1.32.0-wmf.18/includes/MediaWiki.php(294): MediaWiki->performAction(Article, Title)
#19 /srv/mediawiki/php-1.32.0-wmf.18/includes/MediaWiki.php(868): MediaWiki->performRequest()
#20 /srv/mediawiki/php-1.32.0-wmf.18/includes/MediaWiki.php(525): MediaWiki->main()
#21 /srv/mediawiki/php-1.32.0-wmf.18/index.php(42): MediaWiki->run()
#22 /srv/mediawiki/w/index.php(3): include(string)
#23 {main}

Details

Related Gerrit Patches:

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 21 2018, 8:37 PM

Adding StructuredDiscussions just because I see Flow in that stacktrace a lot. Feel free to retag.

Restricted Application added a project: Growth-Team. · View Herald TranscriptAug 21 2018, 8:40 PM
thcipriani triaged this task as Unbreak Now! priority.Aug 21 2018, 9:17 PM
Restricted Application added subscribers: Liuxinyu970226, TerraCodes. · View Herald TranscriptAug 21 2018, 9:17 PM

It looks to me like rMWd31580eeb0e9: [MCR] Render multi-slot diffs made a breaking change to DifferenceEngine. It has a lot of constructor parameters, but they're all marked optional. Flow does something like:

$differenceEngine = new \DifferenceEngine;
$differenceEngine->showDiffStyle();

(actual code here)

This worked in wmf.16 but fails in wmf.18:

catrope@mwmaint1001:~$ mwscript eval.php testwiki
> $de = new DifferenceEngine; $de->showDiffStyle();
[Tue Aug 21 21:49:57 2018] [hphp] [8336:7f5b6de923c0:0:000002] [] 
Catchable fatal error: Argument 1 passed to Revision::newFromTitle() must implement interface MediaWiki\Linker\LinkTarget, null given in /srv/mediawiki/php-1.32.0-wmf.18/includes/diff/DifferenceEngine.php on line 1662

catrope@mwmaint1001:~$ mwscript eval.php enwiki
> $de = new DifferenceEngine; $de->showDiffStyle();

>

It looks like the change to showDiffStyle() made it depend on the state of the object where it previously didn't. Flow tries to (ab)use DifferenceEngine as a stateless diff renderer, and that doesn't work anymore.

Change 454425 had a related patch set uploaded (by Catrope; owner: Catrope):
[mediawiki/extensions/Flow@master] Work around exception in DifferenceEngine::showDiffStyle()

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

Tgr added a subscriber: Tgr.Aug 22 2018, 8:14 AM

new DifferenceEngine() is not an "empty" diff object, it's a diff between the two last revisions of the current page. That did not change; what changed is that some methods including showDiffStyle did not care about what revisions are being diffed, and now they do since they have to delegate to the diff renderer of each slot, so they need to know what slots are there. And when it tries to load the state, somehow RequestContext::getTitle() returns null and things blow up. Not sure how that can happen; from the stack trace it seems MediaWiki is fully set up by this point so $wgTitle should always be a title, Special:BadTitle if nothing else.

I suppose there are two things to fix here:

  • DifferenceEngine should handle the context title being null
  • There should be a way to create an "empty" DifferenceEngine. Technically there is (something like $t = Title::newFromText( 'Special:BadTitle' ); $de = new DifferenceEngine(); $de->setRevisions( new MutableRevisionRecord( $t ), new MutableRevisionRecord( $t ) ); would probably work) but it's a bit obscure. So either have static factory method for that, or just assume that calling the constructor with no arguments means asking for an empty DifferenceEngine (it seems reasonable to assume that no one uses it for getting a diff of the last edit).

Change 454484 had a related patch set uploaded (by Gergő Tisza; owner: Gergő Tisza):
[mediawiki/core@master] DifferenceEngine: use a fake title when there's no real title

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

@Tgr just noting that the patch in 454484 doesn't fix the exception for Flow topic diff. To test, enable Flow on your user page and add a topic, then edit it, and attempt to view the revision. When I'm looking at this locally I'm not seeing the issue as being with RequestContext::getTitle(), but rather that getPrevious() in includes/Revision.php fails to load a previous revision for a Flow topic.

kostajh moved this task from Inbox to Current Sprint on the Growth-Team board.Aug 22 2018, 5:44 PM
kostajh edited projects, added Growth-Team (Current Sprint); removed Growth-Team.

So it seems like reverting the code that caused this would be too onerous. There is some doubt about the RequestContext::getTitle being the root cause. I'd be fine with a workaround in Flow to unblock train over the short-term @Catrope if you could get someone to review your change to Master branch of Flow, I can backport in the train window.

@thcipriani I'll +2 the Flow patch as soon as the tests finish running

Change 454626 had a related patch set uploaded (by Thcipriani; owner: Catrope):
[mediawiki/extensions/Flow@wmf/1.32.0-wmf.18] Work around exception in DifferenceEngine::showDiffStyle()

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

Change 454425 merged by jenkins-bot:
[mediawiki/extensions/Flow@master] Work around exception in DifferenceEngine::showDiffStyle()

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

Change 454626 merged by jenkins-bot:
[mediawiki/extensions/Flow@wmf/1.32.0-wmf.18] Work around exception in DifferenceEngine::showDiffStyle()

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

Mentioned in SAL (#wikimedia-operations) [2018-08-22T19:38:52Z] <thcipriani@deploy1001> Synchronized php-1.32.0-wmf.18/extensions/Flow/includes/TemplateHelper.php: [[gerrit:454626|Work around exception in DifferenceEngine::showDiffStyle()]] T202454 (duration: 00m 57s)

thcipriani lowered the priority of this task from Unbreak Now! to Normal.Aug 22 2018, 7:40 PM

The direct problem seems fixed for wmf.18, but I don't know if this is the Final™ fix or a work-around so: not closing, removing as train blocker, lowering priority.

Thank you all for your work on this!

Tgr added a comment.Aug 22 2018, 7:42 PM

The same issue might affect other extensions that use DifferenceEngine in a similar way (Translate, at least).

Change 454638 had a related patch set uploaded (by Gergő Tisza; owner: Gergő Tisza):
[mediawiki/core@master] DifferenceEngine: unbreak getSlotContents when mOldRev is missing

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

Etonkovidova closed this task as Resolved.Aug 23 2018, 7:46 PM
Etonkovidova claimed this task.
Etonkovidova added a subscriber: Etonkovidova.

Checked in betalabs and testwiki (wmf.18)

Change 454638 merged by jenkins-bot:
[mediawiki/core@master] Fix DifferenceEngine revision loading logic

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

Change 455254 had a related patch set uploaded (by Jforrester; owner: Gergő Tisza):
[mediawiki/core@wmf/1.32.0-wmf.18] Fix DifferenceEngine revision loading logic

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

Legoktm reopened this task as Open.Aug 27 2018, 5:27 AM

Re-opening since this is still an issue in production, and presumably needs a backport.

Legoktm raised the priority of this task from Normal to Unbreak Now!.Aug 27 2018, 5:27 AM

The same issue might affect other extensions that use DifferenceEngine in a similar way (Translate, at least).

That was reported as T202809 and marked as duplicate of this and this bug reopened. It is unclear to me whether the core changes are enough, or whether Translate needs a separate patch similar to Flow's https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Flow/+/454425.

Change 455254 merged by jenkins-bot:
[mediawiki/core@wmf/1.32.0-wmf.18] Fix DifferenceEngine revision loading logic

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

Mentioned in SAL (#wikimedia-operations) [2018-08-27T11:40:08Z] <tgr@deploy1001> Synchronized php-1.32.0-wmf.18/includes/diff/DifferenceEngine.php: SWAT: [[gerrit:455254|Fix DifferenceEngine revision loading logic (T201218, T202454)]] (duration: 00m 49s)

Tgr closed this task as Resolved.Aug 27 2018, 11:43 AM

The core changes should be enough in both cases. The fix is live; I have verified that the diff on top of the edit box for fuzzy translations works, that's my best guess at what triggered the error, but I'm not really sure about it (forgot to test it without the patch) so if you can think of other things to check please do so.

Wargo added a comment.Aug 28 2018, 7:48 AM

Please check on delete "[W4T9wApAIC0AAIMtO9sAAABO] 2018-08-28 07:46:09"

Tgr added a comment.Aug 28 2018, 8:11 AM

Please check on delete "[W4T9wApAIC0AAIMtO9sAAABO] 2018-08-28 07:46:09"

That's T200417: TranslateRenderJob.php: Cannot render translation page.

Change 454484 merged by jenkins-bot:
[mediawiki/core@master] DifferenceEngine: use a fake title when there's no real title

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

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