1724411 - MessageHandler: Improve error reporting when a command is not implemented

Status: RESOLVED DUPLICATE of bug 1726800

(Remote Protocol :: WebDriver BiDi, task, P2)

Description

[Julian Descottes [:jdescottes]]

Errors are hard to understand at the moment, and it can take a long time to understand why a given command is not running as expected.

I anything goes wrong when loading a module to handle a command, the MessageHandler simply falls back on the forwardCommand codepath, which will lead to errors such as Cannot forward command to "ROOT" from "ROOT", which is not helpful.

We discussed with :whimboo about this topic, and we can already see a few class of errors which could be more explicit.

1. Trying to forward from ROOT to ROOT or from WINDOW_GLOBAL

If we attempt to forward a command which has the same destination as the current MessageHandler type, then it's clear that we could not find the correct module & method to handle it and we are in a dead-end. Instead of raising an error about the "forward", we should explicitly say that.

For instance, if module is session and method is subscribe, we should directly log:

Could not find a ROOT "session" module implementing the command "subscribe".

Even better, if we could say whether the issue was that the module could not be imported or that it didn't support the method.

The same applies to WINDOW_GLOBAL MessageHandlers. If they attempt to forward to WINDOW_GLOBAL, it's pointless to log Cannot forward commands from a "WINDOW_GLOBAL" MessageHandler. We should log a similar message to the one suggested for the ROOT module above.

Basically, I imagine we can add a check for the destination.type === this.constructor.type right before https://searchfox.org/mozilla-central/rev/67637695876e27ddbfb6ff99202bb1c951de847a/remote/shared/messagehandler/MessageHandler.jsm#128:

  handleCommand(command) {
    const { moduleName, commandName, destination, params } = command;
    logger.trace(
      `Received command ${moduleName}:${commandName} for destination ${destination.type}`
    );

    const mod = this._moduleCache.getModuleInstance(moduleName, destination);
    if (this._isCommandSupportedByModule(commandName, mod)) {
      return mod[commandName](destination, params);
    }

    if (destination.type === this.constructor.type) {
      throw new Error(
        `Could not find a ${this.constructor.type} "${moduleName}" module implementing the command "${commandName}".`
      );
    }

    return this.forwardCommand(command);
  }

Note: When discussing this usecase, we imagined adding an API in ModuleCache to know if a module was implemented in the next layer to avoid unnecessary forwards. But this actually doesn't apply to this use case because the destination is already the current type, so there is no next layer.

2. Throw if the module file was present but couldn't be loaded

At the moment we just try/catch in the ModuleCache when importing modules, because implementing Modules in all layers should be optional.

However, this means a syntax error in a module jsm will also be silently ignored, and we will just consider the module as not implemented.

It would be great to differentiate between a missing module and a module with an error. For the latter, we should explicitly throw.

3. Avoid unnecessary forwarding

Mentioning this here since the was the original solution discussed with :whimboo, but I think that the approach suggested in point 1. should already improve the error reporting significantly.

The idea here would be to check for the existence of a module in the next layer before trying to forward. We could open an API on the ModuleCache so that a given MessageHandler could check for the existence of modules corresponding to other context.types.

It might add some complexity as we start considering workers. Typically a command intended for a worker can be implemented in three modules:

• worker-in-root/module.jsm
• worker-in-windowglobal/module.jsm
• worker/module.jsm

Before forwarding from the ROOT to WINDOW_GLOBAL, we will have to check whether worker-in-windowglobal/module or worker/module exist. And again check worker/module before forwarding from WINDOW_GLOBAL to WORKER.

It seems like a lot of added logic for a check that should naturally be handled in the MessageHandler that has the same type as the destination

Comment 1

[Julian Descottes [:jdescottes]]

Attachment: Bug 1724411 - MessageHandler: Improve error reporting when a command is not implemented

Comment 2

[Julian Descottes [:jdescottes]]

I'm not planning to work on this at the moment, I just wanted to share a quick WIP patch...

Comment 3

[Julian Descottes [:jdescottes]]

In the end this will probably be related to Bug 1723592.

After Bug 1721627 landed, we can no longer use dynamic imports. And overall we might just benefit from having the list of modules, theirs commands & events, clearly exposed, so that we can query the information upfront.

If we have such information, we should be able to provided a clear error message whenever we receive a command which cannot be correctly executed.