Avoid exception when cursor was removed from DOM already

[sbungartz]

Hi,
we were getting a NotFoundError: Failed to execute 'removeChild' on 'Node': The node to be removed is not a child of this node from this call because the cursor had already been removed from the DOM.
This happens in our case because we use noVNC together with Turbo. This is a library / framework that replaces parts of the page or the entire document with server side rendered content.

So when leaving the page with the connected noVNC on it, we need to call rfb.disconnect() to close the connection, since we remain in the same JS context. By the time we do that, the DOM is already replaced though, so the canvas is no longer in document and the error occurs.

Since this line in noVNC is intended to remove the element from the DOM, we argue, that it is not an error if it was not in the DOM in the first place.

Thank you for your time and effort!

[samhed]

Sounds reasonable. Thank you for the thorough explanation!

I assume you tested your changes?

[sbungartz]

Thank you for the quick response!

I assume you tested your changes?

Yes, I used pnpm link to try our specific case with this change applied and it then got to closing the socket.

[CendioOssman]

I think it's a bit rude of Turbo to remove everything from the body element, but this seems like a reasonable handling of that.

contains() does a recursive search over the entire DOM, though. That seems excessive if we always try to remove the canvas from the body element. Would this._canvas.remove() work instead? Or perhaps examine this._canvas.parentNode?

[Timmitry]

@CendioOssman I do not know how browsers implement body.contains(descendant), but I would guess that they just start at the descendant, and look at the parent. If it is null, the function returns false. If it is the body, the function returns true. If it is another element, the function will call body.contains(parent). That should be really fast, especially since in this case the canvas will either have the body as direct parent, or no parent at all.

Simply using this._canvas.remove() might work as well, but I'm not sure if body.removeChild(this._canvas) does something else, and we would need to keep this behaviour? 🤔

[CendioOssman]

We don't need anything particularly special. We just want to clean up after ourselves as best we can. So we probably don't need to dig too deep in to this. :)

[samhed]

I tested this by using the following steps:

1. Open noVNC master in Chrome
2. Enable the device toolbar and select “Responsive” to simulate a touch device
3. Connect in noVNC
4. Use Chrome's javascript console to remove the canvas
5. Disconnect in noVNC
6. See the Uncaught NotFoundError

Following the same steps but with your patch, disconnecting works well and does not produce an error. Looks good - Merging.