Bug 82100

Hi yutak, bashi Do you think it is necessary to do that? Could you give some comments? Thanks in advance.

Created attachment 133594 [details] Patch

Comment on attachment 133594 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=133594&action=review > LayoutTests/ChangeLog:5 > + Please add description > LayoutTests/http/tests/websocket/tests/hybi/reserved-bits.html:17 > +var bit_extension = ["1_0", "2_0", "3_0", "2_1", "3_1"]; They are not descriptive. Using query string like "compressed=true&bit_number=1" would be better. > LayoutTests/http/tests/websocket/tests/hybi/reserved-bits_wsh.py:19 > + request.ws_extension_processors = [] # using no extension response This will cause unexpected behavior if another extension is added in the future. > LayoutTests/http/tests/websocket/tests/hybi/reserved-bits_wsh.py:20 > + else: You can remove else clause here.

(In reply to comment #3) > (From update of attachment 133594 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=133594&action=review > > > LayoutTests/ChangeLog:5 > > + > > Please add description > > > LayoutTests/http/tests/websocket/tests/hybi/reserved-bits.html:17 > > +var bit_extension = ["1_0", "2_0", "3_0", "2_1", "3_1"]; > > They are not descriptive. Using query string like "compressed=true&bit_number=1" would be better. > > > LayoutTests/http/tests/websocket/tests/hybi/reserved-bits_wsh.py:19 > > + request.ws_extension_processors = [] # using no extension response > > This will cause unexpected behavior if another extension is added in the future. If another extension is added, I think the mod_pywebsocket will be updated to support it, meanwhile, many related test case will be required to update. In addition, the test case echo-with-no-extension_wsh.py also used that statement. > > > LayoutTests/http/tests/websocket/tests/hybi/reserved-bits_wsh.py:20 > > + else: > > You can remove else clause here.

(In reply to comment #3) > (From update of attachment 133594 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=133594&action=review > > > LayoutTests/ChangeLog:5 > > + > > Please add description > > > LayoutTests/http/tests/websocket/tests/hybi/reserved-bits.html:17 > > +var bit_extension = ["1_0", "2_0", "3_0", "2_1", "3_1"]; > > They are not descriptive. Using query string like "compressed=true&bit_number=1" would be better. Good point, thanks. > > > LayoutTests/http/tests/websocket/tests/hybi/reserved-bits_wsh.py:19 > > + request.ws_extension_processors = [] # using no extension response > > This will cause unexpected behavior if another extension is added in the future. > > > LayoutTests/http/tests/websocket/tests/hybi/reserved-bits_wsh.py:20 > > + else: > > You can remove else clause here.

Created attachment 133791 [details] Patch

(In reply to comment #4) > If another extension is added, I think the mod_pywebsocket will be updated to support it, meanwhile, many related test case will be required to update. > In addition, the test case echo-with-no-extension_wsh.py also used that statement. It would be true, but it's not a justification for adding such dependency. We should avoid adding unnecessary dependency like this. Maybe calling disable_outgoing_compression() (see deflate-frame_wsh.py) is enough.

(In reply to comment #7) > (In reply to comment #4) > > If another extension is added, I think the mod_pywebsocket will be updated to support it, meanwhile, many related test case will be required to update. > > In addition, the test case echo-with-no-extension_wsh.py also used that statement. > > It would be true, but it's not a justification for adding such dependency. We should avoid adding unnecessary dependency like this. Maybe calling disable_outgoing_compression() (see deflate-frame_wsh.py) is enough. This test case needs two scenarios, extension and no-extension. disable_outgoing_compression() function just used uncompressed frame to transfer data, but it still had negotiated extension. This is not expected. And request.ws_extension_processors = [] specified no extension, not only compress extension, but also all the extensions, even if another extension will be added in the future.

From the RFC6455 RSV1, RSV2, RSV3: MUST be 0 unless an extension is negotiated that defines meanings for non-zero values. If a nonzero value is received and none of the negotiated extensions defines the meaning of such a nonzero value, the receiving endpoint MUST _Fail the WebSocket Connection.

(In reply to comment #8) > (In reply to comment #7) > > (In reply to comment #4) > > > If another extension is added, I think the mod_pywebsocket will be updated to support it, meanwhile, many related test case will be required to update. > > > In addition, the test case echo-with-no-extension_wsh.py also used that statement. > > > > It would be true, but it's not a justification for adding such dependency. We should avoid adding unnecessary dependency like this. Maybe calling disable_outgoing_compression() (see deflate-frame_wsh.py) is enough. > > This test case needs two scenarios, extension and no-extension. > disable_outgoing_compression() function just used uncompressed frame to transfer data, but it still had negotiated extension. This is not expected. > And request.ws_extension_processors = [] specified no extension, not only compress extension, but also all the extensions, even if another extension will be added in the future. Ok. Please wait formal review.

Comment on attachment 133791 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=133791&action=review > LayoutTests/http/tests/websocket/tests/hybi/reserved-bits.html:17 > +var compress_bitNumber = ["compressed=false&bit_number=1", nit: We usually use camelCase for variable names. compressedAndBitNumberParameters?

Created attachment 134455 [details] Patch

Comment on attachment 134455 [details] Patch ok

Comment on attachment 134455 [details] Patch Clearing flags on attachment: 134455 Committed r112488: <http://trac.webkit.org/changeset/112488>

All reviewed patches have been landed. Closing bug.

This test fails on Qt. :( I will skip this test until It is fixed. * http/tests/websocket/tests/hybi/reserved-bits.html Diff: --- /ramdisk/qt-linux-64-release/build/layout-test-results/http/tests/websocket/tests/hybi/reserved-bits-expected.txt +++ /ramdisk/qt-linux-64-release/build/layout-test-results/http/tests/websocket/tests/hybi/reserved-bits-actual.txt @@ -1,4 +1,3 @@ -CONSOLE MESSAGE: Received unexpected compressed frame CONSOLE MESSAGE: One or more reserved bits are on: reserved2 = 1, reserved3 = 0 CONSOLE MESSAGE: One or more reserved bits are on: reserved2 = 0, reserved3 = 1 CONSOLE MESSAGE: One or more reserved bits are on: reserved2 = 1, reserved3 = 0 @@ -9,8 +8,9 @@ Case compressed=false&bitNumber=1: Test started. onopen() was called. +FAIL onmessage() was called. (message = "This message should be ignored.") onclose() was called. -PASS closeEvent.wasClean is false +FAIL closeEvent.wasClean should be false. Was true. Case compressed=false&bitNumber=2: Test started. onopen() was called. onclose() was called. Pretty Diff: http://build.webkit.sed.hu/results/x86-64%20Linux%20Qt%20Release/r112505%20(35556)/http/tests/websocket/tests/hybi/reserved-bits-pretty-diff.html

I skipped this test. Thanks for the investigation. Landed in: http://trac.webkit.org/changeset/112509

Reopen not to forget to fix it.

Thanks for your report, I will investigate it.

The root cause is that USE(ZLIB) is disabled, it can't use the negotiated extension "x-webkit-defalte-frame". So the test case which used the "x-webkit-defalte-frame" extension should be skipped in the Layout test. For example: # USE(ZLIB) is disabled. http/tests/websocket/tests/hybi/compressed-control-frame.html http/tests/websocket/tests/hybi/deflate-frame-invalid-parameter.html http/tests/websocket/tests/hybi/deflate-frame-parameter.html http/tests/websocket/tests/hybi/handshake-fail-by-extensions-header.html

We add the "reserved-bits.html" in the Skipped in the QT and EFL platform, and wait for the USE(ZLIB) is enabled. What is your opinion?

(In reply to comment #21) > We add the "reserved-bits.html" in the Skipped in the QT and EFL platform, and wait for the USE(ZLIB) is enabled. > What is your opinion? Do they have such plan? AFAIK they don't. I think you can check whether the deflate-frame extension is enabled in web_socket_do_extra_handshake() and abort the test case. You may also need to prepare QT and EFL specific expectations.

(In reply to comment #22) > (In reply to comment #21) > > We add the "reserved-bits.html" in the Skipped in the QT and EFL platform, and wait for the USE(ZLIB) is enabled. > > What is your opinion? > > Do they have such plan? AFAIK they don't. I think you can check whether the deflate-frame extension is enabled in web_socket_do_extra_handshake() and abort the test case. You may also need to prepare QT and EFL specific expectations. It sounds good. Can you provide some detailed information or example to do that? Thanks.

(In reply to comment #23) > It sounds good. Can you provide some detailed information or example to do that? > Thanks. If the port doesn't support deflate-frame extension, the opening handshake should not contain "Sec-WebSocket-Extensions: deflate-frame". You can check it via request.headers_in in web_socket_do_extra_handshake(). If "comressed=true" and there is no deflate-frame extension header in the request, you may want to abort the handshake in similar way handshake-error_wsh.py does.

(In reply to comment #24) > (In reply to comment #23) > > It sounds good. Can you provide some detailed information or example to do that? > > Thanks. > > If the port doesn't support deflate-frame extension, the opening handshake should not contain "Sec-WebSocket-Extensions: deflate-frame". You can check it via request.headers_in in web_socket_do_extra_handshake(). If "comressed=true" and there is no deflate-frame extension header in the request, you may want to abort the handshake in similar way handshake-error_wsh.py does. Yeah, it can work, but it still had another issue. When it had no negotiated compressed extension, if browser received a frame with setting reserved-1(compressed) bit, browser should failed the connection, it makes sense, as chromium did. But it was checked in the #if ZLIB code segment, it can't work in Qt port because of disable ZLIB. WebKit qt-port can receive the unexpected frame and can't fail the connection. So maybe we need another obvious operation before calling m_deflateFramer.inflate function, like the reserved-2 and reserved-3 bit checking. But if we do that, it will have a duplicate check for reserved-1 bit in these enable ZLIB WebKit ports.

As we know, EFL had enabled the ZLIB already, only QT webkit didn't do that. Who knows the plan of qt-webkit enabling ZLIB?

(In reply to comment #25) > (In reply to comment #24) > > (In reply to comment #23) > > > It sounds good. Can you provide some detailed information or example to do that? > > > Thanks. > > > > If the port doesn't support deflate-frame extension, the opening handshake should not contain "Sec-WebSocket-Extensions: deflate-frame". You can check it via request.headers_in in web_socket_do_extra_handshake(). If "comressed=true" and there is no deflate-frame extension header in the request, you may want to abort the handshake in similar way handshake-error_wsh.py does. > > Yeah, it can work, but it still had another issue. > When it had no negotiated compressed extension, if browser received a frame with setting reserved-1(compressed) bit, browser should failed the connection, it makes sense, as chromium did. > But it was checked in the #if ZLIB code segment, it can't work in Qt port because of disable ZLIB. WebKit qt-port can receive the unexpected frame and can't fail the connection. > > So maybe we need another obvious operation before calling m_deflateFramer.inflate function, like the reserved-2 and reserved-3 bit checking. > But if we do that, it will have a duplicate check for reserved-1 bit in these enable ZLIB WebKit ports. How about adding if (frame.compress) { result->fail("reserved1 bit is on"); return result.release(); } into #else block in WebSocketDeflateFramer::inflate() ?

(In reply to comment #27) > How about adding > > if (frame.compress) { > result->fail("reserved1 bit is on"); > return result.release(); > } > > into #else block in WebSocketDeflateFramer::inflate() ? Yeah, good point, I am going to do that, thank you!

Created attachment 134817 [details] Patch

Comment on attachment 134817 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=134817&action=review LGTM. kent-san, could you take a look? > Source/WebCore/ChangeLog:8 > + Test: http/tests/websocket/tests/hybi/reserved-bits.html nit: we usually put Test: line after the description.

Created attachment 134918 [details] Patch

Comment on attachment 134918 [details] Patch Looks ok

Comment on attachment 134918 [details] Patch Clearing flags on attachment: 134918 Committed r112826: <http://trac.webkit.org/changeset/112826>

All reviewed patches have been landed. Closing bug.