Bug 78449 – [WebSocket] Add deflater/inflater classes

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

[Kenichi Ishibashi]

Adding wrapper classes of zlib functions to prepare WebSocket deflate-frame extension.

[Kenichi Ishibashi]

Created attachment 126702 [details]
Patch

[Yuta Kitamura]

Comment on attachment 126702 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=126702&action=review

> Source/WebCore/websockets/WebSocketDeflater.cpp:108
> +    deflateEnd(&m_stream);

Could you check the return value?

> Source/WebCore/websockets/WebSocketDeflater.cpp:125
> +    int result = deflate(&m_stream, Z_SYNC_FLUSH);
> +    if (result != Z_OK || m_stream.avail_in > 0)
> +        return false;

You shouldn't call deflate() with Z_SYNC_FLUSH every time you add something to the deflater, because it puts extra data to the output buffer.

You probably need to call defalte() with Z_NO_FLUSH here, and call it again with Z_FINISH in finish().

> Source/WebCore/websockets/WebSocketDeflater.cpp:135
> +    // Remove 4 octets from the tail as the spec reqested.

typo: reqested
spec -> specification (we don't usually use such abbreviations)

Do you mean: "as the specification requires"

> Source/WebCore/websockets/WebSocketDeflater.h:43
> +class DeflateBuffer {

Is this class necessary? Why don't you use Vector<char>?

> Source/WebCore/websockets/WebSocketDeflater.h:52
> +     ~DeflateBuffer();

Bad indent.

> Source/WebCore/websockets/WebSocketDeflater.h:69
> +    static PassOwnPtr<WebSocketDeflater> create(int windowBits, bool noContextTakeOver);

We prefer enums over booleans in cases where bare literals ("true" or "false") would appear without context.

> Source/WebKit/chromium/tests/WebSocketDeflaterTest.cpp:41
> +    const char* compressData = "Hello";

nit: Maybe "uncompressedData", or "inputData"?

> Source/WebKit/chromium/tests/WebSocketDeflaterTest.cpp:97
> +        compressData = reinterpret_cast<char*>(fastMalloc(compressLength));

Vector<char> should be better than manual memory management with fastMalloc/fastFree.

> Source/WebKit/chromium/tests/WebSocketDeflaterTest.cpp:118
> +    compressData = reinterpret_cast<char*>(fastMalloc(compressLength));

Ditto.

[Kenichi Ishibashi]

Created attachment 126742 [details]
Patch

[Kenichi Ishibashi]

Comment on attachment 126702 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=126702&action=review

Hi Yuta-san,

Thank you for review!

>> Source/WebCore/websockets/WebSocketDeflater.cpp:108
>> +    deflateEnd(&m_stream);
> 
> Could you check the return value?

Done.

>> Source/WebCore/websockets/WebSocketDeflater.cpp:125
>> +        return false;
> 
> You shouldn't call deflate() with Z_SYNC_FLUSH every time you add something to the deflater, because it puts extra data to the output buffer.
> 
> You probably need to call defalte() with Z_NO_FLUSH here, and call it again with Z_FINISH in finish().

Done. (Used Z_SYNC_FLUSH instead Z_FINISH to align on a byte boundary)

>> Source/WebCore/websockets/WebSocketDeflater.cpp:135
>> +    // Remove 4 octets from the tail as the spec reqested.
> 
> typo: reqested
> spec -> specification (we don't usually use such abbreviations)
> 
> Do you mean: "as the specification requires"

Done.

>> Source/WebCore/websockets/WebSocketDeflater.h:43
>> +class DeflateBuffer {
> 
> Is this class necessary? Why don't you use Vector<char>?

Removed this class and use Vector<char>.

>> Source/WebCore/websockets/WebSocketDeflater.h:69
>> +    static PassOwnPtr<WebSocketDeflater> create(int windowBits, bool noContextTakeOver);
> 
> We prefer enums over booleans in cases where bare literals ("true" or "false") would appear without context.

Done.

>> Source/WebKit/chromium/tests/WebSocketDeflaterTest.cpp:41
>> +    const char* compressData = "Hello";
> 
> nit: Maybe "uncompressedData", or "inputData"?

Done.

>> Source/WebKit/chromium/tests/WebSocketDeflaterTest.cpp:97
>> +        compressData = reinterpret_cast<char*>(fastMalloc(compressLength));
> 
> Vector<char> should be better than manual memory management with fastMalloc/fastFree.

Done.

Actually, I rewrote the test completely.

>> Source/WebKit/chromium/tests/WebSocketDeflaterTest.cpp:118
>> +    compressData = reinterpret_cast<char*>(fastMalloc(compressLength));
> 
> Ditto.

Done.

[Kenichi Ishibashi]

Created attachment 126744 [details]
Patch(rebased)

[Yuta Kitamura]

Comment on attachment 126744 [details]
Patch(rebased)

View in context: https://bugs.webkit.org/attachment.cgi?id=126744&action=review

> Source/WebCore/websockets/WebSocketDeflater.cpp:53
> +    if (deflateInit2(&deflater->m_stream, Z_DEFAULT_COMPRESSION, Z_DEFLATED, -windowBits, defaultMemLevel, Z_DEFAULT_STRATEGY) == Z_OK)

To me, it seems strange to call defalateInit2() outside member functions (deflateEnd() is called in the destructor, so Init and End look unpaired).

How about adding initialize() member function which calls deflateInit2(), or doing this in the constructor?

> Source/WebCore/websockets/WebSocketDeflater.cpp:79
> +    if (!m_buffer.tryReserveCapacity(m_buffer.size() + maxLength))

reserveCapacity() increases the size of the backing buffer, but we should not touch the area beyond data() + size(). 

For details, see std::vector<>'s documentation (which is almost identical to Vector<>).
http://www.cplusplus.com/reference/stl/vector/capacity/

Here you should do:
    size_t oldSize = m_buffer.size();
    // Optional: call tryReserveCapacity to make sure we have enough memory.
    m_buffer.grow(oldSize + maxLength);

And instead of line 90:
    m_buffer.shrink(oldSize + compressedSize);

> Source/WebCore/websockets/WebSocketDeflater.cpp:108
> +            if (!m_buffer.tryReserveCapacity(m_buffer.capacity() * 2))

This usage of tryReserveCapacity() is also wrong. See above.

And I'm unsure about the reason of doubling the capacity here.

> Source/WebCore/websockets/WebSocketDeflater.cpp:130
> +    if (inflateInit2(&inflater->m_stream, -windowBits) == Z_OK)

Same as line 53.

> Source/WebCore/websockets/WebSocketDeflater.cpp:153
> +    if (!m_buffer.tryReserveCapacity(m_buffer.size() + initialBufferSize))

Same as line 79.

> Source/WebCore/websockets/WebSocketDeflater.cpp:197
> +            if (!m_buffer.tryReserveCapacity(m_buffer.capacity() * 2))

Here as well.

> Source/WebCore/websockets/WebSocketDeflater.h:46
> +    enum ShouldTakeOverContext {

For names of enum type, FooBarMode or FooBarType seem conventional. Maybe "ContextTakeOverMode"?

> Source/WebCore/websockets/WebSocketDeflater.h:48
> +        DoNotTakeOverContext = 0,
> +        TakeOverContext = 1

"= 0" and "= 1" aren't necessary in this case.

> Source/WebCore/websockets/WebSocketDeflater.h:61
> +    WebSocketDeflater(int, ShouldTakeOverContext);

Could you add a parameter name for the first argument? "int" looks ambiguous.

> Source/WebCore/websockets/WebSocketDeflater.h:82
> +    WebSocketInflater(int);

Could you add "explicit" and parameter name?

[Kenichi Ishibashi]

Created attachment 126931 [details]
Patch

[Kenichi Ishibashi]

Comment on attachment 126744 [details]
Patch(rebased)

View in context: https://bugs.webkit.org/attachment.cgi?id=126744&action=review

>> Source/WebCore/websockets/WebSocketDeflater.cpp:53
>> +    if (deflateInit2(&deflater->m_stream, Z_DEFAULT_COMPRESSION, Z_DEFLATED, -windowBits, defaultMemLevel, Z_DEFAULT_STRATEGY) == Z_OK)
> 
> To me, it seems strange to call defalateInit2() outside member functions (deflateEnd() is called in the destructor, so Init and End look unpaired).
> 
> How about adding initialize() member function which calls deflateInit2(), or doing this in the constructor?

Added initialize() member function.

>> Source/WebCore/websockets/WebSocketDeflater.cpp:79
>> +    if (!m_buffer.tryReserveCapacity(m_buffer.size() + maxLength))
> 
> reserveCapacity() increases the size of the backing buffer, but we should not touch the area beyond data() + size(). 
> 
> For details, see std::vector<>'s documentation (which is almost identical to Vector<>).
> http://www.cplusplus.com/reference/stl/vector/capacity/
> 
> Here you should do:
>     size_t oldSize = m_buffer.size();
>     // Optional: call tryReserveCapacity to make sure we have enough memory.
>     m_buffer.grow(oldSize + maxLength);
> 
> And instead of line 90:
>     m_buffer.shrink(oldSize + compressedSize);

Done.

>> Source/WebCore/websockets/WebSocketDeflater.cpp:108
>> +            if (!m_buffer.tryReserveCapacity(m_buffer.capacity() * 2))
> 
> This usage of tryReserveCapacity() is also wrong. See above.
> 
> And I'm unsure about the reason of doubling the capacity here.

The reason I doubled the capacity was performance issue, but using grow() and shrink() is fast as the previous one so I changed increasing size linearly.

>> Source/WebCore/websockets/WebSocketDeflater.cpp:130
>> +    if (inflateInit2(&inflater->m_stream, -windowBits) == Z_OK)
> 
> Same as line 53.

Done.

>> Source/WebCore/websockets/WebSocketDeflater.cpp:153
>> +    if (!m_buffer.tryReserveCapacity(m_buffer.size() + initialBufferSize))
> 
> Same as line 79.

Done.

>> Source/WebCore/websockets/WebSocketDeflater.cpp:197
>> +            if (!m_buffer.tryReserveCapacity(m_buffer.capacity() * 2))
> 
> Here as well.

Done

>> Source/WebCore/websockets/WebSocketDeflater.h:46
>> +    enum ShouldTakeOverContext {
> 
> For names of enum type, FooBarMode or FooBarType seem conventional. Maybe "ContextTakeOverMode"?

Done.

>> Source/WebCore/websockets/WebSocketDeflater.h:48
>> +        TakeOverContext = 1
> 
> "= 0" and "= 1" aren't necessary in this case.

Done.

>> Source/WebCore/websockets/WebSocketDeflater.h:61
>> +    WebSocketDeflater(int, ShouldTakeOverContext);
> 
> Could you add a parameter name for the first argument? "int" looks ambiguous.

Done

>> Source/WebCore/websockets/WebSocketDeflater.h:82
>> +    WebSocketInflater(int);
> 
> Could you add "explicit" and parameter name?

Done

[Yuta Kitamura]

Comment on attachment 126931 [details]
Patch

Looks OK. [Note: I don't have r+ privilege.]

[Kenichi Ishibashi]

Kent-san, Alexey,

Could you review the patch?

[Kent Tamura]

Comment on attachment 126931 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=126931&action=review

> Source/WebCore/websockets/WebSocketDeflater.cpp:59
> +    ASSERT(m_windowBits >= 8 && m_windowBits <= 15);

Please split this into two ASSERTs.

ASSERT(m_windowBits >= 8);
ASSERT(m_windowBits <= 15);

> Source/WebCore/websockets/WebSocketDeflater.cpp:86
> +    m_stream.next_in = reinterpret_cast<Bytef*>(const_cast<char*>(data));
> +    m_stream.avail_in = length;
> +    m_stream.next_out = reinterpret_cast<Bytef*>(m_buffer.data() + writePosition);
> +    m_stream.avail_out = maxLength;

Repeating reinterpret_cast<> and const_cast<> is not good.
Please add a helper function like setStreamParameters(data, length, m_buffer.data() + writePosition, maxLength);

> Source/WebCore/websockets/WebSocketDeflater.cpp:104
> +        m_stream.next_in = reinterpret_cast<Bytef*>(0);
> +        m_stream.avail_in = 0;
> +        m_stream.next_out = reinterpret_cast<Bytef*>(m_buffer.data() + writePosition);
> +        m_stream.avail_out = availableCapacity;

ditto.

> Source/WebCore/websockets/WebSocketDeflater.cpp:163
> +        m_stream.next_in = reinterpret_cast<Bytef*>(const_cast<char*>(data) + consumedSoFar);
> +        m_stream.avail_in = remainingLength;
> +        m_stream.next_out = reinterpret_cast<Bytef*>(m_buffer.data() + writePosition);
> +        m_stream.avail_out = availableCapacity;

ditto.

> Source/WebCore/websockets/WebSocketDeflater.cpp:191
> +        m_stream.next_in = strippedFields + consumedSoFar;
> +        m_stream.avail_in = remainingLength;
> +        m_stream.next_out = reinterpret_cast<Bytef*>(m_buffer.data() + writePosition);
> +        m_stream.avail_out = availableCapacity;

ditto.

[Kenichi Ishibashi]

Created attachment 126952 [details]
Patch

[WebKit Review Bot]

Attachment 126952 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
From git://git.webkit.org/WebKit
   512e439..3821275  master     -> origin/master
Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ...
Currently at 107699 = 512e43965f3c85a210634c39adbabda26e7d72c0
r107700 = 14993e0b953560e2aabeb23e631330550529886e
r107701 = 934bc755aada5227698952084e81a09172d65db2
r107702 = 3821275ca36d73082abe77a514141ea2d24b147a
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Applying: [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets
Using index info to reconstruct a base tree...
<stdin>:1578: trailing whitespace.
        
<stdin>:1647: trailing whitespace.
    
<stdin>:1657: trailing whitespace.
    
<stdin>:1672: trailing whitespace.
        return 0;        
<stdin>:1674: trailing whitespace.
    
warning: squelched 7 whitespace errors
warning: 12 lines add whitespace errors.
Falling back to patching base and 3-way merge...
warning: too many files (created: 168768 deleted: 3), skipping inexact rename detection
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Auto-merging Tools/ChangeLog
CONFLICT (content): Merge conflict in Tools/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kenichi Ishibashi]

Comment on attachment 126931 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=126931&action=review

Thank you for review. Revised the patch.

Looks like qt and efl doesn't depend on zlib. I removed WebSocketDeflater.(cpp|h) from CMakefileList.txt and Target.pri for now.

>> Source/WebCore/websockets/WebSocketDeflater.cpp:59
>> +    ASSERT(m_windowBits >= 8 && m_windowBits <= 15);
> 
> Please split this into two ASSERTs.
> 
> ASSERT(m_windowBits >= 8);
> ASSERT(m_windowBits <= 15);

Done.

>> Source/WebCore/websockets/WebSocketDeflater.cpp:86
>> +    m_stream.avail_out = maxLength;
> 
> Repeating reinterpret_cast<> and const_cast<> is not good.
> Please add a helper function like setStreamParameters(data, length, m_buffer.data() + writePosition, maxLength);

Done.

>> Source/WebCore/websockets/WebSocketDeflater.cpp:104
>> +        m_stream.avail_out = availableCapacity;
> 
> ditto.

Done.

>> Source/WebCore/websockets/WebSocketDeflater.cpp:163
>> +        m_stream.avail_out = availableCapacity;
> 
> ditto.

Done.

[Kent Tamura]

(In reply to comment #14)
> Looks like qt and efl doesn't depend on zlib. I removed WebSocketDeflater.(cpp|h) from CMakefileList.txt and Target.pri for now.

How will you resolve this issue?

[Kenichi Ishibashi]

(In reply to comment #15)
> (In reply to comment #14)
> > Looks like qt and efl doesn't depend on zlib. I removed WebSocketDeflater.(cpp|h) from CMakefileList.txt and Target.pri for now.
> 
> How will you resolve this issue?

I'll add a compile flag like USE(ZLIB) and wrap zlib dependent code in following patch.

[Kent Tamura]

Comment on attachment 126952 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=126952&action=review

> Source/WebCore/websockets/WebSocketDeflater.cpp:178
> +    static const char strippedFields[] = {0x00, 0x00, 0xff, 0xff};

Windows EWS is red.

3>..\websockets\WebSocketDeflater.cpp(178) : error C2220: warning treated as error - no 'object' file generated
3>..\websockets\WebSocketDeflater.cpp(178) : warning C4309: 'initializing' : truncation of constant value
3>..\websockets\WebSocketDeflater.cpp(178) : warning C4309: 'initializing' : truncation of constant value

[Kenichi Ishibashi]

Created attachment 127075 [details]
Patch

[Kent Tamura]

Comment on attachment 127075 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=127075&action=review

> Source/WebCore/websockets/WebSocketDeflater.cpp:179
> +    // Use Bytef[] instead of char[] to avoid "truncation of constant value" warning of MSVC.
> +    static const Bytef strippedFields[] = {0x00, 0x00, 0xff, 0xff};

static const char strippedFields[4] = "\0\0\xff\xff"; is enough?

[Kenichi Ishibashi]

Created attachment 127078 [details]
Patch

[Kenichi Ishibashi]

Comment on attachment 127075 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=127075&action=review

>> Source/WebCore/websockets/WebSocketDeflater.cpp:179
>> +    static const Bytef strippedFields[] = {0x00, 0x00, 0xff, 0xff};
> 
> static const char strippedFields[4] = "\0\0\xff\xff"; is enough?

It will be the best. Thanks!

[Kent Tamura]

Comment on attachment 127078 [details]
Patch

ok

[Kenichi Ishibashi]

Comment on attachment 127078 [details]
Patch

Thanks!

[WebKit Review Bot]

Comment on attachment 127078 [details]
Patch

Clearing flags on attachment: 127078

Committed r107766: <http://trac.webkit.org/changeset/107766>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kenichi Ishibashi]

This patch was reverted:
http://trac.webkit.org/changeset/107778

I'll upload revised patch to fix the build failure of Chromium Win.

[Kenichi Ishibashi]

Created attachment 127139 [details]
Patch

[Kenichi Ishibashi]

(In reply to comment #27)
> Created an attachment (id=127139) [details]
> Patch

I confirmed this patch built successfully on win_rel trybot.
http://build.chromium.org/p/tryserver.chromium/builders/win_rel/builds/1809

[Kent Tamura]

(In reply to comment #28)
> (In reply to comment #27)
> > Created an attachment (id=127139) [details] [details]
> > Patch
> 
> I confirmed this patch built successfully on win_rel trybot.
> http://build.chromium.org/p/tryserver.chromium/builders/win_rel/builds/1809

Did you confirm WebKit without Chromium build for Chromium/Windows?

[WebKit Review Bot]

Attachment 127139 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
Index mismatch: 0dfd183742a71cb5de5dadc3ae177fc72b63a194 != 9cdcda984def14b8bf8a32b6da6784c8a6ef7b3a
rereading 8567f8d3c2539a28a496edaf1048483e973975c2
	M	LayoutTests/fast/forms/radio-nested-labels.html
	M	LayoutTests/ChangeLog
107798 = 3671b2d23de7ade4cb1d1e78a3f6f7673db6a6c9 already exists! Why are we refetching it?
 at /usr/lib/git-core/git-svn line 5210

Died at Tools/Scripts/update-webkit line 164.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kenichi Ishibashi]

(In reply to comment #29)
> (In reply to comment #28)
> > (In reply to comment #27)
> > > Created an attachment (id=127139) [details] [details] [details]
> > > Patch
> > 
> > I confirmed this patch built successfully on win_rel trybot.
> > http://build.chromium.org/p/tryserver.chromium/builders/win_rel/builds/1809
> 
> Did you confirm WebKit without Chromium build for Chromium/Windows?

No. I'll check it next week.

[Kenichi Ishibashi]

(In reply to comment #31)
> > Did you confirm WebKit without Chromium build for Chromium/Windows?
> 
> No. I'll check it next week.

I confirmed WebKit Chromium/Win port build (DumpRenderTree/webkit_unit_tests) on my Win 7 machine (I use MSVS2010 because I couldn't install older version of MSVS correctly..).

[Kent Tamura]

Comment on attachment 127139 [details]
Patch

ok

[WebKit Review Bot]

Comment on attachment 127139 [details]
Patch

Rejecting attachment 127139 [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:
atching file Source/WebCore/websockets/WebSocketDeflater.cpp
patching file Source/WebCore/websockets/WebSocketDeflater.h
patching file Source/WebKit/chromium/WebKit.gypi
Hunk #1 succeeded at 118 with fuzz 1.
patching file Source/WebKit/chromium/WebKitUnitTests.gyp
patching file Source/WebKit/chromium/tests/WebSocketDeflaterTest.cpp

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/11546406

[Kenichi Ishibashi]

Created attachment 127770 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 127770 [details]
Patch for landing

Clearing flags on attachment: 127770

Committed r108221: <http://trac.webkit.org/changeset/108221>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.