Bug 80103

Created attachment 129824 [details] Patch

Hi Kent-san, Alexey, Could you take a look?

Comment on attachment 129824 [details] Patch https://www.w3.org/Bugs/Public/show_bug.cgi?id=16157 There is a discussion for this check. I'll make consensus before review.

(In reply to comment #3) > There is a discussion for this check. I'll make consensus before review. s/I'll make/I'd like to make/

Comment on attachment 129824 [details] Patch Attachment 129824 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11779859 New failing tests: http/tests/websocket/tests/hybi/unpaired-surrogates-in-message.html

Comment on attachment 129824 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=129824&action=review Current spec say that we must raise SYNTAX_ERROR. So fixing this issue sounds good. I refer the discussion in order to justify that I think this is low priority issue. Please request a review of Kent-san. > Source/WebCore/Modules/websockets/WebSocket.cpp:113 > + if (U16_IS_LEAD(message[index])) { If you increment 'index' at first, following operation '- 1' and '+ 1' could be removed :-)

Comment on attachment 129824 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=129824&action=review > Source/WebCore/Modules/websockets/WebSocket.cpp:108 > + // FIXME: Any other errors except for unpaired surrogates? I think String::utf8(true) will do the trick, including checking unpaired surrogates. If we do this, however, we convert the same string twice in both WebSocket and WebSocketChannel, which may affect the performance. There will probably be two options: (a) Let WebSocket convert the message into CString and pass it to WebSocketChannel. (b) Let WebSocketChannel convert and validate a String, and return a value indicating a conversion error upon failure. (a) sounds a bit strange, because WebSocketChannel is responsible for converting Blob and ArrayBuffer into CString. However, WebSocketChannel already accepts send(const char*, int) for PPAPI implementation purposes, so this might be okay. (b) looks cleaner, but imposes an extra roundtrip between loader and worker if that WebSocket is on workers. The cost should be slight and negligible, though. I think (b) would be better, but a patch for (b) will look hairy due to boilerplates of loader-worker bridge.

Created attachment 130050 [details] Patch

Comment on attachment 129824 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=129824&action=review Thank you for comments! >> Source/WebCore/Modules/websockets/WebSocket.cpp:108 >> + // FIXME: Any other errors except for unpaired surrogates? > > I think String::utf8(true) will do the trick, including checking unpaired surrogates. If we do this, however, we convert the same string twice in both WebSocket and WebSocketChannel, which may affect the performance. > > There will probably be two options: > (a) Let WebSocket convert the message into CString and pass it to WebSocketChannel. > (b) Let WebSocketChannel convert and validate a String, and return a value indicating a conversion error upon failure. > > (a) sounds a bit strange, because WebSocketChannel is responsible for converting Blob and ArrayBuffer into CString. However, WebSocketChannel already accepts send(const char*, int) for PPAPI implementation purposes, so this might be okay. > > (b) looks cleaner, but imposes an extra roundtrip between loader and worker if that WebSocket is on workers. The cost should be slight and negligible, though. > > I think (b) would be better, but a patch for (b) will look hairy due to boilerplates of loader-worker bridge. Took (b) for send(). As for close(), there is already a utf8() call in the close() so I just changed it to be strict. I think it unlikely affects the performance because the valid length of the reason is limited. >> Source/WebCore/Modules/websockets/WebSocket.cpp:113 >> + if (U16_IS_LEAD(message[index])) { > > If you increment 'index' at first, following operation '- 1' and '+ 1' could be removed :-) Good catch:)

Comment on attachment 130050 [details] Patch Attachment 130050 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11801776

Created attachment 130053 [details] Patch(Attempt QT build fix)

Comment on attachment 130053 [details] Patch(Attempt QT build fix) View in context: https://bugs.webkit.org/attachment.cgi?id=130053&action=review Looks good to me. > Source/WebCore/Modules/websockets/WebSocket.cpp:356 > + if (utf8.length() > maxReasonSizeInBytes) { > + ec = SYNTAX_ERR; > + return; > + } Could you add a console message in this case as well? optional: I suggest putting utf8.length() after utf8.isNull() located below.

Created attachment 130220 [details] Patch

Comment on attachment 130053 [details] Patch(Attempt QT build fix) View in context: https://bugs.webkit.org/attachment.cgi?id=130053&action=review Yuta-san, Thank you for review! >> Source/WebCore/Modules/websockets/WebSocket.cpp:356 >> + } > > Could you add a console message in this case as well? > > optional: I suggest putting utf8.length() after utf8.isNull() located below. Done.

(In reply to comment #13) > Created an attachment (id=130220) [details] > Patch Kent-san, could you review the patch?

Comment on attachment 130220 [details] Patch The approach is ok. But would you separate this into two please? 1. Introduce ThreadableWebSocketChannel::SendResult 2. Add surrogate pair checking

(In reply to comment #16) > (From update of attachment 130220 [details]) > The approach is ok. But would you separate this into two please? > 1. Introduce ThreadableWebSocketChannel::SendResult > 2. Add surrogate pair checking Sure. I uploaded a patch for (1) as bug 80356

Created attachment 130260 [details] Patch

Comment on attachment 130260 [details] Patch ok

Comment on attachment 130260 [details] Patch Thank you!

Comment on attachment 130260 [details] Patch Clearing flags on attachment: 130260 Committed r109840: <http://trac.webkit.org/changeset/109840>

All reviewed patches have been landed. Closing bug.

*** Bug 69405 has been marked as a duplicate of this bug. ***

(In reply to comment #21) > (From update of attachment 130260 [details]) > Clearing flags on attachment: 130260 > > Committed r109840: <http://trac.webkit.org/changeset/109840> The new tests fail on Qt with the following strange diffs: --- /ramdisk/qt-linux-64-release/build/layout-test-results/http/tests/websocket/tests/hybi/unpaired-surrogates-in-close-reason-expected.txt +++ /ramdisk/qt-linux-64-release/build/layout-test-results/http/tests/websocket/tests/hybi/unpaired-surrogates-in-close-reason-actual.txt @@ -4,7 +4,7 @@ On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE". Connected. -PASS ws.close(1000, 'í '); threw exception Error: SYNTAX_ERR: DOM Exception 12. +PASS ws.close(1000, '); threw exception Error: SYNTAX_ERR: DOM Exception 12. PASS successfullyParsed is true TEST COMPLETE --- /ramdisk/qt-linux-64-release/build/layout-test-results/http/tests/websocket/tests/hybi/unpaired-surrogates-in-message-expected.txt +++ /ramdisk/qt-linux-64-release/build/layout-test-results/http/tests/websocket/tests/hybi/unpaired-surrogates-in-message-actual.txt @@ -4,7 +4,7 @@ On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE". Connected. -PASS ws.send('í '); threw exception Error: SYNTAX_ERR: DOM Exception 12. +PASS ws.send('); threw exception Error: SYNTAX_ERR: DOM Exception 12. PASS successfullyParsed is true TEST COMPLETE They say PASS, but the strings are absolutely wrong. Have you got any idea what is wrong?

(In reply to comment #24) > -PASS ws.send('í '); threw exception Error: SYNTAX_ERR: DOM Exception 12. > +PASS ws.send('); threw exception Error: SYNTAX_ERR: DOM Exception 12. > PASS successfullyParsed is true > > TEST COMPLETE > > > They say PASS, but the strings are absolutely wrong. Have you got any idea what is wrong? They look similar to https://bugs.webkit.org/show_bug.cgi?id=78541#c8

(In reply to comment #25) > They look similar to https://bugs.webkit.org/show_bug.cgi?id=78541#c8 Similar, but not same. In bug78451 the character is different, but here the character and the closing apostrophe is absolutely missing.

(In reply to comment #24) > They say PASS, but the strings are absolutely wrong. Have you got any idea what is wrong? I'm not sure where the difference came from, but the original string is an invalid UTF8 character(an unpaired surogate) so the string is essentially broken. I think the root cause is the same of https://bugs.webkit.org/show_bug.cgi?id=78541#c8. Looks like the tests were essentially passed.

Reverted r109840 for reason: Broke WebSocket tests on Chrome Mac and Linux Committed r109941: <http://trac.webkit.org/changeset/109941>

Sorry, this change didn't actually break the tests, but it was causing conflicts with the rollout of the one that did (https://bugs.webkit.org/show_bug.cgi?id=80356) so I rolled them both out.

Created attachment 130542 [details] Patch for landing

(In reply to comment #30) > Created an attachment (id=130542) [details] > Patch for landing Minor change in tests. Don't use shouldThrow() to workaround QT failure.

Comment on attachment 130542 [details] Patch for landing Rejecting attachment 130542 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 ERROR: /mnt/git/webkit-commit-queue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://queues.webkit.org/results/11832870

Committed r110037: <http://trac.webkit.org/changeset/110037>