jenkins-bot has submitted this change and it was merged. (
https://gerrit.wikimedia.org/r/325071 )
Change subject: resourceloader: Don't let module exception break startup
......................................................................
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
---
M includes/resourceloader/ResourceLoader.php
M includes/resourceloader/ResourceLoaderStartUpModule.php
M tests/phpunit/ResourceLoaderTestCase.php
M tests/phpunit/includes/resourceloader/ResourceLoaderContextTest.php
M tests/phpunit/includes/resourceloader/ResourceLoaderTest.php
5 files changed, 190 insertions(+), 8 deletions(-)
Approvals:
Aaron Schulz: Looks good to me, approved
jenkins-bot: Verified
diff --git a/includes/resourceloader/ResourceLoader.php
b/includes/resourceloader/ResourceLoader.php
index f9b4b3d..d338e9b 100644
--- a/includes/resourceloader/ResourceLoader.php
+++ b/includes/resourceloader/ResourceLoader.php
@@ -618,7 +618,23 @@
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 8970620..a99305c 100644
--- a/includes/resourceloader/ResourceLoaderStartUpModule.php
+++ b/includes/resourceloader/ResourceLoaderStartUpModule.php
@@ -194,7 +194,6 @@
* @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 @@
$byPassTargetFilter = $this->getConfig()->get(
'EnableJavaScriptTest' ) && $target === 'test';
$out = '';
+ $states = [];
$registryData = [];
// Get registry data
@@ -219,8 +219,23 @@
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 @@
// 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 45cfdbf..68b91bf 100644
--- a/tests/phpunit/ResourceLoaderTestCase.php
+++ b/tests/phpunit/ResourceLoaderTestCase.php
@@ -14,9 +14,12 @@
* @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 @@
$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 @@
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 baf0b69..b658efb 100644
--- a/tests/phpunit/includes/resourceloader/ResourceLoaderContextTest.php
+++ b/tests/phpunit/includes/resourceloader/ResourceLoaderContextTest.php
@@ -14,6 +14,7 @@
'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 1ecdf21..cde1e5a 100644
--- a/tests/phpunit/includes/resourceloader/ResourceLoaderTest.php
+++ b/tests/phpunit/includes/resourceloader/ResourceLoaderTest.php
@@ -14,6 +14,10 @@
'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 @@
$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'
+ );
+ }
}
--
To view, visit https://gerrit.wikimedia.org/r/325071
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ice4ede5ea594bf3fa591134bc9382bd9c24e2f39
Gerrit-PatchSet: 12
Gerrit-Project: mediawiki/core
Gerrit-Branch: master
Gerrit-Owner: Krinkle <[email protected]>
Gerrit-Reviewer: Aaron Schulz <[email protected]>
Gerrit-Reviewer: Catrope <[email protected]>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
