lightclient: Fix wasm socket closure called after being dropped

[lexnv]

The errors reported by the wasm related to Error closure invoked recursively or destroyed already could happen whenever a Closure object is used after being dropped.

This PR makes the assumption that the JS realm socket created by web_sys::WebSocket is able to access the references of the registered Closure callbacks, even after the Rust object has been dropped due to the socket not being closed properly.

subxt/lightclient/src/platform/wasm_socket.rs

Line 103 in f3eb4cd

socket.set_onopen(Some(open_callback.as_ref().unchecked_ref()));

This PR makes the following adjustments to fix and improve the code:

• The main fix is to use Closure::once_into_js for closures that can are called only once

  • This primitive will deallocate the memory of the closure after the call happens (eg. on_close, on_error, on_open callbacks are called once (https://developer.mozilla.org/en-US/docs/Web/API/WebSocket#events)
  • The on_close and on_open are evident, however for the on_error the documentation implies it is called only once

  error
  Fired when a connection with a WebSocket has been closed because of an error, such as when some data couldn't be sent. Also available via the onerror property.

• poll_read verifies if the connected is not opened yet, then it saves the waker and returns Poll::Pending

• poll_close now closes the socket

• Socket is closed from Drop only if its internal state is open

Closes: #1288

[niklasad1]

LGTM :)

[tadeohepperle]

Looks good, very nice!

[jsdw]

The docs for once_into_js say:

Convert a FnOnce(A...) -> R into a JavaScript Function object.

If the JavaScript function is invoked more than once, it will throw an exception.

Unlike Closure::once, this does not return a Closure that can be dropped before the function is invoked to deallocate the closure. The only way the FnOnce is deallocated is by calling the JavaScript function. If the JavaScript function is never called then the FnOnce and everything it closes over will leak.

So, since some of these callbacks might never fire, I think we'd be gradually leaking them here for any connection we make that doesn't fire one of the callbacks.

I wonder if the errors we got happened because previously, if Self is dropped before one of the JS websocket callbacks is fired, then it would have decallocated the closures and we'd get an error in JS land instead.

But anyway, from reading the spec, it sounds like we:

• May never fire onopen if connection can't be opened (but will fire it max once)
• May never fire onerror or onmessage if no errors/messages sent (not certain if onerror will only fire once or not)
• Will always eventualy fire onclose (max once).

So if I read right, I wonder whether we can do some cleanup in onclose closure to ensure that all of the other closures are dropped. (eg Closure::new() for them all, save the Closures, and drop() them all when onclose fires or something like that)

[lexnv]

That makes sense!

I've found another way, which I believe to be more straight forward than performing clean-up on the onclose:

• keep the Closure::new() around for each callback
• while dropping the object:
  • close the socket if in: OPEN or CONNECTING states
  • set closures on the web_sys::WebSocket as None

This shall clean-up all memory associated with the rust closures, and deny the javascript real access to the dropped objects

[jsdw]

So I think this is ook because any of the callbacks will wake the waker when JS clals them, ensuring we call this poll_read again.

[jsdw]

Would it be safer to say if ready_state != CLOSING then close()? Or maybe just call close() regardless to ensure our onclose handler always fires and can tidy up (I think nothing happens if you try closing when it's already closing anyway but not sure!)

[lexnv]

I've extended this to do clean-up for OPEN and CONNECTING, since we might generate console errors if we attempt to close a socket that's already closed 👍

[lexnv]

Attempting to close the socket regardless of its state will generate on --chrome:

WebSocket connection to 'ws://polkadot-boot.dwellir.com:30334/' failed: WebSocket is closed before the connection is established.

The same error is generated if the socket is in the CONNECTING state, note that --firefox is not printing this warning.

[lexnv]

I've changed the following to this PR:

• Closures are created with new and the socket callbacks are set to None in the Drop to avoid recursive calls and leaking closures (prior done with Closure::once_into_js that might never be invoked)
• Closing the socket is performed on drop if the state is not already CLOSING
  • this generates an warning in the Chrome browser (lightclient: Fix wasm socket closure called after being dropped #1289 (comment)); in balance with the alternative of possibly leaking the socket I believe this ok
  • Firefox does not present the warning, which might be specific to Chrome since the documentation states that close does nothing in the unexpected states: https://developer.mozilla.org/en-US/docs/Web/API/WebSocket/close

The bootnode that was causing issues seems to be online now, which makes testing a bit more difficult.

Alternatives for testing:

• For manual testing we could populate the bootnodes with invalid ports / DNS; after this PR is applied the Uncaught errors are resolved:

        .bootnodes([
            "/dns/polkadot-boot.dwellir.com/tcp/1/ws/p2p/12D3KooWKvdDyRKqUfSAaUCbYiLwKY8uK3wDWpCuy2FiDLbkPTDJ",
            "/dns/polkadot-boot.dwellir.com/tcp/1/wss/p2p/12D3KooWKvdDyRKqUfSAaUCbYiLwKY8uK3wDWpCuy2FiDLbkPTDJ",
            "/dns/dot14.rotko.net/tcp/1/p2p/12D3KooWPyEvPEXghnMC67Gff6PuZiSvfx3fmziKiPZcGStZ5xff",
        ])

• For automated testing; I did not find a way to capture the browser's console programmatically in rust
  • I have tried creating hooks to panics and errors, although they seem to only capture the WASM module / errors coming from Rust

    let errors = Box::leak(Box::new(Arc::new(Mutex::new(Vec::with_capacity(16)))));
   std::panic::set_hook(Box::new(|info: &std::panic::PanicInfo| {
       errors
           .lock()
           .expect("Single threaded; qed")
           .push(info.to_string());
   }));

• We might need a different system for testing this to ensure that we report back zero errors or that !error.contains("closure invoked recursively"). Considering the complexity of doing that, we could takle it at a later time

[niklasad1]

• Closing the socket is performed on drop if the state is not already CLOSING
• this generates an warning in the Chrome browser (lightclient: Fix wasm socket closure called after being dropped #1289 (comment)); in balance with the alternative of possibly leaking the socket I believe this ok
• Firefox does not present the warning, which might be specific to Chrome since the documentation states that close does nothing in the unexpected states: https://developer.mozilla.org/en-US/docs/Web/API/WebSocket/close

Fine be me

For manual testing we could populate the bootnodes with invalid ports / DNS; after this PR is applied the Uncaught errors are resolved:

Can we add a GHA cronjob that queries the bootnodes once per day to detect that?

For automated testing; I did not find a way to capture the browser's console programmatically in rust

I have tried creating hooks to panics and errors, although they seem to only capture the WASM module / errors coming from Rust

I guess we could to setup a CI that runs these in chrome and firefox seems a bit overkill for now.

[niklasad1]

An alternative is to wrap https://docs.rs/gloo-net/latest/gloo_net/websocket/futures/struct.WebSocket.html and implement AsyncRead and AsyncWrite for SplitStream (stream, sink).

The implementations looks very similar without the option stuff that I don't really understand, is it because you don't want to box the closures and leak them?

[lexnv]

An alternative is to wrap https://docs.rs/gloo-net/latest/gloo_net/websocket/futures/struct.WebSocket.html and implement AsyncRead and AsyncWrite for SplitStream (stream, sink).

Indeed, I remember one of the reasons to not use gloo-net was because it had no support for AsyncRead and AsyncWrite. As of version 0.4.0 there is no support yet for those (https://crates.io/crates/gloo-net).
However, this unreleased PR rustwasm/gloo#379 should add everything that we need.

In the meanwhile, I would suggest using our WebSocket until there is another release. Then, we could migrate to gloo-net and see if it fits our purpose, what do you think? 🙏

The implementations looks very similar without the option stuff that I don't really understand, is it because you don't want to box the closures and leak them?

Do you mean the option that we set for populating the callbacks?

subxt/lightclient/src/platform/wasm_socket.rs

Line 103 in abe77b5

socket.set_onopen(Some(open_callback.as_ref().unchecked_ref()));

I believe we are using a different API than gloo-net, which uses:

ws.add_event_listener_with_callback_and_add_event_listener_options(
            "open",
            open_callback.as_ref().unchecked_ref(),
            web_sys::AddEventListenerOptions::new().once(true),
        )
        .map_err(js_to_js_error)?;

Can we add a GHA cronjob that queries the bootnodes once per day to detect that?

We could use this CLI for checking bootnodes: https://github.com/lexnv/subp2p-explorer#verify-bootnodes.

It would be good if we can find a way to capture the browser logs, the ones from #1288, because I was not able to reproduce those while running in --headless mode 🤔

[niklasad1]

Do you mean the option that we set for populating the callbacks?

I was wrong I missed that that part...