From 87c8eec1b1e6308bd083291b70caf192b4efa36f Mon Sep 17 00:00:00 2001 From: Simon Mavi Stewart Date: Thu, 7 May 2026 17:44:14 +0200 Subject: [PATCH] Add BinaryMessage.wrap for transferring an owned byte array MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit BinaryMessage's two public constructors both copy their input defensively: the byte[] constructor calls System.arraycopy, and the ByteBuffer constructor copies into a freshly allocated array. That is the right default when the caller cannot promise ownership, but several call sites already produce a fresh array via ByteArrayOutputStream.toByteArray() and then immediately have it copied a second time by the constructor. For a multi-megabyte payload the second copy is wasted work and a wasted allocation. Add a static BinaryMessage.wrap(byte[]) factory that takes ownership of the array without copying. Its javadoc explicitly states that the caller must not mutate the array afterwards. The two public constructors keep their copy semantics, so existing callers that pass in a borrowed array are unaffected. Use the new factory at two slow-path reassemblers that already produce a fresh array we own: - MessageInboundConverter, when a fragmented binary message finishes via buffer.toByteArray() before being forwarded to WebSocketMessageHandler. - WebSocketFrameProxy, when the inbound frame proxy reassembles a fragmented message for an upstream WebSocket that does not support per-frame sends. The hot path through the Router and Node proxies no longer touches BinaryMessage at all — frames travel end-to-end as Netty WebSocketFrame objects — so the saving here is on the fallback paths that handle fragmented binary messages. A focused unit test pins both the copying behaviour of the existing constructor and the no-copy behaviour of the new factory. --- .../netty/server/MessageInboundConverter.java | 3 ++- .../netty/server/WebSocketFrameProxy.java | 3 ++- .../selenium/remote/http/BinaryMessage.java | 22 +++++++++++++++++++ .../remote/http/BinaryMessageTest.java | 11 ++++++++++ 4 files changed, 37 insertions(+), 2 deletions(-) diff --git a/java/src/org/openqa/selenium/netty/server/MessageInboundConverter.java b/java/src/org/openqa/selenium/netty/server/MessageInboundConverter.java index 61a843922e672..17533042d539a 100644 --- a/java/src/org/openqa/selenium/netty/server/MessageInboundConverter.java +++ b/java/src/org/openqa/selenium/netty/server/MessageInboundConverter.java @@ -71,7 +71,8 @@ protected void channelRead0(ChannelHandlerContext ctx, WebSocketFrame frame) { } if (finalFragment) { - message = new BinaryMessage(buffer.toByteArray()); + // toByteArray() returns a fresh copy we own; transfer it without re-copying. + message = BinaryMessage.wrap(buffer.toByteArray()); buffer.reset(); next = Continuation.None; } else { diff --git a/java/src/org/openqa/selenium/netty/server/WebSocketFrameProxy.java b/java/src/org/openqa/selenium/netty/server/WebSocketFrameProxy.java index 93a8a35fdba7c..40f3d9c622531 100644 --- a/java/src/org/openqa/selenium/netty/server/WebSocketFrameProxy.java +++ b/java/src/org/openqa/selenium/netty/server/WebSocketFrameProxy.java @@ -138,7 +138,8 @@ private void forwardFrame(WebSocketFrame frame) { throw new UncheckedIOException("failed to read continuation frame", e); } if (cont.isFinalFragment()) { - upstream.send(new BinaryMessage(binaryBuffer.toByteArray())); + // toByteArray() returns a fresh copy we own; transfer it without re-copying. + upstream.send(BinaryMessage.wrap(binaryBuffer.toByteArray())); binaryBuffer.reset(); next = Continuation.None; } diff --git a/java/src/org/openqa/selenium/remote/http/BinaryMessage.java b/java/src/org/openqa/selenium/remote/http/BinaryMessage.java index e0ba78b1d4344..9f36615ea60a6 100644 --- a/java/src/org/openqa/selenium/remote/http/BinaryMessage.java +++ b/java/src/org/openqa/selenium/remote/http/BinaryMessage.java @@ -37,6 +37,28 @@ public BinaryMessage(byte[] data) { System.arraycopy(data, 0, this.data, 0, data.length); } + /** + * Returns a {@link BinaryMessage} backed directly by {@code data} with no defensive copy. The + * caller transfers ownership of the array — once wrapped it must not be mutated, otherwise a + * downstream reader will see the mutation. Intended for callers that have just produced a fresh + * array (for example {@link java.io.ByteArrayOutputStream#toByteArray()}) and want to avoid the + * second allocation that the public constructor performs. + */ + public static BinaryMessage wrap(byte[] data) { + return new BinaryMessage(Require.nonNull("Data to use", data), Ownership.TRANSFER); + } + + // Sentinel for the private no-copy constructor below — gives it a distinct signature from + // the public defensive-copy BinaryMessage(byte[]) and documents the contract at the call site. + private enum Ownership { + TRANSFER + } + + private BinaryMessage(byte[] data, Ownership ownership) { + Require.nonNull("Ownership", ownership); + this.data = data; + } + public byte[] data() { return data; } diff --git a/java/test/org/openqa/selenium/remote/http/BinaryMessageTest.java b/java/test/org/openqa/selenium/remote/http/BinaryMessageTest.java index 0f5a833f32f1e..08d27b85baa31 100644 --- a/java/test/org/openqa/selenium/remote/http/BinaryMessageTest.java +++ b/java/test/org/openqa/selenium/remote/http/BinaryMessageTest.java @@ -35,4 +35,15 @@ void copiesOnlyTheReadableRegionOfABuffer() { assertThat(message.data()).containsExactly(1, 2, 3, 4); } + + @Test + void wrapTransfersOwnershipWithoutCopying() { + byte[] payload = {9, 8, 7}; + + BinaryMessage wrapped = BinaryMessage.wrap(payload); + + // wrap() shares storage with the caller; the constructor still defends with a copy. + assertThat(wrapped.data()).isSameAs(payload); + assertThat(new BinaryMessage(payload).data()).isNotSameAs(payload); + } }