Page MenuHomePhabricator

Javascript error while clicking on Echo alert icon in Special:CX
Closed, ResolvedPublic

Description

Uncaught TypeError: Cannot read property 'alert' of null
mw.echo.dm.NotificationsModel.getSeenTime @ mw.echo.dm.NotificationsModel.js:164
(anonymous function) @ mw.echo.dm.NotificationsModel.js:274
(anonymous function) @ load.php?debug=true&lang=en&modules=jquery%2Cmediawiki&only=scripts&skin=vector&version=UqQ%2B4baa:3306
jQuery.Callbacks.fire @ load.php?debug=true&lang=en&modules=jquery%2Cmediawiki&only=scripts&skin=vector&version=UqQ%2B4baa:3149
jQuery.Callbacks.self.fireWith @ load.php?debug=true&lang=en&modules=jquery%2Cmediawiki&only=scripts&skin=vector&version=UqQ%2B4baa:3261
jQuery.extend.Deferred.jQuery.each.deferred.(anonymous function) @ load.php?debug=true&lang=en&modules=jquery%2Cmediawiki&only=scripts&skin=vector&version=UqQ%2B4baa:3351
(anonymous function) @ mediawiki.api.js:221
jQuery.Callbacks.fire @ load.php?debug=true&lang=en&modules=jquery%2Cmediawiki&only=scripts&skin=vector&version=UqQ%2B4baa:3149
jQuery.Callbacks.self.fireWith @ load.php?debug=true&lang=en&modules=jquery%2Cmediawiki&only=scripts&skin=vector&version=UqQ%2B4baa:3261
done @ load.php?debug=true&lang=en&modules=jquery%2Cmediawiki&only=scripts&skin=vector&version=UqQ%2B4baa:9315
jQuery.ajaxTransport.options.send.callback @ load.php?debug=true&lang=en&modules=jquery%2Cmediawiki&only=scripts&skin=vector&version=UqQ%2B4baa:9719

Originating from

mw.echo.dm.NotificationsModel.prototype.getSeenTime = function () {
	return this.seenTime[this.type];
};

Event Timeline

santhosh raised the priority of this task from to Needs Triage.
santhosh updated the task description. (Show Details)
santhosh added subscribers: Legoktm, santhosh, Aklapper.

I can't reproduce this. Can you point me to the exact page this happened in? What wiki?

Mooeypoo triaged this task as High priority.
Mooeypoo set Security to None.

You can reproduce this by going to http://en.wikipedia.beta.wmflabs.org/wiki/Special:ContentTranslation and clicking on the alert icon. Note that Special:ContentTranslation is CX special page. So first you need to login, enable CX beta featre and then access the above link.

Okay, I managed to reproduce this, but this is extremely baffling to me.

this.seenTime (which is what the script is complaining about being null) is set in the model's constructor:

		this.seenTime = mw.config.get( 'wgEchoSeenTime' );

The configuration object is created in the Hooks.php file's "onPersonalUrls", which should *always* run before Echo loads:

		$sk->getOutput()->addJsConfigVars( 'wgEchoSeenTime', array(
			'alert' => $seenAlertTime,
			'message' => $seenMsgTime
		) );

There should be no case ever where this isn't the case -- and yet, clearly this is such a case.

The more baffling thing to me is that when I disabled Javascript and reloaded, while the ContentTranslation system didn't load (understandably) the two badges did appear, with their proper styling and the correct number of notifications. This badge is being built in the same place that the line above runs, in Hooks.php#onPersonalUrls

Is there anything at all that could interfere with mw.config.get( ... ) in that page? Or anything that manipulates JsConfigVars that may be clashing with Echo?

I'm continuing to research, but this is really really weird.

This is because Echo ads the 'wgEchoSeenTime' variable in the PersonalUrls hook. ContentTranslation re-invents the wheel and bypasses the normal MW page loading procedure, so you end up with (from SpecialContentTranslation::execute):

  1. OutputPage::headElement() - adds mw.config variables to HTML
  2. SkinTemplate::getPersonalToolsList() - triggers PersonalUrls hook

→ ContentTranslation bug. What Echo is doing is totally fine,

Roan and I just spent a while diagnosing the same thing too.

I'm going to move the variable creation to onBeforePageDisplay just in case anyways. We are caching the getTime() requests anyways, and it is probably a better move in general.

But yes, CX should probably re-evaluate the order of things as well.

Change 236730 had a related patch set uploaded (by Mooeypoo):
Create JsConfigVars in onBeforePageDisplay

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

Change 236731 had a related patch set uploaded (by Legoktm):
Generate personal tools before head element

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

Set of variables present on [[Flow]] but absent on [[Special:CX]]:

> OO.simpleArrayDifference(x,y)
["wgBackendResponseTime", "wgEchoSeenTime", "wgHostname", "wgPageTriagePagePrefixedText", "wgPageTriagelastUseExpired", "wgRestrictionEdit", "wgRestrictionMove"]

The PageTriage and Restriction vars are probably not added for special pages, but wgBackendResponseTime and wgHostName should definitely be added. Those are coming from wfReportTime() and added separately by SkinTemplate, but CX's override seems to skip those too.

We will work around this in Echo by adding wgEchoSeenTime in a different place, but ContentTranslation really really should not override and then duplicate parts of OutputPage this way. More bugs like these in random other extensions (or in MW core, if anything attempts to use wgBackendResponseTime for example) are likely to happen until CX uses the normal page loading code rather than bypassing it.

...or legoktm can put a workaround for this particular issue in CX, that works too

Change 236730 abandoned by Catrope:
Create JsConfigVars in onBeforePageDisplay

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

Checked in betalabs on Special:ContentTranslation.

DannyH added a subscriber: DannyH.