Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Some platforms do not support undefined as WebSocket closing code #485

Open
vla74 opened this issue Dec 18, 2019 · 5 comments
Open

Some platforms do not support undefined as WebSocket closing code #485

vla74 opened this issue Dec 18, 2019 · 5 comments
Assignees

Comments

@vla74
Copy link

vla74 commented Dec 18, 2019

Hi,

In our environment, WAMP session authentication is carried out by a specific RPC.
If an AB client tries to connect to the Crossbar server while it's (re)starting, this RPC might not be registered yet to authenticate the client (especially on slow servers).
As expected Crossbar will return error wamp.error.no_such_procedure to the client.
Then AB's connection handler self._session.onleave executes. In particular it calls:

self._transport.close();

https://github.com/crossbario/autobahn-js/blob/master/packages/autobahn/lib/connection.js#L309

In turn this method calls DOM's WebSocket.close, with, in this case, values undefined for both code and reason:

transport.close = function (code, reason) {
    websocket.close(code, reason);
};

https://github.com/crossbario/autobahn-js/blob/master/packages/autobahn/lib/transport/websocket.js#L149

But the WebSocket implementation of some browsers (Microsoft Edge and IE11; Samsung and LG Smart TVs running Chromium) have a different behaviour when code is not specified vs when code is specified with value undefined. In the latter case, the JS engine interprets undefined as 0 instead of replacing it with internal value 1005 (= "No Status Rcvd").
This causes exceptions (depending on the platform):

InvalidAccessError

Uncaught InvalidAccessError: Failed to execute 'close' on 'WebSocket': The code must be either 1000, or between 3000 and 4999. 0 is neither.

InvalidAccessError: DOM Exception 15: A parameter or an operation was not supported by the underlying object.

In this case the AB connection's onclose handler is not called, and thereby our code is not aware that reconnection must be attempted.

Apparently the implementation of these platforms seems unsuitable, as for a JS function's argument, specifying undefined and specifying no value should give equivalent results.
Nevertheless we can hardly expect that the browser implementation is changed on the affected platforms/browsers.

As an AB workaround suggestion, method transport.close could be completed:

         transport.close = function (code, reason) {
            if ('undefined' === typeof code) {
                websocket.close();
                return;
            }

            websocket.close(code, reason);
         };

Alternatively, calls self._transport.close(); could be replaced with self._transport.close(1000); (1000 = "normal closure")

Thanks

@oberstet
Copy link
Contributor

But the WebSocket implementation of some browsers (Microsoft Edge and IE11; Samsung and LG Smart TVs running Chromium) have a different behaviour when code is not specified vs when code is specified with value undefined.

This is a bug in their implementations. As per

https://www.w3.org/TR/websockets/#the-websocket-interface

the signature is

void close([Clamp] optional unsigned short code, optional DOMString reason);

and

If the method's first argument is present but is not an integer equal to 1000 or in the range 3000 to 4999, throw an InvalidAccessError exception and abort these steps.


that being said, we can of course work around in autobahn rgd above

further, I am wondering why you run into thiss exact line

https://github.com/crossbario/autobahn-js/blob/master/packages/autobahn/lib/transport/websocket.js#L208

in the first place because this part is only used when running under nodejs

and finally, it seems that "ws" (the websocket library we use when outside browser, that is on nodejs) has at some point added the close code argument

https://github.com/websockets/ws/blob/master/doc/ws.md#websocketclosecode-reason

we should forward the close code on this line

https://github.com/crossbario/autobahn-js/blob/master/packages/autobahn/lib/transport/websocket.js#L208

similar to this line

https://github.com/crossbario/autobahn-js/blob/master/packages/autobahn/lib/transport/websocket.js#L149

@vla74
Copy link
Author

vla74 commented Dec 19, 2019

Thanks for your understanding.
Indeed the implementation of these browsers seems wrong, but we can't really expect it to be fixed, so we have to apply a workaround at a higher level.

In our environment the exception occurs on this line of the browser block:
https://github.com/crossbario/autobahn-js/blob/master/packages/autobahn/lib/transport/websocket.js#L150
and not in the NodeJS block.
But of course it seems logical that the NodeJS block is completed as you mentioned, as an improvement for products running in NodeJS environment.

@oberstet
Copy link
Contributor

Ok, thanks for clarification!

So there are actually 2 things we should (the first is addressing the itch you run into):

  1. change line https://github.com/crossbario/autobahn-js/blob/master/packages/autobahn/lib/transport/websocket.js#L150 and include a guard for code being undefined - using a default code==1000
  2. change line https://github.com/crossbario/autobahn-js/blob/master/packages/autobahn/lib/transport/websocket.js#L208 and forward both code and reason (these were historically missing from the ws library .. now its there .. we should use it)

@vla74
Copy link
Author

vla74 commented Dec 19, 2019

Exactly!

@vla74
Copy link
Author

vla74 commented Apr 15, 2020

Hi,
Do you have a schedule for the fix of this ticket? Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants