From b70ab4083904a8118c8d25d1ec5c4ce0f1d92900 Mon Sep 17 00:00:00 2001 From: Timo Tijhof Date: Thu, 30 Aug 2018 03:52:39 +0100 Subject: [PATCH] resourceloader: Use 'enableModuleContentVersion' for startup module This significantly simplifies the getVersionHash implementation for StartupModule, and fixes a couple of bugs. Previously, the startup module's E-Tag was determined by the 'getDefinitionSummary' method, which combined the E-Tag values from all registered modules, plus what we thought is all information used by 'getScript' (config vars, embedded script files, list of base modules, ...) However, this were various things part of the manifest that it forgot about, including: * Changes to the list of dependencies of a module. * Changes to the name of module. * Changes to the cache group of module. * Adding or removing a foreign module source (mw.loader.addSource). These are all quite rare, and when they do change, they usually also involve a change that *was* tracked already. But, sometimes they don't and that's when bugs happened. Instead of the tracking array of getDefinitionSummary, we now use the 'enableModuleContentVersion' option for StartupModule, which simply calls the actual getScript() method and hashes that. Of note: When an exception happens with the version computation of any individual module, we catch it, log it, and continue with the rest. Previously, the first time such error was discovered at run-time would be in the getCombinedVersion() call from StartupModule::getAllModuleHashes(). That public getCombinedVersion() method of ResourceLoader had the benefit of also outputting details of that exception in the HTTP response output. In order to keep that behaviour, I made outputErrorAndLog() public so that StartupModule can call it directly now. This is covered by ResourceLoaderTest::testMakeModuleResponseStartupError. Bug: T201686 Change-Id: I8e8d3a2cd2ccd68d2d78e988bcdd0d77fbcbf1d4 --- includes/resourceloader/ResourceLoader.php | 10 +- .../ResourceLoaderStartUpModule.php | 105 ++++++------------ .../ResourceLoaderStartUpModuleTest.php | 37 +++++- 3 files changed, 72 insertions(+), 80 deletions(-) diff --git a/includes/resourceloader/ResourceLoader.php b/includes/resourceloader/ResourceLoader.php index fc0ca1d5a2..8f5d083ab5 100644 --- a/includes/resourceloader/ResourceLoader.php +++ b/includes/resourceloader/ResourceLoader.php @@ -627,14 +627,13 @@ class ResourceLoader implements LoggerAwareInterface { /** * Add an error to the 'errors' array and log it. * - * Should only be called from within respond(). - * + * @private For internal use by ResourceLoader and ResourceLoaderStartUpModule. * @since 1.29 * @param Exception $e * @param string $msg * @param array $context */ - protected function outputErrorAndLog( Exception $e, $msg, array $context = [] ) { + public function outputErrorAndLog( Exception $e, $msg, array $context = [] ) { MWExceptionHandler::logException( $e ); $this->logger->warning( $msg, @@ -659,9 +658,8 @@ class ResourceLoader implements LoggerAwareInterface { try { return $this->getModule( $module )->getVersionHash( $context ); } catch ( Exception $e ) { - // If modules fail to compute a version, do still consider the versions - // of other modules - don't set an empty string E-Tag for the whole request. - // See also T152266 and StartupModule::getModuleRegistrations(). + // If modules fail to compute a version, don't fail the request (T152266) + // and still compute versions of other modules. $this->outputErrorAndLog( $e, 'Calculating version for "{module}" failed: {exception}', [ diff --git a/includes/resourceloader/ResourceLoaderStartUpModule.php b/includes/resourceloader/ResourceLoaderStartUpModule.php index 18cc4d5aad..8140c2c8c0 100644 --- a/includes/resourceloader/ResourceLoaderStartUpModule.php +++ b/includes/resourceloader/ResourceLoaderStartUpModule.php @@ -40,21 +40,13 @@ use MediaWiki\MediaWikiServices; * See also: OutputPage::disallowUserJs() */ class ResourceLoaderStartUpModule extends ResourceLoaderModule { - - // Cache for getConfigSettings() as it's called by multiple methods - protected $configVars = []; protected $targets = [ 'desktop', 'mobile' ]; /** * @param ResourceLoaderContext $context * @return array */ - protected function getConfigSettings( $context ) { - $hash = $context->getHash(); - if ( isset( $this->configVars[$hash] ) ) { - return $this->configVars[$hash]; - } - + private function getConfigSettings( $context ) { $conf = $this->getConfig(); // We can't use Title::newMainPage() if 'mainpage' is in @@ -136,8 +128,7 @@ class ResourceLoaderStartUpModule extends ResourceLoaderModule { Hooks::run( 'ResourceLoaderGetConfigVars', [ &$vars ] ); - $this->configVars[$hash] = $vars; - return $this->configVars[$hash]; + return $vars; } /** @@ -222,9 +213,23 @@ class ResourceLoaderStartUpModule extends ResourceLoaderModule { $out = ''; $states = []; $registryData = []; + $moduleNames = $resourceLoader->getModuleNames(); + + // Preload with a batch so that the below calls to getVersionHash() for each module + // don't require on-demand loading of more information. + try { + $resourceLoader->preloadModuleInfo( $moduleNames, $context ); + } catch ( Exception $e ) { + // Don't fail the request (T152266) + // Also print the error in the main output + $resourceLoader->outputErrorAndLog( $e, + 'Preloading module info from startup failed: {exception}', + [ 'exception' => $e ] + ); + } // Get registry data - foreach ( $resourceLoader->getModuleNames() as $name ) { + foreach ( $moduleNames as $name ) { $module = $resourceLoader->getModule( $name ); $moduleTargets = $module->getTargets(); if ( @@ -235,18 +240,25 @@ class ResourceLoaderStartUpModule extends ResourceLoaderModule { } if ( $module->isRaw() ) { - // Don't register "raw" modules (like 'jquery' and 'mediawiki') client-side because - // depending on them is illegal anyway and would only lead to them being reloaded - // causing any state to be lost (like jQuery plugins, mw.config etc.) + // Don't register "raw" modules (like 'startup') client-side because depending on them + // is illegal anyway and would only lead to them being loaded a second time, + // causing any state to be lost. + + // ATTENTION: Because of the line below, this is not going to cause infinite recursion. + // Think carefully before making changes to this code! + // The below code is going to call ResourceLoaderModule::getVersionHash() for every module. + // For StartUpModule (this module) the hash is computed based on the manifest content, + // which is the very thing we are computing right here. As such, this must skip iterating + // over 'startup' itself. continue; } try { $versionHash = $module->getVersionHash( $context ); } catch ( Exception $e ) { - // See also T152266 and ResourceLoader::getCombinedVersion() - MWExceptionHandler::logException( $e ); - $context->getLogger()->warning( + // Don't fail the request (T152266) + // Also print the error in the main output + $resourceLoader->outputErrorAndLog( $e, 'Calculating version for "{module}" failed: {exception}', [ 'module' => $name, @@ -445,59 +457,12 @@ class ResourceLoaderStartUpModule extends ResourceLoaderModule { } /** - * Get the definition summary for this module. - * - * @param ResourceLoaderContext $context - * @return array - */ - public function getDefinitionSummary( ResourceLoaderContext $context ) { - global $IP; - $summary = parent::getDefinitionSummary( $context ); - $startup = [ - // getScript() exposes these variables to mw.config (T30899). - 'vars' => $this->getConfigSettings( $context ), - // getScript() uses this to decide how configure mw.Map for mw.config. - 'wgLegacyJavaScriptGlobals' => $this->getConfig()->get( 'LegacyJavaScriptGlobals' ), - // Detect changes to the module registrations output by getScript(). - 'moduleHashes' => $this->getAllModuleHashes( $context ), - // Detect changes to base modules listed by getScript(). - 'baseModules' => $this->getBaseModules(), - - 'fileHashes' => [ - $this->safeFileHash( "$IP/resources/src/startup/startup.js" ), - $this->safeFileHash( "$IP/resources/src/startup/mediawiki.js" ), - $this->safeFileHash( "$IP/resources/src/startup/mediawiki.requestIdleCallback.js" ), - ], - ]; - if ( $context->getDebug() ) { - $startup['fileHashes'][] = $this->safeFileHash( "$IP/resources/src/startup/mediawiki.log.js" ); - } - if ( $this->getConfig()->get( 'ResourceLoaderEnableJSProfiler' ) ) { - $startup['fileHashes'][] = $this->safeFileHash( "$IP/resources/src/startup/profiling.js" ); - } - $summary[] = $startup; - return $summary; - } - - /** - * Helper method for getDefinitionSummary(). - * - * @param ResourceLoaderContext $context - * @return string SHA-1 + * @return bool */ - protected function getAllModuleHashes( ResourceLoaderContext $context ) { - $rl = $context->getResourceLoader(); - // Preload for getCombinedVersion() - $rl->preloadModuleInfo( $rl->getModuleNames(), $context ); - - // ATTENTION: Because of the line below, this is not going to cause infinite recursion. - // Think carefully before making changes to this code! - // Pre-populate versionHash with something because the loop over all modules below includes - // the startup module (this module). - // See ResourceLoaderModule::getVersionHash() for usage of this cache. - $this->versionHash[$context->getHash()] = null; - - return $rl->getCombinedVersion( $context, $rl->getModuleNames() ); + public function enableModuleContentVersion() { + // Enabling this means that ResourceLoader::getVersionHash will simply call getScript() + // and hash it to determine the version (as used by E-Tag HTTP response header). + return true; } /** diff --git a/tests/phpunit/includes/resourceloader/ResourceLoaderStartUpModuleTest.php b/tests/phpunit/includes/resourceloader/ResourceLoaderStartUpModuleTest.php index 86956f2602..aa3f820338 100644 --- a/tests/phpunit/includes/resourceloader/ResourceLoaderStartUpModuleTest.php +++ b/tests/phpunit/includes/resourceloader/ResourceLoaderStartUpModuleTest.php @@ -572,6 +572,7 @@ mw.loader.register( [ $version1 = $module->getVersionHash( $context ); $module = new ResourceLoaderStartupModule(); $version2 = $module->getVersionHash( $context ); + $this->setMwGlobals( 'wgArticlePath', '/w3' ); $module = new ResourceLoaderStartupModule(); $version3 = $module->getVersionHash( $context ); @@ -590,8 +591,7 @@ mw.loader.register( [ } /** - * @covers ResourceLoaderStartupModule::getAllModuleHashes - * @covers ResourceLoaderStartupModule::getDefinitionSummary + * @covers ResourceLoaderStartupModule */ public function testGetVersionHash_varyModule() { $context1 = $this->getResourceLoaderContext(); @@ -621,10 +621,11 @@ mw.loader.register( [ $module = new ResourceLoaderStartupModule(); $version3 = $module->getVersionHash( $context3 ); - $this->assertEquals( + // Module name *is* significant (T201686) + $this->assertNotEquals( $version1, $version2, - 'Module name is insignificant' + 'Module name is significant' ); $this->assertNotEquals( @@ -634,4 +635,32 @@ mw.loader.register( [ ); } + /** + * @covers ResourceLoaderStartupModule + */ + public function testGetVersionHash_varyDeps() { + $context = $this->getResourceLoaderContext(); + $rl = $context->getResourceLoader(); + $rl->register( [ + 'test.a' => new ResourceLoaderTestModule( [ 'dependencies' => [ 'x', 'y' ] ] ), + ] ); + $module = new ResourceLoaderStartupModule(); + $version1 = $module->getVersionHash( $context ); + + $context = $this->getResourceLoaderContext(); + $rl = $context->getResourceLoader(); + $rl->register( [ + 'test.a' => new ResourceLoaderTestModule( [ 'dependencies' => [ 'x', 'z' ] ] ), + ] ); + $module = new ResourceLoaderStartupModule(); + $version2 = $module->getVersionHash( $context ); + + // Dependencies *are* significant (T201686) + $this->assertNotEquals( + $version1, + $version2, + 'Dependencies are significant' + ); + } + } -- 2.20.1