Continuous binary getting swallowed?

[idontusenumbers]

My WS JS hosted in Chrome seems to be sending the following frames:
BINARY, isFin = false
CONTINUOUS, isFin = true

Java-WebSocket seems to drop the CONTINUOUS frame on the floor and the application never gets it:

// org/java_websocket/drafts/Draft_6455.java:542
} else if( frame.isFin() ) {
	if( current_continuous_frame == null )
		throw new InvalidDataException( CloseFrame.PROTOCOL_ERROR, "Continuous frame sequence was not started." );
	//Check if the whole payload is valid utf8, when the opcode indicates a text
	if( current_continuous_frame.getOpcode() == Framedata.Opcode.TEXT ) {
		//Checking a bit more from the frame before this one just to make sure all the code points are correct
		int off = Math.max( current_continuous_frame.getPayloadData().limit() - 64, 0 );
		current_continuous_frame.append( frame );
		if( !Charsetfunctions.isValidUTF8( current_continuous_frame.getPayloadData(), off ) ) {
			throw new InvalidDataException( CloseFrame.NO_UTF8 );
		}
	}
	// **** What about if the current_continuous_frame.getOpcode() == Framedata.Opcode.BINARY ****//
	current_continuous_frame = null;

Is this expected? Is Chrome breaking the spec? Is this something Java-WebSocket should handle differently?

[idontusenumbers]

I replaced the code snippet from above with the following code snippet and it seems to be working now. I did very minimal testing (and no continuous text testing, which seems to be the current focus of this code branch)

} else if( !frame.isFin() || curop == Framedata.Opcode.CONTINUOUS ) {
	if( curop != Framedata.Opcode.CONTINUOUS ) {
		if( current_continuous_frame != null )
			throw new InvalidDataException( CloseFrame.PROTOCOL_ERROR, "Previous continuous frame sequence not completed." );
		current_continuous_frame = frame;
	} else if( frame.isFin() ) {
		if( current_continuous_frame == null )
			throw new InvalidDataException( CloseFrame.PROTOCOL_ERROR, "Continuous frame sequence was not started." );

		try {
			webSocketImpl.getWebSocketListener().onWebsocketMessageFragment( webSocketImpl, frame );
		} catch ( RuntimeException e ) {
			webSocketImpl.getWebSocketListener().onWebsocketError( webSocketImpl, e );
		}

		current_continuous_frame.append( frame );


		//Check if the whole payload is valid utf8, when the opcode indicates a text
		if( current_continuous_frame.getOpcode() == Framedata.Opcode.TEXT ) {
			//Checking a bit more from the frame before this one just to make sure all the code points are correct
			int off = Math.max( current_continuous_frame.getPayloadData().limit() - 64, 0 );
			current_continuous_frame.append( frame );
			if( !Charsetfunctions.isValidUTF8( current_continuous_frame.getPayloadData(), off ) ) {
				throw new InvalidDataException( CloseFrame.NO_UTF8 );
			}
		}

		if( current_continuous_frame.getOpcode() == Framedata.Opcode.TEXT ) {
			try {
				webSocketImpl.getWebSocketListener().onWebsocketMessage( webSocketImpl, Charsetfunctions.stringUtf8( current_continuous_frame.getPayloadData() ) );
			} catch ( RuntimeException e ) {
				webSocketImpl.getWebSocketListener().onWebsocketError( webSocketImpl, e );
			}
			return;
		} else if( current_continuous_frame.getOpcode() == Framedata.Opcode.BINARY ) {
			try {
				webSocketImpl.getWebSocketListener().onWebsocketMessage(webSocketImpl, current_continuous_frame.getPayloadData());
			} catch (RuntimeException e) {
				webSocketImpl.getWebSocketListener().onWebsocketError(webSocketImpl, e);
			}
		}

		current_continuous_frame = null;

[idontusenumbers]

For what it's worth, I copied the Draft_6455 class into my src folder so I could modify it in isolation (which seems to be encouraged by the architecture) but readVersion is not public =(

[marci4]

Hello @idontusenumbers,

do you have some steps to reproduce for me?
I would like to see what causes the problem.
Could you please also answer the following questions:

• On what Java version is your server running?
• On what Chrome version is your client running?
• Are you using SSL?

To your propsed fix: I am sorry to say but these changes cause a lot of autobahn test cases to fail! (Right now we pass all of them!)

Regarding the readVersion() method: You don't have to copy everything over. Just extend from Draft_6455 and overwrite processFrame().

Greetings
marci4

[idontusenumbers]

jdk1.8.0_92
Version 61.0.3163.100 (Official Build) (64-bit)
No SSL

I briefly looked through the tests I found and didn't see anything that actually tests the protocol; Is the bulk of the test cases somewhere else?

I don't know the specs well enough to use the correct terminology, but the outline of what causes the issue is in the original issue description; Send the following frames to the server:
BINARY, isFin = false
CONTINUOUS, isFin = true

Extending and overriding doesn't work or at least is not as easy as you suggest. It uses a private member of Draft_6455, after making a copy for the subclass, looks like copyInstance() on the superclass creates instances of the old instance. copyInstance() accesses additional (not as easily replaced) private members.

[marci4]

Hello @idontusenumbers,

there is a testsuite (https://github.com/crossbario/autobahn-testsuite) which I always use to test any changes.

Well for me the chrome api does not provide the possibility to send fragmented frames.
Could you provide a code snippet, which causes the error.

Gonna add a getter for knownExtensions with the next commit to allow make this possible!

Greetings
marci4

[idontusenumbers]

My app sends files from the client (Chrome) to a server (using this library). The app basically sends an "incoming file" message (string) with the name and size; then when it receives a response back it starts sending the file in a dynamically sized chunks (based on network conditions in an attempt to target between 100 and 500ms per chunk, eventually ending in a "file complete" message (string).

This first manifested when I sent three files in a row triggering messages in a sequence like (String) (ByteBuffer)*m (String) (String) (ByteBuffer)*n (String) etc. Chrome does a terrible job showing what frames are being sent; the lengths are all zero and no information about fin, continuation, etc are present. Only "binary".

[idontusenumbers]

Regardless of what my app is doing, the logic in that method doesn't seem sound, unless I just don't understand how the protocol works. Try tracing through the logic with a hypothetical partial BINARY then the completing CONTINUOUS.

[marci4]

Hello @idontusenumbers,

could you provide a simple example application? I have no clue how to send chunks of messages in chrome and I have not found any documentation.
Are you sure that you don't have to overwrite onWebsocketMessage( WebSocket conn, ByteBuffer blob )?

Could you also activate the debug mode to get additional info?
You can to this with WebSocketImpl.DEBUG = true before you start up the server.
Thx

Greetings
marci4

[idontusenumbers]

I tried to pare down what I'm working on to something small and reproducible. I have attached the source code.

Start WebSocketTest then load the HTML file. The HTML file will connect to the server and send a bunch of dummy data. The blue progress bar should get to the end; it gets to the end with my changes I mentioned earlier. You'll need to add the websocket library to the class path.

The app I'm writing is used to upload files to the server. With my changes, I uploaded numerous test files all at once and they all match the originals so I think it's working properly.

There's still a chance I'm doing something wrong, and if so, please let me know =)

websockettest.zip

[marci4]

Hello @idontusenumbers,

thank you very much for the time you invested to set up this sample.

Here are some of my test results:
It is working fine with the following combinations:

• Firefox 57.0b6 and 1.3.4
• Firefox 57.0b6 and master code
• Microsoft Edge 40.15063.0.0 and 1.3.4
• Microsoft Edge 40.15063.0.0 and master code
  I have an issue with the following combinations:
• Chrome 62.0.3202.45 and 1.3.4
• Chrome 62.0.3202.45 and master code

I will look more into this!

Greetings
marci4

[marci4]

Hello @idontusenumbers,

here is an additional update.
I think you found the cause for one of the oldest issues on this project! THANK YOU VERY MUCH! :)

Need to test the changes against the testsuite again and make them work! :)

Greetings
marci4

[marci4]

Sorry for closing the issue!

Could you please check if #570 is fixing this issue for you?

Thank you again for your support.
Greetings
marci4

[idontusenumbers]

It works!

I cloned the repo (considering it was already merged) and it seems to be working fine now. Thanks!

The other refactoring broke my modified Draft but that's alright since I don't need it anymore.

[marci4]

What exactly did I break?

Thx for your effort!

Greetings
marci4

[idontusenumbers]

Because of the access levels on some of the methods before, it was easiest to bring in the code for whole class and change what I needed. The recent changes that made readVersion private prevent that code from running; removing just the method the override of acceptHandshakeAsServer was not possible since it was abstract. I suspect getting the version information from a handshake is a common task in any Draft, maybe leave that one as final or something.

Anyway, no big deal since I don't need it any more.

[marci4]

Well you should not need to overwrite acceptHandshakeAsServer() since you will always use Draft_6455 as a base class.
Apart from that thank you very much for your help!

Closing this issue since it is solved.
Greetings
marci4