Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Description • 6 years ago

Here a log:

> 1521697924289	Marionette	FATAL	Remote protocol server failed to start: [Exception... "Component returned failure code: 0x804b0036 (NS_ERROR_SOCKET_ADDRESS_IN_USE) [nsIServerSocket.initSpecialConnection]"  nsresult: "0x804b0036 (NS_ERROR_SOCKET_ADDRESS_IN_USE)"  location: "JS frame :: chrome://marionette/content/server.js :: set acceptConnections :: line 84"  data: no] Stack trace: set acceptConnections()@server.js:84
> start()@server.js:111
> init/<()@jar:file:///usr/lib64/firefox/omni.ja!/components/marionette.js:537

We should improve the error output by logging our own message which then should also include the tried port. This would drastically help to investigate connection problems.

	Andreas Tolfsen ❲:ato❳
	Comment 1 • 6 years ago

In addition to including the interface and port we tried binding
to, I think it would be even better if we gated this on the
NS_ERROR_SOCKET_ADDRESS_IN_USE error type, threw away the remainder
of the XPCOM exception, and properly formatted the stacktrace.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 2 • 6 years ago

That's what I wanted to do. Note that there is not only `NS_ERROR_SOCKET_ADDRESS_IN_USE` as available error name, but also `NS_ERROR_CONNECTION_REFUSED`, and maybe others. So best here really is to wrap the call to `ServerSocket()` in a try/catch and raise a custom error like: `throw new Error(`Could not bind to port ${this.port} (${e.name})`);`.

This gives us the port, and the type of internal error beside the stack in our own code as usual.

	Andreas Tolfsen ❲:ato❳
	Comment 3 • 6 years ago

That seems unnecessary.  You just need to look at the error codes
from Components.Results.

But in fact, if we think all exceptions caused by ServerSocket are
important, then I guess it is not so important to look at the error type
unless we want to give a specific error message for each error scenario.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 5 • 6 years ago

With my patch the output (without the stack) will look like:

> 1522256761835	Marionette	FATAL	Remote protocol server failed to start: Error: Could not bind to port 2828 (NS_ERROR_CONNECTION_REFUSED)

	Code Review Bot [:reviewbot]
	Comment 6 • 6 years ago
mozreview-review

Comment on attachment 8963255 [details]
Bug 1449612 - [marionette] Improve logging when server socket failed to start.

https://reviewboard.mozilla.org/r/232138/#review237616


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: testing/marionette/server.js:86
(Diff revision 1)
>        if (!this.socket) {
> +        try {
> -        const flags = KeepWhenOffline | LoopbackOnly;
> +          const flags = KeepWhenOffline | LoopbackOnly;
> -        const backlog = 1;
> +          const backlog = 1;
> -        this.socket = new ServerSocket(this.port, flags, backlog);
> +          this.socket = new ServerSocket(this.port, flags, backlog);
> +        }

Error: Closing curly brace does not appear on the same line as the subsequent block. [eslint: brace-style]

	Andreas Tolfsen ❲:ato❳
	Comment 8 • 6 years ago
mozreview-review

Comment on attachment 8963255 [details]
Bug 1449612 - [marionette] Improve logging when server socket failed to start.

https://reviewboard.mozilla.org/r/232138/#review237634

Yes, this is the right way to fix this.

::: commit-message-56d6d:1
(Diff revision 1)
> +Bug 1449612 - [marionette] Log server socket port in case of remote protocol server failed to start.

Fix the language and lint error, then good to land.

	Pulsebot
	Comment 10 • 6 years ago

Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/478db9be02b0
[marionette] Improve logging when server socket failed to start. r=ato

	Andrei Ciure[:aciure]
	Comment 11 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/478db9be02b0

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 12 • 6 years ago

test-only change which helps in investigating problems for Marionette and geckodriver clients to connect to Firefox. Please uplift to beta. Thanks.

	Ryan VanderMeulen [:RyanVM]
	Comment 13 • 6 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/82c30f3deccb