RFR: 8351339: WebSocket::sendBinary assume that user supplied buffers are BIG_ENDIAN [v2]

[Mark Sheppard]

On Thu, 13 Mar 2025 13:42:40 GMT, Volkan Yazici <vyazici at openjdk.org> wrote:

>> Fixes endian handling `jdk.internal.net.http.websocket.Frame.Masker`.
>> 
>> ### Implementation notes
>> 
>> I deleted the `Frame` clone in tests, and rewired the test code depending on it to the actual `Frame`. To enable this, I relaxed the visibility of the actual `Frame`. I guess the `Frame` clone was introduced to have strict visibility in the actual `Frame`. Though this is not needed since the actual `Frame` is in an internal package. Plus, the fact that bug is in the `Frame` class hints in the direction that there should be one `Frame`.
>
> Volkan Yazici has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix copyright years

Expanding the paint job on the bikeshed for Masker 

I’d proffer some alternative names to the current on offer and a possible restructuring of Masker providing a set of static  utility

I haven’t looked at any use cases for Masker and the use relationship between Frame and Masker. 
As such,  working under the assumption that by not having a factory method or an explicit constructor, its
instantiation is purely meant to be in the context of the mask operation (formerly transferMasking).

Masker purpose is to hold state 

private final int[] maskBytes = new int[4];
private int offset;
private long maskLongBe;  // maybe rename beLongMask
private long maskLongLe;  // maybe rename leLongMask

Which in turn could be encapsulated in a Mask class.  A Mask is instantiated in the Masker.createMask(int mask) method (formerly reset).
And this Mask instance is passed as a parameter to various masking operations in the mask call flow. Masker’s method then become become static methods

Reset is not really a reset but more an init (substituting for a constructor) and its returning Masker is purely to facilitate, the compound invocation chain in mask.
Masker objects have a transient existence, are there any GC implication with the current instantiation use case?
It is possible to collapse reset into a Masker constructor ?

Looking at the proposed methods, then

Alternative names to the static “mask” method  are    copyWithMask to transferWithMask

mask  —> applyMask      // the non static mask
initGallopingMasking —> initLongMask
doGallopingMasking —> applyLongMask
doPlainMAsking  —> endApplyMask

Using a Mask abstraction to contain the state of the mask processing, then reset becomes a factory method createMask() returning a Mask object.

And this instance of Mask is passed in the processing call chain

          class Mask  {
                public  Mask ( int[] maskBytes, int offset,long beLongMask,long leLongMask) {
            . . . 
          }

       } 

      static void mask(ByteBuffer src, ByteBuffer dst, int mask) {
            if (src.remaining() > dst.remaining()) {
                throw new IllegalArgumentException(dump(src, dst));
            }
            Mask theMask = createMask(mask);
            applyMask(src, dot, theMask);
         }

        /*
         * Clears this instance's state and sets the mask.
         *
         * The behaviour is as if the mask was set on a newly created instance.
         */
        public Mask createMask(int mask) {
            ByteBuffer acc = ByteBuffer.allocate(8).putInt(mask).putInt(mask).flip();
            final int[] maskBytes = new int[4];
            for (int i = 0; i < maskBytes.length; i++) {
                maskBytes[i] = acc.get(i);
            }
            offset = 0;
            beLongMask = acc.getLong(0);
            leLongMask = Long.reverseBytes(maskLongBe);
            return new Mask(maskBytes, offset, beLongMask, leLongMask);
        }

        /*
         * Reads as many remaining bytes as possible from the given input
         * buffer, masks them with the previously set mask and writes the
         * resulting bytes to the given output buffer.
         *
         * The source and the destination buffers may be the same instance. If
         * the mask hasn't been previously set it is assumed to be 0.
         */
        public void applyMask(ByteBuffer src, ByteBuffer dst, theMask) {
            if (src.order() == dst.order()) {
                initLongMask(src, dst, theMask);
                applyLongMask(src, dst, theMask);
            }
            endApplyMask(src, dst, theMask);
        }

        /**
         * Positions the {@link #offset} at 0, which is needed for galloping, by masking necessary amount of bytes.
         */
        private static void initLongMask(ByteBuffer src, ByteBuffer dst, Mask aMask) {
         . . .
        }

        
       private static void applyLongMask(ByteBuffer src, ByteBuffer dst, Mask aMask) {
       . . .
       }

       private void endApplyMask(ByteBuffer src, ByteBuffer dst, Mask aMask) {
       . . . 
       }

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

PR Comment: https://git.openjdk.org/jdk/pull/24033#issuecomment-2723180720