From 466939c6318e548e064dcbfbed694c41b425b2a9 Mon Sep 17 00:00:00 2001 From: Timo Tijhof Date: Fri, 2 Dec 2016 16:48:14 -0800 Subject: [PATCH] resourceloader: Don't let module exception break startup When getScript (or some other method used in a module response) throws an error, only that module fails (by outputting mw.loader.state instead of mw.loader.implement). Other modules will work. This has always been the case and is working fine. For example, "load.php?modules=foo|bar", where 'foo' throws, will return: ```js /* exception message: .. */ mw.loader.implement('bar', ..) mw.loader.state('foo', 'error') ``` The problem, however, is that during the generation of the startup module, we iterate over all other modules. In 2011, the getVersionHash method (then: getModifiedTime) was fairly simple and unlikely to throw errors. Nowadays, some modules use enableModuleContentVersion which will involve the same code path as for regular module responses. The try/catch in ResourceLoader::makeModuleResponse() suffices for the case of loading modules other than startup. But when loading the startup module, and an exception happens in getVersionHash, then the entire startup response is replaced with an exception comment. Example case: * A file not existing for a FileModule subclass that uses enableModuleContentVersion. * A database error from a data module, like CiteDataModule or CNChoiceData. Changes: * Ensure E-Tag is still useful while an error happens in production because we respond with 200 OK and one error isn't the same as another. Fixed by try/catch in getCombinedVersion. * Ensure start manifest isn't disrupted by one broken module. Fixed by try/catch in StartupModule::getModuleRegistrations(). Tests: * testMakeModuleResponseError: The case that already worked fined. * testMakeModuleResponseStartupError: The case fixed in this commit. * testGetCombinedVersion: The case fixed in this commit for E-Tag. Bug: T152266 Change-Id: Ice4ede5ea594bf3fa591134bc9382bd9c24e2f39 --- includes/resourceloader/ResourceLoader.php | 18 ++- .../ResourceLoaderStartUpModule.php | 25 +++- tests/phpunit/ResourceLoaderTestCase.php | 19 ++- .../ResourceLoaderContextTest.php | 1 + .../resourceloader/ResourceLoaderTest.php | 135 ++++++++++++++++++ 5 files changed, 190 insertions(+), 8 deletions(-) diff --git a/includes/resourceloader/ResourceLoader.php b/includes/resourceloader/ResourceLoader.php index f9b4b3ddce..d338e9b423 100644 --- a/includes/resourceloader/ResourceLoader.php +++ b/includes/resourceloader/ResourceLoader.php @@ -618,7 +618,23 @@ class ResourceLoader implements LoggerAwareInterface { return ''; } $hashes = array_map( function ( $module ) use ( $context ) { - return $this->getModule( $module )->getVersionHash( $context ); + 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(). + MWExceptionHandler::logException( $e ); + $this->logger->warning( + 'Calculating version for "{module}" failed: {exception}', + [ + 'module' => $module, + 'exception' => $e, + ] + ); + $this->errors[] = self::formatExceptionNoComment( $e ); + return ''; + } }, $moduleNames ); return self::makeHash( implode( '', $hashes ) ); } diff --git a/includes/resourceloader/ResourceLoaderStartUpModule.php b/includes/resourceloader/ResourceLoaderStartUpModule.php index 8970620720..a99305ca61 100644 --- a/includes/resourceloader/ResourceLoaderStartUpModule.php +++ b/includes/resourceloader/ResourceLoaderStartUpModule.php @@ -194,7 +194,6 @@ class ResourceLoaderStartUpModule extends ResourceLoaderModule { * @return string JavaScript code for registering all modules with the client loader */ public function getModuleRegistrations( ResourceLoaderContext $context ) { - $resourceLoader = $context->getResourceLoader(); $target = $context->getRequest()->getVal( 'target', 'desktop' ); // Bypass target filter if this request is Special:JavaScriptTest. @@ -202,6 +201,7 @@ class ResourceLoaderStartUpModule extends ResourceLoaderModule { $byPassTargetFilter = $this->getConfig()->get( 'EnableJavaScriptTest' ) && $target === 'test'; $out = ''; + $states = []; $registryData = []; // Get registry data @@ -219,8 +219,23 @@ class ResourceLoaderStartUpModule extends ResourceLoaderModule { continue; } - $versionHash = $module->getVersionHash( $context ); - if ( strlen( $versionHash ) !== 7 ) { + try { + $versionHash = $module->getVersionHash( $context ); + } catch ( Exception $e ) { + // See also T152266 and ResourceLoader::getCombinedVersion() + MWExceptionHandler::logException( $e ); + $context->getLogger()->warning( + 'Calculating version for "{module}" failed: {exception}', + [ + 'module' => $name, + 'exception' => $e, + ] + ); + $versionHash = ''; + $states[$name] = 'error'; + } + + if ( $versionHash !== '' && strlen( $versionHash ) !== 7 ) { $context->getLogger()->warning( "Module '{module}' produced an invalid version hash: '{version}'.", [ @@ -270,6 +285,10 @@ class ResourceLoaderStartUpModule extends ResourceLoaderModule { // Register modules $out .= "\n" . ResourceLoader::makeLoaderRegisterScript( $registrations ); + if ( $states ) { + $out .= "\n" . ResourceLoader::makeLoaderStateScript( $states ); + } + return $out; } diff --git a/tests/phpunit/ResourceLoaderTestCase.php b/tests/phpunit/ResourceLoaderTestCase.php index 45cfdbfc61..68b91bf3a4 100644 --- a/tests/phpunit/ResourceLoaderTestCase.php +++ b/tests/phpunit/ResourceLoaderTestCase.php @@ -14,9 +14,12 @@ abstract class ResourceLoaderTestCase extends MediaWikiTestCase { * @param array|string $options Language code or options array * - string 'lang' Language code * - string 'dir' Language direction (ltr or rtl) + * - string 'modules' Pipe-separated list of module names + * - string|null 'only' "scripts" (unwrapped script), "styles" (stylesheet), or null + * (mw.loader.implement). * @return ResourceLoaderContext */ - protected function getResourceLoaderContext( $options = [] ) { + protected function getResourceLoaderContext( $options = [], ResourceLoader $rl = null ) { if ( is_string( $options ) ) { // Back-compat for extension tests $options = [ 'lang' => $options ]; @@ -24,12 +27,14 @@ abstract class ResourceLoaderTestCase extends MediaWikiTestCase { $options += [ 'lang' => 'en', 'dir' => 'ltr', + 'modules' => 'startup', + 'only' => 'scripts', ]; - $resourceLoader = new ResourceLoader(); + $resourceLoader = $rl ?: new ResourceLoader(); $request = new FauxRequest( [ 'lang' => $options['lang'], - 'modules' => 'startup', - 'only' => 'scripts', + 'modules' => $options['modules'], + 'only' => $options['only'], 'skin' => 'vector', 'target' => 'phpunit', ] ); @@ -151,6 +156,12 @@ class EmptyResourceLoader extends ResourceLoader { public function __construct( Config $config = null, LoggerInterface $logger = null ) { $this->setLogger( $logger ?: new NullLogger() ); $this->config = $config ?: MediaWikiServices::getInstance()->getMainConfig(); + // Source "local" is required by StartupModule + $this->addSource( 'local', $this->config->get( 'LoadScript' ) ); $this->setMessageBlobStore( new MessageBlobStore( $this, $this->getLogger() ) ); } + + public function getErrors() { + return $this->errors; + } } diff --git a/tests/phpunit/includes/resourceloader/ResourceLoaderContextTest.php b/tests/phpunit/includes/resourceloader/ResourceLoaderContextTest.php index baf0b69e6c..b658efba96 100644 --- a/tests/phpunit/includes/resourceloader/ResourceLoaderContextTest.php +++ b/tests/phpunit/includes/resourceloader/ResourceLoaderContextTest.php @@ -14,6 +14,7 @@ class ResourceLoaderContextTest extends PHPUnit_Framework_TestCase { 'ResourceLoaderDebug' => false, 'DefaultSkin' => 'fallback', 'LanguageCode' => 'nl', + 'LoadScript' => '/w/load.php', ] ) ); } diff --git a/tests/phpunit/includes/resourceloader/ResourceLoaderTest.php b/tests/phpunit/includes/resourceloader/ResourceLoaderTest.php index 1ecdf21d92..cde1e5abb3 100644 --- a/tests/phpunit/includes/resourceloader/ResourceLoaderTest.php +++ b/tests/phpunit/includes/resourceloader/ResourceLoaderTest.php @@ -14,6 +14,10 @@ class ResourceLoaderTest extends ResourceLoaderTestCase { 'Foo' => '#eeeeee', 'bar' => 5, ], + // Clear ResourceLoaderGetConfigVars hooks (called by StartupModule) + // to avoid notices during testMakeModuleResponse for missing + // wgResourceLoaderLESSVars keys in extension hooks. + 'wgHooks' => [], ] ); } @@ -441,4 +445,135 @@ mw.example(); $this->assertTrue( true ); } } + + protected function getFailFerryMock() { + $mock = $this->getMockBuilder( ResourceLoaderTestModule::class ) + ->setMethods( [ 'getScript' ] ) + ->getMock(); + $mock->method( 'getScript' )->will( $this->throwException( + new Exception( 'Ferry not found' ) + ) ); + return $mock; + } + + protected function getSimpleModuleMock( $script = '' ) { + $mock = $this->getMockBuilder( ResourceLoaderTestModule::class ) + ->setMethods( [ 'getScript' ] ) + ->getMock(); + $mock->method( 'getScript' )->willReturn( $script ); + return $mock; + } + + /** + * @covers ResourceLoader::getCombinedVersion + */ + public function testGetCombinedVersion() { + $rl = new EmptyResourceLoader(); + $rl->register( [ + 'foo' => self::getSimpleModuleMock(), + 'ferry' => self::getFailFerryMock(), + 'bar' => self::getSimpleModuleMock(), + ] ); + $context = $this->getResourceLoaderContext( [], $rl ); + + $this->assertEquals( + ResourceLoader::makeHash( self::BLANK_VERSION ), + $rl->getCombinedVersion( $context, [ 'foo' ] ), + 'compute foo' + ); + + // Verify that getCombinedVersion() does not throw when ferry fails. + // Instead it gracefully continues to combine the remaining modules. + $this->assertEquals( + ResourceLoader::makeHash( self::BLANK_VERSION . self::BLANK_VERSION ), + $rl->getCombinedVersion( $context, [ 'foo', 'ferry', 'bar' ] ), + 'compute foo+ferry+bar (T152266)' + ); + } + + /** + * Verify that when building module content in a load.php response, + * an exception from one module will not break script output from + * other modules. + */ + public function testMakeModuleResponseError() { + $modules = [ + 'foo' => self::getSimpleModuleMock( 'foo();' ), + 'ferry' => self::getFailFerryMock(), + 'bar' => self::getSimpleModuleMock( 'bar();' ), + ]; + $rl = new EmptyResourceLoader(); + $rl->register( $modules ); + $context = $this->getResourceLoaderContext( + [ + 'modules' => 'foo|ferry|bar', + 'only' => 'scripts', + ], + $rl + ); + + $response = $rl->makeModuleResponse( $context, $modules ); + $errors = $rl->getErrors(); + + $this->assertCount( 1, $errors ); + $this->assertRegExp( '/Ferry not found/', $errors[0] ); + $this->assertEquals( + 'foo();bar();mw.loader.state( { + "ferry": "error", + "foo": "ready", + "bar": "ready" +} );', + $response + ); + } + + /** + * Verify that when building the startup module response, + * an exception from one module class will not break the entire + * startup module response. See T152266. + */ + public function testMakeModuleResponseStartupError() { + $rl = new EmptyResourceLoader(); + $rl->register( [ + 'foo' => self::getSimpleModuleMock( 'foo();' ), + 'ferry' => self::getFailFerryMock(), + 'bar' => self::getSimpleModuleMock( 'bar();' ), + 'startup' => [ 'class' => 'ResourceLoaderStartUpModule' ], + ] ); + $context = $this->getResourceLoaderContext( + [ + 'modules' => 'startup', + 'only' => 'scripts', + ], + $rl + ); + + $this->assertEquals( + [ 'foo', 'ferry', 'bar', 'startup' ], + $rl->getModuleNames(), + 'getModuleNames' + ); + + $modules = [ 'startup' => $rl->getModule( 'startup' ) ]; + $response = $rl->makeModuleResponse( $context, $modules ); + $errors = $rl->getErrors(); + + $this->assertRegExp( '/Ferry not found/', $errors[0] ); + $this->assertCount( 1, $errors ); + $this->assertRegExp( + '/isCompatible.*function startUp/s', + $response, + 'startup response undisrupted (T152266)' + ); + $this->assertRegExp( + '/register\([^)]+"ferry",\s*""/s', + $response, + 'startup response registers broken module' + ); + $this->assertRegExp( + '/state\([^)]+"ferry":\s*"error"/s', + $response, + 'startup response sets state to error' + ); + } } -- 2.20.1