Bug 72687 – [Qt] Enable WebSocket hybi tests

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Yuta Kitamura]

Currently, WebKitQt only supports the old WebSocket protocol (hixie-76). The old protocol is known to have a security defect, and should be phased out into the new protocol.

The new protocol (hybi-17) is already implemented in WebCore and we can switch to the new protocol using WebCore::Settings. In this bug, I'd like to:
- Add an option to QWebSettings (which corresponds to one in WebCore::Settings),
- Let LayoutTestControllerQt be able to switch WebSocket protocols (using overridePreference()),
- Unskip WebSocket hybi tests.

[Yuta Kitamura]

Created attachment 115735 [details]
Patch

[Simon Hausmann]

(In reply to comment #0)
> Currently, WebKitQt only supports the old WebSocket protocol (hixie-76). The old protocol is known to have a security defect, and should be phased out into the new protocol.
> 
> The new protocol (hybi-17) is already implemented in WebCore and we can switch to the new protocol using WebCore::Settings. In this bug, I'd like to:
> - Add an option to QWebSettings (which corresponds to one in WebCore::Settings),
> - Let LayoutTestControllerQt be able to switch WebSocket protocols (using overridePreference()),
> - Unskip WebSocket hybi tests.

QWebSettings is public API, and at that point I'm wondering: Do we want to expose the users of the API to the complexity of choosing the protocol version?

Could we simply make the newer protocol the default? If we need to be able to test both with layout tests, then I think a helper function to toggle this could be added to DumpRenderTreeSupportQt. My guts feeling is that this change could be done without any addition to the public API.

[Yuta Kitamura]

(In reply to comment #2)
> 
> QWebSettings is public API, and at that point I'm wondering: Do we want to expose the users of the API to the complexity of choosing the protocol version?

Agreed, it shouldn't be exposed to public. Actually, in Apple's Mac/Win ports, this attribute is hidden in WebPreferencePrivate, so we should do the same in Qt port.

> 
> Could we simply make the newer protocol the default? If we need to be able to test both with layout tests, then I think a helper function to toggle this could be added to DumpRenderTreeSupportQt. My guts feeling is that this change could be done without any addition to the public API.

I'd like to make sure the tests are up and running first (actually I found test failures which need to be fixed before switching the default protocols).

I didn't know DumpRenderTreeSupportQt; this seems like a perfect place to put functions that are needed to implement overridePreference(). I'd like to write another patch that implements overridePreference() without changing the public interface.

[Yuta Kitamura]

Comment on attachment 115735 [details]
Patch

I shouldn't have changed the public API.

[Simon Hausmann]

(In reply to comment #3)
> (In reply to comment #2)
> > 
> > QWebSettings is public API, and at that point I'm wondering: Do we want to expose the users of the API to the complexity of choosing the protocol version?
> 
> Agreed, it shouldn't be exposed to public. Actually, in Apple's Mac/Win ports, this attribute is hidden in WebPreferencePrivate, so we should do the same in Qt port.
> 
> > 
> > Could we simply make the newer protocol the default? If we need to be able to test both with layout tests, then I think a helper function to toggle this could be added to DumpRenderTreeSupportQt. My guts feeling is that this change could be done without any addition to the public API.
> 
> I'd like to make sure the tests are up and running first (actually I found test failures which need to be fixed before switching the default protocols).
> 
> I didn't know DumpRenderTreeSupportQt; this seems like a perfect place to put functions that are needed to implement overridePreference(). I'd like to write another patch that implements overridePreference() without changing the public interface.

Sounds good. Thanks for taking the Qt port into account. Much appreciated!

[Yuta Kitamura]

Created attachment 115752 [details]
Patch v2 (don't change public API)

[Simon Hausmann]

Comment on attachment 115752 [details]
Patch v2 (don't change public API)

r=me

[Yuta Kitamura]

Thanks!

[WebKit Review Bot]

Comment on attachment 115752 [details]
Patch v2 (don't change public API)

Clearing flags on attachment: 115752

Committed r100763: <http://trac.webkit.org/changeset/100763>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

One test in still broken:

--- /ramdisk/qt-linux-release/build/layout-test-results/http/tests/websocket/tests/hybi/workers/close-code-and-reason-expected.txt 
+++ /ramdisk/qt-linux-release/build/layout-test-results/http/tests/websocket/tests/hybi/workers/close-code-and-reason-actual.txt 
@@ -3,10 +3,6 @@
 
 On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
 
-ws.onclose() was called.
-PASS PASS: worker: event.wasClean is false
-PASS PASS: worker: event.code is codeAbnormalClosure
-PASS PASS: worker: event.reason is emptyString
 WebSocketTest.onopen() was called with testId = 0.
 WebSocketTest.onclose() was called with testId = 0.
 PASS PASS: worker: event.wasClean is true

[Yuta Kitamura]

Oops sorry, I'm going to skip that. It should be the same one as I mentioned in the Skipped file in this patch, and I'll investigate this failure sometime next week.