1760900 - Incorrect error message when trying to call an unknown internal command

Status: RESOLVED FIXED

(Remote Protocol :: Agent, defect, P3)

Description

[Julian Descottes [:jdescottes]]

See https://searchfox.org/mozilla-central/source/remote/shared/webdriver/Session.jsm#249-251

We translate all messageHandlerError.UnsupportedCommandError to error.UnknownCommandError and we assume that the unknown command is the "topmost" command.

But a message handler command can rely on other message handler commands (most likely internal ones). This means that you can get an UnsupportedCommandError not because the initial command is unknown, but because an internal command has been mispelled.

For instance, imagine you call the command log.someMethod which in turn calls internalModule._internalCommand, but you wrote _internalCommand with a typo: _internalComand. Right now you will get in the logs unknown command, log.someMethod. Which is incorrect. log.someMethod is a known command, but it had a runtime error.

Maybe we should check that the UnsupportedCommandError was thrown for the expected command, and if not log another kind of error. Hopefully preserving the information from the initial error message, which contains the command name which is actually unknown.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

So the root message handler should simply check if the given BiDi command is available and as such throw this error. Once we called the command we might want to ignore the failure or at least log a warning?

Comment 2

[Julian Descottes [:jdescottes]]

To do that we would need to differentiate internal calls to handleCommand from external ones. A root module might call RootMessageHandler.handleCommand, and there is no way to differentiate that call from the one made at https://searchfox.org/mozilla-central/source/remote/shared/webdriver/Session.jsm#232

Otherwise, maybe remote/shared/webdriver/Session.jsm can explicitly check if the command is implemented before calling RootMessageHandler.handleCommand. And avoid translating errors.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Julian Descottes [:jdescottes] from comment #2)

Otherwise, maybe remote/shared/webdriver/Session.jsm can explicitly check if the command is implemented before calling RootMessageHandler.handleCommand. And avoid translating errors.

This actually sounds like a good idea!

Comment 4

[Alexandra Borovova [:Sasha]]

For the context, the approach to solve it would be to move this part https://searchfox.org/mozilla-central/source/remote/shared/messagehandler/MessageHandler.jsm#217-220 into a separate supportCommand method and then use it here https://searchfox.org/mozilla-central/source/remote/shared/webdriver/Session.jsm#232 before calling the command.

There is a remaining question, how to test it in the best way.
We could add a test to messagehandler/test/browser to check introduced supportCommand, but also would be nice to check the actual scenario of calling the wrong internal command. This most likely needs to be at the webdriver level.
The idea from Julian was to add it into remote/shared/webdriver/test/xpcshell/test_Session.js and fake _messageHandler property used by the session, the alternative would be to write a real (integration) test with faking modules to simulate the test case, which might be quite complicated. What do you think, Henrik?

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Alexandra Borovova from comment #4)

We could add a test to messagehandler/test/browser to check introduced supportCommand, but also would be nice to check the actual scenario of calling the wrong internal command. This most likely needs to be at the webdriver level.

Please note that at the WebDriver level, which means as a wdspec test, we won't be able to call internal methods. For these tests we can only access the publicly defined methods of the protocol.

The idea from Julian was to add it into remote/shared/webdriver/test/xpcshell/test_Session.js and fake _messageHandler property used by the session, the alternative would be to write a real (integration) test with faking modules to simulate the test case, which might be quite complicated. What do you think, Henrik?

Hm, I think that having just a normal browser chrome test module like the others we have under remote/shared/messagehandler/test/browser/resources/modules should work? We can have a valid public method which then calls an unknown message. This call should not return as unknown command but as an unknown error. Or what specifically would make it complicated here (which I might have missed).

Comment 6

[Alexandra Borovova [:Sasha]]

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #5)

Hm, I think that having just a normal browser chrome test module like the others we have under remote/shared/messagehandler/test/browser/resources/modules should work? We can have a valid public method which then calls an unknown message. This call should not return as unknown command but as an unknown error. Or what specifically would make it complicated here (which I might have missed).

