Bug 77522

Wasn't the whole idea of WebSockets that such transformations would be performed by client code, not by protocol itself?

(In reply to comment #1) > Wasn't the whole idea of WebSockets that such transformations would be performed by client code, not by protocol itself? The hybi working group is revising their charter and the WG will focus on compression extension first. http://www.ietf.org/mail-archive/web/hybi/current/msg09393.html There are no objections on the ML.

(In reply to comment #1) > Wasn't the whole idea of WebSockets that such transformations would be performed by client code, not by protocol itself? RFC6455 introduced Sec-WebSocket-Extensions header field for purposes where protocol-level transformation is necessary (like compression and multiplexing). Deflate-frame in WebSocket works like Content-Encoding in HTTP; browsers and servers do all the work while script authors don't have to take care of compression. I think deflate-frame extension spec looks fine in this respect.

Created attachment 127896 [details] WIP patch

Uploaded WIP patch - Added USE(ZLIB) flag for ports which don't use zlib (e.g. QT and EFL) - Using vendor-prefixed extension name ("x-webkit-deflate-frame") - There is no runtime flag. If USE(ZLIB) is defined, non-control frames will be compressed if the server accepts the extension. Yuta-san, could you please take a look before formal review?

Comment on attachment 127896 [details] WIP patch Attachment 127896 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11559011

Comment on attachment 127896 [details] WIP patch View in context: https://bugs.webkit.org/attachment.cgi?id=127896&action=review Looks fine to me. > Source/WebCore/websockets/WebSocketChannel.cpp:1031 > + if (WebSocketFrame::isNonControlOpCode(frame.opCode) && dataLength && m_deflateFramer.enabled()) { > + if (!m_deflateFramer.deflate(frame)) { You can merge these two if statements into one.

Created attachment 128096 [details] Patch

Comment on attachment 127896 [details] WIP patch View in context: https://bugs.webkit.org/attachment.cgi?id=127896&action=review Thank you for review, Yuta-san! >> Source/WebCore/websockets/WebSocketChannel.cpp:1031 >> + if (!m_deflateFramer.deflate(frame)) { > > You can merge these two if statements into one. Done.

Hi Kent-san, Alexey, Could you please review the patch?

Comment on attachment 128096 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=128096&action=review Would you make a separated patch to rename 'reserved1' to 'compress' please? > Source/WebCore/websockets/WebSocketChannel.h:236 > + > + WebSocketDeflateFramer m_deflateFramer; I think using WebSocketDefalateFramer directly in WebSocketChannel is not a good design. Will you introduce other extensions updating frames in the future?

(In reply to comment #11) > (From update of attachment 128096 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=128096&action=review > > Would you make a separated patch to rename 'reserved1' to 'compress' please? > Created a entry as bug 79187. > > Source/WebCore/websockets/WebSocketChannel.h:236 > > + > > + WebSocketDeflateFramer m_deflateFramer; > > I think using WebSocketDefalateFramer directly in WebSocketChannel is not a good design. > Will you introduce other extensions updating frames in the future? We might add multiplexing extension(*1). As the RFC6455 says (*2), the order of processing frames depend on combination of extensions used. This means we are not likely to have generic way to process frames without knowledge of such dependency. That's why I didn't add generalized layer to update frames into WebSocketChannel. (*1) http://tools.ietf.org/html/draft-tamplin-hybi-google-mux-02 (*2) http://tools.ietf.org/html/rfc6455#section-9

Comment on attachment 128096 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=128096&action=review >>> Source/WebCore/websockets/WebSocketChannel.h:236 >>> + WebSocketDeflateFramer m_deflateFramer; >> >> I think using WebSocketDefalateFramer directly in WebSocketChannel is not a good design. >> Will you introduce other extensions updating frames in the future? > > We might add multiplexing extension(*1). As the RFC6455 says > (*2), the order of processing frames depend on combination of extensions used. This means we are not likely to have generic way to process frames without knowledge of such dependency. That's why I didn't add generalized layer to update frames into WebSocketChannel. > > (*1) http://tools.ietf.org/html/draft-tamplin-hybi-google-mux-02 > (*2) http://tools.ietf.org/html/rfc6455#section-9 ok. However we had better make WebSocketChannel cleaner as possible. I'm adding some comments in the next post.

Comment on attachment 128096 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=128096&action=review > Source/WebCore/websockets/WebSocketChannel.cpp:234 > + m_deflateFramer.resetDeflateContext(); > + m_deflateFramer.resetInflateContext(); m_deflateFramer should have a function like didFail(). > Source/WebCore/websockets/WebSocketChannel.cpp:635 > + if (frame.compress) { > + if (!m_deflateFramer.enabled() || !WebSocketFrame::isNonControlOpCode(frame.opCode)) { > + fail("Received unexpected compressed frame"); > + return false; > + } > + if (!m_deflateFramer.inflate(frame)) { > + fail("Failed to decompress frame"); > + return false; > + } > + } > + We had better do: String failureMessage; if (m_deflateFramer.inflate(frame, failureMessage)) { fail(failureMessage); return false; } in order to avoid making WebSocketChannel messy. > Source/WebCore/websockets/WebSocketChannel.cpp:773 > + m_deflateFramer.resetInflateContext(); Why don't you reset the context in inflate()? > Source/WebCore/websockets/WebSocketChannel.cpp:1033 > + if (WebSocketFrame::isNonControlOpCode(frame.opCode) && dataLength && m_deflateFramer.enabled() && !m_deflateFramer.deflate(frame)) { > + fail("Failed to compress frame"); > + return false; > + } We had better do: String failureMessage; if (m_deflateFramer.deflate(frame, failureMessage)) { fail(failureMessage); return false; } > Source/WebCore/websockets/WebSocketChannel.cpp:1038 > + m_deflateFramer.resetDeflateContext(); Why don't you reset the context in deflate()?

Created attachment 128129 [details] Patch

Comment on attachment 128096 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=128096&action=review Kent-san, Thank you for review! >> Source/WebCore/websockets/WebSocketChannel.cpp:234 >> + m_deflateFramer.resetInflateContext(); > > m_deflateFramer should have a function like didFail(). Done. >> Source/WebCore/websockets/WebSocketChannel.cpp:635 >> + > > We had better do: > > String failureMessage; > if (m_deflateFramer.inflate(frame, failureMessage)) { > fail(failureMessage); > return false; > } > > in order to avoid making WebSocketChannel messy. I introduced InflateResultHolder not only to do this but also to remove resetInfalteContext() call. Please see the below comment. >> Source/WebCore/websockets/WebSocketChannel.cpp:773 >> + m_deflateFramer.resetInflateContext(); > > Why don't you reset the context in inflate()? frame.payload could refer the inflater's data() so I need to reset the context after the frame is no longer needed. (The interface of WebSocketDeflater/WebSocketInflater might not be good, though). To remove calling resetInflateContext() here, I added InflateResultHolder class that contains the results of inflate() and calls resetInflateContext() when it is destructed. >> Source/WebCore/websockets/WebSocketChannel.cpp:1033 >> + } > > We had better do: > > String failureMessage; > if (m_deflateFramer.deflate(frame, failureMessage)) { > fail(failureMessage); > return false; > } Added DeflateResultHolder as well. >> Source/WebCore/websockets/WebSocketChannel.cpp:1038 >> + m_deflateFramer.resetDeflateContext(); > > Why don't you reset the context in deflate()? Removed. (DeflateResultHolder calls this) >>>> Source/WebCore/websockets/WebSocketChannel.h:236 >>>> + WebSocketDeflateFramer m_deflateFramer; >>> >>> I think using WebSocketDefalateFramer directly in WebSocketChannel is not a good design. >>> Will you introduce other extensions updating frames in the future? >> >> We might add multiplexing extension(*1). As the RFC6455 says >> (*2), the order of processing frames depend on combination of extensions used. This means we are not likely to have generic way to process frames without knowledge of such dependency. That's why I didn't add generalized layer to update frames into WebSocketChannel. >> >> (*1) http://tools.ietf.org/html/draft-tamplin-hybi-google-mux-02 >> (*2) http://tools.ietf.org/html/rfc6455#section-9 > > ok. > However we had better make WebSocketChannel cleaner as possible. I'm adding some comments in the next post. If I want to add further frame updating mechanism in future, I'll create a class that manages the order of frame updating based on the extension used.

Comment on attachment 128129 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=128129&action=review > LayoutTests/http/tests/websocket/tests/hybi/compressed-control-frame_wsh.py:2 > +# Copyright 2012, Google Inc. > +# All rights reserved. We usually write them in one line. BTW, why this and deflate-frame_wsh.py have the copyright notice though deflate-frame-invalid-parameter_wsh.py doesn't?

Created attachment 128136 [details] Patch for landing

Comment on attachment 128129 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=128129&action=review Thank you for review! >> LayoutTests/http/tests/websocket/tests/hybi/compressed-control-frame_wsh.py:2 >> +# All rights reserved. > > We usually write them in one line. > > BTW, why this and deflate-frame_wsh.py have the copyright notice though deflate-frame-invalid-parameter_wsh.py doesn't? Merged them into one line and Added the copyright notice to deflate-frame-invalid-parameter_wsh.py (I forgot to add it).

Comment on attachment 128136 [details] Patch for landing Rejecting attachment 128136 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: webkit-commit-queue/Source/WebKit/chromium/webkit --revision 122745 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' 48>At revision 122745. ________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' ________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' Updating webkit projects from gyp files... Full output: http://queues.webkit.org/results/11557411

Created attachment 128145 [details] Patch for landing

Comment on attachment 128145 [details] Patch for landing Clearing flags on attachment: 128145 Committed r108468: <http://trac.webkit.org/changeset/108468>

All reviewed patches have been landed. Closing bug.

Chromium Windows builder has failed to compile this change: http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Win%20Builder/builds/20781 22>c:\b\build\slave\win-latest-rel\build\src\third_party\webkit\source\webcore\websockets\WebSocketDeflater.h(40) : fatalerror C1083: Cannot open include file: 'zlib.h': No such file or directory Could you fix this ASAP, or roll the change out?

(In reply to comment #24) > Chromium Windows builder has failed to compile this change: > > http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Win%20Builder/builds/20781 > 22>c:\b\build\slave\win-latest-rel\build\src\third_party\webkit\source\webcore\websockets\WebSocketDeflater.h(40) : fatalerror C1083: Cannot open include file: 'zlib.h': No such file or directory > > Could you fix this ASAP, or roll the change out? Rolled out the change. Sorry for that. Maybe I need to update WebCore.gyp.

Created attachment 128441 [details] Patch

Comment on attachment 128441 [details] Patch ok

Comment on attachment 128441 [details] Patch Thank you!

Comment on attachment 128441 [details] Patch Rejecting attachment 128441 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: hird_party/gold/gold64 30>A /mnt/git/webkit-commit-queue/Source/WebKit/chromium/third_party/gold/ld 30>A /mnt/git/webkit-commit-queue/Source/WebKit/chromium/third_party/gold/ld.bfd 30>Checked out revision 120645. ________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' ________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' Updating webkit projects from gyp files... Full output: http://queues.webkit.org/results/11603234

Created attachment 128575 [details] Patch for landing

Comment on attachment 128575 [details] Patch for landing Clearing flags on attachment: 128575 Committed r108731: <http://trac.webkit.org/changeset/108731>

All reviewed patches have been landed. Closing bug.

(In reply to comment #31) > (From update of attachment 128575 [details]) > Clearing flags on attachment: 128575 > > Committed r108731: <http://trac.webkit.org/changeset/108731> The new websocket tests fail on Qt. Maybe on other platforms too, but I don't know, because build.webkit.org is absolutely dead nowadays. http://build.webkit.sed.hu/results/x86-64%20Linux%20Qt%20Release/r108733%20%2834177%29/results.html

(In reply to comment #33) > The new websocket tests fail on Qt. Maybe on other platforms too, but I don't know, because build.webkit.org is absolutely dead nowadays. > > http://build.webkit.sed.hu/results/x86-64%20Linux%20Qt%20Release/r108733%20%2834177%29/results.html According to the change to Platform.h, new tests don't work on Qt and EFL.

Thanks. I added them to the skipped list: http://trac.webkit.org/changeset/108739/trunk/LayoutTests/platform/qt/Skipped

Thank you , ossy!

(In reply to comment #36) > Thank you , ossy! I should have added entries to Skipped file. Thank you ossy and kent-san!

I rolled out the patch again because it broke Chromium Win tests. I also removed entries added LayoutTests/platform/qt/Skipped file as r108739.

Created attachment 129430 [details] Patch

(In reply to comment #39) > Created an attachment (id=129430) [details] > Patch The previous patch relies on return value optimization. If copy constructors are called, (De|In)flateResultHolder objects call clear(De|In)flateContext() before frame sending/reconstructing. I revised the patch to use OwnPtr to avoid copy constructions. I confirmed all WebSocket tests ran as expected on my Win7 machine.

Comment on attachment 129430 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=129430&action=review > Source/WebCore/Modules/websockets/WebSocketDeflateFramer.cpp:182 > + m_deflater = WebSocketDeflater::create(windowBits, mode); > + m_inflater = WebSocketInflater::create(); > + if (!m_deflater || !m_inflater) { Is it possible that m_deflater or m_inflater become 0?

Comment on attachment 129430 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=129430&action=review >> Source/WebCore/Modules/websockets/WebSocketDeflateFramer.cpp:182 >> + if (!m_deflater || !m_inflater) { > > Is it possible that m_deflater or m_inflater become 0? No. WebSocket(De|In)flater::create() could return 0 in work-in-progress patch, but they won't return 0 now. I forgot to remove this check. Will remove this check.

Created attachment 129574 [details] Patch

Comment on attachment 129574 [details] Patch ok

Comment on attachment 129574 [details] Patch Thanks!

Comment on attachment 129574 [details] Patch Rejecting attachment 129574 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: rame_wsh.py patching file LayoutTests/http/tests/websocket/tests/hybi/handshake-fail-by-extensions-header-expected.txt patching file LayoutTests/http/tests/websocket/tests/hybi/send-file-blob_wsh.py patching file LayoutTests/platform/efl/Skipped patching file LayoutTests/platform/qt/Skipped Hunk #1 succeeded at 256 (offset 1 line). Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Kent Tamura']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue/ Full output: http://queues.webkit.org/results/11769305

Created attachment 129663 [details] Patch for landing

Comment on attachment 129663 [details] Patch for landing Rejecting attachment 129663 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: ebKit/chromium/third_party/yasm/source/patched-yasm --revision 73761 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' 48>At revision 73761. ________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' ________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' Updating webkit projects from gyp files... Full output: http://queues.webkit.org/results/11767406

The commit-queue encountered the following flaky tests while processing attachment 129663 [details]: fast/workers/storage/use-same-database-in-page-and-workers.html bug 50995 (author: dumi@chromium.org) The commit-queue is continuing to process your patch.

Created attachment 129786 [details] Patch for landing

Comment on attachment 129786 [details] Patch for landing Clearing flags on attachment: 129786 Committed r109538: <http://trac.webkit.org/changeset/109538>

All reviewed patches have been landed. Closing bug.

Committed build fix r109564: <http://trac.webkit.org/changeset/109564>