websocket-websys: Use-after-free of onclose and onerror event handlers

[sisou]

Summary

Already mentioned in #5229, but overlooked (my fault), onclose and sometimes also onerror event handlers are called from JS after they have been dropped in Rust.

Expected behavior

No errors thrown.

Actual behavior

The Websocket connection gets closed inside the drop implementation for the socket, which then still triggers a close event on the Websocket in JS, for which the handler in Rust has however already been dropped:

Error: closure invoked recursively or after being dropped

This is an uncaught exception, which only produces console output in browsers, but makes NodeJS programs exit.

Possible Solution

A simple solution 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);

I have done this in my fork for now: https://github.com/sisou/rust-libp2p/commit/1b14ee1a5ae5375a11731328afce361fafa8ad6a#diff-c6771ff4ec042ffe20b4e526aa5be035c9e3d532f3edbd965e47541ab56fdffd

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

Version

• libp2p-core: 0.41.2
• libp2p-websocket-websys: 0.3.2

Would you like to work on fixing this bug ?

Yes

[sisou]

The reproduction at https://github.com/sisou/libp2p-websocket-reproduction still shows this issue.

[oblique]

@sisou We also have this issue in our project. Your changes looks good to me. We need this fix. Can you open the PR until you find a way to store the handlers outside the connection?

[sisou]

@oblique I was going to wait for feedback if this approach is the desired one.

But yeah, I'll make a PR next week.

I the meantime you are welcome to use my fork, like this: https://github.com/nimiq/core-rs-albatross/blob/e03175d73d9747641f14f7c5dbfb6f470aef9c96/Cargo.toml#L154

[oblique]

@sisou Cool thanks! Btw in the specification I couldn't find if onmessage/onopen can be called after the close() is invoked in case they are already in the event queue. Since this is left to the implementer, I think it would a good practice to reset them too.