[WebSocket]Reserved bits test case should cover both extension and no-extension scenarios

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Li Yin]

Maybe the test case should be redesigned, it had better cover both no-extension and extension scenarios.

extension bit1 bit2 bit3  covered
   yes     0    1    0      yes
   yes     0    0    1      yes
   no      1    0    0      no
   no      0    1    0      no
   no      0    0    1      no

The current test case only covered the 1st and 2nd.
But the 3rd, 4th and 5th were not covered. 

What's your opinion?

[Li Yin]

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

[Li Yin]

Created attachment 133594 [details]
Patch

[Kenichi Ishibashi]

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.

[Li Yin]

(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.

[Li Yin]

(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.

[Li Yin]

Created attachment 133791 [details]
Patch

[Kenichi Ishibashi]

(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.

[Li Yin]

(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.

[Li Yin]

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.

[Kenichi Ishibashi]

(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.

[Kent Tamura]

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?

[Li Yin]

Created attachment 134455 [details]
Patch

[Kent Tamura]

Comment on attachment 134455 [details]
Patch

ok

[WebKit Review Bot]

Comment on attachment 134455 [details]
Patch

Clearing flags on attachment: 134455

Committed r112488: <http://trac.webkit.org/changeset/112488>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ádám Kallai]

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

[Ádám Kallai]

I skipped this test. Thanks for the investigation.

Landed in: http://trac.webkit.org/changeset/112509

[Csaba Osztrogonác]

Reopen not to forget to fix it.

[Li Yin]

Thanks for your report, I will investigate it.

[Li Yin]

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

[Li Yin]

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?

[Kenichi Ishibashi]

(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.

[Li Yin]

(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.

[Kenichi Ishibashi]

(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.

[Li Yin]

(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.

[Li Yin]

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?

[Kenichi Ishibashi]

(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() ?

[Li Yin]

(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!

[Li Yin]

Created attachment 134817 [details]
Patch

[Kenichi Ishibashi]

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.

[Li Yin]

Created attachment 134918 [details]
Patch

[Kent Tamura]

Comment on attachment 134918 [details]
Patch

Looks ok

[WebKit Review Bot]

Comment on attachment 134918 [details]
Patch

Clearing flags on attachment: 134918

Committed r112826: <http://trac.webkit.org/changeset/112826>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.