[grid] Add BinaryMessage.wrap for transferring an owned byte array

[shs96c]

Summary

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 (#17435) 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.

🤖 Generated with Claude Code

[qodo-code-review]

Review Summary by Qodo

Add BinaryMessage.wrap for zero-copy byte array ownership transfer

✨ Enhancement

Walkthroughs

Description

• Add BinaryMessage.wrap() factory for zero-copy ownership transfer
• Eliminates redundant array copying in fragmented message reassemblers
• Updates MessageInboundConverter and WebSocketFrameProxy to use new factory
• Adds unit test verifying wrap's no-copy behavior vs constructor's defensive copy

Diagram

flowchart LR
  A["Fragmented Binary Message"] -->|reassemble| B["ByteArrayOutputStream.toByteArray()"]
  B -->|old: copy| C["BinaryMessage constructor"]
  B -->|new: wrap| D["BinaryMessage.wrap()"]
  C --> E["BinaryMessage with copied data"]
  D --> F["BinaryMessage with transferred ownership"]
  E -->|2 allocations| G["Performance cost"]
  F -->|1 allocation| H["Optimized path"]

Loading

File Changes

1. java/src/org/openqa/selenium/remote/http/BinaryMessage.java ✨ Enhancement +20/-0

Add wrap factory and private ownership-transfer constructor

• Add static wrap(byte[]) factory method that transfers ownership without copying
• Introduce private constructor accepting Ownership enum for internal use
• Add comprehensive javadoc explaining ownership transfer semantics and use cases
• Define private Ownership enum to distinguish wrap behavior from copy constructors

java/src/org/openqa/selenium/remote/http/BinaryMessage.java

---

2. java/src/org/openqa/selenium/netty/server/MessageInboundConverter.java ✨ Enhancement +2/-1

Use wrap factory in fragmented message finalization

• Replace new BinaryMessage(buffer.toByteArray()) with BinaryMessage.wrap(buffer.toByteArray())
• Add comment explaining that toByteArray() returns a fresh owned copy
• Eliminate redundant array copy when finalizing fragmented binary messages

java/src/org/openqa/selenium/netty/server/MessageInboundConverter.java

---

3. java/src/org/openqa/selenium/netty/server/WebSocketFrameProxy.java ✨ Enhancement +2/-1

Use wrap factory in frame proxy reassembly

• Replace new BinaryMessage(binaryBuffer.toByteArray()) with
 BinaryMessage.wrap(binaryBuffer.toByteArray())
• Add comment clarifying ownership transfer semantics
• Optimize reassembly of fragmented messages for upstream WebSocket proxies

java/src/org/openqa/selenium/netty/server/WebSocketFrameProxy.java

---

View more (1)

4. java/test/org/openqa/selenium/remote/http/BinaryMessageTest.java 🧪 Tests +11/-0

Add test for wrap factory ownership transfer behavior

• Add new test wrapTransfersOwnershipWithoutCopying() verifying wrap behavior
• Test confirms wrap() shares storage with caller via reference equality check
• Test confirms constructor still performs defensive copy for borrowed arrays
• Validates both no-copy and copy semantics are preserved

java/test/org/openqa/selenium/remote/http/BinaryMessageTest.java

---

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0)

1. Unused ownership parameter 🐞 Bug ⚙ Maintainability

Description

BinaryMessage's new private constructor accepts an Ownership parameter but never reads or validates
it, leaving dead code that can trigger static-analysis/build warnings and provides no runtime guard
against accidental misuse if the class evolves.

Code

java/src/org/openqa/selenium/remote/http/BinaryMessage.java[R47-62]

Evidence

The added constructor signature includes Ownership ownership, but the constructor body only
assigns this.data = data; and never references ownership; the Ownership enum exists only to
supply TRANSFER at the call site, so both are effectively unused/dead.

java/src/org/openqa/selenium/remote/http/BinaryMessage.java[47-62]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`BinaryMessage` introduced a private constructor `BinaryMessage(byte[], Ownership)` plus a private `Ownership` enum, but the `ownership` parameter is unused. This is dead code and can trigger static analysis warnings; it also misses an opportunity to enforce the intended invariant (only `TRANSFER` is valid).

### Issue Context
`wrap(byte[])` is intended to be the only entry point to the no-copy path; the constructor currently relies solely on being `private`, while still carrying an unused "ownership" argument.

### Fix Focus Areas
- java/src/org/openqa/selenium/remote/http/BinaryMessage.java[47-62]

### Suggested fix
Either:
1) Use the parameter to enforce the invariant:
  - `Require.nonNull("Ownership", ownership);`
  - `if (ownership != Ownership.TRANSFER) throw new IllegalArgumentException(...)`

Or:
2) Remove the enum/parameter entirely and replace with a clearly named private constructor used only by `wrap(byte[])` (keeping the public constructors’ defensive-copy semantics).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Previous review results

Review updated until commit 87c8eec

Results up to commit ee31f19

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0)

1. Unused ownership parameter 🐞 Bug ⚙ Maintainability

Description

BinaryMessage's new private constructor accepts an Ownership parameter but never reads or validates
it, leaving dead code that can trigger static-analysis/build warnings and provides no runtime guard
against accidental misuse if the class evolves.

Code

java/src/org/openqa/selenium/remote/http/BinaryMessage.java[R47-62]

Evidence

The added constructor signature includes Ownership ownership, but the constructor body only
assigns this.data = data; and never references ownership; the Ownership enum exists only to
supply TRANSFER at the call site, so both are effectively unused/dead.

java/src/org/openqa/selenium/remote/http/BinaryMessage.java[47-62]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`BinaryMessage` introduced a private constructor `BinaryMessage(byte[], Ownership)` plus a private `Ownership` enum, but the `ownership` parameter is unused. This is dead code and can trigger static analysis warnings; it also misses an opportunity to enforce the intended invariant (only `TRANSFER` is valid).

### Issue Context
`wrap(byte[])` is intended to be the only entry point to the no-copy path; the constructor currently relies solely on being `private`, while still carrying an unused "ownership" argument.

### Fix Focus Areas
- java/src/org/openqa/selenium/remote/http/BinaryMessage.java[47-62]

### Suggested fix
Either:
1) Use the parameter to enforce the invariant:
  - `Require.nonNull("Ownership", ownership);`
  - `if (ownership != Ownership.TRANSFER) throw new IllegalArgumentException(...)`

Or:
2) Remove the enum/parameter entirely and replace with a clearly named private constructor used only by `wrap(byte[])` (keeping the public constructors’ defensive-copy semantics).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[qodo-code-review]

Persistent review updated to latest commit 2c15d04

[shs96c]

Addressed in 2c15d04: the private constructor now calls Require.nonNull("Ownership", ownership) on the sentinel parameter so the argument is actually used and the contract is checked at runtime. Comment moved to the enum to explain that it exists purely to give the no-copy constructor a distinct signature from the defensive-copy public one.

[qodo-code-review]

Code review by qodo was updated up to the latest commit 87c8eec