FWD » Core Development » Runtime Infrastructure

================================================================================================

PROPOSAL - [CRITICAL] functional - WebClientProtocol.sendBinaryMessageStreamed / queueChunk

A MSG_CHUNK transfer is not bound to a session; its tail is delivered to the next one

================================================================================================

REVIEW FINDING (abridged)

   "A MSG_CHUNK transfer is bound to nothing but 'whatever session and worker are current right

   now', so a reconnect that straddles a transfer delivers its tail to the *new* browser session

   [...] the piece with flags bit 0 re-enters messageHandler with whole[0] = an arbitrary pixel or

   font byte. 0x85 is MSG_QUIT, which calls doRedirectToLogoutPage() - a spurious logout roughly one

   time in 256."

FILES

   src/com/goldencode/p2j/ui/client/driver/web/WebClientProtocol.java

   src/com/goldencode/p2j/ui/client/driver/web/PushMessagesWorker.java

   src/com/goldencode/p2j/ui/client/driver/web/WebClientMessageTypes.java

   src/com/goldencode/p2j/ui/client/driver/web/res/p2j.socket.js

------------------------------------------------------------------------------------------------

1. ROOT CAUSE

------------------------------------------------------------------------------------------------

queueChunk (WebClientProtocol.java:1003) validates each piece against the *current* session:

   if ((pushWorker == null) || !session.isOpen())

There is no record of which session the transfer began on. Two independent routes therefore carry

a transfer's tail onto a later connection:

   (1) The queue survives the swap. startPushWorker (:1962) reuses the existing worker

       (if (pushWorker == null)) and only calls setCurrentSession; onConnect (:444) never calls

       stopPushWorker - the sole caller is onClose's active-session branch (:811), which the

       session != this.session guard (:782) skips for a replaced session. Pieces already sitting in

       the deque are then written to the new socket by the worker.

   (2) The producer outlives the cleanup. Even where onClose does run stopPushWorker, the thread

       still inside the while (sent < bodyLen) loop (:958) simply observes the freshly created

       worker and open session on its next queueChunk call and queues the remainder there.

This is the *designed* ordering on a half-open link: the page gives up after maxLostPings periods,

roughly half the server idle timeout, so onConnect(new) precedes onClose(old). The endpoint is the

same object across a reconnect - GuiWebDriver creates one GuiWebSocket and GenericWebSocketCreator

returns that same instance for every upgrade, which is precisely why onConnect has to cope with a

pre-existing this.session.

On the page, handleCloseEvent ran resetChunkTransfers(), and collectChunk (p2j.socket.js:3197) keys

only on the transfer id with no notion of where a transfer starts:

   var parts = chunkTransfers[id];

   if (!parts) { parts = chunkTransfers[id] = []; }

so an arriving tail silently begins a fresh reassembly, and the piece carrying flags bit 0

concatenates it and re-enters messageHandler with whole[0] being raw pixel or font data.

Note the two routes need different remedies: (1) is about bytes *already queued* and can only be

caught at the receiver or by purging the queue; (2) is about bytes *not yet produced* and is caught

at the sender. Fixing only one leaves the other open.

------------------------------------------------------------------------------------------------

2. PROPOSED FIX

------------------------------------------------------------------------------------------------

Three changes. (a) and (b) are each sufficient against one route and are both required; (c) is a

bandwidth and latency cleanup that also removes the failure entirely in the common case.

2.1 (a) Bind the transfer to a connection generation  [stops route 2]

----------------------------------------------------------------------

A monotonic counter bumped on every accepted connection is enough; it needs no lock beyond the one

onConnect already holds.

   +  /**

   +   * Incremented on every accepted websocket connection.  A streamed transfer captures it when it

   +   * starts and abandons itself once it no longer matches, so a transfer can never span a

   +   * reconnect: its tail would arrive at a peer that never saw its head.

   +   */

   +  private volatile int connectionGeneration;

in onConnect (:456), alongside the session assignment:

           this.session = session;

   +       connectionGeneration++;

