Skip to content

Guard CloudProvider._sendCloudData against a cleared connection#10061

Open
Chessing234 wants to merge 1 commit intoscratchfoundation:developfrom
Chessing234:fix/cloud-provider-send-after-clear
Open

Guard CloudProvider._sendCloudData against a cleared connection#10061
Chessing234 wants to merge 1 commit intoscratchfoundation:developfrom
Chessing234:fix/cloud-provider-send-after-clear

Conversation

@Chessing234
Copy link
Copy Markdown

Bug

CloudProvider._sendCloudData throws
TypeError: Cannot read properties of null (reading 'send') when a
previously-scheduled cloud-data send fires after the provider has
been cleared (e.g. the user navigates away from a project or
requestCloseConnection() is called while messages are still in
flight).

Root cause

The constructor wraps _sendCloudData in a 100 ms lodash throttle:

this.sendCloudData = throttle(this._sendCloudData, 100);

so a call to sendCloudData(data) can be deferred by up to 100 ms.
In that window the owner can run requestCloseConnection(), which
calls clear():

clear () {
    this.connection = null;
    ...
}

When the throttle's trailing call finally fires it hits
_sendCloudData:

_sendCloudData (data) {
    this.connection.send(`${data}\n`);
}

this.connection is now null, so .send explodes. The call sites
that schedule the send (writeToServer and the onOpen queue flush)
already check this.connection before queueing, but they cannot
speak for what the connection is when the throttle trailing-edge
actually executes.

Why the fix is correct

  • Guarding the actual .send with if (this.connection) makes a
    late-arriving throttled call a no-op instead of a crash; messages
    targeted at an already-closed connection were going to be dropped
    anyway (the server can't receive them).
  • Every existing code path that has a valid connection continues to
    send exactly as before.
  • No behaviour change under normal operation - the guard only fires
    in the post-clear() race window that was previously crashing.

Change

src/lib/cloud-provider.js: wrap this.connection.send(\${data}\n`)in anif (this.connection)` guard.

CloudProvider.sendCloudData is a lodash throttled wrapper around
_sendCloudData (see the constructor's `throttle(this._sendCloudData,
100)`), so a send can be deferred by up to 100ms. In the meantime the
owner can call requestCloseConnection / clear, which sets
this.connection = null. When the deferred throttle fires it runs
    this.connection.send(`${data}\n`);
on a now-null connection and throws
"Cannot read properties of null (reading 'send')".

The call sites that schedule this send (writeToServer and the onOpen
queue flush) already check `this.connection` before queueing, so they
were relying on the connection still existing when the throttled call
eventually runs - which is exactly what clear() breaks. Wrap the
actual send in an `if (this.connection)` guard so a late throttled
call after clear() becomes a no-op instead of a crash.
@github-actions
Copy link
Copy Markdown


Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just posting a Pull Request Comment same as the below format.


I have read the CLA Document and I hereby sign the CLA


You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant