Page MenuHomePhabricator

Make exception handling in mw.loader.using sane
Closed, ResolvedPublic

Description

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.


Version: 1.22.0
Severity: normal

Details

Reference
bz54618

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 22 2014, 2:15 AM
bzimport set Reference to bz54618.
bzimport added a subscriber: Unknown Object (MLST).

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...)

Krinkle claimed this task.

Due to batch loading abstraction, it's hard to tell whether a handler is attached to a dependent module or not. And also the reason for failure would be too much state to keep around when mid-air handling pending modules. I think it's reasonable to provide the callback a consistent generic Module foo failed exception.

The original exception is now exposed via mw.track. Both the original exception (as caught in mw.loader#runScript, or mw.loader#work), and exceptions from callbacks (as caught in mw.loader#handlePending) are now logged and exposed via mw.track( "resourceloader.exception" ).

We also fixed the bug where only error callback exceptions were logged. Those of a success handler (job.ready), are now caught and logged as well.

Krinkle set Security to None.
Krinkle removed a subscriber: Unknown Object (MLST).