resourceloader: Condition-wrap the HTML tag instead of JS response
authorTimo Tijhof <krinklemail@gmail.com>
Mon, 8 Sep 2014 16:31:54 +0000 (18:31 +0200)
committerKrinkle <krinklemail@gmail.com>
Tue, 9 Sep 2014 15:54:16 +0000 (15:54 +0000)
Follows-up 9272bc6c4703c503da221e063f6078.

One can't wrap arbitrary JavaScript in an if-statement and have
its inner-body mean exactly the same.

Certain statements are only allowed in the top of a scope (such
as hoisted function declarations). These are not allowed inside
a block. They're fine in both global scope and local function
scope, but not inside an if-block of any scope.

The ECMAScript spec only describes what is an allowed token.
Any unexpected token should result in a SyntaxError.

Chrome's implementation (V8) allows function declarations in
blocks and hoists them to *outside* the condition. Firefox's
SpiderMonkey silently ignores the statement. Neither throw a
SyntaxError.

Rgular ResourceLoader responses only contain mw.loader.implement()
and mw.loader.state() call which could be wrapped without issues.
However such responses don't need wrapping as they're only made
by mediawiki.js (in which case mw is obviously loaded). The
wrapping is for legacy scripts that execute in the global scope.

For those, let's wrap the script tag itself (instead of the
response). That seems like the most water-tight and semantically
correct solution.

Had to bring in $isRaw from ResourceLoader.php, else the startup
module would have been wrapped as well (added regression test).

Bug: 69924
Change-Id: Iedda0464f734ba5f7a884726487f6c7e07d444f1

includes/OutputPage.php
includes/resourceloader/ResourceLoader.php
tests/phpunit/ResourceLoaderTestCase.php
tests/phpunit/includes/OutputPageTest.php

index af90ca6..22a6012 100644 (file)
@@ -2769,7 +2769,10 @@ $templates
                                );
                                $context = new ResourceLoaderContext( $resourceLoader, new FauxRequest( $query ) );
 
-                               // Extract modules that know they're empty
+
+                               // Extract modules that know they're empty and see if we have one or more
+                               // raw modules
+                               $isRaw = false;
                                foreach ( $grpModules as $key => $module ) {
                                        // Inline empty modules: since they're empty, just mark them as 'ready' (bug 46857)
                                        // If we're only getting the styles, we don't need to do anything for empty modules.
@@ -2779,6 +2782,8 @@ $templates
                                                        $links['states'][$key] = 'ready';
                                                }
                                        }
+
+                                       $isRaw |= $module->isRaw();
                                }
 
                                // If there are no non-empty modules, skip this group
