resourceloader: Fix mw.loader to compute version for current request only
authorTimo Tijhof <krinklemail@gmail.com>
Thu, 1 Mar 2018 04:19:28 +0000 (20:19 -0800)
committerKrinkle <krinklemail@gmail.com>
Wed, 7 Mar 2018 21:09:08 +0000 (21:09 +0000)
Previously, the 'version' query parameter was computed before request-splitting
which meant that all requests within the same 'source/group'-batch carried the
same 'version' parameter. This was then consistently rejected on the server due
to it not batching the combined hash of modules for any given request.

In practice this happened very rarely (if at all) in production, because
urls don't usually hit anywhere near 2000 in common use.

Bug: T188076
Change-Id: I211523d4781623873887a05d048f56cccd28432c

resources/src/mediawiki/mediawiki.js
tests/qunit/data/load.mock.php
tests/qunit/suites/resources/mediawiki/mediawiki.loader.test.js

index aa93ca2..450d4ff 100644 (file)
                         *
                         * @private
                         * @param {string} moduleStr Module list for load.php `module` query parameter
-                        * @param {Object} currReqBase Object with other parameters (other than 'modules') to use in the request
+                        * @param {Array} modules List of module names (not packed)
+                        * @param {Object} currReqBase Object with base parameters for this request
                         * @param {string} sourceLoadScript URL of load.php
                         */