sendBinaryMessageStreamed captures it once and threads it through:

      public void sendBinaryMessageStreamed(byte[] header, InputStream body, long bodyLen)

      {

         int transferId = nextTransferId.incrementAndGet();

   +     int generation = connectionGeneration;

         ...

   -     if (!queueChunk(transferId, first, header.length, !hasBody) || !hasBody)

   +     if (!queueChunk(generation, transferId, first, header.length, true, !hasBody) || !hasBody)

and queueChunk gains the check, inside the existing synchronized (lock) block so it is consistent

with the session read next to it:

   -  private boolean queueChunk(int transferId, byte[] msg, int payloadLen, boolean last)

   +  private boolean queueChunk(int generation, int transferId, byte[] msg, int payloadLen,

   +                             boolean first, boolean last)

      {

         awaitSendCapacity();

         synchronized (lock)

         {

   -        if ((pushWorker == null) || !session.isOpen())

   +        // the generation test must come first: on a reconnect the new session is open, so an

   +        // isOpen() check alone would happily accept this transfer's tail onto it

   +        if ((generation != connectionGeneration) || (pushWorker == null) || !session.isOpen())

            {

               return false;

            }

            msg[0] = MSG_CHUNK;

            writeMessageInt32(msg, 1, transferId);

   -        msg[5] = (byte) (last ? 1 : 0);

   +        msg[5] = (byte) ((last ? CHUNK_FLAG_LAST : 0) | (first ? CHUNK_FLAG_FIRST : 0));

The existing callers already treat a false return as "abandon the transfer" (:950, :975), so the

loop unwinds correctly with no further change. The early-EOF and IOException paths (:970, :983) pass

first = false.

2.2 (b) Refuse to start a reassembly mid-stream  [stops route 1]

------------------------------------------------------------------

Flags bit 1 marks the piece that opens a transfer. In WebClientMessageTypes, beside MSG_CHUNK:

   +  /** {@link #MSG_CHUNK} flags bit 0: this piece completes the transfer. */

   +  public static final byte CHUNK_FLAG_LAST = 0x01;

   +

   +  /**

   +   * {@link #MSG_CHUNK} flags bit 1: this piece opens the transfer.  The peer refuses to begin a

   +   * reassembly without it, so a tail that outlived its connection is discarded rather than

   +   * concatenated into a message whose first byte is then arbitrary payload.

   +   */

   +  public static final byte CHUNK_FLAG_FIRST = 0x02;

and update the MSG_CHUNK javadoc, which currently documents only bit 0.

In p2j.socket.js, collectChunk:

      function collectChunk(message)

      {

         var id = me.readInt32BinaryMessage(message, 1);

         var last = (message[5] & 1) !== 0;

   +     var first = (message[5] & 2) !== 0;

         var piece = message.subarray(6);

         var parts = chunkTransfers[id];

         if (!parts)

         {

   +        if (!first)

   +        {

   +           // a tail whose head went to a previous socket: dispatching it would run an arbitrary

   +           // payload byte as a message type, so drop it and wait for a real transfer start

   +           p2j.logger.error("Discarding orphaned message piece, transfer id " + id);

   +           return null;

   +        }

            parts = chunkTransfers[id] = [];

         }

This is the load-bearing half of the fix: it is the only thing that protects against pieces the

worker had already accepted before the swap, which no sender-side check can recall.

2.3 (c) Purge queued output on session replacement  [optional, recommended]

----------------------------------------------------------------------------

With (a) and (b) the tail is harmless, but it is still transmitted and still discarded at the peer.

Dropping it at the swap saves the bandwidth and shortens the reconnect. In PushMessagesWorker:

   +  /**

   +   * Discard everything still queued and release any throttled producer.

   +   * <p>

   +   * Called when the session is replaced: the queued output was addressed to a socket that is

   +   * gone, and {@code onConnect} resynchronizes the peer through {@code initRemoteClient} anyway.

   +   */

   +  public void discardQueued()

   +  {

   +     messages.clear();

   +     queuedBytes.set(0L);

   +

   +     synchronized (capacityGuard)

   +     {

   +        capacityGuard.notifyAll();

   +     }

   +  }

called from onConnect immediately before startPushWorker(session), i.e. after the generation bump:

   +       if (pushWorker != null)

   +       {

   +          pushWorker.discardQueued();

   +       }

           startWebWorker();

           startPushWorker(session);

CAVEAT worth weighing before taking (c): a reconnect does not reload the page, so the canvas keeps

its contents, and this drops draw output that the peer has not yet received. onConnect calls

callbacks.initRemoteClient() to resynchronize, so this should be safe, but it is the one part of

this proposal that changes behaviour for non-chunk traffic. If that resync is not trusted to be

complete, take (a) and (b) only - they are sufficient for correctness on their own.

------------------------------------------------------------------------------------------------

3. RELATIONSHIP TO OTHER PROPOSALS

------------------------------------------------------------------------------------------------

   proposal-issue-major-functional-1   the page-side cleanup gap (resetChunkTransfers unreachable

                                       on client-initiated closes).  Its "same-id join" case is

                                       closed by 2.2 here; that proposal states the dependency.

   proposal-issue-minor-functional-1   the MAX_CHUNK_BYTES abandon path, which has the same

                                       arbitrary-first-byte hazard and is likewise closed by 2.2.

2.2 is the common remedy for all three findings. If only one change from this whole review is

taken, take that one.

------------------------------------------------------------------------------------------------

4. RISK AND VERIFICATION

------------------------------------------------------------------------------------------------

Risk: low-to-moderate. (a) and (b) are additive guards on a path that currently has none; the wire

format changes only in the interpretation of previously unused flag bits, and server and page ship

together, so there is no mixed-version window. (c) is the only behavioural change and is the part

to drop if the resync assumption does not hold.

Verification

   1. Stream a large image (a maximized window with a full-size background, so bodyLen is several

      MB), and drop the network at the OS level mid-transfer (iptables DROP) so the page reconnects

      while the producer is still looping. Expected: server log shows the transfer abandoned at the

      generation check; page log shows no "Discarding orphaned message piece" (2.3 purged them) or

      a bounded number of them (without 2.3); no spurious logout, no canvas corruption.

   2. Same, with 2.3 omitted, to confirm 2.2 alone contains the damage.

   3. Force the 1-in-256 case deliberately: temporarily make the peer log whole[0] instead of

      dispatching, run test 1 repeatedly, and confirm the pre-fix build produces MSG_QUIT-valued

      first bytes while the fixed build produces none.

   4. Confirm a normal (non-interrupted) large image and a custom font still render, i.e. the

      first-piece flag is actually set on the opening piece of both callers - GuiWebSocket

      defineImageStreamed and createFont.

   5. Confirm the header-only case (body == null) still works: it sends one piece with both the

      first and last bits set.