@@ -2845,6 +2850,17 @@ $templates
                                                );
                                        } else {
                                                $link = Html::linkedScript( $url );
+                                               if ( $context->getOnly() === 'scripts' && !$context->getRaw() && !$isRaw ) {
+                                                       // Wrap only=script requests in a conditional as browsers not supported
+                                                       // by the startup module would unconditionally execute this module.
+                                                       // Otherwise users will get "ReferenceError: mw is undefined" or
+                                                       // "jQuery is undefined" from e.g. a "site" module.
+                                                       $link = Html::inlineScript(
+                                                               ResourceLoader::makeLoaderConditionalScript(
+                                                                       Xml::encodeJsCall( 'document.write', array( $link ) )
+                                                               )
+                                                       );
+                                               }
 
                                                // For modules requested directly in the html via <link> or <script>,
                                                // tell mw.loader they are being loading to prevent duplicate requests.
index 918f8be..f6a9d84 100644 (file)
@@ -1002,15 +1002,6 @@ class ResourceLoader {
                        if ( count( $states ) ) {
                                $out .= self::makeLoaderStateScript( $states );
                        }
-
-                       if ( $context->getOnly() === 'scripts' ) {
-                               // In only=script requests for modules that are not raw (e.g. not the startup module)
-                               // ensure the execution is conditional to avoid situations where browsers with an
-                               // unsupported environment do unconditionally execute a module's scripts. Otherwise users
-                               // will get things like "ReferenceError: mw is undefined" or "jQuery is undefined" from
-                               // legacy scripts loaded with only=scripts (such as the 'site' module).
-                               $out = self::makeLoaderConditionalScript( $out );
-                       }
                } else {
                        if ( count( $states ) ) {
                                $exceptions .= self::makeComment(
index dc5549b..f5f302e 100644 (file)
@@ -46,6 +46,7 @@ class ResourceLoaderTestModule extends ResourceLoaderModule {
        protected $script = '';
        protected $styles = '';
        protected $skipFunction = null;
+       protected $isRaw = false;
        protected $targets = array( 'test' );
 
        public function __construct( $options = array() ) {
@@ -77,6 +78,10 @@ class ResourceLoaderTestModule extends ResourceLoaderModule {
        public function getSkipFunction() {
                return $this->skipFunction;
        }
+
+       public function isRaw() {
+               return $this->isRaw;
+       }
 }
 
 class ResourceLoaderFileModuleTestModule extends ResourceLoaderFileModule {
index 97fa85c..d7e8cd3 100644 (file)
@@ -141,7 +141,15 @@ class OutputPageTest extends MediaWikiTestCase {
                        // Load module script only
                        array(
                                array( 'test.foo', ResourceLoaderModule::TYPE_SCRIPTS ),
-                               '<script src="http://127.0.0.1:8080/w/load.php?debug=false&amp;lang=en&amp;modules=test.foo&amp;only=scripts&amp;skin=fallback&amp;*"></script>
+                               '<script>if(window.mw){
+document.write("\u003Cscript src=\"http://127.0.0.1:8080/w/load.php?debug=false\u0026amp;lang=en\u0026amp;modules=test.foo\u0026amp;only=scripts\u0026amp;skin=fallback\u0026amp;*\"\u003E\u003C/script\u003E");
+}</script>
+'
+                       ),
+                       array(
+                               // Don't condition wrap raw modules (like the startup module)
+                               array( 'test.raw', ResourceLoaderModule::TYPE_SCRIPTS ),
+                               '<script src="http://127.0.0.1:8080/w/load.php?debug=false&amp;lang=en&amp;modules=test.raw&amp;only=scripts&amp;skin=fallback&amp;*"></script>
 '
                        ),
                        // Load module styles only
@@ -152,14 +160,10 @@ class OutputPageTest extends MediaWikiTestCase {
 '
                        ),
                        // Load private module (only=scripts)
-                       // This is asserted for completion (would get two condition wrappers),
-                       // though in practice we'd never embed a module with only=scripts,
-                       // that mode is reserved for hardcoded requests. Embedded modules
-                       // would always be combined.
                        array(
                                array( 'test.quux', ResourceLoaderModule::TYPE_SCRIPTS ),
                                '<script>if(window.mw){
-if(window.mw){mw.test.baz({token:123});mw.loader.state({"test.quux":"ready"});}
+mw.test.baz({token:123});mw.loader.state({"test.quux":"ready"});
 
 }</script>
 '
@@ -244,17 +248,21 @@ mw.loader.implement("test.quux",function($,jQuery){mw.test.baz({token:123});},{"
                                'styles' => '/* pref-animate=off */ .mw-icon { transition: none; }',
                                'group' => 'private',
                        )),
+                       'test.raw' => new ResourceLoaderTestModule( array(
+                               'script' => 'mw.test.baz( { token: 123 } );',
+                               'isRaw' => true,
+                       )),
                        'test.noscript' => new ResourceLoaderTestModule( array(
                                'styles' => '.mw-test-noscript { content: "style"; }',
                                'group' => 'noscript',
                        )),
                        'test.group.bar' => new ResourceLoaderTestModule( array(
-                                       'styles' => '.mw-group-bar { content: "style"; }',
-                                       'group' => 'bar',
+                               'styles' => '.mw-group-bar { content: "style"; }',
+                               'group' => 'bar',
                        )),
                        'test.group.foo' => new ResourceLoaderTestModule( array(
-                                       'styles' => '.mw-group-foo { content: "style"; }',
-                                       'group' => 'foo',
+                               'styles' => '.mw-group-foo { content: "style"; }',
+                               'group' => 'foo',
                        )),
                ) );
                $links = $method->invokeArgs( $out, $args );