resourceloader: Fix mw.loader to compute combined version in packed order

The 'version' param was being computed based on the order of the modules list
before we perform string compression. And this compression can change the order
for its optimisation purposes.

Specifically, when requesting modules like 'a', 'b.1', 'b.2' and 'c'. The
version was computed based on a + b.1 + b.2 + c (standard sort order),
whereas the optimised list is 'a,c|b.1,2', which expands to a + c + b.1 + b.2,
which makes hash validation fail.

Bug: T188076
Change-Id: I00d6985c054fecd88acf73041aa02878e83d62bc

diff --git a/resources/src/mediawiki/mediawiki.js b/resources/src/mediawiki/mediawiki.js
index 7d8c3bc..3fe276b 100644 (file)
--- a/resources/src/mediawiki/mediawiki.js
+++ b/resources/src/mediawiki/mediawiki.js
@@ -1523,17 +1523,28 @@
                         *
                         * @private
                         * @param {Object} moduleMap Module map
-                        * @return {string} Module query string
+                        * @return {Object}
+                        * @return {string} return.str Module query string
+                        * @return {Array} return.list List of module names in matching order
                         */
                        function buildModulesString( moduleMap ) {
                                var p, prefix,
-                                       arr = [];
+                                       str = [],
+                                       list = [];
+
+                               function restore( suffix ) {
+                                       return p + suffix;
+                               }
 
                                for ( prefix in moduleMap ) {
                                        p = prefix === '' ? '' : prefix + '.';
-                                       arr.push( p + moduleMap[ prefix ].join( ',' ) );
+                                       str.push( p + moduleMap[ prefix ].join( ',' ) );
+                                       list.push.apply( list, moduleMap[ prefix ].map( restore ) );
                                }
-                               return arr.join( '|' );
+                               return {
+                                       str: str.join( '|' ),
+                                       list: list
+                               };
                        }
 
                        /**
@@ -1586,9 +1597,15 @@
                                 */
                                function doRequest() {
                                        // Optimisation: Inherit (Object.create), not copy ($.extend)
-                                       var query = Object.create( currReqBase );
-                                       query.modules = buildModulesString( moduleMap );
-                                       query.version = getCombinedVersion( currReqModules );
+                                       var query = Object.create( currReqBase ),
+                                               packed = buildModulesString( moduleMap );
+                                       query.modules = packed.str;
+                                       // The packing logic can change the effective order, even if the input was
+                                       // sorted. As such, the call to getCombinedVersion() must use this
+                                       // effective order, instead of currReqModules, as otherwise the combined
+                                       // version will not match the hash expected by the server based on
+                                       // combining versions from the module query string in-order. (T188076)
+                                       query.version = getCombinedVersion( packed.list );
                                        query = sortQuery( query );
                                        addScript( sourceLoadScript + '?' + $.param( query ) );
                                }

diff --git a/tests/qunit/data/load.mock.php b/tests/qunit/data/load.mock.php
index 43ee0c7..2300949 100644 (file)
--- a/tests/qunit/data/load.mock.php
+++ b/tests/qunit/data/load.mock.php
@@ -59,6 +59,15 @@ mw.loader.implement( 'testUrlInc.a', function () {} );
 ",
        'testUrlInc.b' => "
 mw.loader.implement( 'testUrlInc.b', function () {} );
+",
+       'testUrlOrder' => "
+mw.loader.implement( 'testUrlOrder', function () {} );
+",
+       'testUrlOrder.a' => "
+mw.loader.implement( 'testUrlOrder.a', function () {} );
+",
+       'testUrlOrder.b' => "
+mw.loader.implement( 'testUrlOrder.b', function () {} );
 ",
 ];
 
@@ -70,6 +79,8 @@ $response = '';
 if ( isset( $_GET['modules'] ) ) {
        if ( $_GET['modules'] === 'testUrlInc,testUrlIncDump|testUrlInc.a,b' ) {
                $modules = [ 'testUrlInc', 'testUrlIncDump', 'testUrlInc.a', 'testUrlInc.b' ];
+       } elseif ( $_GET['modules'] === 'testUrlOrder,testUrlOrderDump|testUrlOrder.a,b' ) {
+               $modules = [ 'testUrlOrder', 'testUrlOrderDump', 'testUrlOrder.a', 'testUrlOrder.b' ];
        } else {
                $modules = explode( ',', $_GET['modules'] );
        }

diff --git a/tests/qunit/suites/resources/mediawiki/mediawiki.loader.test.js b/tests/qunit/suites/resources/mediawiki/mediawiki.loader.test.js
index 6c988ef..0b05ac1 100644 (file)
--- a/tests/qunit/suites/resources/mediawiki/mediawiki.loader.test.js
+++ b/tests/qunit/suites/resources/mediawiki/mediawiki.loader.test.js
@@ -583,6 +583,40 @@
                } );
        } );
 
+       // @covers mw.loader#batchRequest
+       // @covers mw.loader#buildModulesString
+       QUnit.test( 'Url composition (order of modules for version) – T188076', function ( assert ) {
+               mw.loader.register( [
+                       // [module, version, dependencies, group, source]
+                       [ 'testUrlOrder', 'url', [], null, 'testloader' ],
+                       [ 'testUrlOrder.a', '1', [], null, 'testloader' ],
+                       [ 'testUrlOrder.b', '2', [], null, 'testloader' ],
+                       [ 'testUrlOrderDump', 'dump', [], null, 'testloader' ]
+               ] );
+
+               return mw.loader.using( [
+                       'testUrlOrderDump',
+                       'testUrlOrder.b',
+                       'testUrlOrder.a',
+                       'testUrlOrder'
+               ] ).then( function ( require ) {
+                       assert.propEqual(
+                               require( 'testUrlOrderDump' ).query,
+                               {
+                                       modules: 'testUrlOrder,testUrlOrderDump|testUrlOrder.a,b',
+                                       // Expected: Combined in order after string packing
+                                       //   $hash = hash( 'fnv132', 'urldump12' );
+                                       //   base_convert( $hash, 16, 36 ); // "1knqzan"
+                                       // Previously: Combined in order of before string packing
+                                       //   $hash = hash( 'fnv132', 'url12dump' );
+                                       //   base_convert( $hash, 16, 36 ); // "11eo3in"
+                                       version: '1knqzan'
+                               },
+                               'Query parameters'
+                       );
+               } );
+       } );
+
        QUnit.test( 'Broken indirect dependency', function ( assert ) {
                // don't emit an error event
                this.sandbox.stub( mw, 'track' );