resourceloader: Don't let module exception break startup
authorTimo Tijhof <krinklemail@gmail.com>
Sat, 3 Dec 2016 00:48:14 +0000 (16:48 -0800)
committerAaron Schulz <aschulz@wikimedia.org>
Thu, 15 Dec 2016 23:25:57 +0000 (23:25 +0000)
commit466939c6318e548e064dcbfbed694c41b425b2a9
tree05d788784f1d1b19a2c41a1b167addfc7758a661
parent1676448145f28cdf5bf399b13a39d909d7e0bb77
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
includes/resourceloader/ResourceLoaderStartUpModule.php
tests/phpunit/ResourceLoaderTestCase.php
tests/phpunit/includes/resourceloader/ResourceLoaderContextTest.php
tests/phpunit/includes/resourceloader/ResourceLoaderTest.php