Page MenuHomePhabricator

Previewing a non-style-only gadget that you already have enabled causes a syntax error
Closed, ResolvedPublic

Description

I'm getting a syntax error when previewing any gadget page (CSS or JS) that belongs to any non-style-only gadget that I already have enabled. This happens in both Chrome and Firefox.
In Chrome, the error is
Uncaught SyntaxError: Unexpected token }.
In Firefox, the error is
SyntaxError: expected expression, got '}'.
The line seems to be the long mw.loader.load line with all the ResourceLoader modules.

It does not happen when previewing pages for pure style modules, like:
https://sv.wikipedia.org/wiki/MediaWiki:Gadget-oldeditlinks.css
https://sv.wikipedia.org/wiki/MediaWiki:Gadget-OldDiff.css

It does happen when previewing pages for non-style-only modules, like:
https://sv.wikipedia.org/wiki/MediaWiki:Gadget-ExkluderaRobotskapadeSidor.js (JavaScript page for JavaScript-only module)
https://sv.wikipedia.org/wiki/MediaWiki:Gadget-SearchEdit.js (JavaScript page for mixed module)
https://sv.wikipedia.org/wiki/MediaWiki:Gadget-InfogaRaderamall.css (CSS page for mixed module)

Is this because of T112474 and/or T180817?

Event Timeline

Nirmos created this task.Jul 27 2018, 10:06 AM
Restricted Application added a project: Performance-Team. · View Herald TranscriptJul 27 2018, 10:06 AM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
aaron assigned this task to Krinkle.Jul 30 2018, 8:10 PM
aaron triaged this task as Normal priority.
aaron moved this task from Inbox to Next In This Quarter on the Performance-Team board.

I can reproduce this on test2.wikipedia.org:

There is now the following syntax error in the console:

Uncaught SyntaxError: Unexpected token }

Source in the HTML:

This looks quite messed up indeed. Several problems:

  • The module closure is passed to a function called h() instead of mw.loader.implement(). No idea what this is.
  • The module closure ends with }); twice instead once. Causing the syntax error.
  • There is an unclosed comment at the end /* which causes another syntax error when it gets merged with an unrelated comment spanning into another module's source (user.options).

The two components that relate to previewing wiki modules are ResourceLoader's "embed module" feature, and the "content override" feature for between OutputPage and ResourceLoaderContext, as used by ResourceLoaderWikiModule.

The last major change to that was 3f1142045f5 / https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/340768/ / T112474.

Krinkle added a comment.EditedAug 7 2018, 3:02 PM

Turns out this isn't directly related to the "embed module" or "content override" behaviour, although it does seem that the underlying bug I found is only triggered by those behaviours (right now).

After tracing the PHP output from ResourceLoaderWikiModule::getContentObj to ResourceLoaderWikiModule::getScript to ResourceLoader::makeModuleResponse to ResourceLoaderClientHtml::makeLoad, I found the following:

if ( $embed ) {
	// ...
	$chunks[] = '' . ResourceLoader::makeInlineScript(
		$rl->makeModuleResponse( $context, $moduleSet ),
		$nonce
	);

Where ResourceLoader::makeInlineScript contains the following (simplified):

public static function makeInlineScript( $script, $nonce = null ) {
		$js = self::makeLoaderConditionalScript( $script );
		// ...
		return new WrappedString(
			Html::inlineScript( $js ),
			"<script>(window.RLQ=window.RLQ||[]).push(function(){",
			'});</script>'
		);
	}

This usually outputs a string like this:

<script>(window.RLQ=window.RLQ||[]).push(function(){mw.loader.implement("ext.gadget.foo@0srnwfl",function(){...});});</script>

However, for the gadget on sv.wikipedia.org and many other ones, it outputs this:

<script>/*<![CDATA[*/(window.RLQ=window.RLQ||[]).push(function(){mw.loader.implement("ext.gadget.foo@0srnwfl",function($,jQuery,require,module){mw.loader.load('https://www.mediawiki.org/w/index.php?title=MediaWiki:Gadget-modrollback.js&action=raw&ctype=text/javascript');});});/*]]>*/</script>

The important difference is the presence of /*<![CDATA[*/. This breaks the offset assumptions from WrappedString, thus, when it is trying to merge two such objects and strip the common prefix/suffix, it produces the following:

h(function(){mw.loader.implement("ext.gadget.foo@0srnwfl",function($,jQuery,require,module){mw.loader.load('https://www.mediawiki.org/w/index.php?title=MediaWiki:Gadget-modrollback.js&action=raw&ctype=text/javascript');});});/*

Which is exactly what we found. The h( is a left over from RLQ.push( which was meant to be stripped in its entirety, but the presence of CDATA made the prefix longer, so it only stripped part of it. The strange unclosed comment at the end is caused by the same problem. It normally ends with });</script>, but now ends with });/*]]>*/</script>. Stripping the expected length of characters, leaves behind });});/*]].

The trigger for this bug is any inline script from OutputPage (gadget, previewed user/site script, mw-config etc.) that contains an ampersand (&) or less-than (<) character.

Change 451006 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/core@master] Html: Add test coverage for inlineScript()

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

Change 451011 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/core@master] Html: Change inlineScript() from XML-specific CDATA to unicode escape

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

This problem would've been easier to find if WrappedString validated the prefix, request filed at T118663.

Change 451006 merged by jenkins-bot:
[mediawiki/core@master] Html: Add test coverage for inlineScript()

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

Change 453892 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/core@master] resourceloader: Refuse to preview content with </script>

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

Change 451011 merged by jenkins-bot:
[mediawiki/core@master] Html: Reject </script> from inlineScript() and leave rest unescaped

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

Change 453892 merged by jenkins-bot:
[mediawiki/core@master] resourceloader: Refuse to preview content with </script>

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

Krinkle closed this task as Resolved.Aug 30 2018, 9:22 PM
Krinkle removed a project: Patch-For-Review.

Change 456765 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/core@wmf/1.32.0-wmf.19] Html: Reject </script> from inlineScript() and leave rest unescaped

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

Change 456765 merged by jenkins-bot:
[mediawiki/core@wmf/1.32.0-wmf.19] Html: Reject </script> from inlineScript() and leave rest unescaped

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

Mentioned in SAL (#wikimedia-operations) [2018-08-31T23:35:51Z] <krinkle@deploy1001> Synchronized php-1.32.0-wmf.19/includes/Html.php: I67ceb34eabf2f - T200506 (duration: 00m 50s)