resourceloader: Use "\n" instead of ";" as separator for scripts

This fixes two bugs:

* 1) When two modules are requested, and the first one ends with ";"
     inside a comment, the second module might become part of
     that comment.
* 2) A request with script=only where the requested module content
     ends in a statement without ";", but has a comment after it
     that does ends with a semicolon, then in debug=false, mw.loader.state()
     would be appended directly after the semicolon-less statement because
     the check is performed before minification.
     Previously:
     script> foo()
     script> // bar();
     states> mw.loader.state( {} );
     Became (minified separately):
     script> foo()
     states> mw.loader.state({});
     Became (concatenated)
     > foo()mw.loader.state();
     Which is invalid code.

Both of these are now fixed.

Bug: T162719
Change-Id: Ic8114c46ce232f5869400eaa40d3027003550533

diff --git a/includes/resourceloader/ResourceLoader.php b/includes/resourceloader/ResourceLoader.php
index c11fe5b..8553116 100644 (file)
--- a/includes/resourceloader/ResourceLoader.php
+++ b/includes/resourceloader/ResourceLoader.php
@@ -1100,7 +1100,12 @@ MESSAGE;
                                        $strContent = self::filter( $filter, $strContent );
                                }
 
-                               $out .= $strContent;
+                               if ( $context->getOnly() === 'scripts' ) {
+                                       // Use a linebreak between module scripts (T162719)
+                                       $out .= $this->ensureNewline( $strContent );
+                               } else {
+                                       $out .= $strContent;
+                               }
 
                        } catch ( Exception $e ) {
                                $this->outputErrorAndLog( $e, 'Generating module package failed: {exception}' );
@@ -1128,7 +1133,8 @@ MESSAGE;
                                if ( !$context->getDebug() ) {
                                        $stateScript = self::filter( 'minify-js', $stateScript );
                                }
-                               $out .= $stateScript;
+                               // Use a linebreak between module script and state script (T162719)
+                               $out = $this->ensureNewline( $out ) . $stateScript;
                        }
                } else {
                        if ( count( $states ) ) {
@@ -1140,6 +1146,19 @@ MESSAGE;
                return $out;
        }
 
+       /**
+        * Ensure the string is either empty or ends in a line break
+        * @param string $str
+        * @return string
+        */
+       private function ensureNewline( $str ) {
+               $end = substr( $str, -1 );
+               if ( $end === false || $end === "\n" ) {
+                       return $str;
+               }
+               return $str . "\n";
+       }
+
        /**
         * Get names of modules that use a certain message.
         *

diff --git a/includes/resourceloader/ResourceLoaderModule.php b/includes/resourceloader/ResourceLoaderModule.php
index 3ad6a84..743b69b 100644 (file)
--- a/includes/resourceloader/ResourceLoaderModule.php
+++ b/includes/resourceloader/ResourceLoaderModule.php
@@ -643,16 +643,18 @@ abstract class ResourceLoaderModule implements LoggerAwareInterface {
                                $scripts = $this->getScriptURLsForDebug( $context );
                        } else {
                                $scripts = $this->getScript( $context );
-                               // rtrim() because there are usually a few line breaks
-                               // after the last ';'. A new line at EOF, a new line
-                               // added by ResourceLoaderFileModule::readScriptFiles, etc.
+                               // Make the script safe to concatenate by making sure there is at least one
+                               // trailing new line at the end of the content. Previously, this looked for
+                               // a semi-colon instead, but that breaks concatenation if the semicolon
+                               // is inside a comment like "// foo();". Instead, simply use a
+                               // line break as separator which matches JavaScript native logic for implicitly
+                               // ending statements even if a semi-colon is missing.
+                               // Bugs: T29054, T162719.
                                if ( is_string( $scripts )
                                        && strlen( $scripts )
-                                       && substr( rtrim( $scripts ), -1 ) !== ';'
+                                       && substr( $scripts, -1 ) !== "\n"
                                ) {
-                                       // Append semicolon to prevent weird bugs caused by files not
-                                       // terminating their statements right (T29054)
-                                       $scripts .= ";\n";
+                                       $scripts .= "\n";
                                }
                        }
                        $content['scripts'] = $scripts;

diff --git a/tests/phpunit/includes/OutputPageTest.php b/tests/phpunit/includes/OutputPageTest.php
index 32fa468..c3ed6dd 100644 (file)
--- a/tests/phpunit/includes/OutputPageTest.php
+++ b/tests/phpunit/includes/OutputPageTest.php
@@ -293,7 +293,7 @@ class OutputPageTest extends MediaWikiTestCase {
                        [
                                [ 'test.quux', ResourceLoaderModule::TYPE_SCRIPTS ],
                                "<script>(window.RLQ=window.RLQ||[]).push(function(){"
-                                       . "mw.test.baz({token:123});mw.loader.state({\"test.quux\":\"ready\"});"
+                                       . "mw.test.baz({token:123});\nmw.loader.state({\"test.quux\":\"ready\"});"
                                        . "});</script>"
                        ],
                ];

diff --git a/tests/phpunit/includes/resourceloader/ResourceLoaderModuleTest.php b/tests/phpunit/includes/resourceloader/ResourceLoaderModuleTest.php
index 6057b97..c861b37 100644 (file)
--- a/tests/phpunit/includes/resourceloader/ResourceLoaderModuleTest.php
+++ b/tests/phpunit/includes/resourceloader/ResourceLoaderModuleTest.php
@@ -98,11 +98,11 @@ class ResourceLoaderModuleTest extends ResourceLoaderTestCase {
                return [
                        [
                                "mw.foo()",
-                               "mw.foo();\n",
+                               "mw.foo()\n",
                        ],
                        [
                                "mw.foo();",
-                               "mw.foo();",
+                               "mw.foo();\n",
                        ],
                        [
                                "mw.foo();\n",
@@ -110,15 +110,19 @@ class ResourceLoaderModuleTest extends ResourceLoaderTestCase {
                        ],
                        [
                                "mw.foo()\n",
-                               "mw.foo()\n;\n",
+                               "mw.foo()\n",
                        ],
                        [
                                "mw.foo()\n// mw.bar();",
-                               "mw.foo()\n// mw.bar();",
+                               "mw.foo()\n// mw.bar();\n",
+                       ],
+                       [
+                               "mw.foo()\n// mw.bar()",
+                               "mw.foo()\n// mw.bar()\n",
                        ],
                        [
                                "mw.foo()// mw.bar();",
-                               "mw.foo()// mw.bar();",
+                               "mw.foo()// mw.bar();\n",
                        ],
                ];
        }

diff --git a/tests/phpunit/includes/resourceloader/ResourceLoaderTest.php b/tests/phpunit/includes/resourceloader/ResourceLoaderTest.php
index b7b2473..1422adc 100644 (file)
--- a/tests/phpunit/includes/resourceloader/ResourceLoaderTest.php
+++ b/tests/phpunit/includes/resourceloader/ResourceLoaderTest.php
@@ -623,10 +623,10 @@ mw.example();
                                'modules' => [
                                        'foo' => 'foo()',
                                ],
-                               'expected' => "foo();\n" . 'mw.loader.state( {
+                               'expected' => "foo()\n" . 'mw.loader.state( {
     "foo": "ready"
 } );',
-                               'minified' => "foo();" . 'mw.loader.state({"foo":"ready"});',
+                               'minified' => "foo()\n" . 'mw.loader.state({"foo":"ready"});',
                                'message' => 'Script without semi-colon',
                        ],
                        [
@@ -634,24 +634,22 @@ mw.example();
                                        'foo' => 'foo()',
                                        'bar' => 'bar()',
                                ],
-                               'expected' => "foo();\nbar();\n" . 'mw.loader.state( {
+                               'expected' => "foo()\nbar()\n" . 'mw.loader.state( {
     "foo": "ready",
     "bar": "ready"
 } );',
-                               'minified' => "foo();bar();" . 'mw.loader.state({"foo":"ready","bar":"ready"});',
+                               'minified' => "foo()\nbar()\n" . 'mw.loader.state({"foo":"ready","bar":"ready"});',
                                'message' => 'Two scripts without semi-colon',
                        ],
                        [
                                'modules' => [
                                        'foo' => "foo()\n// bar();"
                                ],
-                               // FIXME: Invalid code (T162719)
-                               'expected' => "foo()\n// bar();" . 'mw.loader.state( {
+                               'expected' => "foo()\n// bar();\n" . 'mw.loader.state( {
     "foo": "ready"
 } );',
-                               // FIXME: Invalid code (T162719)
-                               'minified' => "foo()" . 'mw.loader.state({"foo":"ready"});',
-                               'message' => 'Script with semi-colon in comment',
+                               'minified' => "foo()\n" . 'mw.loader.state({"foo":"ready"});',
+                               'message' => 'Script with semi-colon in comment (T162719)',
                        ],
                ];
                $ret = [];
@@ -695,7 +693,7 @@ mw.example();
                );
 
                $response = $rl->makeModuleResponse( $context, $modules );
-               $this->assertCount( 0, $rl->getErrors(), 'Error count' );
+               $this->assertSame( [], $rl->getErrors(), 'Errors' );
                $this->assertEquals( $expected, $response, $message ?: 'Response' );
        }
 
@@ -728,7 +726,7 @@ mw.example();
                $this->assertCount( 1, $errors );
                $this->assertRegExp( '/Ferry not found/', $errors[0] );
                $this->assertEquals(
-                       'foo();bar();mw.loader.state( {
+                       "foo();\nbar();\n" . 'mw.loader.state( {
     "ferry": "error",
     "foo": "ready",
     "bar": "ready"