The change which I'm going to introduce will be in https://searchfox.org/mozilla-central/source/remote/shared/webdriver/Session.jsm, in https://searchfox.org/mozilla-central/source/remote/shared/messagehandler/MessageHandler.jsm there will be only a small refactoring. So it looks like the test should be also around https://searchfox.org/mozilla-central/source/remote/shared/webdriver/Session.jsm and in remote/shared/messagehandler/test/browser/ we are testing the MessageHandler itself. But maybe I've also missed something.

Comment 7

[Julian Descottes [:jdescottes]]

I agree that if we want to do this properly, this test should be under remote/shared/webdriver/test/, not under remote/shared/messagehandler/test/. But it comes with some complexity, and I was not sure this was worth the hassle.

We can introduce a new "browser chrome mochitest" suite in this folder (meaning create a new "browser" folder there, and do the initial plumbing to make it work) and reuse a similar setup as the one we used remote/shared/messagehandler/test/. But even if we can use "test" modules instead of the BiDi ones, they are hardcoded to use remote/shared/messagehandler/test/browser/resources/modules/ModuleRegistry.jsm. Using them in remote/shared/webdriver/test/ will be odd. Ideally we'd update our logic so that tests can define the path to a custom ModuleRegistry.jsm in the preference remote.messagehandler.modulecache.useBrowserTestRoot (which is only a boolean for now).

And is it also worth adding a new browser chrome suite here? We tried to be quite strict about testing: message handler tests should be browser chrome mochitests, while webdriver bidi tests should be wdspec. This falls a bit in between , because we want to test something that can only be observed from the "bidi" layer, under certain conditions in the "messagehandler" layer.

Or we can add the new test directly under remote/shared/messagehandler/test/. It's not great, but will "work".

So our options are:

• add a browser chrome mochitest in remote/shared/messagehandler/test/: not clean, but low effort
• add a browser chrome mochitest in remote/shared/webdriver/test/: clean, but additional cost
• add an xpcshell test in remote/shared/webdriver/test/: clean, low effort but not a great test

Comment 8

[Alexandra Borovova [:Sasha]]

(In reply to Julian Descottes [:jdescottes] from comment #7)

Or we can add the new test directly under remote/shared/messagehandler/test/. It's not great, but will "work".

But do I understand it correct that in remote/shared/messagehandler/test/ we can test only supportsCommand part?

Comment 9

[Julian Descottes [:jdescottes]]

(In reply to Alexandra Borovova from comment #8)

(In reply to Julian Descottes [:jdescottes] from comment #7)

Or we can add the new test directly under remote/shared/messagehandler/test/. It's not great, but will "work".

But do I understand it correct that in remote/shared/messagehandler/test/ we can test only supportsCommand part?

No, we can test more. We can load remote/shared/webdriver/Session.jsm and instantiate it. There are no restrictions which limit which modules we can load from a given test folder. So we definitely can do it. It's not the best place to test this class, but technically nothing would prevent us from doing it.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Oh I see. I missed the case that we want to test for a proper error message for internal errors on the WebDriver level.

(In reply to Julian Descottes [:jdescottes] from comment #9)

No, we can test more. We can load remote/shared/webdriver/Session.jsm and instantiate it. There are no restrictions which limit which modules we can load from a given test folder. So we definitely can do it. It's not the best place to test this class, but technically nothing would prevent us from doing it.

I agree that testing code outside of this folder isn't ideal but maybe we can make it an exception here? Given that it saves us the costs to create a new browser chrome test suite and having to adjust the module loading path (which might not be safe in production code). We could have the test in a separate folder for now, and when it turns out that we might need other browser chrome tests for shared/webdriver we could move this test over?

Comment 11

[Alexandra Borovova [:Sasha]]

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #10)

I agree that testing code outside of this folder isn't ideal but maybe we can make it an exception here? Given that it saves us the costs to create a new browser chrome test suite and having to adjust the module loading path (which might not be safe in production code). We could have the test in a separate folder for now, and when it turns out that we might need other browser chrome tests for shared/webdriver we could move this test over?

Sounds good to me

Comment 12

[Alexandra Borovova [:Sasha]]

Attachment: Bug 1760900 - Ensure a correct error message when trying to call an unknown internal command. r=jdescottes

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a813e75186c5
Ensure a correct error message when trying to call an unknown internal command. r=jdescottes,webdriver-reviewers,whimboo

Comment 14

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/a813e75186c5