From 45bec7675547ee7076a2ac899db5e2e9199c5fad Mon Sep 17 00:00:00 2001 From: Timo Tijhof Date: Mon, 12 Sep 2016 13:52:10 -0700 Subject: [PATCH] resourceloader: Don't cache stale responses in mw.loader.store 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 | 7 +- resources/src/mediawiki/mediawiki.js | 89 +++++++++++++------ tests/phpunit/ResourceLoaderTestCase.php | 5 ++ .../ResourceLoaderClientHtmlTest.php | 15 +++- .../ResourceLoaderStartUpModuleTest.php | 7 +- .../mediawiki/mediawiki.loader.test.js | 65 +++++++++++++- 6 files changed, 146 insertions(+), 42 deletions(-) diff --git a/includes/resourceloader/ResourceLoader.php b/includes/resourceloader/ResourceLoader.php index 34a0a99d31..143f5cc678 100644 --- a/includes/resourceloader/ResourceLoader.php +++ b/includes/resourceloader/ResourceLoader.php @@ -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 diff --git a/resources/src/mediawiki/mediawiki.js b/resources/src/mediawiki/mediawiki.js index c7715e5455..27507652ec 100644 --- a/resources/src/mediawiki/mediawiki.js +++ b/resources/src/mediawiki/mediawiki.js @@ -1674,6 +1674,35 @@ } } + /** + * 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 { /** @@ -1861,7 +1890,10 @@ * 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 `' . "\n" @@ -173,6 +179,7 @@ class ResourceLoaderClientHtmlTest extends PHPUnit_Framework_TestCase { . '' . "\n" . ''; // @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 = ''; // @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' => '', + 'output' => '', ], [ '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 ); } } diff --git a/tests/phpunit/includes/resourceloader/ResourceLoaderStartUpModuleTest.php b/tests/phpunit/includes/resourceloader/ResourceLoaderStartUpModuleTest.php index ab1323ecdc..1b756be629 100644 --- a/tests/phpunit/includes/resourceloader/ResourceLoaderStartUpModuleTest.php +++ b/tests/phpunit/includes/resourceloader/ResourceLoaderStartUpModuleTest.php @@ -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 ] ); } diff --git a/tests/qunit/suites/resources/mediawiki/mediawiki.loader.test.js b/tests/qunit/suites/resources/mediawiki/mediawiki.loader.test.js index b69c9e83f2..bfac513cf3 100644 --- a/tests/qunit/suites/resources/mediawiki/mediawiki.loader.test.js +++ b/tests/qunit/suites/resources/mediawiki/mediawiki.loader.test.js @@ -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', @@ -619,6 +626,62 @@ } ); } ); + 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' ], -- 2.20.1