fix(websocket-websys): change close code in drop implementation to 1000

[sisou]

Description

In browsers, only the code 1000 and codes between 3000-4999 are allowed to be set by userland code: https://websockets.spec.whatwg.org/#dom-websocket-close

I found this while debugging use-after-free errors in our WASM application. Turns out, when connections are timing out, libp2p in rust drops them, but does not seem to close them explicitly (we are using libp2p-swarm). This led to these WebSockets still emitting events, the handlers of which were already dropped on the rust side, though.

A long investigation led me to have a look into the Result that gets returned from close_with_code_and_reason, and it turns out it's an error! Specifically:

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

This PR only fixes the failing closing of the WebSocket, not the remaining use-after-free problem.

Notes & open questions

Not ignoring error results

Can Err results not be ignored, but instead logged? That would have shown this problem a long time ago.

Use-after-free event handlers

Calling .close() on the WebSocket leads to the WebSocket still emitting its "close" event (and also an "error" event, if the connection is still pending and ran into the time-out), for which the websocket-websys transport registers a handler. This handler is dropped though together with the connection, so when the event comes into WASM, the handler is already free'd and this error is thrown (by wasm-bindgen?):

Error: closure invoked recursively or after being dropped

This error is not critical in browsers (only annoying in the console) but exits NodeJS programs due to an uncaught exception.

A quick-fix would be to unregister the relevant event handlers in the Drop implementation, like so:

self.inner.socket.set_onclose(None);
self.inner.socket.set_onerror(None);

According to @nibhar, there must be a better way, though. He suggests storing the socket's event handlers outside the connection, having them clean-up themselves when the onclose handler is called.

Change checklist

• I have performed a self-review of my own code
• I have made corresponding changes to the documentation
• I have added tests that prove my fix is effective or that my feature works
• A changelog entry has been made in the appropriate crates

[sisou]

Here is a basic reproduction which I used to create this PR: https://github.com/sisou/libp2p-websocket-reproduction

[jxs]

Hi, and thanks for the thorough report! Looks good to me, can you also add an entry in the CHANGELOG.md?
cc @thomaseizinger you were the one adding the code, you may want to review it, and probably have an opinion on the use after free situation.

[thomaseizinger]

Wow, that is surprising!

Great find. Yeah needs a version bump and changelog entry.

[jsdanielh]

Wow, that is surprising!

Great find. Yeah needs a version bump and changelog entry.

Thanks for reviewing it. We just added the version bump and changelog entry.

[jxs]

Thanks! Just waiting on #5253 so we can get CI green again

[jxs]

@jsdanielh do you require a release?

[jsdanielh]

@jsdanielh do you require a release?

Yeah, that would be great

[jxs]

sorry for the delay @jsdanielh and thanks for your patience.
A libp2p-websocket-websys version has now been released, see #5351 for more information.