Bug 77522 - Adding WebSocket per-frame DEFLATE extension
: Adding WebSocket per-frame DEFLATE extension
Status: RESOLVED FIXED
: WebKit
WebCore Misc.
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
: http://tools.ietf.org/html/draft-tyos...
:
: 78079 78449 78557 79187 79219 79464
:
  Show dependency treegraph
 
Reported: 2012-02-01 01:21 PST by
Modified: 2012-03-02 06:39 PST (History)


Attachments
WIP patch (48.97 KB, patch)
2012-02-20 19:22 PST, Kenichi Ishibashi
no flags Review Patch | Details | Formatted Diff | Diff
Patch (56.23 KB, patch)
2012-02-21 17:40 PST, Kenichi Ishibashi
no flags Review Patch | Details | Formatted Diff | Diff
Patch (52.85 KB, patch)
2012-02-21 22:05 PST, Kenichi Ishibashi
no flags Review Patch | Details | Formatted Diff | Diff
Patch for landing (54.26 KB, patch)
2012-02-21 22:58 PST, Kenichi Ishibashi
no flags Review Patch | Details | Formatted Diff | Diff
Patch for landing (54.29 KB, patch)
2012-02-22 00:19 PST, Kenichi Ishibashi
no flags Review Patch | Details | Formatted Diff | Diff
Patch (55.37 KB, patch)
2012-02-23 02:18 PST, Kenichi Ishibashi
no flags Review Patch | Details | Formatted Diff | Diff
Patch for landing (55.25 KB, patch)
2012-02-23 15:45 PST, Kenichi Ishibashi
no flags Review Patch | Details | Formatted Diff | Diff
Patch (57.48 KB, patch)
2012-02-29 03:14 PST, Kenichi Ishibashi
no flags Review Patch | Details | Formatted Diff | Diff
Patch (57.35 KB, patch)
2012-02-29 18:21 PST, Kenichi Ishibashi
no flags Review Patch | Details | Formatted Diff | Diff
Patch for landing (57.35 KB, patch)
2012-03-01 01:33 PST, Kenichi Ishibashi
no flags Review Patch | Details | Formatted Diff | Diff
Patch for landing (57.86 KB, patch)
2012-03-01 18:00 PST, Kenichi Ishibashi
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-02-01 01:21:53 PST
This bug is the master bug for adding WebSocket per-flame DEFLATE extension.
The curent draft of the spec is at: http://tools.ietf.org/html/draft-tyoshino-hybi-websocket-perframe-deflate-04
------- Comment #1 From 2012-02-01 10:15:23 PST -------
Wasn't the whole idea of WebSockets that such transformations would be performed by client code, not by protocol itself?
------- Comment #2 From 2012-02-01 15:40:05 PST -------
(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.
------- Comment #3 From 2012-02-01 20:32:59 PST -------
(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.
------- Comment #4 From 2012-02-20 19:22:32 PST -------
Created an attachment (id=127896) [details]
WIP patch
------- Comment #5 From 2012-02-20 19:30:03 PST -------
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 #6 From 2012-02-20 20:10:55 PST -------
(From update of attachment 127896 [details])
Attachment 127896 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11559011
------- Comment #7 From 2012-02-21 04:12:27 PST -------
(From update of attachment 127896 [details])
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.
------- Comment #8 From 2012-02-21 17:40:45 PST -------
Created an attachment (id=128096) [details]
Patch
------- Comment #9 From 2012-02-21 17:41:26 PST -------
(From update of attachment 127896 [details])
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.
------- Comment #10 From 2012-02-21 17:42:43 PST -------
Hi Kent-san, Alexey,

Could you please review the patch?
------- Comment #11 From 2012-02-21 19:13:19 PST -------
(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?

> 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?
------- Comment #12 From 2012-02-21 19:56:21 PST -------
(In reply to comment #11)
> (From update of attachment 128096 [details] [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 #13 From 2012-02-21 20:12:48 PST -------
(From update of attachment 128096 [details])
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 #14 From 2012-02-21 20:27:41 PST -------
(From update of attachment 128096 [details])
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()?
------- Comment #15 From 2012-02-21 22:05:33 PST -------
Created an attachment (id=128129) [details]
Patch
------- Comment #16 From 2012-02-21 22:09:44 PST -------
(From update of attachment 128096 [details])
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 #17 From 2012-02-21 22:37:44 PST -------
(From update of attachment 128129 [details])
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?
------- Comment #18 From 2012-02-21 22:58:36 PST -------
Created an attachment (id=128136) [details]
Patch for landing
------- Comment #19 From 2012-02-21 23:01:46 PST -------
(From update of attachment 128129 [details])
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 #20 From 2012-02-22 00:05:14 PST -------
(From update of attachment 128136 [details])
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
------- Comment #21 From 2012-02-22 00:19:33 PST -------
Created an attachment (id=128145) [details]
Patch for landing
------- Comment #22 From 2012-02-22 02:03:33 PST -------
(From update of attachment 128145 [details])
Clearing flags on attachment: 128145

Committed r108468: <http://trac.webkit.org/changeset/108468>
------- Comment #23 From 2012-02-22 02:03:43 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #24 From 2012-02-22 02:49:17 PST -------
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?
------- Comment #25 From 2012-02-22 03:22:33 PST -------
(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.
------- Comment #26 From 2012-02-23 02:18:48 PST -------
Created an attachment (id=128441) [details]
Patch
------- Comment #27 From 2012-02-23 02:20:21 PST -------
(From update of attachment 128441 [details])
ok
------- Comment #28 From 2012-02-23 02:22:25 PST -------
(From update of attachment 128441 [details])
Thank you!
------- Comment #29 From 2012-02-23 05:21:10 PST -------
(From update of attachment 128441 [details])
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
------- Comment #30 From 2012-02-23 15:45:49 PST -------
Created an attachment (id=128575) [details]
Patch for landing
------- Comment #31 From 2012-02-23 22:29:19 PST -------
(From update of attachment 128575 [details])
Clearing flags on attachment: 128575

Committed r108731: <http://trac.webkit.org/changeset/108731>
------- Comment #32 From 2012-02-23 22:29:26 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #33 From 2012-02-23 23:49:23 PST -------
(In reply to comment #31)
> (From update of attachment 128575 [details] [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
------- Comment #34 From 2012-02-23 23:53:14 PST -------
(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.
------- Comment #35 From 2012-02-24 00:01:28 PST -------
Thanks. I added them to the skipped list: http://trac.webkit.org/changeset/108739/trunk/LayoutTests/platform/qt/Skipped
------- Comment #36 From 2012-02-24 00:05:47 PST -------
Thank you , ossy!
------- Comment #37 From 2012-02-24 00:22:51 PST -------
(In reply to comment #36)
> Thank you , ossy!

I should have added entries to Skipped file. Thank you ossy and kent-san!
------- Comment #38 From 2012-02-24 01:57:10 PST -------
I rolled out the patch again because it broke Chromium Win tests. I also removed entries added LayoutTests/platform/qt/Skipped file as r108739.
------- Comment #39 From 2012-02-29 03:14:38 PST -------
Created an attachment (id=129430) [details]
Patch
------- Comment #40 From 2012-02-29 03:16:28 PST -------
(In reply to comment #39)
> Created an attachment (id=129430) [details] [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 #41 From 2012-02-29 17:03:39 PST -------
(From update of attachment 129430 [details])
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 #42 From 2012-02-29 17:22:40 PST -------
(From update of attachment 129430 [details])
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.
------- Comment #43 From 2012-02-29 18:21:03 PST -------
Created an attachment (id=129574) [details]
Patch
------- Comment #44 From 2012-02-29 18:24:39 PST -------
(From update of attachment 129574 [details])
ok
------- Comment #45 From 2012-02-29 18:36:33 PST -------
(From update of attachment 129574 [details])
Thanks!
------- Comment #46 From 2012-03-01 01:23:44 PST -------
(From update of attachment 129574 [details])
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
------- Comment #47 From 2012-03-01 01:33:36 PST -------
Created an attachment (id=129663) [details]
Patch for landing
------- Comment #48 From 2012-03-01 04:55:57 PST -------
(From update of attachment 129663 [details])
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
------- Comment #49 From 2012-03-01 05:59:21 PST -------
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.
------- Comment #50 From 2012-03-01 18:00:08 PST -------
Created an attachment (id=129786) [details]
Patch for landing
------- Comment #51 From 2012-03-02 02:25:01 PST -------
(From update of attachment 129786 [details])
Clearing flags on attachment: 129786

Committed r109538: <http://trac.webkit.org/changeset/109538>
------- Comment #52 From 2012-03-02 02:25:09 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #53 From 2012-03-02 06:39:08 PST -------
Committed build fix r109564: <http://trac.webkit.org/changeset/109564>