Bug 77522 – Adding WebSocket per-frame DEFLATE extension

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Kenichi Ishibashi]

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

[Alexey Proskuryakov]

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

[Kenichi Ishibashi]

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

[Yuta Kitamura]

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

[Kenichi Ishibashi]

Created attachment 127896 [details]
WIP patch

[Kenichi Ishibashi]

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?

[Early Warning System Bot]

Comment on attachment 127896 [details]
WIP patch

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

[Yuta Kitamura]

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.

[Kenichi Ishibashi]

Created attachment 128096 [details]
Patch

[Kenichi Ishibashi]

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.

[Kenichi Ishibashi]

Hi Kent-san, Alexey,

Could you please review the patch?

[Kent Tamura]

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?

[Kenichi Ishibashi]

(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

[Kent Tamura]

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.

[Kent Tamura]

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

[Kenichi Ishibashi]

Created attachment 128129 [details]
Patch

[Kenichi Ishibashi]

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.

[Kent Tamura]

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?

[Kenichi Ishibashi]

Created attachment 128136 [details]
Patch for landing

[Kenichi Ishibashi]

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

[WebKit Review Bot]

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

[Kenichi Ishibashi]

Created attachment 128145 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 128145 [details]
Patch for landing

Clearing flags on attachment: 128145

Committed r108468: <http://trac.webkit.org/changeset/108468>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Yuta Kitamura]

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?

[Kenichi Ishibashi]

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

[Kenichi Ishibashi]

Created attachment 128441 [details]
Patch

[Kent Tamura]

Comment on attachment 128441 [details]
Patch

ok

[Kenichi Ishibashi]

Comment on attachment 128441 [details]
Patch

Thank you!

[WebKit Review Bot]

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

[Kenichi Ishibashi]

Created attachment 128575 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 128575 [details]
Patch for landing

Clearing flags on attachment: 128575

Committed r108731: <http://trac.webkit.org/changeset/108731>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(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

[Kent Tamura]

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

[Csaba Osztrogonác]

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

[Kent Tamura]

Thank you , ossy!

[Kenichi Ishibashi]

(In reply to comment #36)
> Thank you , ossy!

I should have added entries to Skipped file. Thank you ossy and kent-san!

[Kenichi Ishibashi]

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

[Kenichi Ishibashi]

Created attachment 129430 [details]
Patch

[Kenichi Ishibashi]

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

[Kent Tamura]

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?

[Kenichi Ishibashi]

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.

[Kenichi Ishibashi]

Created attachment 129574 [details]
Patch

[Kent Tamura]

Comment on attachment 129574 [details]
Patch

ok

[Kenichi Ishibashi]

Comment on attachment 129574 [details]
Patch

Thanks!

[WebKit Review Bot]

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

[Kenichi Ishibashi]

Created attachment 129663 [details]
Patch for landing

[WebKit Review Bot]

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

[WebKit Review Bot]

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: [e-mail])
The commit-queue is continuing to process your patch.

[Kenichi Ishibashi]

Created attachment 129786 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 129786 [details]
Patch for landing

Clearing flags on attachment: 129786

Committed r109538: <http://trac.webkit.org/changeset/109538>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Patrick R. Gansterer]

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