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

diff --git a/includes/resourceloader/ResourceLoader.php b/includes/resourceloader/ResourceLoader.php
index f9b4b3d..d338e9b 100644 (file)
--- 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 8970620..a99305c 100644 (file)
--- 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 45cfdbf..68b91bf 100644 (file)
--- 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 baf0b69..b658efb 100644 (file)
--- 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 1ecdf21..cde1e5a 100644 (file)
--- 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'
+               );
+       }
 }