Last modified: 2014-02-24 09:01:03 UTC

Wikimedia Bugzilla is closed!

Wikimedia migrated from Bugzilla to Phabricator. Bug reports are handled in Wikimedia Phabricator.
This static website is read-only and for historical purposes. It is not possible to log in and except for displaying bug reports and their history, links might be broken. See T56618, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 54618 - Make exception handling in mw.loader.using sane
Make exception handling in mw.loader.using sane
Status: NEW
Product: MediaWiki
Classification: Unclassified
ResourceLoader (Other open bugs)
1.22.0
All All
: Normal normal (vote)
: Future release
Assigned To: Nobody - You can work on this!
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-09-26 01:59 UTC by Roan Kattouw
Modified: 2014-02-24 09:01 UTC (History)
4 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments

Description Roan Kattouw 2013-09-26 01:59:20 UTC
Right now, if an exception occurs while executing a ResourceLoader module, the behavior is:

* Catch the exception and log it to the console
* Discard the exception
* Set the module's state to 'error'
* For any modules depending on the failed module, set their state to 'error' as well
* For each error handler attached to this module (with mw.loader.using) or one of its dependencies
** Generate a fake exception saying "Module foo failed"
** Throw it
** Catch it
** Pass it to the error handler
** If the error handler throws an exception, catch it and log it

So the error handler doesn't receive the original exception, it receives a fake one that we generate separately for each handler, then throw and then catch again. That seems ridiculous, we should just store the original exception and pass it in. For dependent modules I suppose a fake exception is reasonable, maybe, but for an error handler attached to the module itself (or really to a set of modules including the module itself) surely we should pass the real exception.

-----

What's even weirded is when a ResourceLoader module executes successfully, but a success callback attached to it (with mw.loader.using) throws an exception:
* Module execution succeeds
* Call success callback
* Get exception from success callback
* Catch it
* If there is an error handler, call it and pass the caught exception
** If the error handler also throws an exception, catch it and log it

So the exception from the error handler is caught but never logged. Also, the error handler is invoked after the success handler has already been invoked, which means that if you hook up success/error to a promise's resolve/reject, we'll try to reject a promise that's already been resolved, which is a no-op and doesn't call the .fail() handlers. Meanwhile, the exception is completely buried and untraceable.

mw.loader shouldn't be trying to catch exceptions that occur in success handlers that live in user land and were passed into .using() (this is all ready handlers, we don't use them internally). Exceptions in user land are user land's problem, so we should just let them happen and not catch them. The fact that we call the error callback is even more wrong, because that's for when execution of the module failed, not for when execution of the module succeeded but some random user land code responding to this event ends up crapping itself.
Comment 1 Tisza Gergő 2014-02-24 09:01:03 UTC
Not sure if it's related, but I recently had a syntax error in some less file for a module that was loaded via mw.loader.using. (Or more precisely the less file belonged to another module the first module depended on.) This seems like the first case you describe, but there was no console message, not even in debug mode. (We did not pass an error handler so it took a while to figure out what's going on...)

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links