-                       function doRequest( moduleStr, currReqBase, sourceLoadScript ) {
+                       function doRequest( moduleStr, modules, currReqBase, sourceLoadScript ) {
                                // Optimisation: Inherit (Object.create), not copy ($.extend)
                                var query = Object.create( currReqBase );
                                query.modules = moduleStr;
+                               query.version = getCombinedVersion( modules );
                                query = sortQuery( query );
                                addScript( sourceLoadScript + '?' + $.param( query ) );
                        }
                        function batchRequest( batch ) {
                                var reqBase, splits, maxQueryLength, b, bSource, bGroup, bSourceGroup,
                                        source, group, i, modules, sourceLoadScript,
-                                       currReqBase, currReqBaseLength, moduleMap, l,
+                                       currReqBase, currReqBaseLength, moduleMap, currReqModules, l,
                                        lastDotIndex, prefix, suffix, bytesAdded;
 
                                if ( !batch.length ) {
                                // misses for otherwise identical content.
                                batch.sort();
 
-                               // Build a list of query parameters common to all requests
+                               // Query parameters common to all requests
                                reqBase = {
                                        skin: mw.config.get( 'skin' ),
                                        lang: mw.config.get( 'wgUserLanguage' ),
                                                // modules for this group from this source.
                                                modules = splits[ source ][ group ];
 
+                                               // Query parameters common to requests for this module group
                                                // Optimisation: Inherit (Object.create), not copy ($.extend)
                                                currReqBase = Object.create( reqBase );
-                                               currReqBase.version = getCombinedVersion( modules );
-
-                                               // For user modules append a user name to the query string.
+                                               // User modules require a user name in the query string.
                                                if ( group === 'user' && mw.config.get( 'wgUserName' ) !== null ) {
                                                        currReqBase.user = mw.config.get( 'wgUserName' );
                                                }
-                                               currReqBaseLength = $.param( currReqBase ).length;
+
+                                               // In addition to currReqBase, doRequest() will also add 'modules' and 'version'.
+                                               // > '&modules='.length === 9
+                                               // > '&version=1234567'.length === 16
+                                               // > 9 + 16 = 25
+                                               currReqBaseLength = $.param( currReqBase ).length + 25;
+
                                                // We may need to split up the request to honor the query string length limit,
                                                // so build it piece by piece.
-                                               l = currReqBaseLength + 9; // '&modules='.length == 9
-
+                                               l = currReqBaseLength;
                                                moduleMap = {}; // { prefix: [ suffixes ] }
+                                               currReqModules = [];
 
                                                for ( i = 0; i < modules.length; i++ ) {
                                                        // Determine how many bytes this module would add to the query string
                                                        lastDotIndex = modules[ i ].lastIndexOf( '.' );
-
                                                        // If lastDotIndex is -1, substr() returns an empty string
                                                        prefix = modules[ i ].substr( 0, lastDotIndex );
                                                        suffix = modules[ i ].slice( lastDotIndex + 1 );
-
                                                        bytesAdded = hasOwn.call( moduleMap, prefix ) ?
                                                                suffix.length + 3 : // '%2C'.length == 3
                                                                modules[ i ].length + 3; // '%7C'.length == 3
 
-                                                       // If the url would become too long, create a new one,
-                                                       // but don't create empty requests
-                                                       if ( maxQueryLength > 0 && !$.isEmptyObject( moduleMap ) && l + bytesAdded > maxQueryLength ) {
-                                                               // This url would become too long, create a new one, and start the old one
-                                                               doRequest( buildModulesString( moduleMap ), currReqBase, sourceLoadScript );
+                                                       // If the url would become too long, create a new one, but don't create empty requests
+                                                       if ( maxQueryLength > 0 && currReqModules.length && l + bytesAdded > maxQueryLength ) {
+                                                               // Dispatch what we've got...
+                                                               doRequest( buildModulesString( moduleMap ), currReqModules, currReqBase, sourceLoadScript );
+                                                               // .. and start again.
+                                                               l = currReqBaseLength;
                                                                moduleMap = {};
-                                                               l = currReqBaseLength + 9;
+                                                               currReqModules = [];
+
                                                                mw.track( 'resourceloader.splitRequest', { maxQueryLength: maxQueryLength } );
                                                        }
                                                        if ( !hasOwn.call( moduleMap, prefix ) ) {
                                                                moduleMap[ prefix ] = [];
                                                        }
-                                                       moduleMap[ prefix ].push( suffix );
                                                        l += bytesAdded;
+                                                       moduleMap[ prefix ].push( suffix );
+                                                       currReqModules.push( modules[ i ] );
                                                }
                                                // If there's anything left in moduleMap, request that too
-                                               if ( !$.isEmptyObject( moduleMap ) ) {
-                                                       doRequest( buildModulesString( moduleMap ), currReqBase, sourceLoadScript );
+                                               if ( currReqModules.length ) {
+                                                       doRequest( buildModulesString( moduleMap ), currReqModules, currReqBase, sourceLoadScript );
                                                }
                                        }
                                }
index fed5746..43ee0c7 100644 (file)
@@ -49,18 +49,45 @@ mw.loader.implement( 'testNotSkipped', function () {}, {}, {});
 
        'testUsesSkippable' => "
 mw.loader.implement( 'testUsesSkippable', function () {}, {}, {});
+",
+
+       'testUrlInc' => "
+mw.loader.implement( 'testUrlInc', function () {} );
+",
+       'testUrlInc.a' => "
+mw.loader.implement( 'testUrlInc.a', function () {} );
+",
+       'testUrlInc.b' => "
+mw.loader.implement( 'testUrlInc.b', function () {} );
 ",
 ];
 
 $response = '';
 
-// Only support for non-encoded module names, full module names expected
+// Does not support the full behaviour of ResourceLoaderContext::expandModuleNames(),
+// Only supports dotless module names joined by comma,
+// with the exception of the hardcoded cases for testUrl*.
 if ( isset( $_GET['modules'] ) ) {
-       $modules = explode( ',', $_GET['modules'] );
+       if ( $_GET['modules'] === 'testUrlInc,testUrlIncDump|testUrlInc.a,b' ) {
+               $modules = [ 'testUrlInc', 'testUrlIncDump', 'testUrlInc.a', 'testUrlInc.b' ];
+       } else {
+               $modules = explode( ',', $_GET['modules'] );
+       }
        foreach ( $modules as $module ) {
                if ( isset( $moduleImplementations[$module] ) ) {
                        $response .= $moduleImplementations[$module];
+               } elseif ( preg_match( '/^test.*Dump$/', $module ) === 1 ) {
+                       $queryModules = $_GET['modules'];
+                       $queryVersion = isset( $_GET['version'] ) ? strval( $_GET['version'] ) : null;
+                       $response .= 'mw.loader.implement( ' . json_encode( $module )
+                               . ', function ( $, jQuery, require, module ) {'
+                               . 'module.exports.query = { '
+                               . 'modules: ' . json_encode( $queryModules ) . ','
+                               . 'version: ' . json_encode( $queryVersion )
+                               . ' };'
+                               . '} );';
                } else {
+                       // Default
                        $response .= 'mw.loader.state(' . json_encode( $module ) . ', "missing" );' . "\n";
                }
        }
index e774112..6c988ef 100644 (file)
                assert.strictEqual( mw.loader.getState( 'test.empty' ), 'ready' );
        } );
 
+       // @covers mw.loader#batchRequest
+       // This is a regression test because in the past we called getCombinedVersion()
+       // for all requested modules, before url splitting took place.
+       // Discovered as part of T188076, but not directly related.
+       QUnit.test( 'Url composition (modules considered for version)', function ( assert ) {
+               mw.loader.register( [
+                       // [module, version, dependencies, group, source]
+                       [ 'testUrlInc', 'url', [], null, 'testloader' ],
+                       [ 'testUrlIncDump', 'dump', [], null, 'testloader' ]
+               ] );
+
+               mw.config.set( 'wgResourceLoaderMaxQueryLength', 10 );
+
+               return mw.loader.using( [ 'testUrlIncDump', 'testUrlInc' ] ).then( function ( require ) {
+                       assert.propEqual(
+                               require( 'testUrlIncDump' ).query,
+                               {
+                                       modules: 'testUrlIncDump',
+                                       // Expected: Wrapped hash just for this one module
+                                       //   $hash = hash( 'fnv132', 'dump');
+                                       //   base_convert( $hash, 16, 36 ); // "13e9zzn"
+                                       // Previously: Wrapped hash for both modules, despite being in separate requests
+                                       //   $hash = hash( 'fnv132', 'urldump' );
+                                       //   base_convert( $hash, 16, 36 ); // "18kz9ca"
+                                       version: '13e9zzn'
+                               },
+                               'Query parameters'
+                       );
+
+                       assert.strictEqual( mw.loader.getState( 'testUrlInc' ), 'ready', 'testUrlInc also loaded' );
+               } );
+       } );
+
        QUnit.test( 'Broken indirect dependency', function ( assert ) {
                // don't emit an error event
                this.sandbox.stub( mw, 'track' );