resourceloader: Don't cache stale responses in mw.loader.store
authorTimo Tijhof <krinklemail@gmail.com>
Mon, 12 Sep 2016 20:52:10 +0000 (13:52 -0700)
committerOri.livneh <ori@wikimedia.org>
Mon, 10 Oct 2016 19:48:25 +0000 (19:48 +0000)
Follows-up 6fa1e56. This is already fixed for http caches by
shortening the Cache-Control max-age in case of a version mismatch.

However the client still cached it blindly in mw.loader.store.
Resolve this by communicating to the client what version of the module
was exported. The client can then compare this version to the version
it originally requested and decide not to cache it.

Adopt the module key format (name@version) from mw.loader.store
in mw.loader.implement() as well.

Bug: T117587
Change-Id: I1a7c44d0222893afefac20bef507bdd1a1a87ecd

includes/resourceloader/ResourceLoader.php
resources/src/mediawiki/mediawiki.js
tests/phpunit/ResourceLoaderTestCase.php
tests/phpunit/includes/resourceloader/ResourceLoaderClientHtmlTest.php
tests/phpunit/includes/resourceloader/ResourceLoaderStartUpModuleTest.php
tests/qunit/suites/resources/mediawiki/mediawiki.loader.test.js

index 34a0a99..143f5cc 100644 (file)
@@ -1009,6 +1009,7 @@ MESSAGE;
                foreach ( $modules as $name => $module ) {
                        try {
                                $content = $module->getModuleContent( $context );
+                               $implementKey = $name . '@' . $module->getVersionHash( $context );
                                $strContent = '';
 
                                // Append output
@@ -1020,7 +1021,7 @@ MESSAGE;
                                                        $strContent = $scripts;
                                                } elseif ( is_array( $scripts ) ) {
                                                        // ...except when $scripts is an array of URLs
-                                                       $strContent = self::makeLoaderImplementScript( $name, $scripts, [], [], [] );
+                                                       $strContent = self::makeLoaderImplementScript( $implementKey, $scripts, [], [], [] );
                                                }
                                                break;
                                        case 'styles':
@@ -1046,7 +1047,7 @@ MESSAGE;
                                                        }
                                                }
                                                $strContent = self::makeLoaderImplementScript(
-                                                       $name,
+                                                       $implementKey,
                                                        $scripts,
                                                        isset( $content['styles'] ) ? $content['styles'] : [],
                                                        isset( $content['messagesBlob'] ) ? new XmlJsCode( $content['messagesBlob'] ) : [],
@@ -1125,7 +1126,7 @@ MESSAGE;
        /**
         * Return JS code that calls mw.loader.implement with given module properties.
         *
-        * @param string $name Module name
+        * @param string $name Module name or implement key (format "`[name]@[version]`")
         * @param XmlJsCode|array|string $scripts Code as XmlJsCode (to be wrapped in a closure),
         *  list of URLs to JavaScript files, or a string of JavaScript for `$.globalEval`.
         * @param mixed $styles Array of CSS strings keyed by media type, or an array of lists of URLs
index c7715e5..2750765 100644 (file)
                                }
                        }
 
+                       /**
+                        * Make a versioned key for a specific module.
+                        *
+                        * @private
+                        * @param {string} module Module name
+                        * @return {string|null} Module key in format '`[name]@[version]`',
+                        *  or null if the module does not exist
+                        */
+                       function getModuleKey( module ) {
+                               return hasOwn.call( registry, module ) ?
+                                       ( module + '@' + registry[ module ].version ) : null;
+                       }
+
+                       /**
+                        * @private
+                        * @param {string} key Module name or '`[name]@[version]`'
+                        * @return {Object}
+                        */
+                       function splitModuleKey( key ) {
+                               var index = key.indexOf( '@' );
+                               if ( index === -1 ) {
+                                       return { name: key };
+                               }
+                               return {
+                                       name: key.slice( 0, index ),
+                                       version: key.slice( index )
+                               };
+                       }
+
                        /* Public Members */
                        return {
                                /**
                                 * When #load() or #using() requests one or more modules, the server
                                 * response contain calls to this function.
                                 *
-                                * @param {string} module Name of module
+                                * @param {string} module Name of module and current module version. Formatted
+                                *  as '`[name]@[version]`". This version should match the requested version
+                                *  (from #batchRequest and #registry). This avoids race conditions (T117587).
+                                *  For back-compat with MediaWiki 1.27 and earlier, the version may be omitted.
                                 * @param {Function|Array|string} [script] Function with module code, list of URLs
                                 *  to load via `<script src>`, or string of module code for `$.globalEval()`.
                                 * @param {Object} [style] Should follow one of the following patterns:
                                 * @param {Object} [templates] List of key/value pairs to be added to mw#templates.
                                 */
                                implement: function ( module, script, style, messages, templates ) {
+                                       var split = splitModuleKey( module ),
+                                               name = split.name,
+                                               version = split.version;
                                        // Automatically register module
-                                       if ( !hasOwn.call( registry, module ) ) {
-                                               mw.loader.register( module );
+                                       if ( !hasOwn.call( registry, name ) ) {
+                                               mw.loader.register( name );
                                        }
                                        // Check for duplicate implementation
-                                       if ( hasOwn.call( registry, module ) && registry[ module ].script !== undefined ) {
-                                               throw new Error( 'module already implemented: ' + module );
+                                       if ( hasOwn.call( registry, name ) && registry[ name ].script !== undefined ) {
+                                               throw new Error( 'module already implemented: ' + name );
+                                       }
+                                       if ( version ) {
+                                               // Without this reset, if there is a version mismatch between the
+                                               // requested and received module version, then mw.loader.store would
+                                               // cache the response under the requested key. Thus poisoning the cache
+                                               // indefinitely with a stale value. (T117587)
+                                               registry[ name ].version = version;
                                        }
                                        // Attach components
-                                       registry[ module ].script = script || null;
-                                       registry[ module ].style = style || null;
-                                       registry[ module ].messages = messages || null;
-                                       registry[ module ].templates = templates || null;
+                                       registry[ name ].script = script || null;
+                                       registry[ name ].style = style || null;
+                                       registry[ name ].messages = messages || null;
+                                       registry[ name ].templates = templates || null;
                                        // The module may already have been marked as erroneous
-                                       if ( $.inArray( registry[ module ].state, [ 'error', 'missing' ] ) === -1 ) {
-                                               registry[ module ].state = 'loaded';
-                                               if ( allReady( registry[ module ].dependencies ) ) {
-                                                       execute( module );
+                                       if ( $.inArray( registry[ name ].state, [ 'error', 'missing' ] ) === -1 ) {
+                                               registry[ name ].state = 'loaded';
+                                               if ( allReady( registry[ name ].dependencies ) ) {
+                                                       execute( name );
                                                }
                                        }
                                },
 
                                        MODULE_SIZE_MAX: 100 * 1000,
 
-                                       // The contents of the store, mapping '[module name]@[version]' keys
+                                       // The contents of the store, mapping '[name]@[version]' keys
                                        // to module implementations.
                                        items: {},
 
                                                ].join( ':' );
                                        },
 
-                                       /**
-                                        * Get a key for a specific module. The key format is '[name]@[version]'.
-                                        *
-                                        * @param {string} module Module name
-                                        * @return {string|null} Module key or null if module does not exist
-                                        */
-                                       getModuleKey: function ( module ) {
-                                               return hasOwn.call( registry, module ) ?
-                                                       ( module + '@' + registry[ module ].version ) : null;
-                                       },
-
                                        /**
                                         * Initialize the store.
                                         *
                                                        return false;
                                                }
 
-                                               key = mw.loader.store.getModuleKey( module );
+                                               key = getModuleKey( module );
                                                if ( key in mw.loader.store.items ) {
                                                        mw.loader.store.stats.hits++;
                                                        return mw.loader.store.items[ key ];
                                                        return false;
                                                }
 
-                                               key = mw.loader.store.getModuleKey( module );
+                                               key = getModuleKey( module );
 
                                                if (
                                                        // Already stored a copy of this exact version
 
                                                try {
                                                        args = [
-                                                               JSON.stringify( module ),
+                                                               JSON.stringify( key ),
                                                                typeof descriptor.script === 'function' ?
                                                                        String( descriptor.script ) :
                                                                        JSON.stringify( descriptor.script ),
 
                                                for ( key in mw.loader.store.items ) {
                                                        module = key.slice( 0, key.indexOf( '@' ) );
-                                                       if ( mw.loader.store.getModuleKey( module ) !== key ) {
+                                                       if ( getModuleKey( module ) !== key ) {
                                                                mw.loader.store.stats.expired++;
                                                                delete mw.loader.store.items[ key ];
                                                        } else if ( mw.loader.store.items[ key ].length > mw.loader.store.MODULE_SIZE_MAX ) {
index f0eb12e..8eb1fd5 100644 (file)
@@ -4,6 +4,11 @@ use Psr\Log\LoggerInterface;
 use Psr\Log\NullLogger;
 
 abstract class ResourceLoaderTestCase extends MediaWikiTestCase {
+       // Version hash for a blank file module.
+       // Result of ResourceLoader::makeHash(), ResourceLoaderTestModule
+       // and ResourceLoaderFileModule::getDefinitionSummary().
+       const BLANK_VERSION = '09p30q0';
+
        /**
         * @param string $lang
         * @param string $dir
index 0965b9f..d2a0724 100644 (file)
@@ -5,6 +5,12 @@
  */
 class ResourceLoaderClientHtmlTest extends PHPUnit_Framework_TestCase {
 
+       protected static function expandVariables( $text ) {
+               return strtr( $text, [
+                       '{blankVer}' => ResourceLoaderTestCase::BLANK_VERSION
+               ] );
+       }
+
        protected static function makeContext( $extraQuery = [] ) {
                $conf = new HashConfig( [
                        'ResourceLoaderSources' => [],
@@ -165,7 +171,7 @@ class ResourceLoaderClientHtmlTest extends PHPUnit_Framework_TestCase {
                        . '<script>(window.RLQ=window.RLQ||[]).push(function(){'
                        . 'mw.config.set({"key":"value"});'
                        . 'mw.loader.state({"test.exempt":"ready","test.private.top":"loading","test.styles.pure":"ready","test.styles.private":"ready","test.scripts.top":"loading"});'
-                       . 'mw.loader.implement("test.private.top",function($,jQuery,require,module){},{"css":[]});'
+                       . 'mw.loader.implement("test.private.top@{blankVer}",function($,jQuery,require,module){},{"css":[]});'
                        . 'mw.loader.load(["test.top"]);'
                        . 'mw.loader.load("/w/load.php?debug=false\u0026lang=nl\u0026modules=test.scripts.top\u0026only=scripts\u0026skin=fallback");'
                        . '});</script>' . "\n"
@@ -173,6 +179,7 @@ class ResourceLoaderClientHtmlTest extends PHPUnit_Framework_TestCase {
                        . '<style>.private{}</style>' . "\n"
                        . '<script async="" src="/w/load.php?debug=false&amp;lang=nl&amp;modules=startup&amp;only=scripts&amp;skin=fallback"></script>';
                // @codingStandardsIgnoreEnd
+               $expected = self::expandVariables( $expected );
 
                $this->assertEquals( $expected, $client->getHeadHtml() );
        }
@@ -197,11 +204,12 @@ class ResourceLoaderClientHtmlTest extends PHPUnit_Framework_TestCase {
 
                // @codingStandardsIgnoreStart Generic.Files.LineLength
                $expected = '<script>(window.RLQ=window.RLQ||[]).push(function(){'
-                       . 'mw.loader.implement("test.private.bottom",function($,jQuery,require,module){},{"css":[]});'
+                       . 'mw.loader.implement("test.private.bottom@{blankVer}",function($,jQuery,require,module){},{"css":[]});'
                        . 'mw.loader.load("/w/load.php?debug=false\u0026lang=nl\u0026modules=test.scripts\u0026only=scripts\u0026skin=fallback");'
                        . 'mw.loader.load(["test"]);'
                        . '});</script>';
                // @codingStandardsIgnoreEnd
+               $expected = self::expandVariables( $expected );
 
                $this->assertEquals( $expected, $client->getBodyHtml() );
        }
@@ -225,7 +233,7 @@ class ResourceLoaderClientHtmlTest extends PHPUnit_Framework_TestCase {
                                'context' => [],
                                'modules' => [ 'test.private.top' ],
                                'only' => ResourceLoaderModule::TYPE_COMBINED,
-                               'output' => '<script>(window.RLQ=window.RLQ||[]).push(function(){mw.loader.implement("test.private.top",function($,jQuery,require,module){},{"css":[]});});</script>',
+                               'output' => '<script>(window.RLQ=window.RLQ||[]).push(function(){mw.loader.implement("test.private.top@{blankVer}",function($,jQuery,require,module){},{"css":[]});});</script>',
                        ],
                        [
                                'context' => [],
@@ -273,6 +281,7 @@ class ResourceLoaderClientHtmlTest extends PHPUnit_Framework_TestCase {
                $context = self::makeContext( $extraQuery );
                $context->getResourceLoader()->register( self::makeSampleModules() );
                $actual = ResourceLoaderClientHtml::makeLoad( $context, $modules, $type );
+               $expected = self::expandVariables( $expected );
                $this->assertEquals( $expected, (string)$actual );
        }
 }
index ab1323e..1b756be 100644 (file)
@@ -2,14 +2,9 @@
 
 class ResourceLoaderStartUpModuleTest extends ResourceLoaderTestCase {
 
-       // Version hash for a blank file module.
-       // Result of ResourceLoader::makeHash(), ResourceLoaderTestModule
-       // and ResourceLoaderFileModule::getDefinitionSummary().
-       protected static $blankVersion = '09p30q0';
-
        protected static function expandPlaceholders( $text ) {
                return strtr( $text, [
-                       '{blankVer}' => self::$blankVersion
+                       '{blankVer}' => self::BLANK_VERSION
                ] );
        }
 
index b69c9e8..bfac513 100644 (file)
@@ -1,5 +1,12 @@
 ( function ( mw, $ ) {
-       QUnit.module( 'mediawiki (mw.loader)' );
+       QUnit.module( 'mediawiki (mw.loader)', QUnit.newMwEnvironment( {
+               setup: function () {
+                       mw.loader.store.enabled = false;
+               },
+               teardown: function () {
+                       mw.loader.store.enabled = false;
+               }
+       } ) );
 
        mw.loader.addSource(
                'testloader',
                } );
        } );
 
+       QUnit.test( 'Stale response caching - T117587', function ( assert ) {
+               var count = 0;
+               mw.loader.store.enabled = true;
+               mw.loader.register( 'test.stale', 'v2' );
+               assert.strictEqual( mw.loader.store.get( 'test.stale' ), false, 'Not in store' );
+
+               mw.loader.implement( 'test.stale@v1', function () {
+                       count++;
+               } );
+
+               return mw.loader.using( 'test.stale' )
+                       .then( function () {
+                               assert.strictEqual( count, 1 );
+                               assert.strictEqual( mw.loader.getState( 'test.stale' ), 'ready' );
+                               assert.ok( mw.loader.store.get( 'test.stale' ), 'In store' );
+                       } )
+                       .then( function () {
+                               // Reset run time, but keep mw.loader.store
+                               mw.loader.moduleRegistry[ 'test.stale' ].script = undefined;
+                               mw.loader.moduleRegistry[ 'test.stale' ].state = 'registered';
+                               mw.loader.moduleRegistry[ 'test.stale' ].version = 'v2';
+
+                               // Module was stored correctly as v1
+                               // On future navigations, it will be ignored until evicted
+                               assert.strictEqual( mw.loader.store.get( 'test.stale' ), false, 'Not in store' );
+                       } );
+       } );
+
+       QUnit.test( 'Stale response caching - backcompat', function ( assert ) {
+               var count = 0;
+               mw.loader.store.enabled = true;
+               mw.loader.register( 'test.stalebc', 'v2' );
+               assert.strictEqual( mw.loader.store.get( 'test.stalebc' ), false, 'Not in store' );
+
+               mw.loader.implement( 'test.stalebc', function () {
+                       count++;
+               } );
+
+               return mw.loader.using( 'test.stalebc' )
+                       .then( function () {
+                               assert.strictEqual( count, 1 );
+                               assert.strictEqual( mw.loader.getState( 'test.stalebc' ), 'ready' );
+                               assert.ok( mw.loader.store.get( 'test.stalebc' ), 'In store' );
+                       } )
+                       .then( function () {
+                               // Reset run time, but keep mw.loader.store
+                               mw.loader.moduleRegistry[ 'test.stalebc' ].script = undefined;
+                               mw.loader.moduleRegistry[ 'test.stalebc' ].state = 'registered';
+                               mw.loader.moduleRegistry[ 'test.stalebc' ].version = 'v2';
+
+                               // Legacy behaviour is storing under the expected version,
+                               // which woudl lead to whitewashing and stale values (T117587).
+                               assert.ok( mw.loader.store.get( 'test.stalebc' ), 'In store' );
+                       } );
+       } );
+
        QUnit.test( 'require()', 6, function ( assert ) {
                mw.loader.register( [
                        [ 'test.require1', '0' ],