Trigger hook or event listener when exceptions swallowed without explicit handling

[aaclayton]

Originally in GitLab by @ruipin

Feature Summary

The module 'lib-wrapper' throws an exception on various events, e.g. when a conflict is detected between modules which is otherwise impossible to resolve.

It then listens for unhandled exceptions (using the Javascript error and unhandledrejection events) and notifies the user if any of them were thrown by libWrapper.

The idea here is that modules might choose to handle conflicts with specific code, or otherwise react to the detected conflict.

Unfortunately, there are at least two places in the Foundry code where exceptions get swallowed without actually being handled, i.e. Hooks._call and Application.prototype.render. This means that, for libWrapper to work correctly, it needs to patch these methods and react to the exceptions before they are swallowed. This leads to some dirty code that I am not very happy with: 1, 2.

This request is for an official way for modules to be notified of the existence of unhandled exceptions that are swallowed by Foundry. This could be done with a hook (e.g. Hook.call('UnhandledException')) or by imitating the way JS does the error event (e.g. globalThis.dispatchEvent(new ErrorEvent('error', {error: exc, message: exc.message}));) or something else.

It is not necessary for these hooks to be able to cancel the exception logging to console, all I'd like is for there to be a simple way of being notified of their occurrence.

User Experience

This doesn't really affect the users directly in most scenarios, so is low priority.

The patching libWrapper does to the above methods to be notified of exceptions could lead to incompatibilities after updates, and that would affect the user experience until someone (likely me) gets around to fixing it. Adding this officially supported way of being notified of unhandled exceptions would therefore help avoid libWrapper breakage after updates.

The patching also pollutes the call stack, which can sometimes cause confusion, making people think libWrapper is responsible for a module bug due to it being present in the callstack of a failing hook.

Due to libWrapper being commonly used by modules (as of my count, over 30 modules currently use it, and libWrapper has a 46% Forge install base), any of these issues occuring could have some impact on user experience.

Priority/Importance

The priority is very low.

[aaclayton]

Originally in GitLab by @ruipin

Any chance this will be looked at for v9? I've noticed there seems to be a few more try/catch locations that seem to swallow errors wholesale, e.g. ClientDatabaseBackend._preCreateDocumentArray/_preUpdateDocumentArray, ClientDocumentMixin._initialize, WorldCollection._initialize, etc.

Would be nice to have to avoid hacking each of these in libWrapper individually as they come up.

[aaclayton]

I'll move this into the v9 milestone and get @Fyorl to speak with you about the use case here to see if we can figure out a good solution for how core should handle this type of try/catch.

[aaclayton]

Originally in GitLab by @ruipin

Thanks! Sure, feel free to get in touch with me over e.g. Discord.

Just a quick summary of the use cases I'm aware of:

1. As mentioned above, libWrapper waits for an exception to be unhandled before notifying the user. As such, it needs to be able to detect unhandled exceptions.

2. Since v1.9.1.0, libWrapper detects which packages were involved in the stack trace of an unhandled exception, and injects e.g. [Detected 2 packages: token-animation-tools, drag-ruler] into the exception message, to aid troubleshooting.

3. A WIP module that I have slowly been working on, "Event Viewer", would log all unhandled exceptions and give the user an easy way to share them with e.g. #modules-troubleshooting on Discord. It would also tell the user when an error occurs, which modules might be involved, give an easy way to trigger the "Bug Reporter" module, etc.

4. The League's Bug Reporter module wants to be able to include any unhandled exception signatures in the bug report automatically.

In short, we need a way to be notified there was an unhandled exception. Something like a Hook.callAll("UnhandledException", exception);.

[aaclayton]

Originally in GitLab by @Ethck

I also just wanted to put out there that I am also extremely interested in this for use in Bug Reporter (https://github.com/League-of-Foundry-Developers/bug-reporter). Ideally we would be able to find recent errors, filter it by module, and if from our "target" module attach and file it into the automatically generated bug report. Right now we can do the filtering but there are very few unhandled exceptions that actually traverse up the stream and is swallowed by core.

Feel free to reach out (Discord is definitely my quickest response).

[aaclayton]

Originally in GitLab by @ruipin

For reference, the basic idea of what I am thinking would be something like the following pseudocode-ish example:

class Hooks {
  /* ... */

  onUnhandledException(cause, exception) {
    // NOTE: This is a hack to avoid recursive notifications.
    // There's probably a cleaner way of doing it, by modifying e.g. Hooks._call, but that's outside the scope of this example
    if(this._ignore_unhandled_exceptions)
      return;
  
    try {
      this._ignore_unhandled_exceptions = true;
      this.callAll("UnhandledException", cause, exception);
    }
    catch(e) {
      // If we threw while notifying of an unhandled exception, we simply swallow this to avoid recursive notifications
      console.error(e);
    }
    finally {
      this._ignore_unhandled_exceptions = false;
    }
  }

  /* ... */
}

/* ... */

// Function that wants to swallow exceptions
function foobar() {
  /* ... */

  try {
    do_something()
  }
  catch(e) {
    Hooks.onUnhandledException("foobar", e);
    console.error(e);
    ui.notifications.error(e.message);
  }

  /* ... */
}

Obviously, there's other ways to do it, this is just a possibility.

[aaclayton]

Originally in GitLab by @Fyorl

Thanks for your help with this. We have settled on the following design:

/**
 * A hook event that fires whenever foundry experiences an error.
 *
 * @function error
 * @memberof hookEvents
 * @param {string} location      The method where the error was caught.
 * @param {Error} err            The error.
 * @param {object} [data={}]     Additional data that might be provided, based on the nature of the error.
 */
Hooks.callAll("error", location, err, data);

This includes special handling in Hooks._call to avoid recursion when an "error" hook handler also throws an error.

Around 18 or so call sites have been instrumented with this call, including all those listed in this issue.